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

[EASI-3944] Notifications - On Discussion Replied Notification #990

Merged
merged 16 commits into from
Mar 14, 2024
Merged

[EASI-3944] Notifications - On Discussion Replied Notification #990

merged 16 commits into from
Mar 14, 2024

Conversation

OddTomBrooks
Copy link
Contributor

@OddTomBrooks OddTomBrooks commented Mar 6, 2024 •
edited
Loading

EASI-3944

Changes and Description

  • Created new notification meta structure
  • Updated and generated GQL
  • Updated plan discussion routing to gate sending reply emails based on preferences
  • Updated plan discussion to send reply notifications based on preferences

How to test this change

  • Start the application
  • Using Postman:
    • Create a default model plan
    • Create a default discussion
    • Create a default discussion reply
    • Query notifications
  • Assert postman results are as expected
  • Assert unit tests pass as expected

PR Author Review Checklist

  • Met the ticket's acceptance criteria, or will meet them in a subsequent PR.
  • Added or updated tests for backend resolvers or other functions as needed.
  • Added or updated client tests for new components, parent components, functions, or e2e tests as necessary.
  • Updated the Postman Collection if necessary.

PR Reviewer Guidelines

  • It's best to pull the branch locally and test it, rather than just looking at the code online!
  • Check that all code is adequately covered by tests - if it isn't feel free to suggest the addition of tests.
  • Always make comments, even if it's minor, or if you don't understand why something was done.

StevenWadeOddball
StevenWadeOddball reviewed Mar 6, 2024
View reviewed changes
Copy link
Contributor

@StevenWadeOddball StevenWadeOddball left a comment

Choose a reason for hiding this comment

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

Thanks @OddTomBrooks, this looks like you are on the right track!

I added some more notes as you continue to finalize the implementation.

The other thing not called out is just updating postman as well.

pkg/notifications/new_discussion_replied_meta.go Outdated Show resolved Hide resolved
pkg/notifications/new_discussion_replied_meta.go Show resolved Hide resolved
pkg/notifications/new_discussion_replied_meta_test.go Show resolved Hide resolved
pkg/models/new_discussion_replied_meta.go Show resolved Hide resolved
pkg/graph/resolvers/plan_discussion.go Outdated Show resolved Hide resolved
pkg/graph/resolvers/plan_discussion.go Outdated Show resolved Hide resolved
pkg/graph/resolvers/plan_discussion.go Show resolved Hide resolved
OddTomBrooks and others added 10 commits March 12, 2024 15:10
@OddTomBrooks
wip: implementing PR feedback
0aa147b
@OddTomBrooks
wip: implement various PR feedback
7c03b21
@OddTomBrooks
wip: corrected actor ID to be reply creator ID and added discussion c…
27f21d5 
…reator ID to meta
@OddTomBrooks
fix: corrected notification recipient from actor to discussion creator
e749625
@OddTomBrooks
feat: testing preview for discussion reply notification
e89ad21
@OddTomBrooks
Merge branch 'main' into EASI-3944/on_discussion_replied_notification
892db93 
# Conflicts:
#	pkg/graph/resolvers/activity.resolvers.go
#	pkg/graph/resolvers/plan_discussion.go
@OddTomBrooks
chore: updated postman collection
64fa544
@OddTomBrooks
Merge branch 'main' into EASI-3944/on_discussion_replied_notification
303f9af
@OddTomBrooks
fix: corrected activity meta base struct type
528020d
@OddTomBrooks
Merge branch 'EASI-3944/on_discussion_replied_notification' of github…
314f6aa 
….com:CMSgov/mint-app into EASI-3944/on_discussion_replied_notification
@OddTomBrooks OddTomBrooks marked this pull request as ready for review March 14, 2024 16:10
@OddTomBrooks OddTomBrooks requested review from a team as code owners March 14, 2024 16:10
@OddTomBrooks OddTomBrooks requested review from StevenWadeOddball and garyjzhao and removed request for a team March 14, 2024 16:10
@StevenWadeOddball
Copy link
Contributor

Review is in progress still. @OddTomBrooks, can you target the feature branch here please? This shouldn't merge to main until the frontend is built out.

