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