Skip to content

Conversation

@JPrevost
Copy link
Member

What:

  • This presents an error message to users when things go awry during the
    XHR requests
  • This does not try to get fancy and report errors from the client side,
    but instead assumes the errors would have been logged during the rails
    server side processing (which is probably true)
  • It does log to the end user's JavaScript console the xhr response when
    this is triggered so in theory we could work with tech savvy users to
    get a copy of the request if necessary (probably not)
  • This also adds a timeout of 6 seconds to the EDS API calls and logs
    all URLs to EDS for debug purposes
  • Disables rollbar and enables more logging in dev environment

Why:

  • The previous fail state was an infinitely spinning loader image which
    was obviously not too helpful

Note:

Disables rollbar and enables more logging
What:

* This presents an error message to users when things go awry during the
  XHR requests
* This does not try to get fancy and report errors from the client side,
  but instead assumes the errors would have been logged during the rails
  server side processing (which is probably true)
* It does log to the end user's JavaScript console the xhr response when
  this is triggered so in theory we could work with tech savvy users to
  get a copy of the request if necessary (probably not)
* This also adds a timeout of 6 seconds to the EDS API calls and logs
  all URLs to EDS for debug purposes

Why:

* The previous fail state was an infinitely spinning loader image which
  was obviously not too helpful

Note:
* see also https://mitlibraries.atlassian.net/browse/DI-59
@gravesm gravesm temporarily deployed to mit-bento-staging-pr-103 January 24, 2017 16:29 Inactive
@JPrevost
Copy link
Member Author

@matt-bernhardt I flagged you for review as this affects the GA logging you did. I think I didn't break it, but I also didn't check ;)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 6bb68bb on di59_result_error_handling into 8b7bd75 on master.

@frrrances
Copy link
Contributor

UX lgtm 👍

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 58521f6 on di59_result_error_handling into 8b7bd75 on master.

Copy link
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

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

The deployed branch still records clicked results for the panels that do work, so this doesn't break what's already there.

Seeing this behavior does point out that we're missing a few non-result links that may need to be recorded - but that's outside the scope of this PR.

:shipit:

@JPrevost JPrevost merged commit 6d03697 into master Jan 24, 2017
@JPrevost JPrevost deleted the di59_result_error_handling branch January 24, 2017 17:05
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.

6 participants