StevenWadeOddball
StevenWadeOddball requested changes Mar 14, 2024
View reviewed changes
Copy link
Contributor

@StevenWadeOddball StevenWadeOddball left a comment

Choose a reason for hiding this comment

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

Thanks @OddTomBrooks, this is almost there!

I functionally tested the change with postman. I confirmed that a notification is created as expected, and that the email respects the user preference to only send an email when it is activated.

pkg/notifications/new_discussion_replied_meta_test.go Outdated Show resolved Hide resolved
pkg/notifications/new_discussion_replied_meta_test.go Show resolved Hide resolved
pkg/graph/resolvers/plan_discussion.go Outdated Show resolved Hide resolved
pkg/graph/resolvers/activity.resolvers.go Outdated Show resolved Hide resolved
pkg/notifications/notifications_test.go Show resolved Hide resolved
@OddTomBrooks OddTomBrooks changed the base branch from main to EASI-4023/fe_on_discussion_reply_notification March 14, 2024 18:38
@OddTomBrooks
Copy link
Contributor Author

OddTomBrooks commented Mar 14, 2024 •
edited
Loading

Thanks @OddTomBrooks, this is almost there!

I functionally tested the change with postman. I confirmed that a notification is created as expected, and that the email respects the user preference to only send an email when it is activated.

Thank you for the thorough review. All of your requests are in and ready for re-review. A re-review request has also been sent.

StevenWadeOddball
StevenWadeOddball approved these changes Mar 14, 2024
View reviewed changes
Copy link
Contributor

@StevenWadeOddball StevenWadeOddball left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes @OddTomBrooks, this looks good to go.

@OddTomBrooks OddTomBrooks merged commit d94a42c into EASI-4023/fe_on_discussion_reply_notification Mar 14, 2024
11 checks passed
@OddTomBrooks OddTomBrooks deleted the EASI-3944/on_discussion_replied_notification branch March 14, 2024 21:46
garyjzhao pushed a commit that referenced this pull request Mar 28, 2024
@OddTomBrooks @garyjzhao
[EASI-3944] Notifications - On Discussion Replied Notification (#990)
42f9f71 
* wip: draft of sending notifications on discussion reply

* wip: implementing PR feedback

* wip: implement various PR feedback

* wip: corrected actor ID to be reply creator ID and added discussion creator ID to meta

* fix: corrected notification recipient from actor to discussion creator

* feat: testing preview for discussion reply notification

* chore: updated postman collection

* fix: corrected activity meta base struct type

* chore: added comment to helper function

* chore: deleted unused generated resolver

* fix: revereted incorrectly changed user ID assignment

* removed irrelevant comment

* chore: added more assertions to unit test
garyjzhao added a commit that referenced this pull request Mar 29, 2024
@garyjzhao @OddTomBrooks
[EASI-4023] New Discussion Reply Notification (#1024)
1f216b7 
* [EASI-3944] Notifications - On Discussion Replied Notification (#990)

* wip: draft of sending notifications on discussion reply

* wip: implementing PR feedback

* wip: implement various PR feedback

* wip: corrected actor ID to be reply creator ID and added discussion creator ID to meta

* fix: corrected notification recipient from actor to discussion creator

* feat: testing preview for discussion reply notification

* chore: updated postman collection

* fix: corrected activity meta base struct type

* chore: added comment to helper function

* chore: deleted unused generated resolver

* fix: revereted incorrectly changed user ID assignment

* removed irrelevant comment

* chore: added more assertions to unit test

* Add new reply type guard

* add more conditional renders based on type guard

* Add the new discussion reply gql fragment

* implementing new discussion reply into the UI

* Add a few more queries to the getNotification query

* Notification content to show on discussions

* add model name to util

* move notification settings to its own test

* add e2e test

---------

Co-authored-by: Tom Brooks <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StevenWadeOddball StevenWadeOddball StevenWadeOddball approved these changes

@garyjzhao garyjzhao Awaiting requested review from garyjzhao garyjzhao was automatically assigned from CMSgov/mint-frontend

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

Successfully merging this pull request may close these issues.
2 participants
@OddTomBrooks @StevenWadeOddball