Fixing logic error for feed (#15611)

* Fixing logic error for feed

Prior to this change, we were forcing features into the feed.

* Adding spec to help ensure parameter match

Prior to this commit, but before I had merged changes into main, the I
had implemented `alias default_home_feed call`.  However, this broke the
interface assumed in the feeds controller.
This commit is contained in:
Jeremy Friesen 2021-12-01 13:48:02 -05:00 committed by GitHub
parent 4efcf8c3d3
commit 6d1268f19b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 24 additions and 15 deletions

View file

@ -273,6 +273,7 @@ module Articles
user: @user,
days_since_published: @days_since_published,
)
@must_have_main_image = true
end
# The goal of this query is to generate a list of articles that
@ -363,6 +364,14 @@ module Articles
#
# @note I really dislike this method name as it is opaque on
# it's purpose.
# @note We're specifically In the LargeForemExperimental implementation, the
# default home feed omits the featured story. In this
# case, I don't want to do that. Instead, I want to see
# how this behaves.
def default_home_feed(**)
call
end
alias more_comments_minimal_weight_randomized call
# The featured story should be the article that:
@ -374,8 +383,6 @@ module Articles
# the `featured = true` attribute. In my envisioned
# implementation, the pagination would omit the featured story.
#
# @param must_have_main_image [Boolean] do we mandate that the
# featured story/stories require an image?
# @return [Array<Article, Array<Article>] a featured story
# Article and an array of Article objects.
#
@ -400,33 +407,24 @@ module Articles
# in the featured story. For non-signed in users, we may
# want to use a completely different set of scoring
# methods.
def featured_story_and_default_home_feed(must_have_main_image: true, **)
def featured_story_and_default_home_feed(**)
# We could parameterize this, but callers would need to
# consider the impact of that decision, and it would break the
# current contract.
number_of_featured_stories = 1
featured_story = call(
only_featured: true,
must_have_main_image: must_have_main_image,
must_have_main_image: @must_have_main_image,
limit: number_of_featured_stories,
offset: 0,
).first
articles = call(
must_have_main_image: must_have_main_image,
# Make sure that we don't include the featured_story
omit_article_ids: [featured_story&.id],
)
[featured_story, articles]
end
# @note In the LargeForemExperimental implementation, the
# default home feed omits the featured story. In this
# case, I don't want to do that. Instead, I want to see
# how this behaves.
def default_home_feed(must_have_main_image: true, **)
call(must_have_main_image: must_have_main_image)
end
private
# Concatenate the required group by clauses.

View file

@ -3,6 +3,17 @@ require "rails_helper"
RSpec.describe Articles::Feeds::WeightedQueryStrategy, type: :service do
subject(:feed_strategy) { described_class.new(user: user) }
let(:user) { nil }
describe "#default_home_feed" do
# This test helps test the common interface between the
# WeightedQueryStrategy and the LargeForemExperimental
it "receives `user_signed_in: false` and behaves" do
response = feed_strategy.default_home_feed(user_signed_in: false)
expect(response).to be_a(ActiveRecord::Relation)
end
end
describe "with a nil user" do
let(:user) { nil }
@ -16,7 +27,7 @@ RSpec.describe Articles::Feeds::WeightedQueryStrategy, type: :service do
it "#call is successful with parameterization" do
# NOTE: I'm not testing the SQL logic, merely that the SQL is
# valid.
response = feed_strategy.call(only_featured: true, must_have_main_image: true)
response = feed_strategy.call(only_featured: true)
expect(response).to be_a(ActiveRecord::Relation)
expect(response).to match_array([])
end
@ -62,7 +73,7 @@ RSpec.describe Articles::Feeds::WeightedQueryStrategy, type: :service do
it "#call is successful with parameterization" do
# NOTE: I'm not testing the SQL logic, merely that the SQL is
# valid.
response = feed_strategy.call(only_featured: true, must_have_main_image: true)
response = feed_strategy.call(only_featured: true)
expect(response).to be_a(ActiveRecord::Relation)
expect(response).to match_array([])
end