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

BugsnagJS checkout errors #5877

Merged
merged 2 commits into from Aug 21, 2020

Conversation

Matt-Yorkley
Copy link
Contributor

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

Related to #5858

  • Updates BugsnagJS to latest major version. This should bring a number of improvements.
  • Explicitly notifies Bugsnag on checkout errors instead of throwing unhandled errors

@Matt-Yorkley Matt-Yorkley self-assigned this Aug 7, 2020
@Matt-Yorkley Matt-Yorkley added the pr-staged-uk staging.openfoodnetwork.org.uk label Aug 7, 2020
@Matt-Yorkley Matt-Yorkley force-pushed the bugsnagger branch 3 times, most recently from 3bf35f5 to 1123e08 Compare August 7, 2020 12:31
@Matt-Yorkley Matt-Yorkley changed the title Test BugsnagJS error catching Update BugsnagJS Aug 7, 2020
@Matt-Yorkley Matt-Yorkley changed the title Update BugsnagJS BugsnagJS checkout errors Aug 7, 2020
@Matt-Yorkley Matt-Yorkley marked this pull request as ready for review August 7, 2020 17:30
@Matt-Yorkley Matt-Yorkley removed the pr-staged-uk staging.openfoodnetwork.org.uk label Aug 8, 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.

Great!

We need to find ways to test these things automatically soon.

@Matt-Yorkley
Copy link
Contributor Author

I've updated this a bit to include javascript unit tests for the bugsnag calls.

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.
Can you use the PR template? I think we need some notes for the tester and a line for the release notes.

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.

Actually, let me "request changes" - this needs testing notes.
I am not sure we have bugsnagJS in staging, that will need to be setup, should be easy to do.

@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented Aug 17, 2020

It's a tricky one to test. I think it's a dev-test.

  • Put a bugsnag API key in application.yml with BUGSNAG_JS_KEY:
  • Temporarily change line 8 in app/views/layouts/_bugsnag_js.html.haml so the releaseStage is set to staging
  • Create some exceptions in the code to test the responses; an error in checkout_controller.rb is the simple one and then: a handled error in app/assets/javascripts/darkswarm/services/checkout.js.coffee around line 30 and then an unhandled error around line 26...

@Matt-Yorkley Matt-Yorkley added the dev-test A dev need to test this one 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.

🚀

@luisramos0
Copy link
Contributor

ok, I am testing this.

First a simple server side error to set it up:
image

development stage works:
image

@luisramos0
Copy link
Contributor

Testing JS without this PR doesnt get me far. I get Bugsnag object not defined errors.
Anyway I switched to this PR and things look a lot better with the new version and new initializer (the new initializer defines Bugsnag obj, the previous one was definig only the lower case bugsnag object.

With this:
image

I see this:
image

With this on the console:
image

I get this:
image

BUT with this:
image

I dont get any alert!

I think things are much better now with this PR.
The error will now be notified for sure with the explicit call.
BUT I think we are not exactly logging all errors to bugsnag yet....

So, I am merging this PR now. But maybe we leave the issue open?

@luisramos0 luisramos0 merged commit 19b5a00 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
dev-test A dev need to test this one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants