-
Notifications
You must be signed in to change notification settings - Fork 1
HEEDLS-519 - Change system notification cookie management #570
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
HEEDLS-519 - Change system notification cookie management #570
Conversation
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.
I can't raise against the lines, but does this change not make the SkipNotifications action redundant?
|
In a most cases, SkipNotifications is pretty redundant. But if there are unacknowledged notifications, skipping will reset the expiry date. It will also set the cookie if the user visits the page when no notifications are present, in case the user definitely doesn't want to see any new notifications in the next 24 hours. But these are so minor that we could get rid of them if we think its unnecessary. |
|
It's not doing anything that the else branch isn't doing, and I assume the only page linking to the Skip action is the notification page which will have literally just set it in the else. I think the behaviour you've just described would be unexpected. If a user has no notification and then gets some, I think they should be forced to view them the next time. |
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.
One minor improvement suggestion to avoid double "if" blocks, but no need for rereview :)
| if (unacknowledgedNotifications.Count == 0) | ||
| { | ||
| if (Request.Cookies.HasSkippedNotificationsCookie(adminId)) | ||
| { | ||
| Response.Cookies.DeleteSkipSystemNotificationCookie(); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| Response.Cookies.SetSkipSystemNotificationCookie(adminId, clockService.UtcNow); | ||
| } |
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.
Could this be simplified with:
if (unacknowledgedNotifications.Count > 0) {
Response.Cookies.SetSkipSystemNotificationCookie(adminId, clockService.UtcNow);
} else if (HasCookie) {
Response.Cookies.DeleteSkipSystemNotificationCookie();
}
// continue
JIRA link
https://softwiretech.atlassian.net/browse/HEEDLS-519
Description
Updated the SystemNotificationController so that the cookie set when first visiting the page (Show Later link no longer needs to be clicked, allowing the user to immediately return to the dashboard). Also deleted the cookie when no more unacknowledged notifications exist.
Screenshots
N/A
Developer checks
I have: