feat: turn off notification via email #14520
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
@dannykopping @stirby could you please have a look at this PR |
There was a problem hiding this comment.
This is great work @joobisb 👍
I think we should probably add a frontend test as well, and show a message to the user which template was disabled by highlighting it.
A few notes but nothing major.
Thank you for your contribution! 🎉
site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.tsx
Outdated
Show resolved
Hide resolved
@dannykopping I could not see tests related to the Apart from that, the rest of the comments are addressed, please have a look. |
site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.tsx
Outdated
Show resolved
Hide resolved
@BrunoQuaresma can assist with that, I'm not too familiar with our frontend I'm afraid. |
| @@ -60,6 +62,41 @@ export const NotificationsPage: FC = () => { | |||
| const updatePreferences = useMutation( | |||
| updateUserNotificationPreferences(user.id, queryClient), | |||
| ); | |||
| const [searchParams] = useSearchParams(); | |||
| const templateId = searchParams.get("disabled"); | |||
There was a problem hiding this comment.
I would rename this to something like 'disabledId' to make it clearer about its usage.
| const disableTemplate = async (templateId: string) => { | ||
| try { | ||
| await updatePreferences.mutateAsync({ | ||
| template_disabled_map: { | ||
| [templateId]: true, | ||
| }, | ||
| }); | ||
|
|
||
| const allTemplates = Object.values(templatesByGroup.data ?? {}).flat(); | ||
| const template = allTemplates.find((t) => t.id === templateId); | ||
|
|
||
| if (!template) { | ||
| throw new Error(`Template with ID ${templateId} not found`); | ||
| } | ||
|
|
||
| displaySuccess(`${template.name} notification has been disabled`); | ||
|
|
||
| queryClient.invalidateQueries( | ||
| userNotificationPreferences(user.id).queryKey, | ||
| ); | ||
| } catch (error) { | ||
| console.error(error); | ||
| displayError("Error on disabling notification"); |
There was a problem hiding this comment.
I believe that moving the disableTemplate mutation inside the queries/notifications would improve the readability of the view. This approach aligns with the pattern we've been using in the UI, where all "server data" is handled in the queries module. Eg.:
const queryClient = useQueryClient()
const disableMutation = useMutation(disableNotification(userId, queryClient))
useEffect(() => {
try {
disableMutation
.mutateAsync()
.then(() => {
displaySuccess("...")
}).catch(() => {
displayError("...")
})
}
}, [...])There was a problem hiding this comment.
I need to remember to check for the following scenarios:
-
When a valid template ID is disabled and the request is successful, check if the success message is displayed. Also, ensure that the UI is updated correctly. Verify if the request is being sent properly. Additionally, confirm if the error message is displayed when the request fails.
-
When a not found template ID is disabled, check if an error message is displayed.
You can check how we do that using Storybook interaction tests on the NotificationsPage.stories.tsx.
There was a problem hiding this comment.
I have verified these scnearios
There was a problem hiding this comment.
@joobisb can you add a test please? Manual verification is good but it only helps us know if this works currently, and doesn't catch future degradations.
There was a problem hiding this comment.
@joobisb we are close, we just need to automate these tests using the way I shared before. Thanks for your hard work! 🙏
There was a problem hiding this comment.
@BrunoQuaresma the tests needed to be added to NotificationsPage.stories.tsx right ?
There was a problem hiding this comment.
@joobisb we are close, we just need to automate these tests using the way I shared before. Thanks for your hard work! 🙏
done, please have a look
There was a problem hiding this comment.
@joobisb good work! I just left a few comments but let me know if you need any extra explanation or help. Thank you!
|
@joobisb, thanks for submitting this! The copy looks fine to me. Leaving approval to Danny and Bruno. |
I have addressed the comments. Could you please take a look? |
|
@joobisb great! I will check the chromatic result in a few minutes when it is ready. If everything is okay, I will approve it. ✅ |
|
@joobisb, it seems like the tests you wrote in the storybook are failing. Have you had a chance to check them locally? |
@BrunoQuaresma |
|
@joobisb I think we solved some of those errors last week. Could you please update this branch with the main please? 🙏 |
|
I will try to make some time today to look at the code and see how I can help. |
@BrunoQuaresma I've updated the branch with main |
|
@joobisb we are good! Thanks for all your contributions. I pushed two commits to simplify the effect and messages a bit, and another to fix the stories. Please, let me know if you have any questions about them. |
This PR addresses #14389
As seen in the attached images, users will receive an option saying
Stop receiving emails like this. On clicking, the user will be redirected to notification settings and the template of that particular notification type will be disabled.