From d6fc39c8860f86410b2a20702fa0bac4f37b8978 Mon Sep 17 00:00:00 2001 From: Vinoth Kannan Date: Thu, 15 Jul 2021 19:53:57 +0530 Subject: [PATCH] FEATURE: update existing users when group default notifications changed. (#13434) Currently, the changes will only affect the users added after. --- .../modals/site-setting-default-categories.js | 19 +- .../components/group-manage-save-button.js | 29 ++- .../group-default-notifications.js | 5 + .../app/mixins/modal-update-existing-users.js | 19 ++ .../javascripts/discourse/app/models/group.js | 4 +- .../modal/group-default-notifications.hbs | 8 + app/controllers/groups_controller.rb | 165 ++++++++++++++++++ config/locales/client.en.yml | 5 + spec/requests/groups_controller_spec.rb | 78 ++++++++- 9 files changed, 308 insertions(+), 24 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/controllers/group-default-notifications.js create mode 100644 app/assets/javascripts/discourse/app/mixins/modal-update-existing-users.js create mode 100644 app/assets/javascripts/discourse/app/templates/modal/group-default-notifications.hbs diff --git a/app/assets/javascripts/admin/addon/controllers/modals/site-setting-default-categories.js b/app/assets/javascripts/admin/addon/controllers/modals/site-setting-default-categories.js index c46fe08b7ba..8c3a2648989 100644 --- a/app/assets/javascripts/admin/addon/controllers/modals/site-setting-default-categories.js +++ b/app/assets/javascripts/admin/addon/controllers/modals/site-setting-default-categories.js @@ -1,20 +1,5 @@ import Controller from "@ember/controller"; import ModalFunctionality from "discourse/mixins/modal-functionality"; +import ModalUpdateExistingUsers from "discourse/mixins/modal-update-existing-users"; -export default Controller.extend(ModalFunctionality, { - onShow() { - this.set("updateExistingUsers", null); - }, - - actions: { - updateExistingUsers() { - this.set("updateExistingUsers", true); - this.send("closeModal"); - }, - - cancel() { - this.set("updateExistingUsers", false); - this.send("closeModal"); - }, - }, -}); +export default Controller.extend(ModalFunctionality, ModalUpdateExistingUsers); diff --git a/app/assets/javascripts/discourse/app/components/group-manage-save-button.js b/app/assets/javascripts/discourse/app/components/group-manage-save-button.js index 82cc182b7d7..3c7e17d634d 100644 --- a/app/assets/javascripts/discourse/app/components/group-manage-save-button.js +++ b/app/assets/javascripts/discourse/app/components/group-manage-save-button.js @@ -4,10 +4,12 @@ import I18n from "I18n"; import discourseComputed from "discourse-common/utils/decorators"; import { popupAjaxError } from "discourse/lib/ajax-error"; import { popupAutomaticMembershipAlert } from "discourse/controllers/groups-new"; +import showModal from "discourse/lib/show-modal"; export default Component.extend({ saving: null, disabled: false, + updateExistingUsers: null, @discourseComputed("saving") savingText(saving) { @@ -28,14 +30,37 @@ export default Component.extend({ group.automatic_membership_email_domains ); + const opts = {}; + if (this.updateExistingUsers !== null) { + opts.update_existing_users = this.updateExistingUsers; + } + return group - .save() + .save(opts) .then((data) => { + if (data.user_count) { + const controller = showModal("group-default-notifications", { + model: { + count: data.user_count, + }, + }); + + controller.set("onClose", () => { + this.updateExistingUsers = controller.updateExistingUsers; + this.send("save"); + }); + + return; + } + if (data.route_to) { DiscourseURL.routeTo(data.route_to); } - this.set("saved", true); + this.setProperties({ + saved: true, + updateExistingUsers: null, + }); if (this.afterSave) { this.afterSave(); diff --git a/app/assets/javascripts/discourse/app/controllers/group-default-notifications.js b/app/assets/javascripts/discourse/app/controllers/group-default-notifications.js new file mode 100644 index 00000000000..8c3a2648989 --- /dev/null +++ b/app/assets/javascripts/discourse/app/controllers/group-default-notifications.js @@ -0,0 +1,5 @@ +import Controller from "@ember/controller"; +import ModalFunctionality from "discourse/mixins/modal-functionality"; +import ModalUpdateExistingUsers from "discourse/mixins/modal-update-existing-users"; + +export default Controller.extend(ModalFunctionality, ModalUpdateExistingUsers); diff --git a/app/assets/javascripts/discourse/app/mixins/modal-update-existing-users.js b/app/assets/javascripts/discourse/app/mixins/modal-update-existing-users.js new file mode 100644 index 00000000000..699061545f5 --- /dev/null +++ b/app/assets/javascripts/discourse/app/mixins/modal-update-existing-users.js @@ -0,0 +1,19 @@ +import Mixin from "@ember/object/mixin"; + +export default Mixin.create({ + onShow() { + this.set("updateExistingUsers", null); + }, + + actions: { + updateExistingUsers() { + this.set("updateExistingUsers", true); + this.send("closeModal"); + }, + + cancel() { + this.set("updateExistingUsers", false); + this.send("closeModal"); + }, + }, +}); diff --git a/app/assets/javascripts/discourse/app/models/group.js b/app/assets/javascripts/discourse/app/models/group.js index 23a8e54f26a..5fcbf4618bc 100644 --- a/app/assets/javascripts/discourse/app/models/group.js +++ b/app/assets/javascripts/discourse/app/models/group.js @@ -292,10 +292,10 @@ const Group = RestModel.extend({ }); }, - save() { + save(opts = {}) { return ajax(`/groups/${this.id}`, { type: "PUT", - data: { group: this.asJSON() }, + data: Object.assign({ group: this.asJSON() }, opts), }); }, diff --git a/app/assets/javascripts/discourse/app/templates/modal/group-default-notifications.hbs b/app/assets/javascripts/discourse/app/templates/modal/group-default-notifications.hbs new file mode 100644 index 00000000000..1bb030c8422 --- /dev/null +++ b/app/assets/javascripts/discourse/app/templates/modal/group-default-notifications.hbs @@ -0,0 +1,8 @@ +{{#d-modal-body title="groups.default_notifications.modal_title"}} + {{i18n "groups.default_notifications.modal_description" count=model.count}} +{{/d-modal-body}} + +
+ {{d-button action=(action "updateExistingUsers") class="btn-primary" label="groups.default_notifications.modal_yes"}} + {{d-button action=(action "cancel") label="groups.default_notifications.modal_no"}} +
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 14687f789a7..ec07ac68e02 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -155,10 +155,25 @@ class GroupsController < ApplicationController params_with_permitted = group_params(automatic: group.automatic) clear_disabled_email_settings(group, params_with_permitted) + categories, tags = [] + if !group.automatic || current_user.admin + categories, tags = user_default_notifications(group, params_with_permitted) + + if params[:update_existing_users].blank? + user_count = count_existing_users(group.group_users, categories, tags) + + if user_count > 0 + render json: { user_count: user_count } + return + end + end + end + if group.update(params_with_permitted) GroupActionLogger.new(current_user, group, skip_guardian: true).log_change_group_settings group.record_email_setting_changes!(current_user) group.expire_imap_mailbox_cache + update_existing_users(group.group_users, categories, tags) if categories.present? || tags.present? if guardian.can_see?(group) render json: success_json @@ -757,4 +772,154 @@ class GroupsController < ApplicationController params_with_permitted[:email_password] = nil end end + + def user_default_notifications(group, params) + category_notifications = group.group_category_notification_defaults.pluck(:category_id, :notification_level).to_h + tag_notifications = group.group_tag_notification_defaults.pluck(:tag_id, :notification_level).to_h + categories = {} + tags = {} + + NotificationLevels.all.each do |key, value| + category_ids = (params["#{key}_category_ids".to_sym] || []) - ["-1"] + + category_ids.each do |category_id| + category_id = category_id.to_i + old_value = category_notifications[category_id] + + metadata = { + old_value: old_value, + new_value: value + } + + if old_value.blank? + metadata[:action] = :create + elsif old_value == value + category_notifications.delete(category_id) + next + else + metadata[:action] = :update + end + + categories[category_id] = metadata + end + + tag_names = (params["#{key}_tags".to_sym] || []) - ["-1"] + tag_ids = Tag.where(name: tag_names).pluck(:id) + + tag_ids.each do |tag_id| + old_value = tag_notifications[tag_id] + + metadata = { + old_value: old_value, + new_value: value + } + + if old_value.blank? + metadata[:action] = :create + elsif old_value == value + tag_notifications.delete(tag_id) + next + else + metadata[:action] = :update + end + + tags[tag_id] = metadata + end + end + + (category_notifications.keys - categories.keys).each do |category_id| + categories[category_id] = { action: :delete, old_value: category_notifications[category_id] } + end + + (tag_notifications.keys - tags.keys).each do |tag_id| + tags[tag_id] = { action: :delete, old_value: tag_notifications[tag_id] } + end + + [categories, tags] + end + + %i{ + count + update + }.each do |action| + define_method("#{action}_existing_users") do |group_users, categories, tags| + return 0 if categories.blank? && tags.blank? + + ids = [] + + categories.each do |category_id, data| + if data[:action] == :update || data[:action] == :delete + category_users = CategoryUser.where(category_id: category_id, notification_level: data[:old_value], user_id: group_users.select(:user_id)) + + if action == :update + category_users.delete_all + else + ids += category_users.pluck(:user_id) + end + + categories.delete(category_id) if data[:action] == :delete && action == :update + end + end + + tags.each do |tag_id, data| + if data[:action] == :update || data[:action] == :delete + tag_users = TagUser.where(tag_id: tag_id, notification_level: data[:old_value], user_id: group_users.select(:user_id)) + + if action == :update + tag_users.delete_all + else + ids += tag_users.pluck(:user_id) + end + + tags.delete(tag_id) if data[:action] == :delete && action == :update + end + end + + if categories.present? || tags.present? + group_users.select(:id, :user_id).find_in_batches do |batch| + user_ids = batch.pluck(:user_id) + + categories.each do |category_id, data| + category_users = [] + existing_users = CategoryUser.where(category_id: category_id, user_id: user_ids).where("notification_level IS NOT NULL") + skip_user_ids = existing_users.pluck(:user_id) + + batch.each do |group_user| + next if skip_user_ids.include?(group_user.user_id) + category_users << { category_id: category_id, user_id: group_user.user_id, notification_level: data[:new_value] } + end + + next if category_users.blank? + + if action == :update + CategoryUser.insert_all!(category_users) + else + ids += category_users.pluck(:user_id) + end + end + + tags.each do |tag_id, data| + tag_users = [] + existing_users = TagUser.where(tag_id: tag_id, user_id: user_ids).where("notification_level IS NOT NULL") + skip_user_ids = existing_users.pluck(:user_id) + + batch.each do |group_user| + next if skip_user_ids.include?(group_user.user_id) + tag_users << { tag_id: tag_id, user_id: group_user.user_id, notification_level: data[:new_value], created_at: Time.now, updated_at: Time.now } + end + + next if tag_users.blank? + + if action == :update + TagUser.insert_all!(tag_users) + else + ids += tag_users.pluck(:user_id) + end + end + end + end + + ids.uniq.count + end + end end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index ad549a9313b..6ffc48acc93 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -892,6 +892,11 @@ en: flair_type: icon: "Select an icon" image: "Upload an image" + default_notifications: + modal_title: "User default notifications" + modal_description: "Would you like to apply this change historically? This will change preferences for %{count} existing users." + modal_yes: "Yes" + modal_no: "No, only apply change going forward" user_action_groups: "1": "Likes" diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 4b0e22a2685..b0e31de1d8b 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -714,7 +714,8 @@ describe GroupsController do name: 'testing', tracking_category_ids: [category.id], tracking_tags: [tag.name] - } + }, + update_existing_users: false } end.to change { GroupHistory.count }.by(13) @@ -783,7 +784,8 @@ describe GroupsController do members_visibility_level: 3, tracking_category_ids: [category.id], tracking_tags: [tag.name] - } + }, + update_existing_users: false } expect(response.status).to eq(200) @@ -852,6 +854,75 @@ describe GroupsController do expect(event[:event_name]).to eq(:group_updated) expect(event[:params].first).to eq(group) end + + context "user default notifications" do + it "should update default notification preference for existing users" do + user1 = Fabricate(:user) + user2 = Fabricate(:user) + CategoryUser.create!(user: user1, category: category, notification_level: 4) + TagUser.create!(user: user1, tag: tag, notification_level: 4) + TagUser.create!(user: user2, tag: tag, notification_level: 4) + group.add(user1) + group.add(user2) + + put "/groups/#{group.id}.json", params: { + group: { + flair_color: 'BBB', + name: 'testing', + incoming_email: 'test@mail.org', + primary_group: true, + automatic_membership_email_domains: 'test.org', + grant_trust_level: 2, + visibility_level: 1, + members_visibility_level: 3, + tracking_category_ids: [category.id], + tracking_tags: [tag.name] + } + } + + expect(response.status).to eq(200) + expect(response.parsed_body["user_count"]).to eq(group.group_users.count - 1) + + put "/groups/#{group.id}.json", params: { + group: { + flair_color: 'BBB', + name: 'testing', + incoming_email: 'test@mail.org', + primary_group: true, + automatic_membership_email_domains: 'test.org', + grant_trust_level: 2, + visibility_level: 1, + members_visibility_level: 3, + tracking_category_ids: [category.id], + tracking_tags: [tag.name] + }, + update_existing_users: true + } + + expect(response.status).to eq(200) + expect(response.parsed_body["success"]).to eq("OK") + + put "/groups/#{group.id}.json", params: { + group: { + flair_color: 'BBB', + name: 'testing', + incoming_email: 'test@mail.org', + primary_group: true, + automatic_membership_email_domains: 'test.org', + grant_trust_level: 2, + visibility_level: 1, + members_visibility_level: 3, + watching_category_ids: [category.id], + tracking_tags: [tag.name] + }, + update_existing_users: true + } + + expect(response.status).to eq(200) + expect(response.parsed_body["success"]).to eq("OK") + expect(CategoryUser.exists?(user: user2, category: category, notification_level: 3)).to be_truthy + end + end end context "when user is a site moderator" do @@ -889,7 +960,8 @@ describe GroupsController do members_visibility_level: 3, tracking_category_ids: [category.id], tracking_tags: [tag.name] - } + }, + update_existing_users: false } expect(response.status).to eq(200)