Bring in payment model #5806
Merged
Bring in payment model #5806
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
The tight coupling between doesn't give other option but to check the private method is called. The specs successfully stub `log_entries#create` but for some reason the model instance that gets evaluated it's not the stubbed one.
The exact error is
```
ActiveRecord::RecordNotSaved:
You cannot call create unless the parent is saved
```
raised from app/models/spree/payment_decorator.rb:29:in `ensure_correct_adjustment'
I'll move on to other easier issues and get back to it when we're in a better position.
Merging Spree's an our factory didn't really work.
This way we don't mix contexts while merging in our own decorator tests.
They are now isolated from each other.
If we don't want that callback we can just as well remove it now that we own that code.
Following Rubocop's indications.
`#save_requested_by_customer` is an accessor we added and thus, the Spree's spec didn't consider.
Mostly styling issues.
Closed
|
This change was previously approved by @luisramos0 in #5678 |
* master: (91 commits) Bump ddtrace from 0.37.0 to 0.38.0 Add spec to cover SQL query issue with OCs where the only products from the coordinator inventory are renderer Remove unnecessary order statement, the relation will only be used for counting products Move select out of scope visible_for because it is breaking exchange_product queries and it's just not needed there. The only other use of this product's scope visible_for is the enterprise serializer so we add the select to it. Make OC advanced settings work by permitting the extra parameter Remove conflicting and duplicate route Bump bugsnag from 6.13.1 to 6.14.0 Make charges update method update the first pending payment Move require_login_then_redirect_to to the only place where it is called Make broken spec fail reliably and set it pending Updating translations for config/locales/en_GB.yml Update all locales with the latest Transifex translations Doc defensive coding needed by pin payments Make method a little simple by extracting method Simplify spec, the 2 minutes wait is not necessary anylonger Make unauthorized in ControllerHelpers::Auth the same as in Spree::Admin::BaseController Move unauthorized view to HomeController only, all other calls to unauthorized will go through Auth which will redirect to the home controller IF the user is logged in or to login if user is not logged in Adapt specs to the move of unauthorized route from the spree routes to the main app routes Delete spree_user_signup which is from spree promotions code that we dont use Remove try_spree_current_user ...
After that, we can TDD a second one that also handles validation errors.
This basically catches ActiveRecord::RecordInvalid caused by an invalid credit record, for instance, but also other situations we haven't forseen.
Given the importance of this code, it doesn't bring me much confidence. Apparently, this specs where using a non-existent state by mistake and this went unnoticed because the payment creation was failing silently in payment/processing.rb. This unearthed the fact that our `#ensure_correct_adjustment` needs the order to be persisted to succeed.
No reason to use a callback when custom validation methods can be defined.
The original payment may not be valid because its credit card may be expired. Stripe gives this as a valid scenario returning a success and we should do too. When creating the credit payment we end up validating all sources in a chain as follows. ``` Payment being persisted -> source payment -> original credit card. ``` The source payment was valid when created (It would not be persisted otherwise) but its source card may now be expired, and that's legit. There was also an issue with the `#invalidate_old_payments` callback. It was causing the original payment to be validated again and thus the credit payment failed to be persisted due to the original credit card being expired. Switching this callback to use `#update_column` skips validations and so we don't validate the source payment. We only care about the state there, so it should be fine.
Switching from `#invalidate` to `#update_column` skipped both validations and callbacks and thus, `#ensure_correct_adjustments` was no longer called for older payments.
Handle credit validation errors
|
This was tested as part of #5780 👍 merging now. It's feels totally weird that I am the only code reviewer here, but well... |
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.
What? Why?
Closes #5689 and it's the groundwork needed by #5449 (I'll point a new PR to this one)
This was the solution agreed in openfoodfoundation/spree#46 (review).
Please, go commit by commit.
What should we test?
We should thoroughly test payments. I'll add the scenarios I can think of.
Release notes
Move Payment model and the processing module from Spree into OFN.
Changelog Category: Changed