summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@gitlab.com>2017-06-08 17:10:35 +0100
committerSean McGivern <sean@gitlab.com>2017-06-15 15:15:13 +0100
commite94c1028c1e829f899b0d0f56313cc62df6f9a0a (patch)
treed69f40f69e1ed69ce5c2c6d7fe2367ccca7cdce1
parentf4b5fcbca100769b89e6b06e0df180012d16a7a8 (diff)
downloadgitlab-ce-e94c1028c1e829f899b0d0f56313cc62df6f9a0a.tar.gz
Deserialise existing custom notification settingsdeserialize-custom-notifications
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.
-rw-r--r--app/models/notification_setting.rb26
-rw-r--r--app/services/notification_recipient_service.rb8
-rw-r--r--app/services/notification_service.rb2
-rw-r--r--db/post_migrate/20170607121233_convert_custom_notification_settings_to_columns.rb55
-rw-r--r--db/schema.rb2
-rw-r--r--spec/controllers/notification_settings_controller_spec.rb4
-rw-r--r--spec/migrations/convert_custom_notification_settings_to_columns_spec.rb118
-rw-r--r--spec/models/notification_setting_spec.rb30
-rw-r--r--spec/services/notification_service_spec.rb3
9 files changed, 236 insertions, 12 deletions
diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb
index 2a53484f96f..b0df7aeb323 100644
--- a/app/models/notification_setting.rb
+++ b/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
diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 988bd0a7cdb..8d1820bc504 100644
--- a/app/services/notification_recipient_service.rb
+++ b/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
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 646ccbdb2bf..3a98a5f6b64 100644
--- a/app/services/notification_service.rb
+++ b/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)
diff --git a/db/post_migrate/20170607121233_convert_custom_notification_settings_to_columns.rb b/db/post_migrate/20170607121233_convert_custom_notification_settings_to_columns.rb
new file mode 100644
index 00000000000..9abda6a1d73
--- /dev/null
+++ b/db/post_migrate/20170607121233_convert_custom_notification_settings_to_columns.rb
@@ -0,0 +1,55 @@
+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
diff --git a/db/schema.rb b/db/schema.rb
index 4e897e44f53..956ca2278f4 100644
--- a/db/schema.rb
+++ b/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"
diff --git a/spec/controllers/notification_settings_controller_spec.rb b/spec/controllers/notification_settings_controller_spec.rb
index c2d0970ef88..6b690407ce3 100644
--- a/spec/controllers/notification_settings_controller_spec.rb
+++ b/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
diff --git a/spec/migrations/convert_custom_notification_settings_to_columns_spec.rb b/spec/migrations/convert_custom_notification_settings_to_columns_spec.rb
new file mode 100644
index 00000000000..1396d12e5a9
--- /dev/null
+++ b/spec/migrations/convert_custom_notification_settings_to_columns_spec.rb
@@ -0,0 +1,118 @@
+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
diff --git a/spec/models/notification_setting_spec.rb b/spec/models/notification_setting_spec.rb
index d58673413c8..cc235ad467e 100644
--- a/spec/models/notification_setting_spec.rb
+++ b/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
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index de3bbc6b6a1..f1e00c1163b 100644
--- a/spec/services/notification_service_spec.rb
+++ b/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)