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

Should we need to clear shippingLines while we save new shippingAddress for active order each time? #1195

Closed
tianyingchun opened this issue Oct 23, 2021 · 3 comments
Assignees
Labels
type: bug 🐛 Something isn't working

Comments

@tianyingchun
Copy link
Contributor

Describe the bug
while we first setShippingAddress for order, then we choosed a eligible shipping method

but while we changed shipping address to another, maybe before saved shippingLines is not eligible at this time, we have no chance to reset shippingLines

  1. first we need to clear shippingLines for this order. provider us mutation cleanOrderShippingMethod for us.
  2. or while we invoke mutationsetOrderShippingAddress, it will automatically check if current if shippingMethod is eligible` it not remove it automatically.

To Reproduce
Steps to reproduce the behavior:

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

Expected behavior
A clear and concise description of what you expected to happen.

Environment (please complete the following information):

  • @vendure/core version: 1.3.1
  • Nodejs version 14+
  • Database (mysql/postgres etc): mysql

Additional context

@tianyingchun tianyingchun added the type: bug 🐛 Something isn't working label Oct 23, 2021
@michaelbromley
Copy link
Member

I understand the issue. I think the first solution is better, because with option 2, it assumes that only a change to the shippingAddress will affect eligibility of the current method. However, a ShippingEligibilityChecker is able to perform any arbitrary logic to decide whether a method is eligible. For example, it might be based on the weight of items in the order.

So in order to correctly do an "automatic" check, we would need to run an eligibility check on every change to the Order.

The first method involves more work for the storefront implementation - it needs to decide at which point to remove the current shipping method. But I think that is reasonable.

Another slight variation on this could be to have a verifyOrderShippingMethod mutation which will check whether the current method is still eligible, and if so return true and if not return false and remove that shippingLine from the Order.

In either case, I will assign this to the next minor milestone since it is not just a bugfix, but involves a new feature.

@michaelbromley
Copy link
Member

I just took a deeper look at this. Due to this change which was part of v1.3.4, whenever you execute the setOrderShippingAddress mutation, the order will now be passed through the OrderCalculator which will check the eligibility of the current ShippingMethod. If it is no longer eligible, it will automatically replace the currenct ShippingLine with the next cheapest eligible ShippingMethod.

In that case, can we consider this issue solved?

@michaelbromley
Copy link
Member

Based on further discussion on Slack, the behaviour is fine now, but there exists 1 unhandled case:

When the current ShippingMethod becomes ineligible, and there is no other eligible method, the shippingLines should be cleared (set to []). Currently this does not happen - the ineligible method & shippingLines remain on the Order.

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