Bring in payment model #5678
Bring in payment model #5678
Conversation
sauloperez
force-pushed
the
bring-in-payment-model
branch
3 times, most recently
from
17170f0 to
4b360f1
Compare
| @@ -2,116 +2,9 @@ | |||
|
|
|||
| module Spree | |||
| Payment.class_eval do | |||
There was a problem hiding this comment.
we have to delete the decorator file, right?
There was a problem hiding this comment.
That persist invalid bugs me and I want to understand what consequences can that override have.
|
I'm leaving all the code cleanups/rubocop issues until the end so I don't break any behaviour. I take note of your feedback @luisramos0 |
spec/models/spree/payment_spec.rb
Outdated
| order.process_payments! | ||
| expect(order.payments.count).to eq 1 | ||
| expect(order.payments).to include payment | ||
| expect(payment.state).to eq "failed" |
There was a problem hiding this comment.
I need to see what change for this state to be returned now as invalid. The other expectations are still met but this behaviour change could have other undesired consquences.
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.
sauloperez
force-pushed
the
bring-in-payment-model
branch
from
efabf38 to
2f46483
Compare
If we don't want that callback we can just as well remove it now that we own that code.
|
Now that all specs are passing I'm fixing easy code quality and styling issues one by one. Once done, I'll apply the fix #5449 requires. Thanks @luisramos0 for your help! |
sauloperez
force-pushed
the
bring-in-payment-model
branch
from
64f67fc to
e587b7f
Compare
Following Rubocop's indications.
sauloperez
force-pushed
the
bring-in-payment-model
branch
from
19522cc to
3a64cc4
Compare
`#save_requested_by_customer` is an accessor we added and thus, the Spree's spec didn't consider.
app/models/spree/payment.rb
Outdated
| @@ -89,11 +89,15 @@ def can_credit? | |||
| end | |||
|
|
|||
| # see https://github.com/spree/spree/issues/981 | |||
| # | |||
| # Import from future Spree v.2.3.0 d470b31798f37 | |||
There was a problem hiding this comment.
I'd just remove these 2 comments. It's just history, I dont see them as useful for the future dev.
| @@ -107,22 +107,19 @@ def build_source | |||
| source.user_id = order.user_id if order | |||
| end | |||
|
|
|||
| # Pin payments lacks void and credit methods, but it does have refund | |||
| # Here we swap credit out for refund and remove void as a possible action | |||
There was a problem hiding this comment.
Looking at this now I see this could be done in pin.rb instead of here. Not in scope for this PR.
app/models/spree/payment.rb
Outdated
| end | ||
| end | ||
|
|
||
| delegate :currency, to: :order |
There was a problem hiding this comment.
this was converted from method to delegate by rubocop, probably nicer to move it up next to the other delegate?
app/models/spree/payment.rb
Outdated
|
|
||
| state_will_change! | ||
| save | ||
| end |
|
|
||
| unless payment_method.supports?(source) | ||
| invalidate! | ||
| raise Core::GatewayError.new(Spree.t(:payment_method_not_supported)) |
There was a problem hiding this comment.
why did this go back to .new ?
There was a problem hiding this comment.
weird, I think I copied the snippet you suggested here. Whatever.
| else | ||
| # Standard ActiveMerchant capture usage | ||
| response = payment_method.capture(money.money.cents, | ||
| response = if payment_method.payment_profiles_supported? |
There was a problem hiding this comment.
I dont like this rubocop rule, am I the only one? I always find the result to look worse than before and the solution is always to extract the if clause to a method. I am not requesting any change, just a comment.
There was a problem hiding this comment.
I don't have a strong opinion on this. I do find it a bit more readable when nested because otherwise the assignment is easy to miss. As many things in Ruby though, abusing it by having a long if/else beats readability anyway, so extracting a method in those case makes sense.
Mostly styling issues.
|
I close this in favor of #5806. |
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