initial thoughts on how to extend spree to support extensions that custo... #4546
Conversation
| @@ -274,19 +274,33 @@ def shipped_shipments | |||
| shipments.shipped | |||
| end | |||
|
|
|||
| def contains?(variant) | |||
| find_line_item_by_variant(variant).present? | |||
| def contains?(variant, variant_modifiers_hash = nil) | |||
There was a problem hiding this comment.
Lets simply call these options so that anyone can hook into it for whatever they want. I was thinking along the lines of making things extendable for anything not just variant customizations.
def contains?(variant, options = {})
There was a problem hiding this comment.
had that same thought on my first look, 👍 for options = {}
|
@jsqu99 looks like a good start to me 👍 The big question for me w/r/t customizations on a line item is how to integrate that with spree's shipping. A shipment in spree doesn't correspond with line items, only inventory_units. InventoryUnits only correspond to a variant, so it's impossible to know which line item it came from, and therefore which customizations it has. We need to be able to track this in order to display the customizations in the admin (the order admin displays shipments rather than line items as of spree 2.0) and also to know which "version" of the item is in which shipment. Would appreciate any thoughts on how we can achieve this. |
As of 2.2 inventory units are directly linked to line items. Part of my not yet achieved goal to drop every single class_eval override from product-assembly extension, making all shipment flow extensible without overrides .. |
|
@huoxito Oh, wow that's excellent. Thanks. I missed this change. |
| line_item ? line_item.quantity : 0 | ||
| end | ||
|
|
||
| def find_line_item_by_variant(variant) | ||
| line_items.detect { |line_item| line_item.variant_id == variant.id } | ||
| def find_line_item_by_variant(variant, variant_modifiers_hash = nil) |
There was a problem hiding this comment.
I hate to be this nitpicky, but can we find a better name other than variant_modifiers_hash? Even something like modifiers would make sense. It's implicit that it's either a modification for the variant or the line item, and the code should make that clear. We don't need to include the class of the object in the variable name because this is Ruby.
There was a problem hiding this comment.
This should get the same treatment as above, using options instead.
|
Other than that one comment, I'm 👍 on this change. Thanks for the hard work @jsqu99. |
|
Committed the suggested variable name changes. Are there any other concerns with this approach? Thanks everyone for feedback. Actually some major omissions I'm still working on (like I'm never saving the inbound options). Hopefully I"ll have some updates soon. |
…stomize line items
| line_item ? line_item.quantity : 0 | ||
| end | ||
|
|
||
| def find_line_item_by_variant(variant) | ||
| line_items.detect { |line_item| line_item.variant_id == variant.id } | ||
| def find_line_item_by_variant(variant, options = nil) |
There was a problem hiding this comment.
I think this will break when no options is given, options = {} should be safer
There was a problem hiding this comment.
Definitely should do options = {}
|
All, please have a look at some big updates to this (intended for the 2-3-stable branch currently). #5062 |
Conflicts: core/app/models/spree/order_contents.rb core/app/models/spree/order_populator.rb core/spec/models/spree/order_populator_spec.rb
…ize line items. Adds the build_options hook for extensions to process their own cart params. Fixes spree#4546 Conflicts: core/app/models/spree/order_contents.rb core/app/models/spree/order_populator.rb frontend/app/controllers/spree/orders_controller.rb
Conflicts: core/app/models/spree/order_contents.rb core/app/models/spree/order_populator.rb core/spec/models/spree/order_populator_spec.rb
…reference-decimal-rounding * '2-2-nemo-stable' of github.com:godaddy/spree: (55 commits) Always call registered extension line item comparison hooks Fix issue in Rails 4.0 where object needs to be reloaded explicitly afetr re-association. Address feedback and fix specs. refs spree#4546 Updates changelog for extension options. Enhances ability of extensions to customize line items and the entire cart composition process. Initial work on how to extend spree to support extensions that customize line items. promo_total must be negative instead of positive, tests passing Add UT coverage on frontend and backend extracted validation logic to module using constant in models for database limits, underscore to make easier to read NEMO-2630. Add validations to decimals Eliminate unused eager loading for performance. NEMO-2667 Fix merge error. Bring back our version of code that update order total for open order after a promotion/taxrate is deleted. NEMO-2640 provide querystring value to limit products api to master variants only. this is specifically intended to provide better performance for external integrations that don't intend to use variant information, while keeping backward compatibility with default behavior resolve bundler dependency issues by allowing select2-rails to use latest 3.x version. When 3.4.X is referenced, spree_backend is attemptign to load railtles 3.0.X version, rather than 4.0.X version we need Reduce the number of cascading touches. NEMO-2585 reintroduce column deletion, now that model has overridden attributes and it is safe for deployment override column that is pending deletion. followup deploy will remove these, along with migration to delete column. reviewed w/ gchernoff Revert "Move stubbed method with all the others." Revert "NEMO-2399, temporarily disable api view caching to avoid multi-tenancy issues" ... Conflicts: SPREE_VERSION
...mize line items
Alll (@radar, @BDQ, @JDutil, @simmsy),
Per our email thread, I wanted to get the ball rolling on this subject.
This (along with beginning to work on https://github.com/jsqu99/spree_product_customizations) represents only 3 hours worth of work.
I wanted to get it pushed quickly to make I get feedback early and often.
Unfortunately I have been pulled off of this task, possibly for a few more weeks, but I'd love to get your thoughts.