Improve JS handling for OrdersController #1440
Merged
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
| @@ -5,6 +5,7 @@ class OrdersController < Spree::StoreController | |||
| helper 'spree/products', 'spree/orders' | |||
|
|
|||
| respond_to :html | |||
| respond_to :js, only: [:populate, :update] | |||
There was a problem hiding this comment.
I can actually get rid of this one if you prefer, as one can easily just add the line in a decorator.
There was a problem hiding this comment.
I would prefer that since the JS response will not be in core. Otherwise, 👍
jhawthorn
suggested changes
Sep 14, 2016
jhawthorn
approved these changes
Sep 15, 2016
houndci-bot
reviewed
Sep 18, 2016
| it "should advance the order if :checkout button is pressed" do | ||
| allow(order).to receive(:update_attributes).and_return true | ||
| expect(order).to receive(:next) | ||
| put :update, {checkout: true}, { order_id: 1 } |
There was a problem hiding this comment.
Space inside { missing.
Space inside } missing.
houndci-bot
reviewed
Sep 18, 2016
| @@ -18,10 +18,11 @@ def show | |||
|
|
|||
| def update | |||
| if @order.contents.update_cart(order_params) | |||
| @order.next if params.has_key?(:checkout) && @order.cart? | |||
There was a problem hiding this comment.
Use Hash#key? instead of Hash#has_key?.
mtomov
changed the title
|
I've actually changed the code around, and also modified the |
|
Still looking good to me. @mtomov could you please change the target of this PR to the |
* Using an instance variable allows JS populate operations to have
access to the @order from within the rendered .js.erb files
* Assign all line_item errors to the order's base
* Allow to respond to JS requests for `#populate` and `#update` actions.
It is left for the developer to actually implement the `js.erb` views
To consider / TODO:
- to avoid the whole rescue operation, and manually assigning line item
errors to the order, we'd need to do to be doing `order.save`
instead of `line_item.save!` in [`OrderContents`](https://github.com/solidusio/solidus/blob/master/core/app/models/spree/order_contents.rb#L151)
-> That would have the effect to "automatically" assign the line item
errors on the order in the form: `line_items.quantity` in Rails 4.x
and "line_items[0].quantity" in Rails 5
More info:
- rails/rails#8638
- rails/rails#19686
- move the non-redirect operations out of the html handling block, so
that they will also be executed on a JS format call
- add missing test for the click on the `checkout` button, which is
meant to advance the order instead of only updating the cart
|
Looks reasonable to me 👍 |
|
Thanks again @mtomov |
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.
access to the @order from within the rendered
.js.erbfiles#populateand#updateactions.It is left for the developer to actually implement the
js.erbviews#updateto perform the behave properly for JS actions as wellTo consider:
to avoid the whole rescue operation, and manually assigning line item
errors to the order's
:base, we'd need to do to be doingorder.saveinstead of
line_item.save!inOrderContents-> That would have the effect to "automatically" assign the line item
errors on the order in the form:
line_items.quantityin Rails 4.xand "line_items[0].quantity" in Rails 5
More info:
Improve a way to handle errors for collection nested attributes rails/rails#8638
Errors can be indexed with nested attributes rails/rails#19686