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.

Sign up for GitHub

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

Jump to bottom

PIMS-1928: Cancel and resend notifications #2616

Merged
merged 10 commits into from
Aug 12, 2024
Merged

PIMS-1928: Cancel and resend notifications #2616

merged 10 commits into from
Aug 12, 2024

Conversation

GrahamS-Quartech
Copy link
Contributor

@GrahamS-Quartech GrahamS-Quartech commented Aug 9, 2024 •
edited
Loading

🎯 Summary

PIMS-1928

  • Adds new controllers and services for resending and cancelling notifications
  • Adds new frontend hooks for resending and cancelling notifications
  • Added buttons to the notification table on the project detail for resending or cancelling projects.

Note:

  • The cancel button is greyed out for notifications that are already cancelled or completed, obviously cancelling them doesn't do anything.
  • Resending a cancelled or completed notification does work, but the function to refresh notifications in getNotificationByProjectId is hardcoded to not update notifications in these statuses. Should we loosen that restriction? EDIT: This is still true, but I added a call to update just the single notification in the resend endpoint, so this change will be reflected in the frontend table.

🔰 Checklist

  • I have read and agree with the following checklist and am following the guidelines in our Code of Conduct document.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation where required.
  • I have tested my changes to the best of my ability.
  • My changes generate no new warnings.

@GrahamS-Quartech GrahamS-Quartech changed the title PIMS-1928: Cancel resend notifications PIMS-1928: Cancel and resend notifications Aug 9, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 9, 2024

🚀 Deployment Information

The Express API Image has been built with the tag: 2616. Please make sure to utilize this specific tag when promoting these changes to the TEST and PROD environments during the API deployment. For more updates please monitor Image Tags Page on Wiki.

@codeclimate CodeClimate
Copy link

codeclimate bot commented Aug 9, 2024 •
edited
Loading

Code Climate has analyzed commit 7b1bca2 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 93.8%.

View more on Code Climate.

@GrahamS-Quartech
Now updates notification status after resend to make sure a change is…
949b3f5 
… reflected in the frontend table. Updated unit tests accordingly.
@GrahamS-Quartech GrahamS-Quartech marked this pull request as ready for review August 12, 2024 18:48
dbarkowsky
dbarkowsky reviewed Aug 12, 2024
View reviewed changes
Copy link
Collaborator

@dbarkowsky dbarkowsky left a comment

Choose a reason for hiding this comment

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

Getting this error regarding the ProjectNotificationsTable:
image

Trying to cancel a notification, it returns a status 400 with a NotificationQueue object.
image

The resend option is working as expected.

I wonder if we can't condense the buttons down to an icon. We're so tight for space to begin with.

express-api/src/controllers/notifications/notificationsController.ts Outdated
return res.status(404).send('Notification not found.');
}
const kcUser = req.user;
if (!(isAdmin(kcUser) || isAuditor(kcUser))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If they are not an admin, should we even allow them to trigger this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, I wasn't sure how strict this should be so I just applied similar logic as elsewhere.

express-api/src/controllers/notifications/notificationsController.ts Outdated
return res.status(404).send('Notification not found.');
}
const kcUser = req.user;
if (!(isAdmin(kcUser) || isAuditor(kcUser))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question about admin-only logic.

express-api/src/routes/notificationsRouter.ts Show resolved Hide resolved
@GrahamS-Quartech
Copy link
Contributor Author

Getting this error regarding the ProjectNotificationsTable:

I don't believe this is introduced with this ticket but I will see about fixing it regardless.

I wonder if we can't condense the buttons down to an icon. We're so tight for space to begin with.

I think this is a good idea, I'll choose some appropriate icons.

@GrahamS-Quartech
Must be admin to cancel or resend. Changed textual buttons to icon bu…
3c00710 
…ttons with tooltips to save some horizontal real estate.
dbarkowsky
dbarkowsky approved these changes Aug 12, 2024
View reviewed changes
Copy link
Collaborator

@dbarkowsky dbarkowsky left a comment

Choose a reason for hiding this comment

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

Buttons are working well for me now. Like the look of the icons chosen.

@GrahamS-Quartech GrahamS-Quartech merged commit 503cbf5 into main Aug 12, 2024
13 checks passed
@GrahamS-Quartech GrahamS-Quartech deleted the PIMS-1928-CancelResendNotifications branch August 12, 2024 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbarkowsky dbarkowsky dbarkowsky approved these changes

@LawrenceLau2020 LawrenceLau2020 Awaiting requested review from LawrenceLau2020 LawrenceLau2020 is a code owner

@Sharala-Perumal Sharala-Perumal Awaiting requested review from Sharala-Perumal Sharala-Perumal is a code owner

@TaylorFries TaylorFries Awaiting requested review from TaylorFries TaylorFries is a code owner

@ManishSihag ManishSihag Awaiting requested review from ManishSihag ManishSihag is a code owner

Assignees
No one assigned
Labels
Express
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@GrahamS-Quartech @dbarkowsky