-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Permitted attributes improvements #1112
Permitted attributes improvements #1112
Conversation
@@ -182,7 +190,7 @@ module Core | |||
currency: "GBP", | |||
line_items_attributes: line_items | |||
} | |||
line_items["0"].merge! currency: "GBP", price: 1.99 | |||
line_items["0"].merge! options: {currency: "GBP", price: 1.99} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use line_items["0"][:options] = {currency: "GBP", price: 1.99}
instead of line_items["0"].merge! options: {currency: "GBP", price: 1.99}
.
Space inside { missing.
Space inside } missing.
There are multiple places where additional attributes are being combined with attributes defined in PermittedAttributes to allow updates based on other permissions. This commit moves that logic into a new PermittedAttributes module. This will simplify controllers that are currently tracking these additional attributes with class attribtues and will also make those definitions easier to resuse.
Allow other parts of the codebase to use the line_item_options attributes list. This also moves the permitting of these attributes out of order_contents.rb. Nowhere else in the code are non-controller objects permitting attributes.
OrderContents add takes an options attribute to set arbitrary attribtues on a line item, this commit simplifies the order importer to use this logic rather than updating additional attributes itself.
======= | ||
options: base_attributes.line_item_option_attributes + admin_attributes.line_item_option_attributes | ||
>>>>>>> 3862505... Simplify order_contents line_item add | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unexpected token tRBRACK
(Using Ruby 2.3 parser; configure using TargetRubyVersion
parameter, under AllCops
)
The changes in these commits address some extensibility issues we've run into with permitted attributes. I understand there are some significant changes in here, I am looking for feedback on the general direction and from there implementation specifics.
In multiple places throughout the api controllers additional "admin attributes" are being combined with attributes defined in PermittedAttributes to allow admins to make certain changes. Here is one example from the api/orders_controller:
These are supported by class attributes defined on specific controllers, here is one from api/orders_controller:
A setup like this is currently being used in:
The first commit
This should make sense for all the same reasons the original PermittedAttributes module exists: code reuse, simpler controllers, etc. The next two commits use this new module to simplify logic in the api/controllers, import/order and order_contents.
The second commit:
The third commit addresses an issue that prevents options from being passed in on a line_item when creating a new order through the Api. Because that line_item_options logic was specifically in the api/line_items_controller, permitting the line_item_options attributes in the orders_controller became non-trivial.
One of the major changes here is that any line item attribute that isn't variant_id and quantity is now passed in one consistent way, through options. So anyone doing
line_item: {variant_id: 1, price: 33}
would need to move toline_item: {variant_id: 1, options: {price: 33}}
.The benefits to the consistency can immediately been seen in the simplification of the order importer. Rather than the importer needing to run a conditional update based on additional parameters, all adding/assigning logic runs cleanly through order_contents.add.