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

Change how line item options are allowed in line items controller #1943

Merged
merged 1 commit into from
May 31, 2017

Conversation

gus4no
Copy link

@gus4no gus4no commented May 23, 2017

#1131

Don't really know if this is the right way.. but old logic and issue reported seemed confusing to me, apparently it was just a bad setup of permitted attributes.

I just had the line items controller changed to make more sense, and got rid of that class attribute.

Permitted controllers are working now, old test are still green and I added a new test to confirm that orders are being created properly with line items with options.


context 'when the line items have custom attributes' do
it "can create an order with line items that have custom permitted attributes" do
PermittedAttributes.line_item_attributes << {options: [:some_option] }

Choose a reason for hiding this comment

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

Space inside { missing.

@mamhoff
Copy link
Contributor

mamhoff commented May 23, 2017

This looks good to me, but needs a changelog entry for people who have overridden or used that class attribute. Also, would you mind squashing the Hound appeasement into the previous commit?

@gus4no gus4no changed the title Change how options are allowed in line items controller Change how line item options are allowed in line items controller May 23, 2017
@gus4no gus4no force-pushed the line_item_options branch from 0789f71 to 8f9d825 Compare May 23, 2017 12:34
@gus4no
Copy link
Author

gus4no commented May 23, 2017

@mamhoff Changelog updated and commits squashed! :)

Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

Looks goot to me. @jhawthorn would you mind evaluating this PR?

@gus4no gus4no force-pushed the line_item_options branch from 8f9d825 to 49f9dde Compare May 23, 2017 15:37
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thanks!

@mamhoff mamhoff merged commit 4354667 into solidusio:master May 31, 2017
@deodad
Copy link
Contributor

deodad commented Jun 2, 2017

Thanks, looking forward to trying this out!

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.

6 participants