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

Different behaviour between `Spree::Api::LineItemsController#create` and `#update` #2916

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@jbourassa

jbourassa commented Apr 27, 2013

The create method uses :as => :api but the update does not in Spree::Api::LineItemsController

Is this intended? It looks weird to me, but there may be some reason I don't see. If that's unintended, I'd gladly file a pull request.

Cheers and thanks for Spree!

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Apr 22, 2013

Member

Hey @jbourassa, you're right that it should use :as => :api. Could you please submit a PR to fix this problem and include some tests? Thanks!

You can convert this issue into a PR by creating a new branch on your repository, making the necessary changes, committing them, pushing them to GitHub and then (with the hub gem): hub pull-request -i 2916. That way we can continue the discussion on this one thread rather than having two.

Member

radar commented Apr 22, 2013

Hey @jbourassa, you're right that it should use :as => :api. Could you please submit a PR to fix this problem and include some tests? Thanks!

You can convert this issue into a PR by creating a new branch on your repository, making the necessary changes, committing them, pushing them to GitHub and then (with the hub gem): hub pull-request -i 2916. That way we can continue the discussion on this one thread rather than having two.

Jimmy Bourassa
Add `:as => :api` to `Spree::Api::LineItemsController#update`
Ensures a consistent API so `attr_accesible`'ing fields
specifically for the API works in both `create` and `update` actions.
@jbourassa

This comment has been minimized.

Show comment
Hide comment
@jbourassa

jbourassa Apr 27, 2013

Hey @radar, I fixed the issue. The only way I could test this is by listening to the method call explicitly and asserting the parameters, uck!

This test looks kinda useless to me, but I included it anyway since you mentioned tests for this in the first place. Let me know if you see anything better, I'd be happy to improve it.

Cheers,

jbourassa commented Apr 27, 2013

Hey @radar, I fixed the issue. The only way I could test this is by listening to the method call explicitly and asserting the parameters, uck!

This test looks kinda useless to me, but I included it anyway since you mentioned tests for this in the first place. Let me know if you see anything better, I'd be happy to improve it.

Cheers,

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Apr 29, 2013

Member

I don't even know why we needed the LineItem decorator in the first place. Both variant_id and quantity are mass-assignable on LineItem already. I'll remove the decorator now, which will remove the need for your patch.

Sorry!

Member

radar commented Apr 29, 2013

I don't even know why we needed the LineItem decorator in the first place. Both variant_id and quantity are mass-assignable on LineItem already. I'll remove the decorator now, which will remove the need for your patch.

Sorry!

@radar radar closed this Apr 29, 2013

@jbourassa

This comment has been minimized.

Show comment
Hide comment
@jbourassa

jbourassa Apr 29, 2013

@radar : would you mind adding the :as => :api anyways? That would be more consistent and also allow adding attr_accessible'ing only for the API.

One use case of this would be allowing to use API as a backend only and inject price in the line_items base on a custom logic.

Thanks!

jbourassa commented Apr 29, 2013

@radar : would you mind adding the :as => :api anyways? That would be more consistent and also allow adding attr_accessible'ing only for the API.

One use case of this would be allowing to use API as a backend only and inject price in the line_items base on a custom logic.

Thanks!

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Apr 30, 2013

Member

@jbourassa Sure, I don't see why not. There's two commits now on radar/spree (one for 1-3-stable and one for master) that'll add :as => :api to both the create and update methods in that controller. They'll be available on spree/spree once CI is happy.

Member

radar commented Apr 30, 2013

@jbourassa Sure, I don't see why not. There's two commits now on radar/spree (one for 1-3-stable and one for master) that'll add :as => :api to both the create and update methods in that controller. They'll be available on spree/spree once CI is happy.

@jbourassa

This comment has been minimized.

Show comment
Hide comment
@jbourassa

jbourassa commented May 1, 2013

Thanks @radar !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment