-
Notifications
You must be signed in to change notification settings - Fork 347
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 Notification suppression logic #3218
Conversation
✅ Deploy Preview for care-egov-staging ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
LGTM
Kudos, SonarCloud Quality Gate passed!
|
Closing this PR as it is already fixed in PR #3217 |
@cp-Coder I missed your earlier comment, 😅 just noticed it's already fixed, good to close! |
There are minor issues in the way
fireRequest
decides when to suppress error notifications.It was recently changed in da40a71
suppressNotif? : true,
This defines an optional parameter
suppressNotif
which has a type oftrue | undefined
. This is not the intended behavior. It should rather be a parameter namedsuppressNotif
of typeBoolean
which defaults tofalse
.false
because the old behavior of the CARE platform was not to suppress these errors unless explicitly specified. It seems that the below logic does keep that old behavior but the variable usage is confusing.Changed to
suppressNotif = false
if (suppressNotif && error.response) {
This is a confusing use of the variable
suppressNotif
. In this case, ifsuppressNotif
is true (We want to suppress error messages), it will actually end up showing the error messages, and if it is false (we want to show the error messages), it actually hides all the error messages.Changed to
if (!suppressNotif && error.response) {
export const getUserDetails = (username: string, suppress? : true) => {
This again defines an optional parameter
suppress
which has a type ofundefined | true
, instead it should be a Boolean parameter with default value oftrue
Changed to
export const getUserDetails = (username: string, suppress = true) => {