From b39c30045bd158db4d304d51f38ae7297ce49c66 Mon Sep 17 00:00:00 2001 From: Gabriel Grubba <70247653+Grubba27@users.noreply.github.com> Date: Thu, 14 Nov 2024 13:00:15 -0300 Subject: [PATCH] FEATURE: Add skip notification option to group invite to topic (#29741) * FEATURE: Add skip notification option to group invite to topic * DEV: rename `skip_notification` to `should_notify` * DEV: update `should_notify` param to be default `true` in controllers * DEV: update spec to use `greater than` instead of `equal to` to prevent flakiness * Update app/controllers/topics_controller.rb Co-authored-by: David Taylor * DEV: merged two `#invite_group` specs into one * DEV: Added test case for `invite-group` in requests spec --------- Co-authored-by: David Taylor --- app/controllers/topics_controller.rb | 8 +++- app/models/topic.rb | 8 ++-- spec/models/topic_spec.rb | 17 +++++++ spec/requests/api/topics_spec.rb | 56 +++++++++++++++++++++++ spec/requests/topics_controller_spec.rb | 59 +++++++++++++++---------- 5 files changed, 120 insertions(+), 28 deletions(-) diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index f755f6eb2e5..f843308c42a 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -757,7 +757,13 @@ class TopicsController < ApplicationController if topic.private_message? guardian.ensure_can_invite_group_to_private_message!(group, topic) - topic.invite_group(current_user, group) + should_notify = + if params[:should_notify].blank? + true + else + params[:should_notify].to_s == "true" + end + topic.invite_group(current_user, group, should_notify: should_notify) render_json_dump BasicGroupSerializer.new(group, scope: guardian, root: "group") else render json: failed_json, status: 422 diff --git a/app/models/topic.rb b/app/models/topic.rb index 2a72df4a541..c15302d5d2f 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -1186,16 +1186,18 @@ class Topic < ActiveRecord::Base SiteSetting.max_allowed_message_recipients end - def invite_group(user, group) + def invite_group(user, group, should_notify: true) TopicAllowedGroup.create!(topic_id: self.id, group_id: group.id) self.allowed_groups.reload last_post = self.posts.order("post_number desc").where("not hidden AND posts.deleted_at IS NULL").first if last_post - Jobs.enqueue(:post_alert, post_id: last_post.id) add_small_action(user, "invited_group", group.name) - Jobs.enqueue(:group_pm_alert, user_id: user.id, group_id: group.id, post_id: last_post.id) + if should_notify + Jobs.enqueue(:post_alert, post_id: last_post.id) + Jobs.enqueue(:group_pm_alert, user_id: user.id, group_id: group.id, post_id: last_post.id) + end end # If the group invited includes the OP of the topic as one of is members, diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 0df2e451a56..2ccce311fb0 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -1239,6 +1239,23 @@ RSpec.describe Topic do expect(notification.notification_type).to eq(Notification.types[:group_message_summary]) end + it "does not create notifications if invite is set to skip notifications" do + Fabricate(:post, topic: topic) + user_watching = Fabricate(:user) + + group.add(topic.user) + group.add(user_watching) + + set_state!(group, topic.user, :watching) + set_state!(group, user_watching, :watching) + + Notification.delete_all + Jobs.run_immediately! + topic.invite_group(topic.user, group, should_notify: false) + + expect(Notification.count).to eq(0) + end + it "removes users in topic_allowed_users who are part of the added group" do admins = Group[:admins] admins.update!(messageable_level: Group::ALIAS_LEVELS[:everyone]) diff --git a/spec/requests/api/topics_spec.rb b/spec/requests/api/topics_spec.rb index adc166264b0..df80458a237 100644 --- a/spec/requests/api/topics_spec.rb +++ b/spec/requests/api/topics_spec.rb @@ -372,6 +372,62 @@ RSpec.describe "topics" do end end + path "/t/{id}/invite-group.json" do + post "Invite group to topic" do + tags "Topics", "Invites" + operationId "inviteGroupToTopic" + consumes "application/json" + parameter name: "Api-Key", in: :header, type: :string, required: true + parameter name: "Api-Username", in: :header, type: :string, required: true + parameter name: :id, in: :path, schema: { type: :string } + + parameter name: :request_body, + in: :body, + schema: { + type: :object, + properties: { + group: { + type: :string, + description: "The name of the group to invite", + }, + should_notify: { + type: :boolean, + description: "Whether to notify the group, it defaults to true", + }, + }, + } + + produces "application/json" + response "200", "invites to a PM" do + schema type: :object, + properties: { + group: { + type: :object, + properties: { + id: { + type: :integer, + }, + name: { + type: :string, + }, + }, + }, + } + + let!(:admins) { Group[:admins] } + let(:request_body) { { group: admins.name } } + let(:pm) { Fabricate(:private_message_topic) } + let(:id) { pm.id } + + run_test! do |response| + data = JSON.parse(response.body) + expect(data["group"]["name"]).to eq(admins.name) + expect(pm.allowed_groups.first.id).to eq(admins.id) + end + end + end + end + path "/t/{id}/bookmark.json" do put "Bookmark topic" do tags "Topics" diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index d8b375ee67f..60d76a353a2 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -3624,29 +3624,6 @@ RSpec.describe TopicsController do end end - describe "#invite_group" do - let!(:admins) { Group[:admins] } - - before do - sign_in(admin) - admins.messageable_level = Group::ALIAS_LEVELS[:everyone] - admins.save! - end - - it "disallows inviting a group to a topic" do - post "/t/#{topic.id}/invite-group.json", params: { group: "admins" } - - expect(response.status).to eq(422) - end - - it "allows inviting a group to a PM" do - post "/t/#{pm.id}/invite-group.json", params: { group: "admins" } - - expect(response.status).to eq(200) - expect(pm.allowed_groups.first.id).to eq(admins.id) - end - end - describe "#make_banner" do it "needs you to be a staff member" do tl4_topic = Fabricate(:topic, user: sign_in(trust_level_4)) @@ -5272,7 +5249,7 @@ RSpec.describe TopicsController do end end - describe "invite_group" do + describe "#invite_group" do let!(:admins) { Group[:admins] } def invite_group(topic, expected_status) @@ -5318,6 +5295,40 @@ RSpec.describe TopicsController do invite_group(pm, 200) expect(pm.allowed_groups.first.id).to eq(admins.id) end + + it "sends a notification to the group" do + user = Fabricate(:user) + Fabricate(:post, topic: pm) + admins.add(user) + admins + .group_users + .find_by(user_id: user.id) + .update!(notification_level: NotificationLevels.all[:watching]) + + Notification.delete_all + Jobs.run_immediately! + post "/t/#{pm.id}/invite-group.json", params: { group: "admins" } + + expect(response.status).to eq(200) + expect(Notification.count).to be > 0 + end + + it "allows disabling notifications for that invite" do + user = Fabricate(:user) + Fabricate(:post, topic: pm) + admins.add(user) + admins + .group_users + .find_by(user_id: user.id) + .update!(notification_level: NotificationLevels.all[:watching]) + + Notification.delete_all + Jobs.run_immediately! + post "/t/#{pm.id}/invite-group.json", params: { group: "admins", should_notify: false } + + expect(response.status).to eq(200) + expect(Notification.count).to eq(0) + end end context "when PM has reached maximum allowed numbers of recipients" do