Merge branch '33620-remove-events-from-notification_settings' into 'master'

Resolve "Remove `events` from `notification_settings`"

Closes #33620

See merge request !13152

app/models/notification_setting.rb

@@ -1,4 +1,8 @@
 class NotificationSetting < ActiveRecord::Base
+  include IgnorableColumn
+
+  ignore_column :events
+
   enum level: { global: 3, watch: 2, mention: 4, participating: 1, disabled: 0, custom: 5 }
 
   default_value_for :level, NotificationSetting.levels[:global]
@@ -41,9 +45,6 @@ class NotificationSetting < ActiveRecord::Base
     :success_pipeline
   ].freeze
 
-  store :events, coder: JSON
-  before_save :convert_events
-
   def self.find_or_create_for(source)
     setting = find_or_initialize_by(source: source)
 
@@ -54,42 +55,17 @@ class NotificationSetting < ActiveRecord::Base
     setting
   end
 
-  # 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 = 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)
+    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

changelogs/unreleased/33620-remove-events-from-notification_settings.yml

@@ -0,0 +1,4 @@
+---
+title: Remove events column from notification settings table
+merge_request:
+author:

db/post_migrate/20170728101014_remove_events_from_notification_settings.rb

@@ -0,0 +1,9 @@
+class RemoveEventsFromNotificationSettings < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    remove_column :notification_settings, :events, :text
+  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: 20170725145659) do
+ActiveRecord::Schema.define(version: 20170728101014) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -981,7 +981,6 @@ ActiveRecord::Schema.define(version: 20170725145659) do
     t.integer "level", default: 0, null: false
     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"

spec/factories/notification_settings.rb

@@ -3,6 +3,5 @@ FactoryGirl.define do
     source factory: :empty_project
     user
     level 3
-    events []
   end
 end

spec/models/notification_setting_spec.rb

@@ -63,24 +63,20 @@ RSpec.describe NotificationSetting do
     end
   end
 
-  describe 'event_enabled?' do
+  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)
+        subject.update!(new_note: true)
 
-        expect(subject.event_enabled?(:new_note)).to be(false)
+        expect(subject.event_enabled?(:new_note)).to be(true)
       end
 
       context 'when the column has a nil value' do
-        it 'returns the value from the events hash' do
+        it 'returns false' do
           expect(subject.event_enabled?(:new_note)).to be(false)
         end
       end

spec/requests/api/notification_settings_spec.rb

@@ -72,8 +72,8 @@ describe API::NotificationSettings do
 
       expect(response).to have_http_status(200)
       expect(json_response['level']).to eq(user.reload.notification_settings_for(project).level)
-      expect(json_response['events']['new_note']).to eq(true)
+      expect(json_response['events']['new_note']).to be_truthy
-      expect(json_response['events']['new_issue']).to eq(false)
+      expect(json_response['events']['new_issue']).to be_falsey
     end
   end
 

spec/spec_helper.rb

@@ -129,10 +129,14 @@ RSpec.configure do |config|
   config.before(:example, :migration) do
     ActiveRecord::Migrator
       .migrate(migrations_paths, previous_migration.version)
+
+    ActiveRecord::Base.descendants.each(&:reset_column_information)
   end
 
   config.after(:example, :migration) do
     ActiveRecord::Migrator.migrate(migrations_paths)
+
+    ActiveRecord::Base.descendants.each(&:reset_column_information)
   end
 
   config.around(:each, :nested_groups) do |example|