- Notifications
You must be signed in to change notification settings - Fork927
feat: allow notification templates to be disabled by default#16093
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
feat: allow notification templates to be disabled by default#16093
Uh oh!
There was an error while loading.Please reload this page.
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.
LGTM modulo a couple nits
Could you please deploy this to the preview env? I'd like to play around with the frontend.
| -- Disable 'workspace created' notification by default | ||
| UPDATE notification_templates | ||
| SET enabled_by_default = FALSE | ||
| WHERE id = '281fdf73-c6d6-4cbb-8ff5-888baf8a2fff'; | ||
| -- Disable 'workspace manually updated' notification by default | ||
| UPDATE notification_templates | ||
| SET enabled_by_default = FALSE | ||
| WHERE id = 'd089fe7b-d5c5-4c0c-aaf5-689859f7d392'; |
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.
Nit: this might've been better suited in its own migration.
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.
Can do! 👍
| AND (EXISTS (SELECT 1 | ||
| FROM notification_templates | ||
| WHERE id = NEW.notification_template_id | ||
| AND enabled_by_default = FALSE)) THEN |
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.
Nit: could this not be anOR on the other branch?
If we're gonna keep this separate branch I'd rather we raised a more specific error since this one is no longer accurate.
Perhapsnotification is not enabled covers both.
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.
Can do!
So make this anOR to the other branch and rename the error message tonotification is not enabled?
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.
Thanks! Yes please, I think that'd be a more precise error message
github-actionsbot commentedJan 13, 2025 • 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.
✔️ PR 16093 Updated successfully. |
| RAISE EXCEPTION 'cannot enqueue message: user has disabled this notification'; | ||
| END IF; | ||
| -- Fails if the notification template is disabled by default and the |
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.
pinging@mafredri to review SQL changes
| -- Fail the insertion if one of the following: | ||
| -- * the user has disabled this notification. | ||
| -- * the notification template is disabled by default and hasn't | ||
| -- been explicitly enabled by the user. | ||
| IF EXISTS (SELECT 1 | ||
| FROM notification_preferences | ||
| WHERE disabled = TRUE | ||
| AND user_id = NEW.user_id | ||
| AND notification_template_id = NEW.notification_template_id) | ||
| OR (NOT EXISTS (SELECT 1 | ||
| FROM notification_preferences | ||
| WHERE disabled = FALSE | ||
| AND user_id = NEW.user_id | ||
| AND notification_template_id = NEW.notification_template_id)) | ||
| AND (EXISTS (SELECT 1 | ||
| FROM notification_templates | ||
| WHERE id = NEW.notification_template_id | ||
| AND enabled_by_default = FALSE) ) THEN | ||
| RAISE EXCEPTION 'cannot enqueue message: notification is not enabled'; | ||
| END IF; |
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.
How about simplifying this with a join instead?
| -- Fail the insertion if one of the following: | |
| -- * the user has disabled this notification. | |
| -- * the notification template is disabled by default and hasn't | |
| -- been explicitly enabled by the user. | |
| IF EXISTS (SELECT1 | |
| FROM notification_preferences | |
| WHERE disabled= TRUE | |
| AND user_id=NEW.user_id | |
| AND notification_template_id=NEW.notification_template_id) | |
| OR (NOT EXISTS (SELECT1 | |
| FROM notification_preferences | |
| WHERE disabled= FALSE | |
| AND user_id=NEW.user_id | |
| AND notification_template_id=NEW.notification_template_id)) | |
| AND (EXISTS (SELECT1 | |
| FROM notification_templates | |
| WHERE id=NEW.notification_template_id | |
| AND enabled_by_default= FALSE) ) THEN | |
| RAISE EXCEPTION'cannot enqueue message: notification is not enabled'; | |
| END IF; | |
| IF EXISTS ( | |
| SELECT1 | |
| FROM notification_templates nt | |
| LEFT JOIN notification_preferences np | |
| ONnp.notification_template_id=nt.id | |
| ANDnp.user_id=NEW.user_id | |
| WHEREnt.id=NEW.notification_template_id | |
| AND ( | |
| -- Case 1: The user explicitly disabled this template | |
| np.disabled= TRUE | |
| OR | |
| -- Case 2: Template is disabled by default | |
| -- AND user never explicitly enabled it | |
| (np.notification_template_id ISNULLANDnt.enabled_by_default= FALSE) | |
| ) | |
| ) | |
| THEN | |
| RAISE EXCEPTION'cannot enqueue message: notification is not enabled'; | |
| END IF; |
I feel this is a bit more readable and concise. What do you think? (PS. Feel free to double check the logic, I actually used ChatGPT to rewrite it.)
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 think I agree with you, this definitely feels easier (at least for me) to read
009069c intomainUh oh!
There was an error while loading.Please reload this page.
Change as part of#16071
It has been decided that we want to be able to have some notification templates be disabledby default#16071 (comment).
This adds a new column (
enabled_by_default) tonotification_templatesthat defaults toTRUE. It also modifies theinhibit_enqueue_if_disabledfunction to reject notifications for templates that haveenabled_by_default = FALSEwith the user not explicitly enabling it.