Merge branch 'deserialize-custom-notifications' into 'master'

Deserialize custom notifications

Closes #24892

See merge request !12055

3 files changed, 33 insertions, 22 deletions

diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb
index 42412a9a44b..b0df7aeb323 100644
--- a/app/models/notification_setting.rb
+++ b/app/models/notification_setting.rb
@@ -41,10 +41,8 @@ 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
+  before_save :convert_events
 
   def self.find_or_create_for(source)
     setting = find_or_initialize_by(source: source)
@@ -56,21 +54,18 @@ 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
+  # 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()
 
-  # 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))
-
-      events[event] = bool
+      bool.nil? ? !!events[event] : bool
     end
+
+    alias_method :"#{event}?", event
   end
 
   # Allow people to receive failed pipeline notifications if they already have
@@ -78,7 +73,23 @@ class NotificationSetting < ActiveRecord::Base
   # custom settings.
   def 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)