Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2520 Remove `process_payments!` override #3653

Merged

Conversation

Projects
None yet
5 participants
@mkllnk
Copy link
Member

commented Mar 26, 2019

What? Why?

Closes #2520.
This PR replaces #3012 and #2771.

A while ago we backported the Spree 2 version of Spree::Order#process_payments!. It can be removed now. But in the meantime, the subscriptions code needed to modify the order model as well and was relying on our process_payments! version.

This pull request simplifies the subscriptions modification to work with Spree 2 code and removes some overrides.

What should we test?

  • Checkout with cash payment.
  • Checkout with credit card payment.
  • Payment of a subscription at the end of an order cycle.

@mkllnk mkllnk self-assigned this Mar 26, 2019

@luisramos0 luisramos0 requested a review from kristinalim Mar 26, 2019

@luisramos0

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

I think this is a good opportunity to test subscriptions v2 in a detailed way in terms of checkout and payments.

@kristinalim
Copy link
Member

left a comment

Very nice, @mkllnk!

@RachL RachL added the pr-staged-fr label Apr 1, 2019

@RachL RachL self-assigned this Apr 4, 2019

@RachL RachL added pr-staged-au and removed pr-staged-fr labels Apr 4, 2019

@RachL

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@mkllnk @luisramos0 I'm not able to add an item to the cart on staging AU. The button loads infinitely, even if I cleared my browser cache:

image

Is it linked to the staging server? I will create an enterprise from scratch to see if the error remains.

@luisramos0

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

product distributions table missing... I merged 2-0-stable into this branch. redeploying now.

@luisramos0

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@mkllnk I have merged 2-0-stable into this PR to get it staged on top of a DB without product distributions but the build is showing some errors. Looks like it needs to be looked at.

@luisramos0 luisramos0 removed the pr-staged-au label Apr 4, 2019

@mkllnk mkllnk force-pushed the mkllnk:2520-spree2-order-subscription branch from 15a2130 to 20444be Apr 10, 2019

@luisramos0

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

@mkllnk I see the build is broken on the sales tax report.
I have fixed that spec in #3718
#3718 is merged already so a rebase here will probably do the trick ;-)

mkllnk added some commits Sep 25, 2018

Reduce the override of Order for subscriptions
Orders belonging to subscriptions get completed without payment. That
requires overriding Spree's functionality.

In Spree 2, an order in payment state without pending orders is invalid.
Instead we skip the payment state by not requiring a payment for
automatically generated orders until the order cycle is closed.
Update specs for Spree v2 payment requirement
This pull request removed the override of `process_payments!` which was based on v1. Spree v2 has an additional check: An order in payment state requires a payment. Some specs didn't care and didn't create payments before transitioning to `complete`.

@mkllnk mkllnk force-pushed the mkllnk:2520-spree2-order-subscription branch from 20444be to 278190a Apr 16, 2019

@mkllnk

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

Great. I rebased and the specs pass now. Ready for re-review. The last commit is new.

@luisramos0 luisramos0 requested a review from kristinalim Apr 16, 2019

@RachL

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

I had troubles with payment methods in Katuma staging, but not is staging FR, so I guess this is good to go. Will stage something else to katuma staging to see if it was linked to the staging. https://docs.google.com/document/d/1x9OooII1jJc-8Zorfsst1u8WNeRqH9snejmS71YVeF4/edit#heading=h.hj26umg3kyxb

@RachL RachL removed the pr-staged-fr label Apr 18, 2019

@mkllnk mkllnk merged commit 99a4878 into openfoodfoundation:2-0-stable Apr 18, 2019

1 check passed

semaphoreci The build passed on Semaphore.
Details

@mkllnk mkllnk deleted the mkllnk:2520-spree2-order-subscription branch Apr 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.