Skip to content
Snippets Groups Projects

Allow email notifications to be disabled for all users of a group or project

Closed Dustin Spicuzza requested to merge virtuald/gitlab-ce:disable-notification-emails into master
8 unresolved threads

This MR is being continued in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/30755

What does this MR do?

Allows administrators of groups/projects to disable email notifications on a per-group or per-project basis, regardless of the users settings

Note that this is only labeled WIP because it lacks tests. It works and is functional in my local GDK environment. Additionally, have been using this in two 10.x production sites since the beginning of July.

Are there points in the code the reviewer needs to double check?

I don't really know ruby (though, I did test this in a GDK environment). I looked briefly at writing tests, and it's a bit confusing. I'd like to make sure that this MR will actually be accepted before taking the time to write tests.

Why was this MR needed?

Some organizations may not want any code or data going out over email.

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/50020

Edited by Brett Walker

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
10 10 %br/
11 11 %span.descr This setting can be overridden in each project.
12 12
13 .form-group.row
14 = f.label :emails_enabled, 'Email Notifications', class: 'col-form-label col-sm-2'
15 .col-sm-10
16 .form-check
17 = f.check_box :emails_enabled, class: 'form-check-input'
18 = f.label :emails_enabled, class: 'form-check-label' do
19 %strong
20 Allow projects within this group to send email notifications
  • 10 10 %br/
    11 11 %span.descr This setting can be overridden in each project.
    12 12
    13 .form-group.row
    14 = f.label :emails_enabled, 'Email Notifications', class: 'col-form-label col-sm-2'
    15 .col-sm-10
    16 .form-check
    17 = f.check_box :emails_enabled, class: 'form-check-input'
    18 = f.label :emails_enabled, class: 'form-check-label' do
    19 %strong
    20 Allow projects within this group to send email notifications
    21 %br/
    22 %span.descr If disabled, this cannot be overridden by individual projects.
    23
  • 162 162 Gitlab.config.lfs.enabled
    163 163 end
    164 164
    165 def emails_enabled?
  • Jan Provaznik
  • 210 210 expose :only_allow_merge_if_pipeline_succeeds
    211 211 expose :request_access_enabled
    212 212 expose :only_allow_merge_if_all_discussions_are_resolved
    213 expose :emails_enabled
  • @gl-database: could you please review the migration in this MR?

  • Thanks @virtuald, this looks great! I left some comments inline, please let me know WDYT?

    Could you please also rebase the MR against current master? We will also need test coverage for this cool feature.

  • assigned to @virtuald

  • 144 144 :avatar,
    145 145 :description,
    146 146 :lfs_enabled,
    147 :emails_enabled,
  • 328 328 :container_registry_enabled,
    329 329 :default_branch,
    330 330 :description,
    331 :emails_enabled,
  • @jprovaznik Thanks for the comments.

    10 months ago I had plenty of time to fix this up. Unfortunately, that isn't the case at this present moment. Given that the current MR meets our needs, I don't forsee much time freeing up for this before September.

  • 1 ---
    2 title: Allow project administrator to disable email notifications for all users of
    3 a group or project
    4 merge_request:
  • 1 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
    2 # for more information on how to write migrations for GitLab.
    3
    4 class AddEmailEnabled < ActiveRecord::Migration
    5 include Gitlab::Database::MigrationHelpers
    6
    7 # Set this constant to true if this migration requires downtime.
    8 DOWNTIME = false
    9
    10 disable_ddl_transaction!
    11
    12 def up
    13 add_column_with_default :projects, :emails_enabled, :boolean, default: true, allow_null: false
    • Each of those add_column_with_default should go in their own migration. This is because we disable transactions and in case one step fails, we would have left-overs and cannot retry the migration before cleaning them up manually.

    • Please register or sign in to reply
  • 10 months ago I had plenty of time to fix this up. Unfortunately, that isn't the case at this present moment. Given that the current MR meets our needs, I don't forsee much time freeing up for this before September.

    @jacopo-beschi I completely understand. I'm sorry there was no quicker feedback from GitLab side in this case - I think this MR just slipped reviewers' radar until recently when ~"ready for review" label was added.

    Let's keep ~"Accepting merge requests" on the linked issue if someone else wants to work on this meantime. Thanks again for working on this :thumbsup:

  • @virtuald if you don't mind, I'm going to be taking this MR over and work on getting it into master, or a variant of it. Since it's on your own fork, I will end up creating a new MR off of master and shifting the code there so it's easier for me to work on.

    Since last I read you had been running it for awhile in production, have there been any problems or concerns you've run up against?

  • @digitalmoksha we're still running this in production, and I'm not aware of any issues that have come up. Feel free to take this MR over.

  • Brett Walker mentioned in merge request !30755 (merged)

    mentioned in merge request !30755 (merged)

  • Brett Walker changed the description

    changed the description

  • closed

  • mentioned in issue gitlab#34141

  • Please register or sign in to reply
    Loading