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.
This commit is contained in:
Martin Brennan
2021-06-17 08:20:09 +10:00
committed by GitHub
parent 651b8a23b8
commit 6fe78cd542
5 changed files with 64 additions and 17 deletions

View File

@ -968,7 +968,7 @@ class TopicsController < ApplicationController
Topic.joins(:tags).where(tags: { name: params[:tag_id] }) Topic.joins(:tags).where(tags: { name: params[:tag_id] })
else else
if params[:tracked].to_s == "true" 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 else
current_user.user_stat.update_column(:new_since, Time.zone.now) current_user.user_stat.update_column(:new_since, Time.zone.now)
Topic Topic

View File

@ -7,6 +7,7 @@
class TopicQuery class TopicQuery
PG_MAX_INT ||= 2147483647 PG_MAX_INT ||= 2147483647
DEFAULT_PER_PAGE_COUNT ||= 30
def self.validators def self.validators
@validators ||= begin @validators ||= begin
@ -578,7 +579,7 @@ class TopicQuery
protected protected
def per_page_setting def per_page_setting
30 DEFAULT_PER_PAGE_COUNT
end end
def private_messages_for(user, type) def private_messages_for(user, type)
@ -702,7 +703,7 @@ class TopicQuery
# Create results based on a bunch of default options # Create results based on a bunch of default options
def default_results(options = {}) def default_results(options = {})
options.reverse_merge!(@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 # Whether to return visible topics
options[:visible] = true if @user.nil? || @user.regular? options[:visible] = true if @user.nil? || @user.regular?

View File

@ -749,16 +749,6 @@ HTML
describe "automatic recompile" do describe "automatic recompile" do
it 'must recompile after bumping theme_field version' 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: :common, name: "header", value: "World")
child.set_field(target: :extra_js, name: "test.js.es6", value: "const hello = 'world';") child.set_field(target: :extra_js, name: "test.js.es6", value: "const hello = 'world';")
child.save! child.save!

View File

@ -2974,7 +2974,7 @@ RSpec.describe TopicsController do
expect(user.user_stat.new_since.to_date).to eq(old_date.to_date) expect(user.user_stat.new_since.to_date).to eq(old_date.to_date)
end 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) sign_in(user)
user.user_stat.update_column(:new_since, 2.years.ago) 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, CategoryUser.set_notification_level_for_category(user,
NotificationLevels.all[:tracking], NotificationLevels.all[:tracking],
tracked_category.id) tracked_category.id)
tracked_topic = create_post.topic tracked_topic = create_post(category: tracked_category).topic
tracked_topic.update!(category_id: tracked_category.id)
create_post # This is a new post, but is not tracked so a record will not be created for it 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
end end

View File

@ -181,4 +181,14 @@ module Helpers
`cd #{repo_dir} && git commit -am 'first commit'` `cd #{repo_dir} && git commit -am 'first commit'`
repo_dir repo_dir
end 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 end