Allow email notifications to be disabled for all users of a group or project
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?
-
Changelog entry added, if necessary -
Documentation created/updated -
API support added -
Tests added for this feature/bug - Conform by the code review guidelines
-
Has been reviewed by a UX Designer -
Has been reviewed by a Frontend maintainer -
Has been reviewed by a Backend maintainer -
Has been reviewed by a Database specialist
-
-
Conform by the merge request performance guides -
Conform by the style guides -
If you have multiple commits, please combine them into a few logically organized commits by squashing them -
Internationalization required/considered -
End-to-end tests pass ( package-and-qa
manual pipeline job)
What are the relevant issue numbers?
Merge request reports
Activity
mentioned in issue #36073 (moved)
added 1 commit
- e404a282 - Allow email notifications to be disabled for all users of a group or project
added 1 commit
- 62fc4b9e - Allow email notifications to be disabled for all users of a group or project
added 1 commit
- b332761a - Allow email notifications to be disabled for all users of a group or project
added 1 commit
- 8760ee3d - Allow email notifications to be disabled for all users of a group or project
added 1 commit
- ae2871f4 - Allow email notifications to be disabled for all users of a group or project
added Community contribution label
added 432 commits
-
ae2871f4...52dea971 - 431 commits from branch
gitlab-org:master
- 73a5d75d - Allow email notifications to be disabled for all users of a group or project
-
ae2871f4...52dea971 - 431 commits from branch
added 1 commit
- b5f06bee - Allow email notifications to be disabled for all users of a group or project
added 1374 commits
-
b5f06bee...49d7f92f - 1373 commits from branch
gitlab-org:master
- c10ce068 - Allow email notifications to be disabled for all users of a group or project
-
b5f06bee...49d7f92f - 1373 commits from branch
added 1 commit
- 44ea24b2 - Allow email notifications to be disabled for all users of a group or project
added 625 commits
-
44ea24b2...e659b398 - 624 commits from branch
gitlab-org:master
- b36fa73f - Allow email notifications to be disabled for all users of a group or project
-
44ea24b2...e659b398 - 624 commits from branch
added Plan [DEPRECATED] emails notifications settings labels
I created a new issue which matches what is being implemented in this MR. https://gitlab.com/gitlab-org/gitlab-ce/issues/50020. The previous issue was not exactly the same.
Hi @virtuald,
First of all, thank you for creating a Merge Request to help improve the GitLab product. We are running through old Merge Requests and asking authors to update their Merge Requests
This Merge Request was chosen, as it meets the following criteria:
- Open
- Labelled Community contribution
- No activity in the past 2 months (since 2018-08-06T16:00:08.111Z)
We'd like to ask you to help us out and let us know whether:
- You would like to continue the work here
- You would like us to take on the Merge Request for you
- You would like us to close down the Merge Request
Thanks for your help!
Improve this comment.
added auto updated awaiting feedback labels
Hi @gitlab-bot , I'm interested in getting this merged as this fixes a problem that my organization has, and I'm willing to update the PR to a point where it can be considered to get merged (including adding tests if I can get some pointers).
However, I'm not willing to do the extra work unless there's an indication that the feature will be accepted. We're currently using the patch and it works great for us.
Thanks for the update. @victorwu please could you confirm?
Thanks @markglenfletcher for the ping. Let's continue the conversation in the issue. See https://gitlab.com/gitlab-org/gitlab-ce/issues/50020#note_108441429.
mentioned in issue #55804 (closed)
I think this is an important feature needed while doing project export/import. We did attempt to migrate a repo from one server to another, we tried export/import. Post import we noticed the MR approval status are not restored and created ticket 51658. However the workaround for this is to fetch the approval status using API from source server and restore the same on the destination server. When we tried this the MR state change triggered emails to all the project members. There were about 2k+ emails for 800 users of the repo! We did notice there's Project level settings to disable notifications for all users and then noticed this ticket about the same.
mentioned in issue #58764 (closed)
added workflowready for review label
assigned to @jprovaznik
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 Do we need similar setting also on project settings page?
https://gitlab.com/gitlab-org/gitlab-ce/issues/50020 description is now updated with the preferred location of this setting. It should be available to group owners only (similar to visibility setting).
- Resolved by Andreas Brandl
@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
328 328 :container_registry_enabled, 329 329 :default_branch, 330 330 :description, 331 :emails_enabled, Similar to https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/19686/diffs#note_167775799, permission check on update will be needed.
@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 # 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 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
unassigned @jprovaznik
mentioned in issue #50020 (moved)
assigned to @digitalmoksha
@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 ofmaster
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.
mentioned in merge request !30755 (merged)
I'm closing this MR in favor of https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/30755
added devopsplan label
mentioned in issue gitlab#34141