From f4b5fcbca100769b89e6b06e0df180012d16a7a8 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 6 Jun 2017 17:37:15 +0100 Subject: Add columns for custom notification settings Add columns for each custom notification level, defaulting to null. Read from those columns if non-null, otherwise fall back to the serialized column. Writing will write to the new column if `events` isn't manually set. --- app/models/notification_setting.rb | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) (limited to 'app') diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb index 42412a9a44b..2a53484f96f 100644 --- a/app/models/notification_setting.rb +++ b/app/models/notification_setting.rb @@ -41,10 +41,7 @@ class NotificationSetting < ActiveRecord::Base :success_pipeline ].freeze - store :events, accessors: EMAIL_EVENTS, coder: JSON - - before_create :set_events - before_save :events_to_boolean + store :events, coder: JSON def self.find_or_create_for(source) setting = find_or_initialize_by(source: source) @@ -56,20 +53,11 @@ class NotificationSetting < ActiveRecord::Base setting end - # Set all event attributes to false when level is not custom or being initialized for UX reasons - def set_events - return if custom? - - self.events = {} - end - - # Validates store accessors values as boolean - # It is a text field so it does not cast correct boolean values in JSON - def events_to_boolean - EMAIL_EVENTS.each do |event| - bool = ActiveRecord::ConnectionAdapters::Column::TRUE_VALUES.include?(public_send(event)) + EMAIL_EVENTS.each do |event| + define_method(event) do + bool = super() - events[event] = bool + bool.nil? ? !!events[event] : bool end end @@ -77,7 +65,8 @@ class NotificationSetting < ActiveRecord::Base # custom notifications enabled, as these are more like mentions than the other # custom settings. def failed_pipeline - bool = super + bool = read_attribute(:failed_pipeline) + bool = events[:failed_pipeline] if bool.nil? bool.nil? || bool end -- cgit v1.2.3 From e94c1028c1e829f899b0d0f56313cc62df6f9a0a Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 8 Jun 2017 17:10:35 +0100 Subject: Deserialise existing custom notification settings Create a post-deployment migration to update all existing notification settings with at least one custom level enabled to the new format. Also handle the same conversion when updating settings, to catch any stragglers. --- app/models/notification_setting.rb | 26 ++++++++++++++++++++++++-- app/services/notification_recipient_service.rb | 8 ++++---- app/services/notification_service.rb | 2 +- 3 files changed, 29 insertions(+), 7 deletions(-) (limited to 'app') diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb index 2a53484f96f..b0df7aeb323 100644 --- a/app/models/notification_setting.rb +++ b/app/models/notification_setting.rb @@ -42,6 +42,7 @@ class NotificationSetting < ActiveRecord::Base ].freeze store :events, coder: JSON + before_save :convert_events def self.find_or_create_for(source) setting = find_or_initialize_by(source: source) @@ -53,21 +54,42 @@ class NotificationSetting < ActiveRecord::Base setting end - EMAIL_EVENTS.each do |event| + # 1. Check if this event has a value stored in its database column. + # 2. If it does, return that value. + # 3. If it doesn't (the value is nil), return the value from the serialized + # JSON hash in `events`. + (EMAIL_EVENTS - [:failed_pipeline]).each do |event| define_method(event) do bool = super() bool.nil? ? !!events[event] : bool end + + alias_method :"#{event}?", event end # Allow people to receive failed pipeline notifications if they already have # custom notifications enabled, as these are more like mentions than the other # custom settings. def failed_pipeline - bool = read_attribute(:failed_pipeline) + bool = super bool = events[:failed_pipeline] if bool.nil? bool.nil? || bool end + alias_method :failed_pipeline?, :failed_pipeline + + def event_enabled?(event) + respond_to?(event) && public_send(event) + end + + def convert_events + return if events_before_type_cast.nil? + + EMAIL_EVENTS.each do |event| + write_attribute(event, public_send(event)) + end + + write_attribute(:events, nil) + end end diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index 988bd0a7cdb..8d1820bc504 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -8,7 +8,7 @@ class NotificationRecipientService @project = project end - def build_recipients(target, current_user, action: nil, previous_assignee: nil, skip_current_user: true) + def build_recipients(target, current_user, action:, previous_assignee: nil, skip_current_user: true) custom_action = build_custom_key(action, target) recipients = target.participants(current_user) @@ -59,7 +59,7 @@ class NotificationRecipientService return [] if notification_setting.mention? || notification_setting.disabled? - return [] if notification_setting.custom? && !notification_setting.public_send(custom_action) + return [] if notification_setting.custom? && !notification_setting.event_enabled?(custom_action) return [] if (notification_setting.watch? || notification_setting.participating?) && NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action) @@ -176,7 +176,7 @@ class NotificationRecipientService if notification_level settings = resource.notification_settings.where(level: NotificationSetting.levels[notification_level]) - settings = settings.select { |setting| setting.events[action] } if action.present? + settings = settings.select { |setting| setting.event_enabled?(action) } if action.present? settings.map(&:user_id) else resource.notification_settings.pluck(:user_id) @@ -225,7 +225,7 @@ class NotificationRecipientService def user_ids_with_global_level_custom(ids, action) settings = settings_with_global_level_of(:custom, ids) - settings = settings.select { |setting| setting.events[action] } + settings = settings.select { |setting| setting.event_enabled?(action) } settings.map(&:user_id) end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 646ccbdb2bf..3a98a5f6b64 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -273,7 +273,7 @@ class NotificationService end def issue_moved(issue, new_issue, current_user) - recipients = NotificationRecipientService.new(issue.project).build_recipients(issue, current_user) + recipients = NotificationRecipientService.new(issue.project).build_recipients(issue, current_user, action: 'moved') recipients.map do |recipient| email = mailer.issue_moved_email(recipient, issue, new_issue, current_user) -- cgit v1.2.3