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.

4 files changed, 151 insertions, 4 deletions

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)