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 flash error issues in checkout requests #5906

Merged

Conversation

Matt-Yorkley
Copy link
Contributor

@Matt-Yorkley Matt-Yorkley commented Aug 14, 2020

What? Why?

Closes #5895

Flash error messages created during checkout submission were being persisted in the flash session in unexpected ways, e.g. showing a checkout error message again after navigating away from the checkout page, or showing a message twice.

This was due to saving the errors in the flash session in XHR requests. The session is usually cleared after the subsequent action picks up the flash message, but here we needed to clear it after the current action, as the error is shown immediately via Angular on the checkout page (and shouldn't be persisted after that).

What should we test?

  • Using different test cards with Stripe Connect that should give different errors shows the flash errors as expected, instead of showing the previous error message.
  • Submitting the checkout with a test card that generates an error message and then navigating to the home page does not re-display the flash error on the home page.

Release notes

Improved flash message display on checkout

@Matt-Yorkley Matt-Yorkley self-assigned this Aug 14, 2020
@Matt-Yorkley Matt-Yorkley marked this pull request as draft August 14, 2020 11:53
@Matt-Yorkley Matt-Yorkley added pr-staged-au staging.openfoodnetwork.org.au pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-au staging.openfoodnetwork.org.au labels Aug 14, 2020
@Matt-Yorkley Matt-Yorkley marked this pull request as ready for review August 14, 2020 13:29
@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented Aug 14, 2020

I've tried writing controller tests for this, but the Capybara flash object doesn't seem to behave as expected. We need to check what it looks like after the action has finished, but it doesn't seem to be updated within the controller test. I've tested this in the browser and it's working as intended.

Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

Nice one!

@Matt-Yorkley Matt-Yorkley removed the pr-staged-uk staging.openfoodnetwork.org.uk label Aug 17, 2020
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Yes, man! 💐

@filipefurtad0 filipefurtad0 self-assigned this Aug 21, 2020
@filipefurtad0 filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Aug 21, 2020
@filipefurtad0
Copy link
Contributor

Awesome @Matt-Yorkley !

I tested the two scenarios described in the issue and on your test notes:

  1. Using different test cards with Stripe Connect that should give different errors shows the flash errors as expected, instead of showing the previous error message. - all good now: the displayed messages correspond to the used cards, as in the video below:

no_memory_effect

  1. Submitting the checkout with a test card that generates an error message and then navigating to the home page does not re-display the flash error on the home page. - no error is seen on the homepage, after triggering an error on checkout.

Good to go, thanks for fixing this! 💪

PS: Outside of the scope of this PR, and to be addressed in a separate issue: - I noticed the case in which a memory-effect is visible after correcting the stock in an order, after reduction on checkout as described here still occurs; also, after triggering this flash message and navigating to the homepage the insufficient stock error appears there as well:
image

@filipefurtad0 filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Aug 21, 2020
@luisramos0
Copy link
Contributor

Nice finding Filipe, would be good to get an issue for that last point. We will need a similar PR to this one but on another controller 👌

@luisramos0 luisramos0 merged commit 4a5a6a2 into openfoodfoundation:master Aug 21, 2020
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.

Failed checkout shows incorrect error message
4 participants