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

OrderMergeStrategy not working as intended #669

Closed
hendrik-advantitge opened this issue Jan 25, 2021 · 1 comment
Closed

OrderMergeStrategy not working as intended #669

hendrik-advantitge opened this issue Jan 25, 2021 · 1 comment
Assignees
Labels
type: bug 🐛 Something isn't working
Milestone

Comments

@hendrik-advantitge
Copy link
Contributor

Describe the bug
First of all, the OrderMergeStrategies described in the documentation (https://www.vendure.io/docs/typescript-api/orders/merge-strategies/) are not exported and thus not usable in the config.
Secondly, the strategies do not work as intended. The OrderMerger returns something of this type:
type MergeResult = { order?: Order; linesToInsert?: Array<{ productVariantId: ID; quantity: number }>; orderToDelete?: Order; };
It only gives functionality to insert extra OrderLines, but not remove any. On top of that, only the quantity property is merged, while other customFields might require merging as well.

Expected behavior
OrderMergeStrategy should have more control over the resulting Order:

  • Remove OrderLines
  • Merge CustomFields

Environment (please complete the following information):

  • @vendure/core version: 0.18.2
@michaelbromley
Copy link
Member

michaelbromley commented Feb 2, 2021

Thanks for this report. Can you clarify what you mean by "Merge CustomFields"?

My understanding is that we should take custom fields into account when deciding whether OrderLines are equivalent.

E.g.

Existing Order

{
  productVariantId: 1,
  quantity: 1,
  customFields: { foo: 'bar' },
}

Guest Order

{
  productVariantId: 1,
  quantity: 2,
  customFields: { foo: 'quux' },
}

In this case, the OrderLines are not equivalent and merging would result in a new OrderLine being added to the existing Order, with { foo: 'quux' } as the custom fields.

If, however, the guest OrderLine had the same customFields value ({ foo: 'bar' }), then the merge would result in no additional OrderLine, but potentially a modification of the quantity of the existing OrderLine.

michaelbromley added a commit that referenced this issue Feb 2, 2021
michaelbromley added a commit that referenced this issue Feb 2, 2021
Relates to #669. The old implementation was not able to correctly handle OrderLine removal nor
correctly account for custom fields.

BREAKING CHANGE: The signature of the `OrderMergeStrategy.merge()` method has changed. If you have
implemented a custom OrderMergeStrategy, you'll need to update it to return the expected type.
michaelbromley added a commit that referenced this issue Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants