From f4b5fcbca100769b89e6b06e0df180012d16a7a8 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Tue, 6 Jun 2017 17:37:15 +0100
Subject: [PATCH] Add columns for custom notification settings

Add columns for each custom notification level, defaulting to null. Read from
those columns if non-null, otherwise fall back to the serialized column. Writing
will write to the new column if `events` isn't manually set.
---
 app/models/notification_setting.rb            | 25 +++++-------------
 ...154216_add_notification_setting_columns.rb | 26 +++++++++++++++++++
 db/schema.rb                                  | 12 +++++++++
 .../notification_settings_controller_spec.rb  | 10 +++++--
 4 files changed, 53 insertions(+), 20 deletions(-)
 create mode 100644 db/migrate/20170606154216_add_notification_setting_columns.rb

diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb
index 42412a9a44b..2a53484f96f 100644
--- a/app/models/notification_setting.rb
+++ b/app/models/notification_setting.rb
@@ -41,10 +41,7 @@ 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
 
   def self.find_or_create_for(source)
     setting = find_or_initialize_by(source: source)
@@ -56,20 +53,11 @@ 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?
+  EMAIL_EVENTS.each do |event|
+    define_method(event) do
+      bool = super()
 
-    self.events = {}
-  end
-
-  # 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
   end
 
@@ -77,7 +65,8 @@ class NotificationSetting < ActiveRecord::Base
   # custom notifications enabled, as these are more like mentions than the other
   # custom settings.
   def failed_pipeline
-    bool = super
+    bool = read_attribute(:failed_pipeline)
+    bool = events[:failed_pipeline] if bool.nil?
 
     bool.nil? || bool
   end
diff --git a/db/migrate/20170606154216_add_notification_setting_columns.rb b/db/migrate/20170606154216_add_notification_setting_columns.rb
new file mode 100644
index 00000000000..0a9b5da6583
--- /dev/null
+++ b/db/migrate/20170606154216_add_notification_setting_columns.rb
@@ -0,0 +1,26 @@
+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
diff --git a/db/schema.rb b/db/schema.rb
index 9d79fe162b7..4e897e44f53 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -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
diff --git a/spec/controllers/notification_settings_controller_spec.rb b/spec/controllers/notification_settings_controller_spec.rb
index cf136e72bac..c2d0970ef88 100644
--- a/spec/controllers/notification_settings_controller_spec.rb
+++ b/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.send(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.send(event)).to eq(value)
+            end
           end
         end
       end