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

@@ -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

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/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"

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

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)