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

Deserialize custom notifications

Closes #24892

See merge request !12055

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

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
 

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)

db/migrate/20170606154216_add_notification_setting_columns.rb

+class AddNotificationSettingColumns < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  COLUMNS = [
+    :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
+  ]
+
+  def change
+    COLUMNS.each do |column|
+      add_column(:notification_settings, column, :boolean)
+    end
+  end
+end

db/post_migrate/20170607121233_convert_custom_notification_settings_to_columns.rb

+class ConvertCustomNotificationSettingsToColumns < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class NotificationSetting < ActiveRecord::Base
+    self.table_name = 'notification_settings'
+
+    store :events, coder: JSON
+  end
+
+  EMAIL_EVENTS = [
+    :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
+  ]
+
+  # We only need to migrate (up or down) rows where at least one of these
+  # settings is set.
+  def up
+    NotificationSetting.where("events LIKE '%true%'").find_each do |notification_setting|
+      EMAIL_EVENTS.each do |event|
+        notification_setting[event] = notification_setting.events[event]
+      end
+
+      notification_setting[:events] = nil
+      notification_setting.save!
+    end
+  end
+
+  def down
+    NotificationSetting.where(EMAIL_EVENTS.join(' OR ')).find_each do |notification_setting|
+      events = {}
+
+      EMAIL_EVENTS.each do |event|
+        events[event] = !!notification_setting.public_send(event)
+        notification_setting[event] = nil
+      end
+
+      notification_setting[:events] = events
+      notification_setting.save!
+    end
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170606202615) do
+ActiveRecord::Schema.define(version: 20170607121233) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -878,6 +878,18 @@ ActiveRecord::Schema.define(version: 20170606202615) do
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.text "events"
+    t.boolean "new_note"
+    t.boolean "new_issue"
+    t.boolean "reopen_issue"
+    t.boolean "close_issue"
+    t.boolean "reassign_issue"
+    t.boolean "new_merge_request"
+    t.boolean "reopen_merge_request"
+    t.boolean "close_merge_request"
+    t.boolean "reassign_merge_request"
+    t.boolean "merge_merge_request"
+    t.boolean "failed_pipeline"
+    t.boolean "success_pipeline"
   end
 
   add_index "notification_settings", ["source_id", "source_type"], name: "index_notification_settings_on_source_id_and_source_type", using: :btree

spec/controllers/notification_settings_controller_spec.rb

@@ -58,7 +58,10 @@ describe NotificationSettingsController do
 
             expect(response.status).to eq 200
             expect(notification_setting.level).to eq("custom")
-            expect(notification_setting.events).to eq(custom_events)
+
+            custom_events.each do |event, value|
+              expect(notification_setting.event_enabled?(event)).to eq(value)
+            end
           end
         end
       end
@@ -86,7 +89,10 @@ describe NotificationSettingsController do
 
             expect(response.status).to eq 200
             expect(notification_setting.level).to eq("custom")
-            expect(notification_setting.events).to eq(custom_events)
+
+            custom_events.each do |event, value|
+              expect(notification_setting.event_enabled?(event)).to eq(value)
+            end
           end
         end
       end

spec/migrations/convert_custom_notification_settings_to_columns_spec.rb

