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

[Spree Upgrade] Fix admin new order page flow (from order details to customer details) #3436

Merged

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Feb 3, 2019

What? Why?

Closes #3103

The new spree admin orders controller together with the variant search JS, after adding the variant, run a window.reload on order#update which redirects to customer details after the first variant is added to the order...

I dont think this behaviour of jumping to customer details after the first variant is added is acceptable. So, we need to find a way to keep the user in the order details page when variants are added and keep the behaviour of jumping to customer details if user clicks "update and recalculate" and the order is not complete.

This PR changes the spree admin orders controller to adapt to our needs by redirecting from update to edit when there are errors (and sends the errors in flash errors) so that when a variant is added and the page is reloaded, the current action is edit and the page doesn't jump to the customer details (the jump is configured on the update action).

What should we test?

The 3 specs in #3103 should be green but only together with #3194 (I have verified this in a previous build)

@luisramos0 luisramos0 self-assigned this Feb 3, 2019
@luisramos0 luisramos0 changed the title [Spree Upgrade] [Spree Upgrade] Fix admin new order page flow (from order details to customer details) Feb 3, 2019
@luisramos0 luisramos0 removed the pr-wip label Feb 5, 2019
@luisramos0 luisramos0 force-pushed the 2-0-orders-edit branch 3 times, most recently from 6a54ad6 to 264fe05 Compare February 5, 2019 20:20
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

Copy link
Contributor

@sauloperez sauloperez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deserves some tests, don't you think?

@luisramos0
Copy link
Contributor Author

@sauloperez re tests, this is fixing 2 tests already.

@luisramos0
Copy link
Contributor Author

reconsidering testing, I need to add some controller level tests here: spec/controllers/spree/admin/orders_controller_spec.rb

@luisramos0
Copy link
Contributor Author

I have added specs for the update method (I added them to the same commit without changing the controller).
@sauloperez pls re-review, thanks!

@sauloperez sauloperez merged commit 0cc2bd0 into openfoodfoundation:2-0-stable Feb 12, 2019
@luisramos0 luisramos0 deleted the 2-0-orders-edit branch February 12, 2019 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants