Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: send workspace create/update notifications to template admins only#16071

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

Conversation

DanielleMaywood
Copy link
Contributor

Relates to#15845

Rather than sending the notification to the user, we send it to the template admins. We also do not send it to the person that created the request.

Copy link
Contributor

@dannykoppingdannykopping left a comment

Choose a reason for hiding this comment

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

I'm nervous about this notification being enabled by default.

It will getvery noisy in large environments, and will be fairly low signal. Only if an adminexplicitly wants to add an integration should they enable this, and likely with webhooks only.

WDYT@bpmct@DanielleMaywood think about disabling this notification by default?

We don't really have a precedent for this (i.e. disabling a notification by default), but now might be the time to introduce it.
Blowing up the scope is better than blowing up admins' mailboxes, IMHO.

This PR largely LGTM modulo some minor points, but I'm blocking its merging until we come to a conclusion on the above.

@DanielleMaywood
Copy link
ContributorAuthor

WDYT@bpmct@DanielleMaywood think about disabling this notification by default?

I'm in two minds about this:

  • If an operator is unhappy about the volume they could always disable it when/if it becomes an issue.
  • We should aim to have nice defaults, so disabling it by default avoids this unhappy path.

@dannykopping
Copy link
Contributor

I'm a big fan ofPrinciple of Least Surprise. I think for existing customers who upgrade to a more recent version and now start getting hundreds of emails they didn't expect violates that principle.

DanielleMaywood reacted with thumbs up emoji

@bpmct
Copy link
Member

bpmct commentedJan 9, 2025
edited
Loading

Hey folks - apologies for the confusion here. Let's disable this notification by default both for users and admins. In fact, we should probably not make it available for users to configure. The second point is less important to me, but I just can't think of a good use case for it.

Some of these notifications are really interesting from a webhook POV (fails, creates/updates to an external system) but emails themselves with no filtering would create a lot of spam.

@dannykopping
Copy link
Contributor

Let's disable this notification by default both for users and admins.

Sounds good 👍 thanks@bpmct - and no need to apologise!

In fact, we should probably not make it available for users to configure.

Fortunately, regular (unprivileged) users will only be able to modify their own workspaces, and we're now inhibiting these notifications if the initiator is owner of the workspace; so we won't have to build in this out.

DanielleMaywood added a commit that referenced this pull requestJan 13, 2025
Change as part of#16071It has been decided that we want to be able to have some notificationtemplates be disabled _by default_#16071 (comment).This adds a new column (`enabled_by_default`) to`notification_templates` that defaults to `TRUE`. It also modifies the`inhibit_enqueue_if_disabled` function to reject notifications fortemplates that have `enabled_by_default = FALSE` with the user notexplicitly enabling it.
Copy link
Contributor

@dannykoppingdannykopping left a comment

Choose a reason for hiding this comment

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

LGTM

@DanielleMaywoodDanielleMaywood merged commit3e3de05 intomainJan 15, 2025
31 checks passed
@DanielleMaywoodDanielleMaywood deleted the dm-send-workspace-created-update-notifications-to-template-admins-only branchJanuary 15, 2025 17:43
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJan 15, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@dannykoppingdannykoppingdannykopping approved these changes

@defelmnqdefelmnqAwaiting requested review from defelmnq

Assignees

@DanielleMaywoodDanielleMaywood

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@DanielleMaywood@dannykopping@bpmct

[8]ページ先頭

©2009-2025 Movatter.jp