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

Cleanse JS errors from DOM on Stripe failure #11915

Merged

Conversation

murjax
Copy link
Contributor

@murjax murjax commented Dec 9, 2023

What? Why?

This change removes the [object Object]true output from the bottom of the page that appears when Stripe payments fail.

This output appears to be generated when the Angular resource code parses the response. Upon inspecting the backend response, it does not contain this output. Angular is likely expecting a JSON response but the endpoint returns an HTML response. As such, the response object is an object mapping each character to its own key like this:

{ 0: "<", 1: "!", 2: "D", 3: "O", 4: "C", 5: "T", 6: "Y", 7: "P", 8: "E", 9: " ", … }

The [object Object]true output is observed upon parsing this response object. This change simply removes this substring from the final HTML.

What should we test?

  1. Sign in as a hub
  2. Add a Stripe payment to an order: use the card 4100000000000019.
  3. You should see the flash message introduced in Fix Stripe payment flash rendering issue #11730.
  4. The output [object Object]true does not show on the bottom left corner of the screen.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@murjax murjax force-pushed the cleanse-stripe-js-errors-11886 branch from ba3b683 to 1251b3d Compare December 11, 2023 12:23
@murjax murjax force-pushed the cleanse-stripe-js-errors-11886 branch from 1251b3d to b84707e Compare December 11, 2023 12:46
@murjax murjax marked this pull request as ready for review December 11, 2023 13:47
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Thanks, this removes a useless message from the view, so seems worthwhile.

However it doesn't solve the root problem. It looks like Angular is sending a request for JSON (https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/assets/javascripts/admin/resources/resources/payment_resource.js.coffee#L2), and is receiving HTML, so can we fix that instead?

Hrmm, I think it's calling this controller, which doesn't seem to support JSON at all, and redirects to another one. https://github.com/openfoodfoundation/openfoodnetwork/blob/ae04716391b755da6fcbed737e61961f55b6897b/app/controllers/spree/admin/payments_controller.rb

Would you be up for looking into this?

Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Nice, like @dacook said we should fix underlying issue, so that angular doesn't request/expect JSON.

@murjax
Copy link
Contributor Author

murjax commented Dec 16, 2023

Thanks, this removes a useless message from the view, so seems worthwhile.

However it doesn't solve the root problem. It looks like Angular is sending a request for JSON (https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/assets/javascripts/admin/resources/resources/payment_resource.js.coffee#L2), and is receiving HTML, so can we fix that instead?

Hrmm, I think it's calling this controller, which doesn't seem to support JSON at all, and redirects to another one. https://github.com/openfoodfoundation/openfoodnetwork/blob/ae04716391b755da6fcbed737e61961f55b6897b/app/controllers/spree/admin/payments_controller.rb

Would you be up for looking into this?

@dacook Yes definitely! This should be refactored to support JSON across requests. This will take some time but I'm happy to look into this.

@drummer83 drummer83 self-assigned this Dec 18, 2023
@drummer83 drummer83 added the pr-staged-uk staging.openfoodnetwork.org.uk label Dec 18, 2023
@drummer83
Copy link
Contributor

Hello all,
I started testing this - at least I reproduced the issue before staging the PR.
Then I read the latest comments and I was surprised that this PR is in Test Ready. I have the impression that the solution should be reworked and the PR should be in In Dev - or has this already happened?

Ping @murjax, @dacook, @rioug.

@drummer83
Copy link
Contributor

From a user perspective the PR is solving the issue though. Let me know how to proceed, please.

BEFORE staging this PR:
image
image

AFTER staging this PR:
image
image

@drummer83 drummer83 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Dec 18, 2023
@drummer83 drummer83 removed their assignment Dec 18, 2023
@murjax
Copy link
Contributor Author

murjax commented Dec 19, 2023

@drummer83 My interpretation of the above discussion is that we should deploy this so the message is removed, but the page should be reworked separately as this fix is simply covering up the bug. Reworking the page will be more involved and take some time. I'm happy to discuss further if I missed anything.

@drummer83
Copy link
Contributor

I was wondering because @dacook wrote 'instead'. Let's wait for his response after Christmas. 🎅

However it doesn't solve the root problem. It looks like Angular is sending a request for JSON (https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/assets/javascripts/admin/resources/resources/payment_resource.js.coffee#L2), and is receiving HTML, so can we fix that instead?

@dacook
Copy link
Member

dacook commented Dec 28, 2023

Hi Konrad, sorry I wasn't clear in my last statement. Thanks for testing, which confirms that this makes an improvement, so we can merge this.

@dacook dacook merged commit 263f68f into openfoodfoundation:master Dec 28, 2023
54 checks passed
@dacook
Copy link
Member

dacook commented Dec 28, 2023

@murjax thanks for being willing to look into the underlying issue. Coming back to this, I have a different perspective. We intend to replace the Angular implementation in the long run, so it might not be worth investing in further fixes.
From from the user point of view, it looks like everything is now working ok (the error message is correctly displayed), so I would suggest to see if there's another issue you'd like to work on instead.

Thanks for your contribution!

@drummer83 drummer83 added no-staging-AU A tag which does not trigger deployments, indicating a server is being used and removed feedback-needed no-staging-AU A tag which does not trigger deployments, indicating a server is being used labels Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BO Stripe payments] javascript variable appearing on screen
5 participants