diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 314bcc7f939..bc5cb894bb3 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -1045,39 +1045,47 @@ class TopicsController < ApplicationController end def reset_new + topic_scope = + if current_user.new_new_view_enabled? + if (params[:dismiss_topics] && params[:dismiss_posts]) + TopicQuery.new(current_user).new_and_unread_results(limit: false) + elsif params[:dismiss_topics] + TopicQuery.new(current_user).new_results(limit: false) + elsif params[:dismiss_posts] + TopicQuery.new(current_user).unread_results(limit: false) + else + Topic.none + end + else + TopicQuery.new(current_user).new_results(limit: false) + end + if tag_name = params[:tag_id] + tag_name = DiscourseTagging.visible_tags(guardian).where(name: tag_name).pluck(:name).first + end topic_scope = if params[:category_id].present? - category_ids = [params[:category_id]] - if params[:include_subcategories] == "true" + category_ids = [params[:category_id].to_i] + if ActiveModel::Type::Boolean.new.cast(params[:include_subcategories]) category_ids = category_ids.concat(Category.where(parent_category_id: params[:category_id]).pluck(:id)) end - scope = Topic.where(category_id: category_ids) - scope = scope.joins(:tags).where(tags: { name: params[:tag_id] }) if params[:tag_id] + category_ids &= guardian.allowed_category_ids + if category_ids.blank? + scope = topic_scope.none + else + scope = topic_scope.where(category_id: category_ids) + scope = scope.joins(:tags).where(tags: { name: tag_name }) if tag_name + end scope - elsif params[:tag_id].present? - Topic.joins(:tags).where(tags: { name: params[:tag_id] }) + elsif tag_name.present? + topic_scope.joins(:tags).where(tags: { name: tag_name }) else - new_results = - if current_user.new_new_view_enabled? - if (params[:dismiss_topics] && params[:dismiss_posts]) - TopicQuery.new(current_user).new_and_unread_results(limit: false) - elsif params[:dismiss_topics] - TopicQuery.new(current_user).new_results(limit: false) - elsif params[:dismiss_posts] - TopicQuery.new(current_user).unread_results(limit: false) - else - Topic.none - end - else - TopicQuery.new(current_user).new_results(limit: false) - end if params[:tracked].to_s == "true" - TopicQuery.tracked_filter(new_results, current_user.id) + TopicQuery.tracked_filter(topic_scope, current_user.id) else current_user.user_stat.update_column(:new_since, Time.zone.now) - new_results + topic_scope end end @@ -1086,7 +1094,7 @@ class TopicsController < ApplicationController raise Discourse::InvalidParameters.new("Expecting topic_ids to contain a list of topic ids") end - topic_ids = params[:topic_ids].map { |t| t.to_i } + topic_ids = params[:topic_ids].map(&:to_i) topic_scope = topic_scope.where(id: topic_ids) end @@ -1097,7 +1105,6 @@ class TopicsController < ApplicationController TopicsBulkAction.new(current_user, topic_scope.pluck(:id), type: "dismiss_topics").perform! TopicTrackingState.publish_dismiss_new(current_user.id, topic_ids: dismissed_topic_ids) end - if params[:dismiss_posts] if params[:untrack] dismissed_post_topic_ids = diff --git a/lib/topics_bulk_action.rb b/lib/topics_bulk_action.rb index 89ed9f446d2..4ea47b6eb03 100644 --- a/lib/topics_bulk_action.rb +++ b/lib/topics_bulk_action.rb @@ -98,7 +98,7 @@ class TopicsBulkAction end def dismiss_topics - rows = + ids = Topic .where(id: @topic_ids) .joins( @@ -108,9 +108,15 @@ class TopicsBulkAction .where("topic_users.last_read_post_number IS NULL") .order("topics.created_at DESC") .limit(SiteSetting.max_new_topics) - .map { |topic| { topic_id: topic.id, user_id: @user.id, created_at: Time.zone.now } } - DismissedTopicUser.insert_all(rows) if rows.present? - @changed_ids = rows.map { |row| row[:topic_id] } + .filter { |t| guardian.can_see?(t) } + .map(&:id) + + if ids.present? + now = Time.zone.now + rows = ids.map { |id| { topic_id: id, user_id: @user.id, created_at: now } } + DismissedTopicUser.insert_all(rows) + end + @changed_ids = ids end def destroy_post_timing diff --git a/spec/lib/topics_bulk_action_spec.rb b/spec/lib/topics_bulk_action_spec.rb index b6af276f982..555e15e5ef3 100644 --- a/spec/lib/topics_bulk_action_spec.rb +++ b/spec/lib/topics_bulk_action_spec.rb @@ -12,7 +12,7 @@ RSpec.describe TopicsBulkAction do before { topic.destroy! } it "dismisses private messages" do - pm = Fabricate(:private_message_topic) + pm = Fabricate(:private_message_topic, recipient: user) TopicsBulkAction.new(user, [pm.id], type: "dismiss_topics").perform! @@ -73,6 +73,29 @@ RSpec.describe TopicsBulkAction do expect(dismissed_topic_user.topic_id).to eq(topic2.id) expect(dismissed_topic_user.created_at).not_to be_nil end + + it "doesn't dismiss topics the user can't see" do + group = Fabricate(:group) + private_category = Fabricate(:private_category, group: group) + topic2.update!(category_id: private_category.id) + + expect do + TopicsBulkAction.new(user, [topic2.id, topic3.id], type: "dismiss_topics").perform! + end.to change { DismissedTopicUser.count }.by(1) + + expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to eq([topic3.id]) + + group.add(user) + + expect do + TopicsBulkAction.new(user, [topic2.id, topic3.id], type: "dismiss_topics").perform! + end.to change { DismissedTopicUser.count }.by(1) + + expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to contain_exactly( + topic2.id, + topic3.id, + ) + end end describe "dismiss_posts" do diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 745cd75a448..b8e94e72742 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -3975,6 +3975,113 @@ RSpec.describe TopicsController do [category_topic.id, subcategory_topic.id].sort, ) end + + context "when the category has private child categories" do + fab!(:category) { Fabricate(:category) } + fab!(:group) { Fabricate(:group) } + fab!(:private_child_category) do + Fabricate(:private_category, parent_category: category, group: group) + end + fab!(:public_child_category) { Fabricate(:category, parent_category: category) } + fab!(:topic_in_private_child_category) do + Fabricate(:topic, category: private_child_category) + end + fab!(:topic_in_public_child_category) do + Fabricate(:topic, category: public_child_category) + end + + it "doesn't dismiss topics in private child categories that the user can't see" do + messages = + MessageBus.track_publish do + put "/topics/reset-new.json", + params: { + category_id: category.id, + include_subcategories: true, + } + end + expect(response.status).to eq(200) + expect(messages.size).to eq(1) + expect(messages[0].channel).to eq(TopicTrackingState.unread_channel_key(user.id)) + expect(messages[0].user_ids).to eq([user.id]) + expect(messages[0].data["message_type"]).to eq( + TopicTrackingState::DISMISS_NEW_MESSAGE_TYPE, + ) + expect(messages[0].data["payload"]["topic_ids"]).to eq( + [topic_in_public_child_category.id], + ) + expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to eq( + [topic_in_public_child_category.id], + ) + end + + it "dismisses topics in private child categories that the user can see" do + group.add(user) + messages = + MessageBus.track_publish do + put "/topics/reset-new.json", + params: { + category_id: category.id, + include_subcategories: true, + } + end + expect(response.status).to eq(200) + expect(messages.size).to eq(1) + expect(messages[0].channel).to eq(TopicTrackingState.unread_channel_key(user.id)) + expect(messages[0].user_ids).to eq([user.id]) + expect(messages[0].data["message_type"]).to eq( + TopicTrackingState::DISMISS_NEW_MESSAGE_TYPE, + ) + expect(messages[0].data["payload"]["topic_ids"]).to contain_exactly( + topic_in_public_child_category.id, + topic_in_private_child_category.id, + ) + expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to contain_exactly( + topic_in_public_child_category.id, + topic_in_private_child_category.id, + ) + end + end + + context "when the category is private" do + fab!(:group) { Fabricate(:group) } + fab!(:private_category) { Fabricate(:private_category, group: group) } + fab!(:topic_in_private_category) { Fabricate(:topic, category: private_category) } + + it "doesn't dismiss topics or publish topic IDs via MessageBus if the user can't access the category" do + messages = + MessageBus.track_publish do + put "/topics/reset-new.json", params: { category_id: private_category.id } + end + expect(response.status).to eq(200) + expect(messages.size).to eq(1) + expect(messages[0].channel).to eq(TopicTrackingState.unread_channel_key(user.id)) + expect(messages[0].user_ids).to eq([user.id]) + expect(messages[0].data["message_type"]).to eq( + TopicTrackingState::DISMISS_NEW_MESSAGE_TYPE, + ) + expect(messages[0].data["payload"]["topic_ids"]).to eq([]) + expect(DismissedTopicUser.where(user_id: user.id).count).to eq(0) + end + + it "dismisses topics and publishes the dismissed topic IDs if the user can access the category" do + group.add(user) + messages = + MessageBus.track_publish do + put "/topics/reset-new.json", params: { category_id: private_category.id } + end + expect(response.status).to eq(200) + expect(messages.size).to eq(1) + expect(messages[0].channel).to eq(TopicTrackingState.unread_channel_key(user.id)) + expect(messages[0].user_ids).to eq([user.id]) + expect(messages[0].data["message_type"]).to eq( + TopicTrackingState::DISMISS_NEW_MESSAGE_TYPE, + ) + expect(messages[0].data["payload"]["topic_ids"]).to eq([topic_in_private_category.id]) + expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to eq( + [topic_in_private_category.id], + ) + end + end end context "with tag" do @@ -3986,6 +4093,54 @@ RSpec.describe TopicsController do put "/topics/reset-new.json?tag_id=#{tag.name}" expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to eq([tag_topic.id]) end + + context "when the tag is restricted" do + fab!(:restricted_tag) { Fabricate(:tag, name: "restricted-tag") } + fab!(:topic_with_restricted_tag) { Fabricate(:topic, tags: [restricted_tag]) } + fab!(:group) { Fabricate(:group) } + fab!(:topic_without_tag) { Fabricate(:topic) } + fab!(:tag_group) do + Fabricate( + :tag_group, + name: "Restricted Tag Group", + tag_names: ["restricted-tag"], + permissions: [[group, TagGroupPermission.permission_types[:full]]], + ) + end + + it "respects the tag param and only dismisses topics tagged with this tag if the user can see it" do + group.add(user) + messages = + MessageBus.track_publish do + put "/topics/reset-new.json", params: { tag_id: restricted_tag.name } + end + expect(messages.size).to eq(1) + expect(messages[0].data["payload"]["topic_ids"]).to contain_exactly( + topic_with_restricted_tag.id, + ) + expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to contain_exactly( + topic_with_restricted_tag.id, + ) + end + + it "ignores the tag param and dismisses all topics if the user can't see the tag" do + messages = + MessageBus.track_publish do + put "/topics/reset-new.json", params: { tag_id: restricted_tag.name } + end + expect(messages.size).to eq(1) + expect(messages[0].data["payload"]["topic_ids"]).to contain_exactly( + topic_with_restricted_tag.id, + tag_topic.id, + topic_without_tag.id, + ) + expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to contain_exactly( + topic_with_restricted_tag.id, + tag_topic.id, + topic_without_tag.id, + ) + end + end end context "with tag and category" do @@ -4034,6 +4189,24 @@ RSpec.describe TopicsController do expect(response.parsed_body["errors"]).to eq(nil) end + it "doesn't dismiss topics that the user can't see" do + private_category = Fabricate(:private_category, group: Fabricate(:group)) + topic2.update!(category_id: private_category.id) + + messages = + MessageBus.track_publish do + put "/topics/reset-new.json", params: { topic_ids: [topic2.id, topic3.id] } + end + expect(messages.size).to eq(1) + expect(messages[0].channel).to eq(TopicTrackingState.unread_channel_key(user.id)) + expect(messages[0].user_ids).to eq([user.id]) + expect(messages[0].data["message_type"]).to eq( + TopicTrackingState::DISMISS_NEW_MESSAGE_TYPE, + ) + expect(messages[0].data["payload"]["topic_ids"]).to eq([topic3.id]) + expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to eq([topic3.id]) + end + describe "when tracked param is true" do it "does not update user_stat.new_since and does not dismiss untracked topics" do put "/topics/reset-new.json?tracked=true",