From 73c1d3e7fe242fd3d0c8b0e039d39cea25522067 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Thu, 29 Mar 2018 15:08:22 -0400 Subject: [PATCH] FIX: tag notification preferences were being cleared when other preferences were changed --- .../javascripts/discourse/models/user.js.es6 | 6 ++++ app/controllers/users_controller.rb | 2 +- app/services/user_updater.rb | 4 ++- spec/controllers/users_controller_spec.rb | 8 ++++- spec/services/user_updater_spec.rb | 29 +++++++++++++++---- 5 files changed, 41 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/discourse/models/user.js.es6 b/app/assets/javascripts/discourse/models/user.js.es6 index e6aeb9f676b..5816cd2642f 100644 --- a/app/assets/javascripts/discourse/models/user.js.es6 +++ b/app/assets/javascripts/discourse/models/user.js.es6 @@ -283,6 +283,12 @@ const User = RestModel.extend({ } }); + ['muted_tags', 'tracked_tags', 'watched_tags', 'watching_first_post_tags'].forEach(prop => { + if (fields === undefined || fields.includes(prop)) { + data[prop] = this.get(prop) ? this.get(prop).join(',') : ''; + } + }); + // TODO: We can remove this when migrated fully to rest model. this.set('isSaving', true); return ajax(userPath(`${this.get('username_lower')}.json`), { diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 1a0118a152d..cb88f94fbba 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1064,7 +1064,7 @@ class UsersController < ApplicationController permitted.concat UserUpdater::OPTION_ATTR permitted.concat UserUpdater::CATEGORY_IDS.keys.map { |k| { k => [] } } - permitted.concat UserUpdater::TAG_NAMES.keys.map { |k| { k => [] } } + permitted.concat UserUpdater::TAG_NAMES.keys result = params .permit(permitted) diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb index 305f96fddc1..d2e92f63086 100644 --- a/app/services/user_updater.rb +++ b/app/services/user_updater.rb @@ -73,7 +73,9 @@ class UserUpdater end TAG_NAMES.each do |attribute, level| - TagUser.batch_set(user, level, attributes[attribute]) + if attributes.has_key?(attribute) + TagUser.batch_set(user, level, attributes[attribute]&.split(',') || []) + end end save_options = false diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 62a91144417..8b3cfc4743c 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -1541,12 +1541,14 @@ describe UsersController do it 'allows the update' do user2 = Fabricate(:user) user3 = Fabricate(:user) + tags = [Fabricate(:tag), Fabricate(:tag)] put :update, params: { username: user.username, name: 'Jim Tom', custom_fields: { test: :it }, - muted_usernames: "#{user2.username},#{user3.username}" + muted_usernames: "#{user2.username},#{user3.username}", + watched_tags: "#{tags[0].name},#{tags[1].name}" }, format: :json expect(response).to be_success @@ -1556,6 +1558,10 @@ describe UsersController do expect(user.name).to eq 'Jim Tom' expect(user.custom_fields['test']).to eq 'it' expect(user.muted_users.pluck(:username).sort).to eq [user2.username, user3.username].sort + expect(TagUser.where( + user: user, + notification_level: TagUser.notification_levels[:watching] + ).pluck(:tag_id)).to contain_exactly(tags[0].id, tags[1].id) theme = Theme.create(name: "test", user_selectable: true, user_id: -1) diff --git a/spec/services/user_updater_spec.rb b/spec/services/user_updater_spec.rb index a1003ab4f93..a65004ed7e3 100644 --- a/spec/services/user_updater_spec.rb +++ b/spec/services/user_updater_spec.rb @@ -27,6 +27,10 @@ describe UserUpdater do end describe '#update' do + let(:category) { Fabricate(:category) } + let(:tag) { Fabricate(:tag) } + let(:tag2) { Fabricate(:tag) } + it 'saves user' do user = Fabricate(:user, name: 'Billy Bob') updater = UserUpdater.new(acting_user, user) @@ -37,18 +41,21 @@ describe UserUpdater do end it 'can update categories and tags' do - category = Fabricate(:category) - tag = Fabricate(:tag) - user = Fabricate(:user) updater = UserUpdater.new(acting_user, user) - updater.update(watched_tags: [tag.name], muted_category_ids: [category.id]) + updater.update(watched_tags: "#{tag.name},#{tag2.name}", muted_category_ids: [category.id]) expect(TagUser.where( user_id: user.id, tag_id: tag.id, notification_level: TagUser.notification_levels[:watching] - ).count).to eq(1) + ).exists?).to eq(true) + + expect(TagUser.where( + user_id: user.id, + tag_id: tag2.id, + notification_level: TagUser.notification_levels[:watching] + ).exists?).to eq(true) expect(CategoryUser.where( user_id: user.id, @@ -58,6 +65,18 @@ describe UserUpdater do end + it "doesn't remove notification prefs when updating something else" do + user = Fabricate(:user) + TagUser.create!(user: user, tag: tag, notification_level: TagUser.notification_levels[:watching]) + CategoryUser.create!(user: user, category: category, notification_level: CategoryUser.notification_levels[:muted]) + + updater = UserUpdater.new(acting_user, user) + updater.update(name: "Steve Dave") + + expect(TagUser.where(user: user).count).to eq(1) + expect(CategoryUser.where(user: user).count).to eq(1) + end + it 'updates various fields' do user = Fabricate(:user) updater = UserUpdater.new(acting_user, user)