From 084e15b4476803f3e62090a85696fb0f07cddbfa Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Tue, 1 Sep 2020 13:07:41 +1000 Subject: [PATCH] FIX: modify notification after remove auto_watch_category (#10568) When a category is removed from `auto_watch_category` we are removing CategoryUser. However, there are still TopicUser with notification level set to `watching` which was inherited from Category. We should move them back to `regular` unless they were modified by a user. --- .../javascripts/admin/models/site-setting.js | 2 +- .../admin/site_settings_controller.rb | 11 ++++-- .../admin/site_settings_controller_spec.rb | 36 ++++++++++++++++--- 3 files changed, 41 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/admin/models/site-setting.js b/app/assets/javascripts/admin/models/site-setting.js index 01e34566f5..a131e6dfde 100644 --- a/app/assets/javascripts/admin/models/site-setting.js +++ b/app/assets/javascripts/admin/models/site-setting.js @@ -32,7 +32,7 @@ SiteSetting.reopenClass({ data[key] = value; if (opts["updateExistingUsers"] === true) { - data["updateExistingUsers"] = true; + data["update_existing_user"] = true; } return ajax(`/admin/site_settings/${key}`, { type: "PUT", data }); diff --git a/app/controllers/admin/site_settings_controller.rb b/app/controllers/admin/site_settings_controller.rb index 3b7ac11b41..f8695f2f0c 100644 --- a/app/controllers/admin/site_settings_controller.rb +++ b/app/controllers/admin/site_settings_controller.rb @@ -20,7 +20,7 @@ class Admin::SiteSettingsController < Admin::AdminController value = Upload.get_from_url(value) || "" end - update_existing_users = params[:updateExistingUsers].present? + update_existing_users = params[:update_existing_user].present? previous_value = SiteSetting.send(id) || "" if update_existing_users SiteSetting.set_and_log(id, value, current_user) @@ -58,7 +58,14 @@ class Admin::SiteSettingsController < Admin::AdminController notification_level = NotificationLevels.all[:regular] end - CategoryUser.where(category_id: (previous_category_ids - new_category_ids), notification_level: notification_level).delete_all + categories_to_unwatch = previous_category_ids - new_category_ids + CategoryUser.where(category_id: categories_to_unwatch, notification_level: notification_level).delete_all + TopicUser + .joins(:topic) + .where(notification_level: TopicUser.notification_levels[:watching], + notifications_reason_id: TopicUser.notification_reasons[:auto_watch_category], + topics: { category_id: categories_to_unwatch }) + .update_all(notification_level: TopicUser.notification_levels[:regular]) (new_category_ids - previous_category_ids).each do |category_id| skip_user_ids = CategoryUser.where(category_id: category_id).pluck(:user_id) diff --git a/spec/requests/admin/site_settings_controller_spec.rb b/spec/requests/admin/site_settings_controller_spec.rb index e01752f97d..48cc8236bb 100644 --- a/spec/requests/admin/site_settings_controller_spec.rb +++ b/spec/requests/admin/site_settings_controller_spec.rb @@ -65,7 +65,7 @@ describe Admin::SiteSettingsController do put "/admin/site_settings/default_email_in_reply_to.json", params: { default_email_in_reply_to: false, - updateExistingUsers: true + update_existing_user: true } user2.reload @@ -86,14 +86,14 @@ describe Admin::SiteSettingsController do expect { put "/admin/site_settings/default_email_digest_frequency.json", params: { default_email_digest_frequency: 30, - updateExistingUsers: true + update_existing_user: true } }.to change { UserOption.where(email_digests: true).count }.by(1) expect { put "/admin/site_settings/default_email_digest_frequency.json", params: { default_email_digest_frequency: 0, - updateExistingUsers: true + update_existing_user: true } }.to change { UserOption.where(email_digests: false).count }.by(User.count) end @@ -120,11 +120,25 @@ describe Admin::SiteSettingsController do it 'should update existing users user preference' do put "/admin/site_settings/default_categories_watching.json", params: { default_categories_watching: category_ids.last(2).join("|"), - updateExistingUsers: true + update_existing_user: true } + expect(response.status).to eq(200) expect(CategoryUser.where(category_id: category_ids.first, notification_level: watching).count).to eq(0) expect(CategoryUser.where(category_id: category_ids.last, notification_level: watching).count).to eq(User.real.where(staged: false).count - 1) + + topic = Fabricate(:topic, category_id: category_ids.last) + topic_user1 = Fabricate(:topic_user, topic: topic, notification_level: TopicUser.notification_levels[:watching], notifications_reason_id: TopicUser.notification_reasons[:auto_watch_category]) + topic_user2 = Fabricate(:topic_user, topic: topic, notification_level: TopicUser.notification_levels[:watching], notifications_reason_id: TopicUser.notification_reasons[:user_changed]) + + put "/admin/site_settings/default_categories_watching.json", params: { + default_categories_watching: "", + update_existing_user: true + } + expect(response.status).to eq(200) + expect(CategoryUser.where(category_id: category_ids, notification_level: watching).count).to eq(0) + expect(topic_user1.reload.notification_level).to eq(TopicUser.notification_levels[:regular]) + expect(topic_user2.reload.notification_level).to eq(TopicUser.notification_levels[:watching]) end it 'should not update existing users user preference' do @@ -134,7 +148,19 @@ describe Admin::SiteSettingsController do } }.to change { CategoryUser.where(category_id: category_ids.first, notification_level: watching).count }.by(0) + expect(response.status).to eq(200) expect(CategoryUser.where(category_id: category_ids.last, notification_level: watching).count).to eq(0) + + topic = Fabricate(:topic, category_id: category_ids.last) + topic_user1 = Fabricate(:topic_user, topic: topic, notification_level: TopicUser.notification_levels[:watching], notifications_reason_id: TopicUser.notification_reasons[:auto_watch_category]) + topic_user2 = Fabricate(:topic_user, topic: topic, notification_level: TopicUser.notification_levels[:watching], notifications_reason_id: TopicUser.notification_reasons[:user_changed]) + put "/admin/site_settings/default_categories_watching.json", params: { + default_categories_watching: "", + } + expect(response.status).to eq(200) + expect(CategoryUser.where(category_id: category_ids.first, notification_level: watching).count).to eq(0) + expect(topic_user1.reload.notification_level).to eq(TopicUser.notification_levels[:watching]) + expect(topic_user2.reload.notification_level).to eq(TopicUser.notification_levels[:watching]) end end @@ -159,7 +185,7 @@ describe Admin::SiteSettingsController do it 'should update existing users user preference' do put "/admin/site_settings/default_tags_watching.json", params: { default_tags_watching: tags.last(2).pluck(:name).join("|"), - updateExistingUsers: true + update_existing_user: true } expect(TagUser.where(tag_id: tags.first.id, notification_level: watching).count).to eq(0)