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

Fix stacking invalid order requests #2796

Conversation

lukeromanowicz
Copy link
Contributor

@lukeromanowicz lukeromanowicz commented Apr 25, 2019

Related issues

#2663 #2740 #2751

closes #2663 #2740

Short description and why it's useful

It prevents repeating order requests by disabling order caching in case of invalid request body (http code 400).

Screenshots of visual changes before/after (if there are any)

Which environment this relates to

Check your case. In case of any doubts please read about Release Cycle

  • Test version (https://test.storefrontcloud.io) - this is a new feature or improvement for Vue Storefront. I've created branch from develop branch and want to merge it back to develop
  • RC version (https://next.storefrontcloud.io) - this is a stabilisation fix for Release Candidate of Vue Storefront. I've created branch from release branch and want to merge it back to release
  • Stable version (https://demo.storefrontcloud.io) - this is an important fix for current stable version. I've created branch from hotfix or master branch and want to merge it back to hotfix

Upgrade Notes and Changelog

  • No upgrade steps required (100% backward compatibility and no breaking changes)
  • I've updated the Upgrade notes and Changelog on how to port existing VS sites with this new feature

IMPORTANT NOTICE - Remember to update CHANGELOG.md with description of your change

Contribution and currently important rules acceptance

@lukeromanowicz lukeromanowicz added this to the 1.9.0 milestone Apr 25, 2019
@pkarw
Copy link
Collaborator

pkarw commented Apr 25, 2019

@lukeromanowicz for sure You will add it - but we probably need to check the response statuses and not stack the orders with failing validation, but probably stack the orders with server error - and surely with the lack of internet connection. However, when the server is down because of traffic peak in 9/10 cases it will return 502/503 error - so kind of 5xxx

@lukeromanowicz lukeromanowicz force-pushed the fix/stacking-invalid-order-requests branch from 9c5f7e6 to debe392 Compare April 25, 2019 08:26
@lukeromanowicz lukeromanowicz changed the title WIP: Fix stacking invalid order requests Fix stacking invalid order requests Apr 25, 2019
@lukeromanowicz
Copy link
Contributor Author

This PR fixes only handling HTTP 400 responses to order create requests with an invalid address on directOrderSync turned on, so a proper popup is being shown and bad requests are not stacking in the queue. In my opinion, it's not possible to make it work well with directOrderSync being off without #2753

@pkarw
Copy link
Collaborator

pkarw commented Apr 25, 2019

In the rare case of the offline mode order error we should show the notification like: “There was an error processing You’re offline order. Probably the ordered products went out of the stock. Please re-order the products or contact us”

In this case we have two options for a work around:
A) show this notification, remove order from the queue, add the products to shopping cart and redirect to cart (Remake order exactly as it works ok User Account - order history - remake order)
B) show this notification, remove order from the queue

Of course A is preferred - I bet You can make it :-)

Then some day we’ll implement the #2753

We can do B to 1.9 and A to 1.10rc1 if you feel more safe with that but we
Probably shouldn’t leave this issue unaddressed as we’re aware of it in the end...

@lukeromanowicz lukeromanowicz changed the title Fix stacking invalid order requests WIP: Fix stacking invalid order requests Apr 25, 2019
@pkarw
Copy link
Collaborator

pkarw commented Apr 25, 2019

We can merge it in but handling the error messages in case of validation issue plus offline order errors (A or B) is crucial.

@pkarw
Copy link
Collaborator

pkarw commented Apr 26, 2019

OK could you move the comments out of this PR to kind of issue to not loose the ideas then? @lukeromanowicz

@lukeromanowicz
Copy link
Contributor Author

lukeromanowicz commented Apr 26, 2019

@pkarw I've implemented B) for now. Instead of doing A I would suggest rushing #2753 because this part of code needs refactor anyway.

@lukeromanowicz lukeromanowicz changed the title WIP: Fix stacking invalid order requests Fix stacking invalid order requests Apr 26, 2019
Copy link
Collaborator

@patzick patzick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One quick change in if

core/lib/sync/task.ts Outdated Show resolved Hide resolved
@patzick patzick merged commit 6f5ab61 into vuestorefront:release/1.9 Apr 26, 2019
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.

None yet

3 participants