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
lukeromanowicz opened this issue Mar 27, 2019 · 9 comments

Comments

@lukeromanowicz
Copy link
Collaborator

commented Mar 27, 2019

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
@pkarw

This comment has been minimized.

Copy link
Collaborator

commented Mar 27, 2019

@pkarw pkarw added the vs-hackathon label Mar 30, 2019

@pkarw

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

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

@AndreiBelokopytov

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2019

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

@AndreiBelokopytov

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 17, 2019

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

@pkarw

This comment has been minimized.

Copy link
Collaborator

commented Apr 25, 2019

Somehow related to #2740

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.