From 64fd4e78008bbce2d4196a03327b32941e46e59b Mon Sep 17 00:00:00 2001 From: Ben Halpern Date: Tue, 7 Nov 2023 08:22:22 -0500 Subject: [PATCH] Adjust VariantQuery to dynamically account for recent last_comment_at (#20323) * Adjust VariantQuery to dynamically account for recent last_comment_at * Adjust test line which was temporarily adjusted --- app/services/articles/feeds/variant_query.rb | 19 ++++++++------- .../articles/feeds/variant_query_spec.rb | 24 +++++++++++++++++++ 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/app/services/articles/feeds/variant_query.rb b/app/services/articles/feeds/variant_query.rb index 50f9f19c9..c4def78a9 100644 --- a/app/services/articles/feeds/variant_query.rb +++ b/app/services/articles/feeds/variant_query.rb @@ -58,7 +58,11 @@ module Articles user: @user, days_since_published: config.max_days_since_published, ) - @query_parameters = { oldest_published_at: oldest_published_at } + @query_parameters = { + oldest_published_at: oldest_published_at, + conditional_lookback: oldest_published_at - 12.hours, + conditional_comment_timeframe: 6.hours.ago + } configure! end @@ -261,14 +265,13 @@ module Articles end def build_sql_with_where_clauses(only_featured:, must_have_main_image:, omit_article_ids:) - where_clauses = "articles.published = true AND articles.published_at > :oldest_published_at" - # See Articles.published scope discussion regarding the query planner + # Hardcode the values for lookback_hours and comment_hours. + where_clauses = "articles.published = true" where_clauses += " AND articles.published_at < :now" - where_clauses += " AND articles.score >= 0" # We only want positive values here. - - # Without the compact, if we have `omit_article_ids: [nil]` we - # have the following SQL clause: `articles.id NOT IN (NULL)` - # which will immediately omit EVERYTHING from the query. + where_clauses += " AND articles.score >= 0" + where_clauses += " AND ((articles.published_at > :oldest_published_at) + OR (articles.published_at > :conditional_lookback + AND articles.last_comment_at > :conditional_comment_timeframe))" where_clauses += " AND articles.id NOT IN (:omit_article_ids)" unless omit_article_ids.compact.empty? where_clauses += " AND articles.featured = true" if only_featured where_clauses += " AND articles.main_image IS NOT NULL" if must_have_main_image diff --git a/spec/services/articles/feeds/variant_query_spec.rb b/spec/services/articles/feeds/variant_query_spec.rb index 38c9a8a7b..191e505bc 100644 --- a/spec/services/articles/feeds/variant_query_spec.rb +++ b/spec/services/articles/feeds/variant_query_spec.rb @@ -34,6 +34,30 @@ RSpec.describe Articles::Feeds::VariantQuery, type: :service do expect(query_call).to be_a(ActiveRecord::Relation) expect(query_call.to_a).not_to match_array(article) end + + it "does not return an article published 27 hours before last page view if last comment at is too old" do + create_list(:page_view, 5, user: user) # Recent pageviews + article = create(:article) + article.update_columns(published_at: 27.hours.ago, last_comment_at: 22.hours.ago) + expect(query_call).to be_a(ActiveRecord::Relation) + expect(query_call.to_a).not_to include(article) + end + + it "does return article published 27 hours before last page view if last comment is within 6 hours" do + create_list(:page_view, 5, user: user) # Recent pageviews + article = create(:article) + article.update_columns(published_at: 27.hours.ago, last_comment_at: 4.hours.ago) + expect(query_call).to be_a(ActiveRecord::Relation) + expect(query_call.to_a).to include(article) + end + + it "does not return article published 38 hours before last page view if last comment is within 6 hours" do + create_list(:page_view, 5, user: user) # Recent pageviews + article = create(:article) + article.update_columns(published_at: 38.hours.ago, last_comment_at: 4.hours.ago) + expect(query_call).to be_a(ActiveRecord::Relation) + expect(query_call.to_a).not_to include(article) + end end describe "#featured_story_and_default_home_feed" do