+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20170607121233_convert_custom_notification_settings_to_columns')
+
+describe ConvertCustomNotificationSettingsToColumns, :migration do
+  let(:settings_params) do
+    [
+      { level: 0, events: [:new_note] }, # disabled, single event
+      { level: 3, events: [:new_issue, :reopen_issue, :close_issue, :reassign_issue] }, # global, multiple events
+      { level: 5, events: described_class::EMAIL_EVENTS }, # custom, all events
+      { level: 5, events: [] } # custom, no events
+    ]
+  end
+
+  let(:notification_settings_before) do
+    settings_params.map do |params|
+      events = {}
+
+      params[:events].each do |event|
+        events[event] = true
+      end
+
+      user = create(:user)
+      create_params = { user_id: user.id, level: params[:level], events: events }
+      notification_setting = described_class::NotificationSetting.create(create_params)
+
+      [notification_setting, params]
+    end
+  end
+
+  let(:notification_settings_after) do
+    settings_params.map do |params|
+      events = {}
+
+      params[:events].each do |event|
+        events[event] = true
+      end
+
+      user = create(:user)
+      create_params = events.merge(user_id: user.id, level: params[:level])
+      notification_setting = described_class::NotificationSetting.create(create_params)
+
+      [notification_setting, params]
+    end
+  end
+
+  describe '#up' do
+    it 'migrates all settings where a custom event is enabled, even if they are not currently using the custom level' do
+      notification_settings_before
+
+      described_class.new.up
+
+      notification_settings_before.each do |(notification_setting, params)|
+        notification_setting.reload
+
+        expect(notification_setting.read_attribute_before_type_cast(:events)).to be_nil
+        expect(notification_setting.level).to eq(params[:level])
+
+        described_class::EMAIL_EVENTS.each do |event|
+          # We don't set the others to false, just let them default to nil
+          expected = params[:events].include?(event) || nil
+
+          expect(notification_setting.read_attribute(event)).to eq(expected)
+        end
+      end
+    end
+  end
+
+  describe '#down' do
+    it 'creates a custom events hash for all settings where at least one event is enabled' do
+      notification_settings_after
+
+      described_class.new.down
+
+      notification_settings_after.each do |(notification_setting, params)|
+        notification_setting.reload
+
+        expect(notification_setting.level).to eq(params[:level])
+
+        if params[:events].empty?
+          # We don't migrate empty settings
+          expect(notification_setting.events).to eq({})
+        else
+          described_class::EMAIL_EVENTS.each do |event|
+            expected = params[:events].include?(event)
+
+            expect(notification_setting.events[event]).to eq(expected)
+            expect(notification_setting.read_attribute(event)).to be_nil
+          end
+        end
+      end
+    end
+
+    it 'reverts the database to the state it was in before' do
+      notification_settings_before
+
+      described_class.new.up
+      described_class.new.down
+
+      notification_settings_before.each do |(notification_setting, params)|
+        notification_setting.reload
+
+        expect(notification_setting.level).to eq(params[:level])
+
+        if params[:events].empty?
+          # We don't migrate empty settings
+          expect(notification_setting.events).to eq({})
+        else
+          described_class::EMAIL_EVENTS.each do |event|
+            expected = params[:events].include?(event)
+
+            expect(notification_setting.events[event]).to eq(expected)
+            expect(notification_setting.read_attribute(event)).to be_nil
+          end
+        end
+      end
+    end
+  end
+end

spec/models/notification_setting_spec.rb

@@ -55,4 +55,34 @@ RSpec.describe NotificationSetting, type: :model do
       expect(user.notification_settings.for_projects.map(&:project)).to all(have_attributes(pending_delete: false))
     end
   end
+
+  describe 'event_enabled?' do
+    before do
+      subject.update!(user: create(:user))
+    end
+
+    context 'for an event with a matching column name' do
+      before do
+        subject.update!(events: { new_note: true }.to_json)
+      end
+
+      it 'returns the value of the column' do
+        subject.update!(new_note: false)
+
+        expect(subject.event_enabled?(:new_note)).to be(false)
+      end
+
+      context 'when the column has a nil value' do
+        it 'returns the value from the events hash' do
+          expect(subject.event_enabled?(:new_note)).to be(false)
+        end
+      end
+    end
+
+    context 'for an event without a matching column name' do
+      it 'returns false' do
+        expect(subject.event_enabled?(:foo_event)).to be(false)
+      end
+    end
+  end
 end

spec/services/notification_service_spec.rb

@@ -1539,8 +1539,7 @@ describe NotificationService, services: true do
   # When resource is nil it means global notification
   def update_custom_notification(event, user, resource: nil, value: true)
     setting = user.notification_settings_for(resource)
-    setting.events[event] = value
-    setting.save
+    setting.update!(event => value)
   end
 
   def add_users_with_subscription(project, issuable)