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

Improve line item packaging #11932

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Tashows
Copy link

@Tashows Tashows commented Sep 15, 2023

Fixes #8621
More context is provided in the original issue and in each commit body.

When an order has two line_items that have the same variant_id (for example when using line_item_comparison_hooks), they were effectively getting merged and added into a package as one inventory_unit. Grouping by line_item_id fixes that.
The @adjuster[hash_item item] would cause the adjuster to discard additional package content items that reference the same variant_id as a content item that has already been iterated over, but which should be processed separately from each other (for example when using line_item_comparison_hooks).
@viezly
Copy link

viezly bot commented Sep 15, 2023

Changes preview:

Legend:

👀 Review pull request on Viezly

@rafalcymerys
Copy link
Member

Hi @Tashows thank you for submitting this. Let me review in more detail and get back to you, at first sight it seems to make sense.
I see that there are some tests failing in this PR, around the area that you've changed. Could you also take a look at them?

@Tashows
Copy link
Author

Tashows commented Sep 29, 2023

@rafalcymerys I do plan on following up with a more complete PR on this, because I am working on it anyway for another project (things break in the admin interface as well), but things have come up. Will update once I get the chance to finish this.

@rafalcymerys
Copy link
Member

@Tashows that sounds great, anytime you have the updated PR just ping me and I'll take a look.
Thanks!

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.

Spree::Order.register_line_item_comparison_hook not implemented for Spree::Stock::Coordinator
2 participants