From ef2362a30fe1a1cb5e7c15e70c5332adf14c7d0b Mon Sep 17 00:00:00 2001 From: Tarek Khalil <45508821+khalilovcmded@users.noreply.github.com> Date: Wed, 27 Mar 2019 09:41:50 +0000 Subject: [PATCH] FEATURE: Introducing new UI for changing User's notification levels (#7248) * FEATURE: Introducing new UI for tracking User's ignored or muted states --- .../discourse/controllers/user.js.es6 | 14 ++- .../javascripts/discourse/models/user.js.es6 | 13 +-- .../javascripts/discourse/templates/user.hbs | 14 +-- .../user-notifications-dropdown.js.es6 | 85 +++++++++++++++++++ app/assets/stylesheets/common.scss | 1 + app/assets/stylesheets/common/base/user.scss | 1 - .../user-notifications-dropdown.scss | 20 +++++ app/controllers/users_controller.rb | 25 +++--- app/serializers/user_serializer.rb | 10 +++ app/serializers/web_hook_user_serializer.rb | 3 + config/locales/client.en.yml | 8 +- config/routes.rb | 3 +- lib/guardian.rb | 11 +++ spec/components/guardian_spec.rb | 56 ++++++++++++ spec/requests/users_controller_spec.rb | 77 +++++++---------- .../web_hook_user_serializer_spec.rb | 2 +- 16 files changed, 251 insertions(+), 92 deletions(-) create mode 100644 app/assets/javascripts/select-kit/components/user-notifications-dropdown.js.es6 create mode 100644 app/assets/stylesheets/common/select-kit/user-notifications-dropdown.scss diff --git a/app/assets/javascripts/discourse/controllers/user.js.es6 b/app/assets/javascripts/discourse/controllers/user.js.es6 index 48d271183eb..038dfccfbd0 100644 --- a/app/assets/javascripts/discourse/controllers/user.js.es6 +++ b/app/assets/javascripts/discourse/controllers/user.js.es6 @@ -33,7 +33,10 @@ export default Ember.Controller.extend(CanCheckEmails, { } return (!indexStream || viewingSelf) && !forceExpand; }, - + canMuteOrIgnoreUser: Ember.computed.or( + "model.can_ignore_user", + "model.can_mute_user" + ), hasGivenFlags: Ember.computed.gt("model.number_of_flags_given", 0), hasFlaggedPosts: Ember.computed.gt("model.number_of_flagged_posts", 0), hasDeletedPosts: Ember.computed.gt("model.number_of_deleted_posts", 0), @@ -147,14 +150,9 @@ export default Ember.Controller.extend(CanCheckEmails, { this.get("adminTools").deleteUser(this.get("model.id")); }, - ignoreUser() { + updateNotificationLevel(level) { const user = this.get("model"); - user.ignore().then(() => user.set("ignored", true)); - }, - - unignoreUser() { - const user = this.get("model"); - user.unignore().then(() => user.set("ignored", false)); + return user.updateNotificationLevel(level); } } }); diff --git a/app/assets/javascripts/discourse/models/user.js.es6 b/app/assets/javascripts/discourse/models/user.js.es6 index 1405295b730..72da24ddb03 100644 --- a/app/assets/javascripts/discourse/models/user.js.es6 +++ b/app/assets/javascripts/discourse/models/user.js.es6 @@ -615,17 +615,10 @@ const User = RestModel.extend({ } }, - ignore() { - return ajax(`${userPath(this.get("username"))}/ignore.json`, { + updateNotificationLevel(level) { + return ajax(`${userPath(this.get("username"))}/notification_level.json`, { type: "PUT", - data: { ignored_user_id: this.get("id") } - }); - }, - - unignore() { - return ajax(`${userPath(this.get("username"))}/ignore.json`, { - type: "DELETE", - data: { ignored_user_id: this.get("id") } + data: { notification_level: level } }); }, diff --git a/app/assets/javascripts/discourse/templates/user.hbs b/app/assets/javascripts/discourse/templates/user.hbs index 0c3669d7899..1a7afc307d8 100644 --- a/app/assets/javascripts/discourse/templates/user.hbs +++ b/app/assets/javascripts/discourse/templates/user.hbs @@ -48,19 +48,9 @@ icon="envelope" label="user.private_message"}} - {{#if model.can_ignore_user}} + {{#if canMuteOrIgnoreUser}}
  • - {{#if model.ignored}} - {{d-button class="btn-default" - action=(action "unignoreUser") - icon="far-eye" - label="user.unignore"}} - {{else}} - {{d-button class="btn-default" - action=(action "ignoreUser") - icon="eye-slash" - label="user.ignore"}} - {{/if}} + {{user-notifications-dropdown user=model updateNotificationLevel=(action "updateNotificationLevel")}}
    • {{/if}} {{/if}} diff --git a/app/assets/javascripts/select-kit/components/user-notifications-dropdown.js.es6 b/app/assets/javascripts/select-kit/components/user-notifications-dropdown.js.es6 new file mode 100644 index 00000000000..4f90a2e5b63 --- /dev/null +++ b/app/assets/javascripts/select-kit/components/user-notifications-dropdown.js.es6 @@ -0,0 +1,85 @@ +import DropdownSelectBox from "select-kit/components/dropdown-select-box"; +import { popupAjaxError } from "discourse/lib/ajax-error"; + +export default DropdownSelectBox.extend({ + classNames: ["user-notifications", "user-notifications-dropdown"], + nameProperty: "label", + allowInitialValueMutation: false, + + computeHeaderContent() { + let content = this._super(...arguments); + if (this.get("user.ignored")) { + this.set("headerIcon", "eye-slash"); + content.name = `${I18n.t("user.user_notifications_ignore_option")}`; + } else if (this.get("user.muted")) { + this.set("headerIcon", "times-circle"); + content.name = `${I18n.t("user.user_notifications_mute_option")}`; + } else { + this.set("headerIcon", "user"); + content.name = `${I18n.t("user.user_notifications_normal_option")}`; + } + return content; + }, + + computeContent() { + const content = []; + + content.push({ + icon: "user", + id: "change-to-normal", + description: I18n.t("user.user_notifications_normal_option_title"), + action: () => this.send("reset"), + label: I18n.t("user.user_notifications_normal_option") + }); + + content.push({ + icon: "times-circle", + id: "change-to-muted", + description: I18n.t("user.user_notifications_mute_option_title"), + action: () => this.send("mute"), + label: I18n.t("user.user_notifications_mute_option") + }); + + if (this.get("user.can_ignore_user")) { + content.push({ + icon: "eye-slash", + id: "change-to-ignored", + description: I18n.t("user.user_notifications_ignore_option_title"), + action: () => this.send("ignore"), + label: I18n.t("user.user_notifications_ignore_option") + }); + } + + return content; + }, + + actions: { + reset() { + this.get("updateNotificationLevel")("normal") + .then(() => { + this.set("user.ignored", false); + this.set("user.muted", false); + this.computeHeaderContent(); + }) + .catch(popupAjaxError); + }, + mute() { + this.get("updateNotificationLevel")("mute") + .then(() => { + this.set("user.ignored", false); + this.set("user.muted", true); + this.computeHeaderContent(); + }) + .catch(popupAjaxError); + }, + ignore() { + this.get("updateNotificationLevel")("ignore") + .then(() => { + this.set("user.ignored", true); + this.set("user.muted", false); + this.computeHeaderContent(); + }) + .catch(popupAjaxError); + } + } +}); diff --git a/app/assets/stylesheets/common.scss b/app/assets/stylesheets/common.scss index 050d1c394b4..c23f1800d6e 100644 --- a/app/assets/stylesheets/common.scss +++ b/app/assets/stylesheets/common.scss @@ -27,6 +27,7 @@ @import "common/select-kit/tag-drop"; @import "common/select-kit/toolbar-popup-menu-options"; @import "common/select-kit/topic-notifications-button"; +@import "common/select-kit/user-notifications-dropdown"; @import "common/select-kit/color-palettes"; @import "common/components/*"; @import "common/input_tip"; diff --git a/app/assets/stylesheets/common/base/user.scss b/app/assets/stylesheets/common/base/user.scss index 62120489435..050a3ade44f 100644 --- a/app/assets/stylesheets/common/base/user.scss +++ b/app/assets/stylesheets/common/base/user.scss @@ -97,7 +97,6 @@ .user-main { .about { - overflow: hidden; width: 100%; margin-bottom: 15px; diff --git a/app/assets/stylesheets/common/select-kit/user-notifications-dropdown.scss b/app/assets/stylesheets/common/select-kit/user-notifications-dropdown.scss new file mode 100644 index 00000000000..a529f38d419 --- /dev/null +++ b/app/assets/stylesheets/common/select-kit/user-notifications-dropdown.scss @@ -0,0 +1,20 @@ +.select-kit { + &.dropdown-select-box { + &.user-notifications-dropdown { + text-align: left; + + .select-kit-body { + width: 485px; + max-width: 485px; + } + + .select-kit-header { + justify-content: center; + } + + .dropdown-select-box-header { + align-items: center; + } + } + } +} diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 239388c2072..d55d07a99b3 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -14,7 +14,7 @@ class UsersController < ApplicationController :pick_avatar, :destroy_user_image, :destroy, :check_emails, :topic_tracking_state, :preferences, :create_second_factor, :update_second_factor, :create_second_factor_backup, :select_avatar, - :ignore, :unignore, :revoke_auth_token + :notification_level, :revoke_auth_token ] skip_before_action :check_xhr, only: [ @@ -995,18 +995,23 @@ class UsersController < ApplicationController render json: success_json end - def ignore + def notification_level raise Discourse::NotFound unless SiteSetting.ignore_user_enabled - guardian.ensure_can_ignore_user!(params[:ignored_user_id]) + user = fetch_user_from_params - IgnoredUser.find_or_create_by!(user: current_user, ignored_user_id: params[:ignored_user_id]) - render json: success_json - end + if params[:notification_level] == "ignore" + guardian.ensure_can_ignore_user!(user.id) + MutedUser.where(user: current_user, muted_user: user).delete_all + IgnoredUser.find_or_create_by!(user: current_user, ignored_user: user) + elsif params[:notification_level] == "mute" + guardian.ensure_can_mute_user!(user.id) + IgnoredUser.where(user: current_user, ignored_user: user).delete_all + MutedUser.find_or_create_by!(user: current_user, muted_user: user) + elsif params[:notification_level] == "normal" + MutedUser.where(user: current_user, muted_user: user).delete_all + IgnoredUser.where(user: current_user, ignored_user: user).delete_all + end - def unignore - raise Discourse::NotFound unless SiteSetting.ignore_user_enabled - - IgnoredUser.where(user: current_user, ignored_user_id: params[:ignored_user_id]).delete_all render json: success_json end diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index ca80c051122..74ac617e861 100644 --- a/app/serializers/user_serializer.rb +++ b/app/serializers/user_serializer.rb @@ -50,7 +50,9 @@ class UserSerializer < BasicUserSerializer :can_edit_name, :stats, :ignored, + :muted, :can_ignore_user, + :can_mute_user, :can_send_private_messages, :can_send_private_message_to_user, :bio_excerpt, @@ -281,6 +283,14 @@ class UserSerializer < BasicUserSerializer IgnoredUser.where(user_id: scope.user&.id, ignored_user_id: object.id).exists? end + def muted + MutedUser.where(user_id: scope.user&.id, muted_user_id: object.id).exists? + end + + def can_mute_user + scope.can_mute_user?(object.id) + end + def can_ignore_user scope.can_ignore_user?(object.id) end diff --git a/app/serializers/web_hook_user_serializer.rb b/app/serializers/web_hook_user_serializer.rb index 1e89fc34d21..183e4cb66bd 100644 --- a/app/serializers/web_hook_user_serializer.rb +++ b/app/serializers/web_hook_user_serializer.rb @@ -12,6 +12,9 @@ class WebHookUserSerializer < UserSerializer can_edit_name can_send_private_messages can_send_private_message_to_user + can_ignore_user + can_mute_user + ignored uploaded_avatar_id has_title_badges bio_cooked diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 42942883094..0b6f2719dba 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -637,8 +637,12 @@ en: new_private_message: "New Message" private_message: "Message" private_messages: "Messages" - ignore: "Ignore" - unignore: "Unignore" + user_notifications_ignore_option: "Ignored" + user_notifications_ignore_option_title: "You will not receive notifications related to this user and all of their topics and replies will be hidden from you." + user_notifications_mute_option: "Muted" + user_notifications_mute_option_title: "You will not receive any notifications related to this user." + user_notifications_normal_option: "Normal" + user_notifications_normal_option_title: "You will be notified if this user replies to you or mentions you, and you will see their topics." activity_stream: "Activity" preferences: "Preferences" profile_hidden: "This user's public profile is hidden." diff --git a/config/routes.rb b/config/routes.rb index 2ba524acba6..64bb07a8c54 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -426,8 +426,7 @@ Discourse::Application.routes.draw do post "#{root_path}/:username/preferences/revoke-auth-token" => "users#revoke_auth_token", constraints: { username: RouteFormat.username } get "#{root_path}/:username/staff-info" => "users#staff_info", constraints: { username: RouteFormat.username } get "#{root_path}/:username/summary" => "users#summary", constraints: { username: RouteFormat.username } - put "#{root_path}/:username/ignore" => "users#ignore", constraints: { username: RouteFormat.username } - delete "#{root_path}/:username/ignore" => "users#unignore", constraints: { username: RouteFormat.username } + put "#{root_path}/:username/notification_level" => "users#notification_level", constraints: { username: RouteFormat.username } get "#{root_path}/:username/invited" => "users#invited", constraints: { username: RouteFormat.username } get "#{root_path}/:username/invited_count" => "users#invited_count", constraints: { username: RouteFormat.username } get "#{root_path}/:username/invited/:filter" => "users#invited", constraints: { username: RouteFormat.username } diff --git a/lib/guardian.rb b/lib/guardian.rb index c582668628f..c5fe976c2f7 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -390,6 +390,17 @@ class Guardian UserExport.where(user_id: @user.id, created_at: (Time.zone.now.beginning_of_day..Time.zone.now.end_of_day)).count == 0 end + def can_mute_user?(user_id) + can_mute_users? && + @user.id != user_id && + User.where(id: user_id, admin: false, moderator: false).exists? + end + + def can_mute_users? + return false if anonymous? + @user.staff? || @user.trust_level >= TrustLevel.levels[:basic] + end + def can_ignore_user?(user_id) can_ignore_users? && @user.id != user_id && User.where(id: user_id, admin: false, moderator: false).exists? end diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index ac827e01ee8..0ceee2a822d 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -9,6 +9,7 @@ describe Guardian do let(:user) { Fabricate(:user) } let(:moderator) { Fabricate(:moderator) } let(:admin) { Fabricate(:admin) } + let(:trust_level_1) { build(:user, trust_level: 1) } let(:trust_level_2) { build(:user, trust_level: 2) } let(:trust_level_3) { build(:user, trust_level: 3) } let(:trust_level_4) { build(:user, trust_level: 4) } @@ -2698,6 +2699,61 @@ describe Guardian do end end + describe '#can_mute_user?' do + + let(:guardian) { Guardian.new(trust_level_1) } + + context "when muted user is the same as guardian user" do + it 'does not allow muting user' do + expect(guardian.can_mute_user?(trust_level_1.id)).to eq(false) + end + end + + context "when muted user is a staff user" do + let!(:admin) { Fabricate(:user, admin: true) } + + it 'does not allow muting user' do + expect(guardian.can_mute_user?(admin.id)).to eq(false) + end + end + + context "when muted user is a normal user" do + let!(:another_user) { Fabricate(:user) } + + it 'allows muting user' do + expect(guardian.can_mute_user?(another_user.id)).to eq(true) + end + end + + context "when muter's trust level is below tl1" do + let(:guardian) { Guardian.new(trust_level_0) } + let!(:another_user) { Fabricate(:user) } + let!(:trust_level_0) { build(:user, trust_level: 0) } + + it 'does not allow muting user' do + expect(guardian.can_mute_user?(another_user.id)).to eq(false) + end + end + + context "when muter is staff" do + let(:guardian) { Guardian.new(admin) } + let!(:another_user) { Fabricate(:user) } + + it 'allows muting user' do + expect(guardian.can_mute_user?(another_user.id)).to eq(true) + end + end + + context "when muters's trust level is tl1" do + let(:guardian) { Guardian.new(trust_level_1) } + let!(:another_user) { Fabricate(:user) } + + it 'allows muting user' do + expect(guardian.can_mute_user?(another_user.id)).to eq(true) + end + end + end + describe "#allow_themes?" do let(:theme) { Fabricate(:theme) } let(:theme2) { Fabricate(:theme) } diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index fe25b3a04e3..ccff1761278 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -2022,7 +2022,7 @@ describe UsersController do describe '#ignore' do it 'raises an error when not logged in' do - put "/u/#{user.username}/ignore.json", params: { ignored_user_id: "" } + put "/u/#{user.username}/notification_level.json", params: { notification_level: "" } expect(response.status).to eq(403) end @@ -2033,58 +2033,43 @@ describe UsersController do sign_in(user) end - describe 'when SiteSetting.ignore_user_enabled is false' do - it 'raises an error' do - SiteSetting.ignore_user_enabled = false - put "/u/#{user.username}/ignore.json" + context 'when ignore_user_enable is OFF' do + it 'raises an error when not logged in' do + put "/u/#{another_user.username}/notification_level.json", params: { notification_level: "" } expect(response.status).to eq(404) end end - describe 'when SiteSetting.ignore_user_enabled is true' do - it 'creates IgnoredUser record' do - SiteSetting.ignore_user_enabled = true - put "/u/#{user.username}/ignore.json", params: { ignored_user_id: another_user.id } - expect(response.status).to eq(200) - expect(IgnoredUser.find_by(user_id: user.id, - ignored_user_id: another_user.id)).to be_present - end - end - end - end - - describe '#watch' do - it 'raises an error when not logged in' do - delete "/u/#{user.username}/ignore.json" - expect(response.status).to eq(403) - end - - context 'while logged in' do - let(:user) { Fabricate(:user) } - let(:another_user) { Fabricate(:user) } - before do - sign_in(user) - end - - describe 'when SiteSetting.ignore_user_enabled is false' do - it 'raises an error' do - SiteSetting.ignore_user_enabled = false - delete "/u/#{user.username}/ignore.json", params: { ignored_user_id: another_user.id } - expect(response.status).to eq(404) - end - end - - describe 'when SiteSetting.ignore_user_enabled is true' do + context 'when ignore_user_enable is ON' do before do - Fabricate(:ignored_user, user_id: user.id, ignored_user_id: another_user.id) + SiteSetting.ignore_user_enabled = true end - it 'destroys IgnoredUser record' do - SiteSetting.ignore_user_enabled = true - delete "/u/#{user.username}/ignore.json", params: { ignored_user_id: another_user.id } - expect(response.status).to eq(200) - expect(IgnoredUser.find_by(user_id: user.id, - ignored_user_id: another_user.id)).to be_blank + let!(:ignored_user) { Fabricate(:ignored_user, user: user, ignored_user: another_user) } + let!(:muted_user) { Fabricate(:muted_user, user: user, muted_user: another_user) } + + context 'when changing notification level to normal' do + it 'changes notification level to normal' do + put "/u/#{another_user.username}/notification_level.json", params: { notification_level: "normal" } + expect(IgnoredUser.count).to eq(0) + expect(MutedUser.count).to eq(0) + end + end + + context 'when changing notification level to mute' do + it 'changes notification level to mute' do + put "/u/#{another_user.username}/notification_level.json", params: { notification_level: "mute" } + expect(IgnoredUser.count).to eq(0) + expect(MutedUser.find_by(user_id: user.id, muted_user_id: another_user.id)).to be_present + end + end + + context 'when changing notification level to ignore' do + it 'changes notification level to mute' do + put "/u/#{another_user.username}/notification_level.json", params: { notification_level: "ignore" } + expect(MutedUser.count).to eq(0) + expect(IgnoredUser.find_by(user_id: user.id, ignored_user_id: another_user.id)).to be_present + end end end end diff --git a/spec/serializers/web_hook_user_serializer_spec.rb b/spec/serializers/web_hook_user_serializer_spec.rb index f74d5cfce71..65b5c8d876d 100644 --- a/spec/serializers/web_hook_user_serializer_spec.rb +++ b/spec/serializers/web_hook_user_serializer_spec.rb @@ -21,7 +21,7 @@ RSpec.describe WebHookUserSerializer do it 'should only include the required keys' do count = serializer.as_json.keys.count - difference = count - 46 + difference = count - 45 expect(difference).to eq(0), lambda { message = ""