Allow bad payloads to be saved in payment log entries #4953
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Allow a rescue to catch both in one go
github-actions
bot
added
the
changelog:solidus_core
|
@kennyadsl looks like it's already marked as such on master https://github.com/solidusio/solidus/blob/master/core/spec/models/spree/order_inventory_spec.rb#L174 |
It failed on multiple retries than 😢 |
kennyadsl
reviewed
Feb 23, 2023
| console. | ||
|
|
||
| WARNING: | ||
| Future solidus versions might enforce compatibility and discard |
There was a problem hiding this comment.
This leaves some doubts that people can stay with the current code for some time, while the deprecation has a clear horizon instead (next major). I think we should decide if we want to remove the possibility of saving non-deserializable strings on the next major and be clear about it.
Also, instead of discard, I'd use reject, because the former makes me think that we could also remove existing entries.
Raising an exception at this time could prevent a payment from being properly processed. Any incompatible response will be wrapped in a valid one and saved with proper reporting. This allows it to be both recorded and reported without breaking the payment details page in the admin.
elia
force-pushed
the
elia/payment-log-entries-yaml-cleanup
branch
from
212aacd to
a36e681
Compare
gsmendoza
added a commit
to solidusio/solidus_braintree
that referenced
this pull request
Feb 24, 2023
Fixes #110. Summary -------- A error Braintree response would raise the following error: ``` Spree::LogEntry::DisallowedClass: Tried to dump unspecified class: Symbol You can specify custom classes to be loaded in config/initializers/spree.rb. E.g: Spree.config do |config| config.log_entry_permitted_classes = ['MyClass'] end ``` Solidus Version: -------- 3.4.0.dev Cause -------- When `SolidusBraintree::Response.build(result)` accepts an error result, the `result.params` it passes to the new response has symbol keys. Here's a sample of the `result.params`: ``` {:transaction=> {:amount=>"20.00", :order_id=>"R300000001", :channel=>"Solidus", :options=>{:store_in_vault_on_success=>"true"}, :payment_method_token=>"0ev7m4dt", :customer_id=>"180763858", :type=>"sale"}} ``` Demonstration -------- See https://github.com/solidusio/solidus_braintree/tree/gsmendoza/110-log-entry-disallowed-class-demo for a demonstration of the error and the my attempts to fix it. Start from the "Try enabling venmo specs" commit. Additional context -------- Related to #108. There is also a PR in Solidus that will temporarily allow bad payloads to be saved in payment log entries. See solidusio/solidus#4953
3 tasks
waiting-for-dev
approved these changes
Feb 24, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
A followup to #4950 after @gsmendoza and @kennyadsl pointed out that if a payment integration was feeding content that couldn't be deserialized the payment processing could have been disrupted vs. just breaking the payment details page in the admin console.
So we're now falling back to the old and very generousresponse.to_yaml, but not before showing a proper warning asking it to be fixed. The wording of the warning is intentionally avoiding a promise that we'll reject bad payloads but instead just says that we might skip them, so we ensure to never interrupt payment processing for a secondary issue in the payment logs.So we're wrapping the bad response into a serializable one that will also report that a problem occurred.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: