From 8411d1cffc05171e82d727d883f03e279c8e9e05 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Mon, 23 Jul 2018 10:42:19 +0800 Subject: [PATCH 1/5] Add email_events to replace EMAIL_EVENTS because it needs to be dynamic, allowing override for EE. --- .../notification_settings_controller.rb | 8 ++--- app/helpers/notifications_helper.rb | 2 +- app/models/notification_setting.rb | 8 +++++ .../notification_recipient_service.rb | 2 +- .../_custom_notifications.html.haml | 2 +- doc/api/notification_settings.md | 2 +- lib/api/entities.rb | 2 +- lib/api/notification_settings.rb | 4 +-- .../notification_settings_controller_spec.rb | 9 ++--- spec/models/notification_setting_spec.rb | 36 +++++++++++++++++-- 10 files changed, 57 insertions(+), 18 deletions(-) diff --git a/app/controllers/notification_settings_controller.rb b/app/controllers/notification_settings_controller.rb index ed20302487c..461f26561f1 100644 --- a/app/controllers/notification_settings_controller.rb +++ b/app/controllers/notification_settings_controller.rb @@ -5,14 +5,14 @@ class NotificationSettingsController < ApplicationController return render_404 unless can_read?(resource) @notification_setting = current_user.notification_settings_for(resource) - @saved = @notification_setting.update(notification_setting_params) + @saved = @notification_setting.update(notification_setting_params_for(resource)) render_response end def update @notification_setting = current_user.notification_settings.find(params[:id]) - @saved = @notification_setting.update(notification_setting_params) + @saved = @notification_setting.update(notification_setting_params_for(@notification_setting.source)) render_response end @@ -42,8 +42,8 @@ class NotificationSettingsController < ApplicationController } end - def notification_setting_params - allowed_fields = NotificationSetting::EMAIL_EVENTS.dup + def notification_setting_params_for(source) + allowed_fields = NotificationSetting.email_events(source).dup allowed_fields << :level params.require(:notification_setting).permit(allowed_fields) end diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index 3e42063224e..7ff201a321b 100644 --- a/app/helpers/notifications_helper.rb +++ b/app/helpers/notifications_helper.rb @@ -83,7 +83,7 @@ module NotificationsHelper end def notification_event_name(event) - # All values from NotificationSetting::EMAIL_EVENTS + # All values from NotificationSetting.email_events case event when :success_pipeline s_('NotificationEvent|Successful pipeline') diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb index 1df3a51a7fc..c5d49bd00ca 100644 --- a/app/models/notification_setting.rb +++ b/app/models/notification_setting.rb @@ -45,6 +45,14 @@ class NotificationSetting < ActiveRecord::Base :success_pipeline ].freeze + def self.email_events(source = nil) + EMAIL_EVENTS + end + + def email_events + self.class.email_events(source) + end + EXCLUDED_PARTICIPATING_EVENTS = [ :success_pipeline ].freeze diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index 4389fd89538..1d6c45c515b 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -279,7 +279,7 @@ module NotificationRecipientService end # Build event key to search on custom notification level - # Check NotificationSetting::EMAIL_EVENTS + # Check NotificationSetting.email_events def custom_action @custom_action ||= "#{action}_#{target.class.model_name.name.underscore}".to_sym end diff --git a/app/views/shared/notifications/_custom_notifications.html.haml b/app/views/shared/notifications/_custom_notifications.html.haml index 1f6e8f98bbb..43a87fd8397 100644 --- a/app/views/shared/notifications/_custom_notifications.html.haml +++ b/app/views/shared/notifications/_custom_notifications.html.haml @@ -19,7 +19,7 @@ - paragraph = _('Custom notification levels are the same as participating levels. With custom notification levels you will also receive notifications for select events. To find out more, check out %{notification_link}.') % { notification_link: notification_link.html_safe } #{ paragraph.html_safe } .col-lg-8 - - NotificationSetting::EMAIL_EVENTS.each_with_index do |event, index| + - notification_setting.email_events.each_with_index do |event, index| - field_id = "#{notifications_menu_identifier("modal", notification_setting)}_notification_setting[#{event}]" .form-group .form-check{ class: ("prepend-top-0" if index == 0) } diff --git a/doc/api/notification_settings.md b/doc/api/notification_settings.md index 682b90361bd..165b9a11c7a 100644 --- a/doc/api/notification_settings.md +++ b/doc/api/notification_settings.md @@ -15,7 +15,7 @@ mention custom ``` -If the `custom` level is used, specific email events can be controlled. Notification email events are defined in the `NotificationSetting::EMAIL_EVENTS` model variable. Currently, these events are recognized: +If the `custom` level is used, specific email events can be controlled. Available events are returned by `NotificationSetting.email_events`. Currently, these events are recognized: ``` new_note diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 27f28e1df93..453ebb9c669 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -856,7 +856,7 @@ module API class NotificationSetting < Grape::Entity expose :level expose :events, if: ->(notification_setting, _) { notification_setting.custom? } do - ::NotificationSetting::EMAIL_EVENTS.each do |event| + ::NotificationSetting.email_events.each do |event| expose event end end diff --git a/lib/api/notification_settings.rb b/lib/api/notification_settings.rb index 0266bf2f717..9c824d9953c 100644 --- a/lib/api/notification_settings.rb +++ b/lib/api/notification_settings.rb @@ -23,7 +23,7 @@ module API params do optional :level, type: String, desc: 'The global notification level' optional :notification_email, type: String, desc: 'The email address to send notifications' - NotificationSetting::EMAIL_EVENTS.each do |event| + NotificationSetting.email_events.each do |event| optional event, type: Boolean, desc: 'Enable/disable this notification' end end @@ -73,7 +73,7 @@ module API end params do optional :level, type: String, desc: "The #{source_type} notification level" - NotificationSetting::EMAIL_EVENTS.each do |event| + NotificationSetting.email_events(source_type.to_sym).each do |event| optional event, type: Boolean, desc: 'Enable/disable this notification' end end diff --git a/spec/controllers/notification_settings_controller_spec.rb b/spec/controllers/notification_settings_controller_spec.rb index e133950e684..a3356a86d4b 100644 --- a/spec/controllers/notification_settings_controller_spec.rb +++ b/spec/controllers/notification_settings_controller_spec.rb @@ -21,10 +21,11 @@ describe NotificationSettingsController do end context 'when authorized' do + let(:notification_setting) { user.notification_settings_for(source) } let(:custom_events) do events = {} - NotificationSetting::EMAIL_EVENTS.each do |event| + NotificationSetting.email_events(source).each do |event| events[event.to_s] = true end @@ -36,7 +37,7 @@ describe NotificationSettingsController do end context 'for projects' do - let(:notification_setting) { user.notification_settings_for(project) } + let(:source) { project } it 'creates notification setting' do post :create, @@ -67,7 +68,7 @@ describe NotificationSettingsController do end context 'for groups' do - let(:notification_setting) { user.notification_settings_for(group) } + let(:source) { group } it 'creates notification setting' do post :create, @@ -145,7 +146,7 @@ describe NotificationSettingsController do let(:custom_events) do events = {} - NotificationSetting::EMAIL_EVENTS.each do |event| + notification_setting.email_events.each do |event| events[event] = "true" end end diff --git a/spec/models/notification_setting_spec.rb b/spec/models/notification_setting_spec.rb index 77c475b9f52..e545b674b4f 100644 --- a/spec/models/notification_setting_spec.rb +++ b/spec/models/notification_setting_spec.rb @@ -94,9 +94,39 @@ RSpec.describe NotificationSetting do end end - context 'email events' do - it 'includes EXCLUDED_WATCHER_EVENTS in EMAIL_EVENTS' do - expect(described_class::EMAIL_EVENTS).to include(*described_class::EXCLUDED_WATCHER_EVENTS) + describe '.email_events' do + subject { described_class.email_events } + + it 'returns email events' do + expect(subject).to include( + :new_note, + :new_issue, + :reopen_issue, + :close_issue, + :reassign_issue, + :new_merge_request, + :reopen_merge_request, + :close_merge_request, + :reassign_merge_request, + :merge_merge_request, + :failed_pipeline, + :success_pipeline + ) + end + + it 'includes EXCLUDED_WATCHER_EVENTS' do + expect(subject).to include(*described_class::EXCLUDED_WATCHER_EVENTS) + end + end + + describe '#email_events' do + let(:source) { build(:group) } + + subject { build(:notification_setting, source: source) } + + it 'calls email_events' do + expect(described_class).to receive(:email_events).with(source) + subject.email_events end end end From e07a1a5bb43f659f5289a7b317a081465945af4d Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Mon, 23 Jul 2018 20:51:42 +0800 Subject: [PATCH 2/5] Allow global event list containing all events This way we can globally set group only ee events too. Use nil to indicate global. --- lib/api/notification_settings.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/api/notification_settings.rb b/lib/api/notification_settings.rb index 9c824d9953c..bf0d6b9e434 100644 --- a/lib/api/notification_settings.rb +++ b/lib/api/notification_settings.rb @@ -50,7 +50,9 @@ module API end end - %w[group project].each do |source_type| + [Group, Project].each do |source_class| + source_type = source_class.name.underscore + params do requires :id, type: String, desc: "The #{source_type} ID" end @@ -73,7 +75,7 @@ module API end params do optional :level, type: String, desc: "The #{source_type} notification level" - NotificationSetting.email_events(source_type.to_sym).each do |event| + NotificationSetting.email_events(source_class).each do |event| optional event, type: Boolean, desc: 'Enable/disable this notification' end end From 59564df15f5fc0893fcd09cfb00d8c8e6534696d Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Mon, 30 Jul 2018 17:38:20 +0800 Subject: [PATCH 3/5] Allow NotificationRecipientService::Builder::Default to handle target without project (e.g. Epic) --- app/services/notification_recipient_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index 1d6c45c515b..1172fdbea8d 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -130,7 +130,7 @@ module NotificationRecipientService end def add_project_watchers - add_recipients(project_watchers, :watch, nil) + add_recipients(project_watchers, :watch, nil) if project end def add_group_watchers From bf88e9afe70e6589dd0c1c089a5a73c8d2b687c6 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Mon, 30 Jul 2018 18:30:36 +0800 Subject: [PATCH 4/5] Allow extensible mention type action for EE --- app/services/notification_recipient_service.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index 1172fdbea8d..5c0e8a35cb0 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -220,6 +220,8 @@ module NotificationRecipientService end class Default < Base + MENTION_TYPE_ACTIONS = [:new_issue, :new_merge_request].freeze + attr_reader :target attr_reader :current_user attr_reader :action @@ -252,7 +254,7 @@ module NotificationRecipientService add_subscribed_users - if [:new_issue, :new_merge_request].include?(custom_action) + if self.class.mention_type_actions.include?(custom_action) # These will all be participants as well, but adding with the :mention # type ensures that users with the mention notification level will # receive them, too. @@ -283,6 +285,10 @@ module NotificationRecipientService def custom_action @custom_action ||= "#{action}_#{target.class.model_name.name.underscore}".to_sym end + + def self.mention_type_actions + MENTION_TYPE_ACTIONS.dup + end end class NewNote < Base From f8ee861cd42b35ff5d35c18dafbe4d1c425af926 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Wed, 8 Aug 2018 14:38:39 +0800 Subject: [PATCH 5/5] Move N_ calls into separate files These are dynamic translations, so has to be marked explicitly using `N_`, but they are not used in runtime, so can exist in separate file. https://github.com/grosser/gettext_i18n_rails#unfound-translations-with-rake-gettextfind --- .rubocop.yml | 2 ++ app/helpers/notifications_helper.rb | 10 ---------- app/models/notification_setting.rb | 1 + locale/unfound_translations.rb | 16 ++++++++++++++++ 4 files changed, 19 insertions(+), 10 deletions(-) create mode 100644 locale/unfound_translations.rb diff --git a/.rubocop.yml b/.rubocop.yml index a586190319b..9858bbe0ddd 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -48,6 +48,8 @@ Naming/FileName: - 'qa/bin/*' - 'config/**/*' - 'lib/generators/**/*' + - 'locale/unfound_translations.rb' + - 'ee/locale/unfound_translations.rb' - 'ee/lib/generators/**/*' IgnoreExecutableScripts: true AllowedAcronyms: diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index 7ff201a321b..a185f2916d4 100644 --- a/app/helpers/notifications_helper.rb +++ b/app/helpers/notifications_helper.rb @@ -88,16 +88,6 @@ module NotificationsHelper when :success_pipeline s_('NotificationEvent|Successful pipeline') else - N_('NotificationEvent|New note') - N_('NotificationEvent|New issue') - N_('NotificationEvent|Reopen issue') - N_('NotificationEvent|Close issue') - N_('NotificationEvent|Reassign issue') - N_('NotificationEvent|New merge request') - N_('NotificationEvent|Close merge request') - N_('NotificationEvent|Reassign merge request') - N_('NotificationEvent|Merge merge request') - N_('NotificationEvent|Failed pipeline') s_(event.to_s.humanize) end end diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb index c5d49bd00ca..1600acfc575 100644 --- a/app/models/notification_setting.rb +++ b/app/models/notification_setting.rb @@ -45,6 +45,7 @@ class NotificationSetting < ActiveRecord::Base :success_pipeline ].freeze + # Update unfound_translations.rb when events are changed def self.email_events(source = nil) EMAIL_EVENTS end diff --git a/locale/unfound_translations.rb b/locale/unfound_translations.rb new file mode 100644 index 00000000000..0826d64049b --- /dev/null +++ b/locale/unfound_translations.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +# Dynamic translations which needs to be marked by `N_` so they can be found by `rake gettext:find`, see: +# https://github.com/grosser/gettext_i18n_rails#unfound-translations-with-rake-gettextfind + +# NotificationSetting.email_events +N_('NotificationEvent|New note') +N_('NotificationEvent|New issue') +N_('NotificationEvent|Reopen issue') +N_('NotificationEvent|Close issue') +N_('NotificationEvent|Reassign issue') +N_('NotificationEvent|New merge request') +N_('NotificationEvent|Close merge request') +N_('NotificationEvent|Reassign merge request') +N_('NotificationEvent|Merge merge request') +N_('NotificationEvent|Failed pipeline')