From 6fe78cd542194576b0a02ff52cb9dbaa79bba82d Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 17 Jun 2021 08:20:09 +1000 Subject: [PATCH] FIX: Make sure reset-new for tracked is not limited by per_page count (#13395) When dismissing new topics for the Tracked filter, the dismiss was limited to 30 topics which is the default per page count for TopicQuery. This happened even if you specified which topic IDs you were selectively dismissing. This PR fixes that bug, and also moves the per_page_count into a DEFAULT_PER_PAGE_COUNT for the TopicQuery so it can be stubbed in tests. Also moves the unused stub_const method into the spec helpers for cases like this; it is much better to handle this in one place with an ensure. In a follow up PR I will clean up other specs that do the same thing and make them use stub_const. --- app/controllers/topics_controller.rb | 2 +- lib/topic_query.rb | 5 ++- spec/models/theme_spec.rb | 10 ----- spec/requests/topics_controller_spec.rb | 54 +++++++++++++++++++++++-- spec/support/helpers.rb | 10 +++++ 5 files changed, 64 insertions(+), 17 deletions(-) diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 010c51bd7f3..7f5e36a4570 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -968,7 +968,7 @@ class TopicsController < ApplicationController Topic.joins(:tags).where(tags: { name: params[:tag_id] }) else if params[:tracked].to_s == "true" - TopicQuery.tracked_filter(TopicQuery.new(current_user).new_results, current_user.id) + TopicQuery.tracked_filter(TopicQuery.new(current_user).new_results(limit: false), current_user.id) else current_user.user_stat.update_column(:new_since, Time.zone.now) Topic diff --git a/lib/topic_query.rb b/lib/topic_query.rb index dd402eb23b9..d09701fae7a 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -7,6 +7,7 @@ class TopicQuery PG_MAX_INT ||= 2147483647 + DEFAULT_PER_PAGE_COUNT ||= 30 def self.validators @validators ||= begin @@ -578,7 +579,7 @@ class TopicQuery protected def per_page_setting - 30 + DEFAULT_PER_PAGE_COUNT end def private_messages_for(user, type) @@ -702,7 +703,7 @@ class TopicQuery # Create results based on a bunch of default options def default_results(options = {}) options.reverse_merge!(@options) - options.reverse_merge!(per_page: per_page_setting) + options.reverse_merge!(per_page: per_page_setting) unless options[:limit] == false # Whether to return visible topics options[:visible] = true if @user.nil? || @user.regular? diff --git a/spec/models/theme_spec.rb b/spec/models/theme_spec.rb index 6d8f12d8432..0d929a29b17 100644 --- a/spec/models/theme_spec.rb +++ b/spec/models/theme_spec.rb @@ -749,16 +749,6 @@ HTML describe "automatic recompile" do it 'must recompile after bumping theme_field version' do - def stub_const(target, const, value) - old = target.const_get(const) - target.send(:remove_const, const) - target.const_set(const, value) - yield - ensure - target.send(:remove_const, const) - target.const_set(const, old) - end - child.set_field(target: :common, name: "header", value: "World") child.set_field(target: :extra_js, name: "test.js.es6", value: "const hello = 'world';") child.save! diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index f72e7d833f7..444fc31afca 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -2974,7 +2974,7 @@ RSpec.describe TopicsController do expect(user.user_stat.new_since.to_date).to eq(old_date.to_date) end - it "creates topic user records for each unread topic" do + it "creates dismissed topic user records for each new topic" do sign_in(user) user.user_stat.update_column(:new_since, 2.years.ago) @@ -2982,11 +2982,57 @@ RSpec.describe TopicsController do CategoryUser.set_notification_level_for_category(user, NotificationLevels.all[:tracking], tracked_category.id) - tracked_topic = create_post.topic - tracked_topic.update!(category_id: tracked_category.id) + tracked_topic = create_post(category: tracked_category).topic create_post # This is a new post, but is not tracked so a record will not be created for it - expect { put "/topics/reset-new.json?tracked=true" }.to change { DismissedTopicUser.where(user_id: user.id).count }.by(1) + expect do + put "/topics/reset-new.json?tracked=true" + end.to change { + DismissedTopicUser.where(user_id: user.id, topic_id: tracked_topic.id).count + }.by(1) + end + + it "creates dismissed topic user records if there are > 30 (default pagination) topics" do + sign_in(user) + tracked_category = Fabricate(:category) + CategoryUser.set_notification_level_for_category(user, + NotificationLevels.all[:tracking], + tracked_category.id) + + topic_ids = [] + 5.times do + topic_ids << create_post(category: tracked_category).topic.id + end + + expect do + stub_const(TopicQuery, "DEFAULT_PER_PAGE_COUNT", 2) do + put "/topics/reset-new.json?tracked=true" + end + end.to change { + DismissedTopicUser.where(user_id: user.id, topic_id: topic_ids).count + }.by(5) + end + + it "creates dismissed topic user records if there are > 30 (default pagination) topics and topic_ids are provided" do + sign_in(user) + tracked_category = Fabricate(:category) + CategoryUser.set_notification_level_for_category(user, + NotificationLevels.all[:tracking], + tracked_category.id) + + topic_ids = [] + 5.times do + topic_ids << create_post(category: tracked_category).topic.id + end + dismissing_topic_ids = topic_ids.sample(4) + + expect do + stub_const(TopicQuery, "DEFAULT_PER_PAGE_COUNT", 2) do + put "/topics/reset-new.json?tracked=true", params: { topic_ids: dismissing_topic_ids } + end + end.to change { + DismissedTopicUser.where(user_id: user.id, topic_id: topic_ids).count + }.by(4) end end diff --git a/spec/support/helpers.rb b/spec/support/helpers.rb index aabf9f97686..417dcdca985 100644 --- a/spec/support/helpers.rb +++ b/spec/support/helpers.rb @@ -181,4 +181,14 @@ module Helpers `cd #{repo_dir} && git commit -am 'first commit'` repo_dir end + + def stub_const(target, const, value) + old = target.const_get(const) + target.send(:remove_const, const) + target.const_set(const, value) + yield + ensure + target.send(:remove_const, const) + target.const_set(const, old) + end end