From 824c2357608dd4b227fab861a3306cf56c539f07 Mon Sep 17 00:00:00 2001 From: cpradio Date: Mon, 14 Nov 2016 22:03:16 -0500 Subject: [PATCH 1/2] FEATURE: Notify user when mention can't see the reply they were mentioned in FIX: Group Mention Notifications --- .../components/composer-editor.js.es6 | 30 ++++++++++++---- .../discourse/controllers/composer.js.es6 | 16 +++++++++ .../discourse/lib/link-mentions.js.es6 | 21 +++++++---- .../discourse/templates/composer.hbs | 1 + app/controllers/users_controller.rb | 9 ++++- config/locales/client.en.yml | 3 ++ spec/controllers/users_controller_spec.rb | 35 +++++++++++++++++++ 7 files changed, 100 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6 index 4ba0c200574..06e04cf75c1 100644 --- a/app/assets/javascripts/discourse/components/composer-editor.js.es6 +++ b/app/assets/javascripts/discourse/components/composer-editor.js.es6 @@ -137,9 +137,10 @@ export default Ember.Component.extend({ }, _renderUnseenMentions($preview, unseen) { - fetchUnseenMentions(unseen).then(() => { + fetchUnseenMentions(unseen, this.get('topic.id')).then(() => { linkSeenMentions($preview, this.siteSettings); this._warnMentionedGroups($preview); + this._warnCannotSeeMention($preview); }); }, @@ -170,19 +171,33 @@ export default Ember.Component.extend({ _warnMentionedGroups($preview) { Ember.run.scheduleOnce('afterRender', () => { - this._warnedMentions = this._warnedMentions || []; - var found = []; + var found = this.get('warnedGroupMentions') || []; $preview.find('.mention-group.notify').each((idx,e) => { const $e = $(e); var name = $e.data('name'); - found.push(name); - if (this._warnedMentions.indexOf(name) === -1){ - this._warnedMentions.push(name); + if (found.indexOf(name) === -1){ this.sendAction('groupsMentioned', [{name: name, user_count: $e.data('mentionable-user-count')}]); + found.push(name); } }); - this._warnedMentions = found; + this.set('warnedGroupMentions', found); + }); + }, + + _warnCannotSeeMention($preview) { + Ember.run.scheduleOnce('afterRender', () => { + var found = this.get('warnedCannotSeeMentions') || []; + $preview.find('.mention.cannot-see').each((idx,e) => { + const $e = $(e); + var name = $e.data('name'); + if (found.indexOf(name) === -1) { + this.sendAction('cannotSeeMention', [{name: name}]); + found.push(name); + } + }); + + this.set('warnedCannotSeeMentions', found); }); }, @@ -490,6 +505,7 @@ export default Ember.Component.extend({ } this._warnMentionedGroups($preview); + this._warnCannotSeeMention($preview); // Paint category hashtags const unseenCategoryHashtags = linkSeenCategoryHashtags($preview); diff --git a/app/assets/javascripts/discourse/controllers/composer.js.es6 b/app/assets/javascripts/discourse/controllers/composer.js.es6 index da85c4a66da..b25d7d76f65 100644 --- a/app/assets/javascripts/discourse/controllers/composer.js.es6 +++ b/app/assets/javascripts/discourse/controllers/composer.js.es6 @@ -344,6 +344,22 @@ export default Ember.Controller.extend({ }); }); } + }, + + cannotSeeMention(mentions) { + mentions.forEach(mention => { + const translation = (this.get('topic.isPrivateMessage')) ? + 'composer.cannot_see_mention.private' : + 'composer.cannot_see_mention.category'; + const body = I18n.t(translation, { + username: "@" + mention.name + }); + this.appEvents.trigger('composer-messages:create', { + extraClass: 'custom-body', + templateName: 'custom-body', + body + }); + }); } }, diff --git a/app/assets/javascripts/discourse/lib/link-mentions.js.es6 b/app/assets/javascripts/discourse/lib/link-mentions.js.es6 index d41bd7e5b9a..bed1ba9b515 100644 --- a/app/assets/javascripts/discourse/lib/link-mentions.js.es6 +++ b/app/assets/javascripts/discourse/lib/link-mentions.js.es6 @@ -1,16 +1,21 @@ import { ajax } from 'discourse/lib/ajax'; function replaceSpan($e, username, opts) { + let extra = ""; + let extraClass = ""; + if (opts && opts.group) { - let extra = ""; - let extraClass = ""; if (opts.mentionable) { extra = `data-name='${username}' data-mentionable-user-count='${opts.mentionable.user_count}'`; extraClass = "notify"; } $e.replaceWith(`@${username}`); } else { - $e.replaceWith(`@${username}`); + if (opts && opts.cannot_see) { + extra = `data-name='${username}'`; + extraClass = "cannot-see"; + } + $e.replaceWith(`@${username}`); } } @@ -18,6 +23,7 @@ const found = {}; const foundGroups = {}; const mentionableGroups = {}; const checked = {}; +const cannotSee = []; function updateFound($mentions, usernames) { Ember.run.scheduleOnce('afterRender', function() { @@ -25,7 +31,7 @@ function updateFound($mentions, usernames) { const $e = $(e); const username = usernames[i]; if (found[username.toLowerCase()]) { - replaceSpan($e, username); + replaceSpan($e, username, { cannot_see: cannotSee[username] }); } else if (foundGroups[username]) { replaceSpan($e, username, { group: true, mentionable: mentionableGroups[username] }); } else if (checked[username]) { @@ -45,11 +51,12 @@ export function linkSeenMentions($elem, siteSettings) { return []; } -export function fetchUnseenMentions(usernames) { - return ajax("/users/is_local_username", { data: { usernames } }).then(r => { +export function fetchUnseenMentions(usernames, topic_id) { + return ajax("/users/is_local_username", { data: { usernames, topic_id } }).then(r => { r.valid.forEach(v => found[v] = true); r.valid_groups.forEach(vg => foundGroups[vg] = true); - r.mentionable_groups.forEach(mg => mentionableGroups[mg] = true); + r.mentionable_groups.forEach(mg => mentionableGroups[mg.name] = mg); + r.cannot_see.forEach(cs => cannotSee[cs] = true); usernames.forEach(u => checked[u] = true); return r; }); diff --git a/app/assets/javascripts/discourse/templates/composer.hbs b/app/assets/javascripts/discourse/templates/composer.hbs index 2c47fc8fcd2..144427d9ef9 100644 --- a/app/assets/javascripts/discourse/templates/composer.hbs +++ b/app/assets/javascripts/discourse/templates/composer.hbs @@ -88,6 +88,7 @@ draftStatus=model.draftStatus isUploading=isUploading groupsMentioned="groupsMentioned" + cannotSeeMention="cannotSeeMention" importQuote="importQuote" showOptions="showOptions" hideOptions="hideOptions" diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 4f85726b510..ea8e2c12a86 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -242,11 +242,18 @@ class UsersController < ApplicationController usernames -= groups usernames.each(&:downcase!) + cannot_see = [] + topic_id = params[:topic_id] + unless topic_id.blank? + topic = Topic.find_by(id: topic_id) + usernames.each{ |username| cannot_see.push(username) unless Guardian.new(User.find_by_username(username)).can_see?(topic) } + end + result = User.where(staged: false) .where(username_lower: usernames) .pluck(:username_lower) - render json: {valid: result, valid_groups: groups, mentionable_groups: mentionable_groups} + render json: {valid: result, valid_groups: groups, mentionable_groups: mentionable_groups, cannot_see: cannot_see} end def render_available_true diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 99415e82d0b..9fe08a651d0 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1045,6 +1045,9 @@ en: group_mentioned: one: "By mentioning {{group}}, you are about to notify 1 person – are you sure?" other: "By mentioning {{group}}, you are about to notify {{count}} people – are you sure?" + cannot_see_mention: + category: "You mentioned {{username}} but they won't be notified because they do not have access to this category. Would you like to add them to a group that has access to this category?" + private: "You mentioned {{username}} but they won't be notified because they are unable to see this personal message. Would you like to add them to this PM?" duplicate_link: "It looks like your link to {{domain}} was already posted in the topic by @{{username}} in a reply {{ago}} – are you sure you want to post it again?" error: diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 3f6331f0bc6..95a8f2e5737 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -1602,6 +1602,9 @@ describe UsersController do let(:user) { Fabricate(:user) } let(:group) { Fabricate(:group, name: "Discourse") } + let(:topic) { Fabricate(:topic) } + let(:allowed_user) { Fabricate(:user) } + let(:private_topic) { Fabricate(:private_message_topic, user: allowed_user) } it "finds the user" do xhr :get, :is_local_username, username: user.username @@ -1632,6 +1635,38 @@ describe UsersController do expect(json["valid"].size).to eq(0) end + it "returns user who cannot see topic" do + Guardian.any_instance.expects(:can_see?).with(topic).returns(false) + xhr :get, :is_local_username, usernames: [user.username], topic_id: topic.id + expect(response).to be_success + json = JSON.parse(response.body) + expect(json["cannot_see"].size).to eq(1) + end + + it "never returns a user who can see the topic" do + Guardian.any_instance.expects(:can_see?).with(topic).returns(true) + xhr :get, :is_local_username, usernames: [user.username], topic_id: topic.id + expect(response).to be_success + json = JSON.parse(response.body) + expect(json["cannot_see"].size).to eq(0) + end + + it "returns user who cannot see a private topic" do + Guardian.any_instance.expects(:can_see?).with(private_topic).returns(false) + xhr :get, :is_local_username, usernames: [user.username], topic_id: private_topic.id + expect(response).to be_success + json = JSON.parse(response.body) + expect(json["cannot_see"].size).to eq(1) + end + + it "never returns a user who can see the topic" do + Guardian.any_instance.expects(:can_see?).with(private_topic).returns(true) + xhr :get, :is_local_username, usernames: [allowed_user.username], topic_id: private_topic.id + expect(response).to be_success + json = JSON.parse(response.body) + expect(json["cannot_see"].size).to eq(0) + end + end describe '.topic_tracking_state' do From beefc50407a9e441ad6d0eba4e3b05ff1fc91551 Mon Sep 17 00:00:00 2001 From: cpradio Date: Mon, 14 Nov 2016 22:07:07 -0500 Subject: [PATCH 2/2] Update copy to use a statement instead of a question (per @jomaxro) --- config/locales/client.en.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 9fe08a651d0..f4689a68430 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1046,8 +1046,8 @@ en: one: "By mentioning {{group}}, you are about to notify 1 person – are you sure?" other: "By mentioning {{group}}, you are about to notify {{count}} people – are you sure?" cannot_see_mention: - category: "You mentioned {{username}} but they won't be notified because they do not have access to this category. Would you like to add them to a group that has access to this category?" - private: "You mentioned {{username}} but they won't be notified because they are unable to see this personal message. Would you like to add them to this PM?" + category: "You mentioned {{username}} but they won't be notified because they do not have access to this category. You will need to add them to a group that has access to this category." + private: "You mentioned {{username}} but they won't be notified because they are unable to see this personal message. You will need to invite them to this PM." duplicate_link: "It looks like your link to {{domain}} was already posted in the topic by @{{username}} in a reply {{ago}} – are you sure you want to post it again?" error: