Deserialise existing custom notification settingsdeserialize-custom-notifications

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.

9 files changed, 236 insertions, 12 deletions

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)
diff --git a/db/post_migrate/20170607121233_convert_custom_notification_settings_to_columns.rb b/db/post_migrate/20170607121233_convert_custom_notification_settings_to_columns.rb
new file mode 100644
index 00000000000..9abda6a1d73
--- /dev/null
+++ b/db/post_migrate/20170607121233_convert_custom_notification_settings_to_columns.rb
@@ -0,0 +1,55 @@
+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
diff --git a/db/schema.rb b/db/schema.rb
index 4e897e44f53..956ca2278f4 100644
--- a/db/schema.rb
+++ b/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"
diff --git a/spec/controllers/notification_settings_controller_spec.rb b/spec/controllers/notification_settings_controller_spec.rb
index c2d0970ef88..6b690407ce3 100644
--- a/spec/controllers/notification_settings_controller_spec.rb
+++ b/spec/controllers/notification_settings_controller_spec.rb
@@ -60,7 +60,7 @@ describe NotificationSettingsController do
             expect(notification_setting.level).to eq("custom")
 
             custom_events.each do |event, value|
-              expect(notification_setting.send(event)).to eq(value)
+              expect(notification_setting.event_enabled?(event)).to eq(value)
             end
           end
         end
@@ -91,7 +91,7 @@ describe NotificationSettingsController do
             expect(notification_setting.level).to eq("custom")
 
             custom_events.each do |event, value|
-              expect(notification_setting.send(event)).to eq(value)
+              expect(notification_setting.event_enabled?(event)).to eq(value)
             end
           end
         end
diff --git a/spec/migrations/convert_custom_notification_settings_to_columns_spec.rb b/spec/migrations/convert_custom_notification_settings_to_columns_spec.rb
new file mode 100644
index 00000000000..1396d12e5a9
--- /dev/null
+++ b/spec/migrations/convert_custom_notification_settings_to_columns_spec.rb
@@ -0,0 +1,118 @@
+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
diff --git a/spec/models/notification_setting_spec.rb b/spec/models/notification_setting_spec.rb
index d58673413c8..cc235ad467e 100644
--- a/spec/models/notification_setting_spec.rb
+++ b/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
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index de3bbc6b6a1..f1e00c1163b 100644
--- a/spec/services/notification_service_spec.rb
+++ b/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)