From a8ec0c30589218103fcfec59f0e8720463751088 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Fri, 1 Apr 2022 10:25:28 -0400 Subject: [PATCH] Ensuring that established tags don't respond 404 (#17058) Prior to this commit, we determined the 404 status based on the final scoped query of the articles. That scoped query would limit to a timeframe. However, what we want is to not render a 404 status if we have an "established" tag. An established tag means that we have at least one published article (regardless of when we published). Fixes forem/forem#16932 --- .../stories/tagged_articles_controller.rb | 19 +++++++++++++++---- spec/requests/stories/tagged_articles_spec.rb | 14 +++++++++----- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/app/controllers/stories/tagged_articles_controller.rb b/app/controllers/stories/tagged_articles_controller.rb index 3982d7063..d0c6bcc42 100644 --- a/app/controllers/stories/tagged_articles_controller.rb +++ b/app/controllers/stories/tagged_articles_controller.rb @@ -21,7 +21,6 @@ module Stories set_number_of_articles(tag: @tag) set_stories(number_of_articles: @number_of_articles, tag: @tag, page: @page) - not_found_if_not_established(tag: @tag, stories: @stories) set_surrogate_key_header "articles-#{@tag}" set_cache_control_headers(600, @@ -45,11 +44,16 @@ module Stories @number_of_articles = user_signed_in? ? 5 : SIGNED_OUT_RECORD_COUNT end + # @raise [ActiveRecord::NotFound] if we don't have an "established" tag def set_stories(number_of_articles:, page:, tag:) stories = Articles::Feeds::Tag.call(tag.name, number_of_articles: number_of_articles, page: page) stories = stories.approved if tag.requires_approval? + # NOTE: We want to check if there are any stories regardless of timeframe. + not_found unless established?(stories: stories, tag: tag) + + # Now, apply the filter. stories = stories_by_timeframe(stories: stories) @stories = stories.decorate end @@ -58,10 +62,17 @@ module Stories tag.articles.published.where(score: Settings::UserExperience.tag_feed_minimum_score..).count end - def not_found_if_not_established(stories:, tag:) - return if tag.supported? + # Do we have an established tag? That means it's supported OR we have at least one published story. + # + # @param stories [ActiveRecord::Relation
] + # @param tag [Tag] + # @return [TrueClass] if we have published stories for this tag + # @return [FalseClass] if we do not + def established?(stories:, tag:) + return true if tag.supported? + return true if stories.published.exists? - not_found unless stories.any?(&:published?) + false end def stories_by_timeframe(stories:) diff --git a/spec/requests/stories/tagged_articles_spec.rb b/spec/requests/stories/tagged_articles_spec.rb index bad793e22..29f512c47 100644 --- a/spec/requests/stories/tagged_articles_spec.rb +++ b/spec/requests/stories/tagged_articles_spec.rb @@ -63,16 +63,20 @@ RSpec.describe "Stories::TaggedArticlesIndex", type: :request do it "renders page when tag is not supported but has at least one approved article" do unsupported_tag = create(:tag, supported: false) - create(:article, published: true, approved: true, tags: unsupported_tag) + create(:article, published: true, approved: true, tags: unsupported_tag, published_at: 5.years.ago) get "/t/#{unsupported_tag.name}/top/week" - expect(response.body).to include(unsupported_tag.name) + + expect(response).to be_successful + get "/t/#{unsupported_tag.name}/top/month" - expect(response.body).to include(unsupported_tag.name) + expect(response).to be_successful + get "/t/#{unsupported_tag.name}/top/year" - expect(response.body).to include(unsupported_tag.name) + expect(response).to be_successful + get "/t/#{unsupported_tag.name}/top/infinity" - expect(response.body).to include(unsupported_tag.name) + expect(response).to be_successful end it "returns not found if no published posts and tag not supported" do