-
Notifications
You must be signed in to change notification settings - Fork 21
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
[BIOMAGE-1747] Expired token #335
Conversation
Codecov Report
@@ Coverage Diff @@
## master #335 +/- ##
==========================================
+ Coverage 83.98% 83.99% +0.01%
==========================================
Files 126 126
Lines 3503 3506 +3
Branches 392 393 +1
==========================================
+ Hits 2942 2945 +3
Misses 512 512
Partials 49 49
Continue to review full report at Codecov.
|
c60b133
to
b65c6ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the console logs and the commented out line and it's good to merge. Are these storks (the cute animals photo)?
a47845e
to
7535a61
Compare
I assume they are haha japanese ones maybe |
Description
The issue was caused by the
sendNotification
hook. This hook uses thejwtToken
to get the current user information to know who to send the email notifications too (and also to add the user name to our slack notifications). Whenever, the token was expired, the hookRunner failed, crashing the qc|gem2s response handlers.Changes
sendNotification
now ignores if thejwtToken
is expired because it only cares about the user infohookRunner.run()
runs inside a try/catch to prevent any hook problem from propagating and interrupting the original calling function (hooks are by definition side-effects, so any error should not stop the main program).Details
URL to issue
https://biomage.atlassian.net/browse/BIOMAGE-1747
Link to staging deployment URL (or set N/A)
N/A
Links to any PRs or resources related to this PR
Integration test branch
master
Merge checklist
Your changes will be ready for merging after all of the steps below have been completed.
Code updates
Have best practices and ongoing refactors being observed in this PR
Manual/unit testing
Integration testing
You must check the box below to run integration tests on the latest commit on your PR branch.
Integration tests have to pass before the PR can be merged. Without checking the box, your PR
will not pass the required status checks for merging.
Documentation updates
Optional