- Notifications
You must be signed in to change notification settings - Fork928
feat: turn off notification via email#14520
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
github-actionsbot commentedSep 1, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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! 🎉
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
@dannykopping I could not see tests related to the Apart from that, the rest of the comments are addressed, please have a look. |
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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"); |
BrunoQuaresmaSep 3, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 believe that moving thedisableTemplate mutation inside thequeries/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.:
constqueryClient=useQueryClient()constdisableMutation=useMutation(disableNotification(userId,queryClient))useEffect(()=>{try{disableMutation.mutateAsync().then(()=>{displaySuccess("...")}).catch(()=>{displayError("...")})}},[...])
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 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 usingStorybook interaction tests on theNotificationsPage.stories.tsx.
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 have verified these scnearios
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@BrunoQuaresma the tests needed to be added toNotificationsPage.stories.tsx right ?
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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. |
3301212 intocoder:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.