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

Errors in checkout stack into multiple orders per submit #2663

Closed
2 of 5 tasks
lukeromanowicz opened this issue Mar 27, 2019 · 9 comments
Closed
2 of 5 tasks

Errors in checkout stack into multiple orders per submit #2663

lukeromanowicz opened this issue Mar 27, 2019 · 9 comments
Assignees
Labels
1: Easy bug Bug reports good first issue Tasks that requires just basic understanding of Vue Storefront P1: Urgent Priority mark - high priority QA approved after merge Testers will add this label after positive check on merged changes vs-hackathon Tasks for the Hackathon
Milestone

Comments

@lukeromanowicz
Copy link
Contributor

Current behavior

The more times you submit an order with invalid data (i.e. only numbers in the name fields) the more times there will be a submit order request sent to the backend. First submit will send only one request, another submit w/o fixing errors will send the order twice, etc.

Expected behavior

Order requests having error are being repeated over and over.

Steps to reproduce the issue

  1. try to place an order with invalid user data (i.e. a few digits in name fields)
  2. hit submit button several times
  3. the more times you hit the button the more requests will be sent to the backend incrementally.

Can you handle fixing this bug by yourself?

  • YES but not anytime soon, feel free to fix it
  • NO

Which Release Cycle state this refers to? Info for developer.

Pick one option.

  • This is a bug report for test version on https://test.storefrontcloud.io - In this case Developer should create branch from develop branch and create Pull Request 2. Feature / Improvement back to develop.
  • This is a bug report for current Release Candidate version on https://next.storefrontcloud.io - In this case Developer should create branch from release branch and create Pull Request 3. Stabilisation fix back to release.
  • This is a bug report for current Stable version on https://demo.storefrontcloud.io and should be placed in next stable version hotfix - In this case Developer should create branch from hotfix or master branch and create Pull Request 4. Hotfix back to hotfix.

Environment details

  • Browser: Chrome 72
  • OS: Ubuntu 18.04
  • Node: 8
  • Code Version: develop
@lukeromanowicz lukeromanowicz added bug Bug reports good first issue Tasks that requires just basic understanding of Vue Storefront P1: Urgent Priority mark - high priority 1: Easy labels Mar 27, 2019
@pkarw
Copy link
Collaborator

pkarw commented Mar 27, 2019

@patzick

@pkarw pkarw added the vs-hackathon Tasks for the Hackathon label Mar 30, 2019
@pkarw
Copy link
Collaborator

pkarw commented Apr 4, 2019

This should be fixed to 1.9.0 @lukeromanowicz can you please take a look?

@AndreiBelokopytov
Copy link
Contributor

I investigated a bit this issue.
The problem: each new order is cached in LocalStorage before transmission.
In the case when order POST request was not finished successfully the cache remains until next "order/PROCESS_QUEUE" will be published. So after the first incorrect request there is 1 unpublished order in the cache, after the second – 2 unpublished requests and so on. These unpublished orders never will be deleted from the queue (since they contains invalid data).
"order/PROCESS_QUEUE" is emitted immediately after order placement (PLACE_ORDER) if the order was not transmitted so it does 1 retry for each unpublished order
The possible solution may be to

  1. remove unpublished order from the cache if the API returns 500 status for order POST request
  2. immediate retry for the order POST may be unnecesserey so it is possible to remove "order/PROCESS_QUEUE" event emitter after the order was saved in LocalStorage

This solution works for me

@pkarw
Copy link
Collaborator

pkarw commented Apr 12, 2019

@AndreiBelokopytov thanks for Your insights! Could you provide the fix in a form of Pull Request please?

@AndreiBelokopytov
Copy link
Contributor

Sure, I will do it

@lukeromanowicz lukeromanowicz self-assigned this Apr 12, 2019
@pkarw pkarw added this to the 1.10.0-rc.1 milestone Apr 16, 2019
@AndreiBelokopytov
Copy link
Contributor

Hmm ... I looked at it again and thought that the problem is more complicated.
For now request will be cached for future attempt if server didn't return HTTP status 200.
It's ok for statuses >= 500, because we need to retry request if the problem is on the server, but it is wrong for statuses from 400 to 499 (client error). If something is wrong with request itself we should not retry it!
In this particular case we have invalid order data on the client and from my point of view server should return 400 status, but instead it returns 500 status. So API response is the first thing to change and the second thing to change is placeOrder action in VS to skip caching requests in case of 400-499 status.
Finally:

  • a hotfix for this issue may be realized as disabling request retry for all incorrect (not 200) statuses (which may be not the ideal solution). I described this solution in my previous comment.
  • more correct solution may be to change the API to return correct status (400) for invalid order and to disable retry only fo requests where 400-499 status was received.

Which will be better to implement now?

@pkarw
Copy link
Collaborator

pkarw commented Apr 17, 2019

I belive we should do both: changing error statuses inside Vue-storefront-api. Add general workaround - hotifx to not queue orders in case of http error other than 200 (we should use the queue only for network outage / offline mode - in this case there is no http error at all rather there we have a fetch exception)

@lukeromanowicz
Copy link
Contributor Author

I believe that #2753 would help a lot in this case.

@pkarw
Copy link
Collaborator

pkarw commented Apr 25, 2019

Somehow related to #2740

@lukeromanowicz lukeromanowicz modified the milestones: 1.10.0-rc.1, 1.9.0 Apr 25, 2019
@patzick patzick added the QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. label Apr 26, 2019
@patzick patzick closed this as completed Apr 26, 2019
@alinadivante alinadivante added QA approved after merge Testers will add this label after positive check on merged changes and removed QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. labels May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1: Easy bug Bug reports good first issue Tasks that requires just basic understanding of Vue Storefront P1: Urgent Priority mark - high priority QA approved after merge Testers will add this label after positive check on merged changes vs-hackathon Tasks for the Hackathon
Projects
None yet
Development

No branches or pull requests

5 participants