From ed78f8f479ef02c1eae2b475f04c1583f82faa1e Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Wed, 27 Oct 2021 04:45:53 -0400 Subject: [PATCH] Extracting a common calculator for feed weights (#15199) Prior to this commit, there existed duplicate logic between two of the primary user feeds. This refactor introduces a new object which is solely concerned with calculating scores to add to an article base score. With this commit, we can introduce A/B testing by changing passing different config values to `ArticleScoreCalculatorForUser.new`. My suspicion is that for some of this, we might be able to better leverage the database via select statements and SQL sums. However, that is presently outside of what I'm prepared to tackle. --- .../article_score_calculator_for_user.rb | 89 ++++++++ app/services/articles/feeds/basic.rb | 31 ++- .../feeds/large_forem_experimental.rb | 58 +---- .../article_score_calculator_for_user_spec.rb | 201 ++++++++++++++++++ spec/services/articles/feeds/basic_spec.rb | 4 +- .../feeds/large_forem_experimental_spec.rb | 192 ----------------- 6 files changed, 311 insertions(+), 264 deletions(-) create mode 100644 app/services/articles/feeds/article_score_calculator_for_user.rb create mode 100644 spec/services/articles/feeds/article_score_calculator_for_user_spec.rb diff --git a/app/services/articles/feeds/article_score_calculator_for_user.rb b/app/services/articles/feeds/article_score_calculator_for_user.rb new file mode 100644 index 000000000..c5a898007 --- /dev/null +++ b/app/services/articles/feeds/article_score_calculator_for_user.rb @@ -0,0 +1,89 @@ +module Articles + module Feeds + # @api private + # + # The purpose of this class is to encapsulate how we apply scores + # to an article. + # + # @see Articles::Feeds::Basic + # @see Articles::Feeds::LargeForemExperimental + class ArticleScoreCalculatorForUser + # This constant defines some of the levers that we use to help + # calculate an article's score. + DEFAULT_CONFIGURATION = { + comment_weight: 0.2, + xp_level_weight: 1, + default_user_xp_level: 5, + followed_user_score: 1, + not_followed_user_score: 0, + nil_user_tag_score: 0, + followed_tag_weight: 1, + not_followed_tag_score: 0, + followed_org_score: 1, + not_followed_org_score: 0 + }.freeze + + # @param user [User] the user for whom we're calculating the article score. + # @param config [Hash] exposes the means + # for overriding the default configuration values + # + # @see DEFAULT_CONFIGURATION + def initialize(user:, config: {}) + @user = user + DEFAULT_CONFIGURATION.each_pair do |key, value| + instance_variable_set("@#{key}", config.fetch(key, value)) + end + end + + # @api private + def score_followed_user(article) + user_following_users_ids.include?(article.user_id) ? @followed_user_score : @not_followed_user_score + end + + # @api private + def score_followed_tags(article) + return @nil_user_tag_score unless @user + + article_tags = article.decorate.cached_tag_list_array + user_followed_tags.sum do |tag| + article_tags.include?(tag.name) ? tag.points * @followed_tag_weight : @not_followed_tag_score + end + end + + # @api private + def score_followed_organization(article) + return @not_followed_org_score unless article.organization_id? + + user_following_org_ids.include?(article.organization_id) ? @followed_org_score : @not_followed_org_score + end + + # @api private + def score_experience_level(article) + user_experience_level = @user&.setting&.experience_level || @default_user_xp_level + + # Calculate the distance between the user's experience level + # and that of the article's experience level. + - (((article.experience_level_rating - user_experience_level).abs / 2) * @xp_level_weight) + end + + # @api private + def score_comments(article) + article.comments_count * @comment_weight + end + + private + + def user_followed_tags + @user_followed_tags ||= (@user&.decorate&.cached_followed_tags || []) + end + + def user_following_org_ids + @user_following_org_ids ||= (@user&.cached_following_organizations_ids || []) + end + + def user_following_users_ids + @user_following_users_ids ||= (@user&.cached_following_users_ids || []) + end + end + end +end diff --git a/app/services/articles/feeds/basic.rb b/app/services/articles/feeds/basic.rb index f1c73d695..51a9e7241 100644 --- a/app/services/articles/feeds/basic.rb +++ b/app/services/articles/feeds/basic.rb @@ -6,6 +6,7 @@ module Articles @number_of_articles = number_of_articles @page = page @tag = tag + @article_score_applicator = Articles::Feeds::ArticleScoreCalculatorForUser.new(user: @user) end def feed @@ -18,29 +19,21 @@ module Articles articles = articles.where.not(user_id: UserBlock.cached_blocked_ids_for_blocker(@user.id)) articles.sort_by.with_index do |article, index| - article_tags = article.decorate.cached_tag_list_array - tag_score = user_followed_tags.sum do |tag| - article_tags.include?(tag.name) ? tag.points : 0 - end - user_score = user_following_users_ids.include?(article.user_id) ? 1 : 0 - org_score = user_following_org_ids.include?(article.organization_id) ? 1 : 0 + tag_score = score_followed_tags(article) + user_score = score_followed_user(article) + org_score = score_followed_organization(article) + + # NOTE: Not quite understanding the purpose of the `- + # index`. My guess is that it helps reduce the impact of the + # hotness score on the sort order. tag_score + org_score + user_score - index end.reverse! end - private - - def user_followed_tags - @user_followed_tags ||= (@user&.decorate&.cached_followed_tags || []) - end - - def user_following_org_ids - @user_following_org_ids ||= (@user&.cached_following_organizations_ids || []) - end - - def user_following_users_ids - @user_following_users_ids ||= (@user&.cached_following_users_ids || []) - end + delegate(:score_followed_tags, + :score_followed_user, + :score_followed_organization, + to: :@article_score_applicator) end end end diff --git a/app/services/articles/feeds/large_forem_experimental.rb b/app/services/articles/feeds/large_forem_experimental.rb index 671c7301c..05d5cf148 100644 --- a/app/services/articles/feeds/large_forem_experimental.rb +++ b/app/services/articles/feeds/large_forem_experimental.rb @@ -1,15 +1,12 @@ module Articles module Feeds class LargeForemExperimental - DEFAULT_USER_XP_LEVEL = 5 def initialize(user: nil, number_of_articles: 50, page: 1, tag: nil) @user = user @number_of_articles = number_of_articles @page = page @tag = tag - @tag_weight = 1 # default weight tags play in rankings - @comment_weight = 0.2 # default weight comments play in rankings - @xp_level_weight = 1 # default weight for user experience level + @article_score_applicator = Articles::Feeds::ArticleScoreCalculatorForUser.new(user: user) end def default_home_feed(user_signed_in: false) @@ -58,37 +55,12 @@ module Articles article_points end - # @api private - def score_followed_user(article, follow_user_score: 1, not_followed_user_score: 0) - user_following_users_ids.include?(article.user_id) ? follow_user_score : not_followed_user_score - end - - # @api private - def score_followed_tags(article, nil_user_tag_score: 0, followed_tag_weight: @tag_weight, unfollowed_tag_score: 0) - return nil_user_tag_score unless @user - - article_tags = article.decorate.cached_tag_list_array - user_followed_tags.sum do |tag| - article_tags.include?(tag.name) ? tag.points * followed_tag_weight : unfollowed_tag_score - end - end - - # @api private - def score_followed_organization(article, followed_org_score: 1, not_followed_org_score: 0) - return not_followed_org_score unless article.organization_id? - - user_following_org_ids.include?(article.organization_id) ? followed_org_score : not_followed_org_score - end - - # @api private - def score_experience_level(article, xp_level_weight: @xp_level_weight) - - (((article.experience_level_rating - user_experience_level).abs / 2) * xp_level_weight) - end - - # @api private - def score_comments(article, comment_weight: @comment_weight) - article.comments_count * comment_weight - end + delegate(:score_followed_user, + :score_followed_tags, + :score_followed_organization, + :score_experience_level, + :score_comments, + to: :@article_score_applicator) # @api private def globally_hot_articles(user_signed_in, article_score_threshold: -15, min_rand_limit: 15, max_rand_limit: 80) @@ -113,10 +85,6 @@ module Articles private - def user_experience_level - @user_experience_level ||= @user&.setting&.experience_level || DEFAULT_USER_XP_LEVEL - end - def experimental_hot_story_grab start_time = [(@user.page_views.second_to_last&.created_at || 7.days.ago) - 18.hours, 7.days.ago].max Article.published.limited_column_select.includes(top_comments: :user) @@ -125,18 +93,6 @@ module Articles .order(score: :desc) end - def user_followed_tags - @user_followed_tags ||= (@user&.decorate&.cached_followed_tags || []) - end - - def user_following_org_ids - @user_following_org_ids ||= (@user&.cached_following_organizations_ids || []) - end - - def user_following_users_ids - @user_following_users_ids ||= (@user&.cached_following_users_ids || []) - end - def first_quarter(array) array[0...(array.length / 4)] end diff --git a/spec/services/articles/feeds/article_score_calculator_for_user_spec.rb b/spec/services/articles/feeds/article_score_calculator_for_user_spec.rb new file mode 100644 index 000000000..d900d1223 --- /dev/null +++ b/spec/services/articles/feeds/article_score_calculator_for_user_spec.rb @@ -0,0 +1,201 @@ +require "rails_helper" + +RSpec.describe Articles::Feeds::ArticleScoreCalculatorForUser, type: :service do + let(:user) { create(:user) } + let!(:article) { create(:article) } + let(:applicator) { described_class.new(user: user) } + + describe "#score_followed_user" do + context "when article is written by a followed user" do + before { user.follow(article.user) } + + it "returns a score of 1" do + expect(applicator.score_followed_user(article)).to eq 1 + end + end + + context "when article is not written by a followed user" do + it "returns a score of 0" do + expect(applicator.score_followed_user(article)).to eq 0 + end + end + end + + describe "#score_followed_organization" do + let(:organization) { create(:organization) } + let(:article) { create(:article, organization: organization) } + + context "when article is from a followed organization" do + before { user.follow(organization) } + + it "returns a score of 1" do + expect(applicator.score_followed_organization(article)).to eq 1 + end + end + + context "when article is not from a followed organization" do + it "returns a score of 0" do + expect(applicator.score_followed_organization(article)).to eq 0 + end + end + + context "when article has no organization" do + let(:article) { create(:article) } + + it "returns a score of 0" do + expect(applicator.score_followed_organization(article)).to eq 0 + end + end + end + + describe "#score_followed_tags" do + let(:tag) { create(:tag) } + let(:unfollowed_tag) { create(:tag) } + + context "when article includes a followed tag" do + let(:article) { create(:article, tags: tag.name) } + + before do + user.follow(tag) + user.save + user.follows.last.update(explicit_points: 2) + end + + it "returns the followed tag point value" do + expect(applicator.score_followed_tags(article)).to eq 2 + end + end + + context "when article includes multiple followed tags" do + let(:tag2) { create(:tag) } + let(:article) { create(:article, tags: "#{tag.name}, #{tag2.name}") } + + before do + user.follow(tag) + user.follow(tag2) + user.save + user.follows.each { |follow| follow.update(explicit_points: 2) } + end + + it "returns the sum of followed tag point values" do + expect(applicator.score_followed_tags(article)).to eq 4 + end + end + + context "when article includes an unfollowed tag" do + let(:article) { create(:article, tags: "#{tag.name}, #{unfollowed_tag.name}") } + + before do + user.follow(tag) + user.save + end + + it "doesn't score the unfollowed tag" do + expect(applicator.score_followed_tags(article)).to eq 1 + end + end + + context "when article doesn't include any followed tags" do + let(:article) { create(:article, tags: unfollowed_tag.name) } + + it "returns 0" do + expect(applicator.score_followed_tags(article)).to eq 0 + end + end + + context "when user doesn't follow any tags" do + it "returns 0" do + expect(user.cached_followed_tag_names).to be_empty + expect(applicator.score_followed_tags(article)).to eq 0 + end + end + end + + describe "#score_experience_level" do + let(:article) { create(:article, experience_level_rating: 7) } + + context "when user has a further experience level" do + let(:user) { create(:user) } + + before do + user.setting.update(experience_level: 1) + end + + it "returns negative of (absolute value of the difference between article and user experience) divided by 2" do + expect(applicator.score_experience_level(article)).to eq(-3) + end + + it "returns proper negative when fractional" do + article.experience_level_rating = 8 + expect(applicator.score_experience_level(article)).to eq(-3.5) + end + end + + context "when user has a closer experience level" do + let(:user) { create(:user) } + + before do + user.setting.update(experience_level: 9) + end + + it "returns negative of (absolute value of the difference between article and user experience) divided by 2" do + expect(applicator.score_experience_level(article)).to eq(-1) + end + end + + context "when the user does not have an experience level set" do + let(:user) { create(:user) } + + before do + user.setting.update(experience_level: nil) + end + + it "uses a value of 5 for user experience level" do + expect(applicator.score_experience_level(article)).to eq(-1) + end + end + end + + describe "#score_comments" do + let(:applicator) { described_class.new(user: user, config: { comment_weight: 1 }) } + let(:article_with_one_comment) { create(:article) } + let(:article_with_five_comments) { create(:article) } + + before do + create(:comment, user: user, commentable: article_with_one_comment) + create_list(:comment, 5, user: user, commentable: article_with_five_comments) + article_with_one_comment.update_score + article_with_five_comments.update_score + article_with_one_comment.reload + article_with_five_comments.reload + end + + context "when comment_weight is default of 0" do + it "returns 0 for uncommented articles" do + expect(applicator.score_comments(article)).to eq(0) + end + + it "returns a multiple of the parameterized weight for articles with comments" do + expect(article_with_five_comments.comments_count).to eq(5) + expect(applicator.score_comments(article_with_five_comments)).to eq(5) + end + end + + context "when comment_weight is higher than 0" do + before { applicator.instance_variable_set(:@comment_weight, 2) } + + it "returns 0 for uncommented articles" do + expect(applicator.score_comments(article)).to eq(0) + end + + it "returns a non-zero score for commented upon articles" do + expect(applicator.score_comments(article_with_one_comment)).to be > 0 + end + + it "scores article with more comments high than others" do + expect(applicator.score_comments(article_with_five_comments)) + .to be > applicator.score_comments(article_with_one_comment) + end + end + end +end diff --git a/spec/services/articles/feeds/basic_spec.rb b/spec/services/articles/feeds/basic_spec.rb index 624ef9a36..faf65fb86 100644 --- a/spec/services/articles/feeds/basic_spec.rb +++ b/spec/services/articles/feeds/basic_spec.rb @@ -28,10 +28,10 @@ RSpec.describe Articles::Feeds::Basic, type: :service do let(:feed) { described_class.new(user: user, number_of_articles: 100, page: 1) } it "returns articles with score above 0 sorted by user preference scores" do - allow(feed).to receive(:user_following_users_ids).and_return([old_story.user_id]) + user.follow(old_story.user) old_story_tag = Tag.find_by(name: unique_tag_name) old_story_tag.update(points: 10) - allow(feed).to receive(:user_followed_tags).and_return([old_story_tag]) + user.follow(old_story_tag) result = feed.feed expect(result.first).to eq old_story diff --git a/spec/services/articles/feeds/large_forem_experimental_spec.rb b/spec/services/articles/feeds/large_forem_experimental_spec.rb index ebe8edcb6..13966db03 100644 --- a/spec/services/articles/feeds/large_forem_experimental_spec.rb +++ b/spec/services/articles/feeds/large_forem_experimental_spec.rb @@ -132,198 +132,6 @@ RSpec.describe Articles::Feeds::LargeForemExperimental, type: :service do end end - describe "#score_followed_user" do - context "when article is written by a followed user" do - before { user.follow(article.user) } - - it "returns a score of 1" do - expect(feed.score_followed_user(article)).to eq 1 - end - end - - context "when article is not written by a followed user" do - it "returns a score of 0" do - expect(feed.score_followed_user(article)).to eq 0 - end - end - end - - describe "#score_followed_organization" do - let(:organization) { create(:organization) } - let(:article) { create(:article, organization: organization) } - - context "when article is from a followed organization" do - before { user.follow(organization) } - - it "returns a score of 1" do - expect(feed.score_followed_organization(article)).to eq 1 - end - end - - context "when article is not from a followed organization" do - it "returns a score of 0" do - expect(feed.score_followed_organization(article)).to eq 0 - end - end - - context "when article has no organization" do - let(:article) { create(:article) } - - it "returns a score of 0" do - expect(feed.score_followed_organization(article)).to eq 0 - end - end - end - - describe "#score_followed_tags" do - let(:tag) { create(:tag) } - let(:unfollowed_tag) { create(:tag) } - - context "when article includes a followed tag" do - let(:article) { create(:article, tags: tag.name) } - - before do - user.follow(tag) - user.save - user.follows.last.update(explicit_points: 2) - end - - it "returns the followed tag point value" do - expect(feed.score_followed_tags(article)).to eq 2 - end - end - - context "when article includes multiple followed tags" do - let(:tag2) { create(:tag) } - let(:article) { create(:article, tags: "#{tag.name}, #{tag2.name}") } - - before do - user.follow(tag) - user.follow(tag2) - user.save - user.follows.each { |follow| follow.update(explicit_points: 2) } - end - - it "returns the sum of followed tag point values" do - expect(feed.score_followed_tags(article)).to eq 4 - end - end - - context "when article includes an unfollowed tag" do - let(:article) { create(:article, tags: "#{tag.name}, #{unfollowed_tag.name}") } - - before do - user.follow(tag) - user.save - end - - it "doesn't score the unfollowed tag" do - expect(feed.score_followed_tags(article)).to eq 1 - end - end - - context "when article doesn't include any followed tags" do - let(:article) { create(:article, tags: unfollowed_tag.name) } - - it "returns 0" do - expect(feed.score_followed_tags(article)).to eq 0 - end - end - - context "when user doesn't follow any tags" do - it "returns 0" do - expect(user.cached_followed_tag_names).to be_empty - expect(feed.score_followed_tags(article)).to eq 0 - end - end - end - - describe "#score_experience_level" do - let(:article) { create(:article, experience_level_rating: 7) } - - context "when user has a further experience level" do - let(:user) { create(:user) } - - before do - user.setting.update(experience_level: 1) - end - - it "returns negative of (absolute value of the difference between article and user experience) divided by 2" do - expect(feed.score_experience_level(article)).to eq(-3) - end - - it "returns proper negative when fractional" do - article.experience_level_rating = 8 - expect(feed.score_experience_level(article)).to eq(-3.5) - end - end - - context "when user has a closer experience level" do - let(:user) { create(:user) } - - before do - user.setting.update(experience_level: 9) - end - - it "returns negative of (absolute value of the difference between article and user experience) divided by 2" do - expect(feed.score_experience_level(article)).to eq(-1) - end - end - - context "when the user does not have an experience level set" do - let(:user) { create(:user) } - - before do - user.setting.update(experience_level: nil) - end - - it "uses a value of 5 for user experience level" do - expect(feed.score_experience_level(article)).to eq(-1) - end - end - end - - describe "#score_comments" do - let(:article_with_one_comment) { create(:article) } - let(:article_with_five_comments) { create(:article) } - - before do - create(:comment, user: user, commentable: article_with_one_comment) - create_list(:comment, 5, user: user, commentable: article_with_five_comments) - article_with_one_comment.update_score - article_with_five_comments.update_score - article_with_one_comment.reload - article_with_five_comments.reload - end - - context "when comment_weight is default of 0" do - it "returns 0 for uncommented articles" do - expect(feed.score_comments(article, comment_weight: 1)).to eq(0) - end - - it "returns a multiple of the parameterized weight for articles with comments" do - expect(article_with_five_comments.comments_count).to eq(5) - expect(feed.score_comments(article_with_five_comments, comment_weight: 1)).to eq(5) - end - end - - context "when comment_weight is higher than 0" do - before { feed.instance_variable_set(:@comment_weight, 2) } - - it "returns 0 for uncommented articles" do - expect(feed.score_comments(article)).to eq(0) - end - - it "returns a non-zero score for commented upon articles" do - expect(feed.score_comments(article_with_one_comment)).to be > 0 - end - - it "scores article with more comments high than others" do - expect(feed.score_comments(article_with_five_comments)).to be > feed.score_comments(article_with_one_comment) - end - end - end - describe "#rank_and_sort_articles" do let(:article1) { create(:article) } let(:article2) { create(:article) }