From e3b4998efc2d62dfa8401af17fa6a543572c6ddf Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Thu, 3 Mar 2022 00:27:45 +0200 Subject: [PATCH] DEV: Remove notify user topic from share modal (#16085) This feature was rarely used, could be used for spamming users and was impossible to add a context to why the user was notified of a topic. A simple private messages that includes the link and personalized message can be used instead. --- .../discourse/app/controllers/share-topic.js | 54 ------------------- .../app/templates/modal/share-topic.hbs | 30 ----------- .../tests/acceptance/share-topic-test.js | 7 --- app/controllers/topics_controller.rb | 42 --------------- config/locales/client.en.yml | 6 --- config/routes.rb | 1 - spec/requests/topics_controller_spec.rb | 40 -------------- 7 files changed, 180 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/share-topic.js b/app/assets/javascripts/discourse/app/controllers/share-topic.js index 31599e3b14f..6c0822bf35f 100644 --- a/app/assets/javascripts/discourse/app/controllers/share-topic.js +++ b/app/assets/javascripts/discourse/app/controllers/share-topic.js @@ -2,8 +2,6 @@ import Controller from "@ember/controller"; import { action } from "@ember/object"; import { getAbsoluteURL } from "discourse-common/lib/get-url"; import discourseComputed from "discourse-common/utils/decorators"; -import { ajax } from "discourse/lib/ajax"; -import { extractError } from "discourse/lib/ajax-error"; import Sharing from "discourse/lib/sharing"; import showModal from "discourse/lib/show-modal"; import { bufferedProperty } from "discourse/mixins/buffered-content"; @@ -19,7 +17,6 @@ export default Controller.extend( topic: null, post: null, allowInvites: false, - showNotifyUsers: false, restrictedGroups: null, onShow() { @@ -27,7 +24,6 @@ export default Controller.extend( topic: null, post: null, allowInvites: false, - showNotifyUsers: false, }); if (this.model && this.model.read_restricted) { @@ -71,66 +67,16 @@ export default Controller.extend( ); }, - @action - onChangeUsers(usernames) { - this.set("users", usernames.uniq()); - }, - @action share(source) { - this.set("showNotifyUsers", false); Sharing.shareSource(source, { title: this.topic.title, url: this.url, }); }, - @action - toggleNotifyUsers() { - if (this.showNotifyUsers) { - this.set("showNotifyUsers", false); - } else { - this.setProperties({ - showNotifyUsers: true, - users: [], - }); - } - }, - - @action - notifyUsers() { - if (this.users.length === 0) { - return; - } - - ajax(`/t/${this.topic.id}/invite-notify`, { - type: "POST", - data: { - usernames: this.users, - post_number: this.post ? this.post.post_number : undefined, - }, - }) - .then(() => { - this.setProperties({ showNotifyUsers: false }); - this.appEvents.trigger("modal-body:flash", { - text: I18n.t("topic.share.notify_users.success", { - count: this.users.length, - username: this.users[0], - }), - messageClass: "success", - }); - }) - .catch((error) => { - this.appEvents.trigger("modal-body:flash", { - text: extractError(error), - messageClass: "error", - }); - }); - }, - @action inviteUsers() { - this.set("showNotifyUsers", false); const controller = showModal("create-invite"); controller.setProperties({ inviteToTopic: true, diff --git a/app/assets/javascripts/discourse/app/templates/modal/share-topic.hbs b/app/assets/javascripts/discourse/app/templates/modal/share-topic.hbs index 8c4c3a7a857..fa70e7a10a5 100644 --- a/app/assets/javascripts/discourse/app/templates/modal/share-topic.hbs +++ b/app/assets/javascripts/discourse/app/templates/modal/share-topic.hbs @@ -21,13 +21,6 @@ {{share-source source=s title=topic.title action=(action "share")}} {{/each}} - {{d-button - class="btn-default notify" - label="topic.share.notify_users.title" - icon="hand-point-right" - action=(action "toggleNotifyUsers") - }} - {{#if allowInvites}} {{d-button class="btn-default invite" @@ -37,29 +30,6 @@ }} {{/if}} - - {{#if showNotifyUsers}} -
- -
- {{user-chooser - value=users - onChange=(action "onChangeUsers") - options=(hash - topicId=topic.id - maximum=(unless currentUser.staff 1) - excludeCurrentUser=true - ) - }} - {{d-button - icon="check" - class="btn-primary" - disabled=(if users false true) - action=(action "notifyUsers") - }} -
-
- {{/if}} {{/d-modal-body}} diff --git a/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js b/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js index 0823af574f4..fdd6601452b 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js @@ -1,7 +1,6 @@ import { click, visit } from "@ember/test-helpers"; import { acceptance, - count, exists, queryAll, } from "discourse/tests/helpers/qunit-helpers"; @@ -30,12 +29,6 @@ acceptance("Share and Invite modal", function (needs) { "it shows the topic sharing url" ); - assert.ok(count("button[class*='share-']") > 1, "it shows social sources"); - assert.ok( - exists(".link-share-actions .notify"), - "it shows the notify button" - ); - assert.ok( exists(".link-share-actions .invite"), "it shows the invite button" diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index ceedbdcd408..5d1513fbcfd 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -684,48 +684,6 @@ class TopicsController < ApplicationController end end - def invite_notify - topic = Topic.find_by(id: params[:topic_id]) - guardian.ensure_can_see!(topic) - - usernames = params[:usernames] - raise Discourse::InvalidParameters.new(:usernames) if !usernames.kind_of?(Array) || (!current_user.staff? && usernames.size > 1) - - users = User.where(username_lower: usernames.map(&:downcase)) - raise Discourse::InvalidParameters.new(:usernames) if usernames.size != users.size - - post_number = 1 - post_number = params[:post_number].to_i if params[:post_number].present? - raise Discourse::InvalidParameters.new(:post_number) if post_number < 1 || post_number > topic.highest_post_number - - topic.rate_limit_topic_invitation(current_user) - - users.find_each do |user| - if !user.guardian.can_see_topic?(topic) - return render json: failed_json.merge(error: I18n.t('topic_invite.user_cannot_see_topic', username: user.username)), status: 422 - end - end - - users.find_each do |user| - last_notification = user.notifications - .where(notification_type: Notification.types[:invited_to_topic]) - .where(topic_id: topic.id) - .where(post_number: post_number) - .where('created_at > ?', 1.hour.ago) - - if !last_notification.exists? - topic.create_invite_notification!( - user, - Notification.types[:invited_to_topic], - current_user.username, - post_number: post_number - ) - end - end - - render json: success_json - end - def invite_group group = Group.find_by(name: params[:group]) raise Discourse::NotFound unless group diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index bf21b38470f..320cb50c290 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2851,12 +2851,6 @@ en: restricted_groups: one: "Only visible to members of group: %{groupNames}" other: "Only visible to members of groups: %{groupNames}" - notify_users: - title: "Notify" - instructions: "Notify the following users about this topic:" - success: - one: "Successfully notified %{username} about this topic." - other: "Successfully notified all users about this topic." invite_users: "Invite" print: diff --git a/config/routes.rb b/config/routes.rb index 2c4aa4eae54..d11419c4c5e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -848,7 +848,6 @@ Discourse::Application.routes.draw do post "t/:topic_id/timings" => "topics#timings", constraints: { topic_id: /\d+/ } post "t/:topic_id/invite" => "topics#invite", constraints: { topic_id: /\d+/ } post "t/:topic_id/invite-group" => "topics#invite_group", constraints: { topic_id: /\d+/ } - post "t/:topic_id/invite-notify" => "topics#invite_notify", constraints: { topic_id: /\d+/ } post "t/:topic_id/move-posts" => "topics#move_posts", constraints: { topic_id: /\d+/ } post "t/:topic_id/merge-topic" => "topics#merge_topic", constraints: { topic_id: /\d+/ } post "t/:topic_id/change-owner" => "topics#change_post_owners", constraints: { topic_id: /\d+/ } diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index ecbab8ff7b1..6b7ca165742 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -2873,46 +2873,6 @@ RSpec.describe TopicsController do end end - describe '#invite_notify' do - before do - topic.update!(highest_post_number: 1) - end - - it 'does not notify same user multiple times' do - sign_in(user) - - expect { post "/t/#{topic.id}/invite-notify.json", params: { usernames: [user_2.username] } } - .to change { Notification.count }.by(1) - expect(response.status).to eq(200) - - expect { post "/t/#{topic.id}/invite-notify.json", params: { usernames: [user_2.username] } } - .to change { Notification.count }.by(0) - expect(response.status).to eq(200) - - freeze_time 1.day.from_now - - expect { post "/t/#{topic.id}/invite-notify.json", params: { usernames: [user_2.username] } } - .to change { Notification.count }.by(1) - expect(response.status).to eq(200) - end - - it 'does not let regular users to notify multiple users' do - sign_in(user) - - expect { post "/t/#{topic.id}/invite-notify.json", params: { usernames: [admin.username, user_2.username] } } - .to change { Notification.count }.by(0) - expect(response.status).to eq(400) - end - - it 'lets staff to notify multiple users' do - sign_in(admin) - - expect { post "/t/#{topic.id}/invite-notify.json", params: { usernames: [user.username, user_2.username] } } - .to change { Notification.count }.by(2) - expect(response.status).to eq(200) - end - end - describe '#invite_group' do let!(:admins) { Group[:admins] }