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

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

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

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

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