diff --git a/app/controllers/follows_controller.rb b/app/controllers/follows_controller.rb index 961e1ba37..2b981d49e 100644 --- a/app/controllers/follows_controller.rb +++ b/app/controllers/follows_controller.rb @@ -82,7 +82,8 @@ class FollowsController < ApplicationController def update @follow = Follow.find(params[:id]) authorize @follow - redirect_to "/dashboard/following" if @follow.update(follow_params) + @follow.explicit_points = follow_params[:explicit_points] + redirect_to dashboard_following_path if @follow.save end private diff --git a/app/models/follow.rb b/app/models/follow.rb index f911a5796..c1bda3b51 100644 --- a/app/models/follow.rb +++ b/app/models/follow.rb @@ -31,6 +31,7 @@ class Follow < ApplicationRecord after_create :send_email_notification before_destroy :modify_chat_channel_status after_save :touch_follower + before_save :calculate_points after_create_commit :create_chat_channel validates :blocked, inclusion: { in: [true, false] } @@ -46,6 +47,10 @@ class Follow < ApplicationRecord private + def calculate_points + self.points = explicit_points + implicit_points + end + def touch_follower follower.touch(:updated_at, :last_followed_at) end diff --git a/app/models/reaction.rb b/app/models/reaction.rb index dbc0402b2..aad360c81 100644 --- a/app/models/reaction.rb +++ b/app/models/reaction.rb @@ -91,7 +91,7 @@ class Reaction < ApplicationRecord private def update_reactable - Reactions::UpdateReactableWorker.perform_async(id) + Reactions::UpdateRelevantScoresWorker.perform_async(id) end def bust_reactable_cache @@ -107,7 +107,7 @@ class Reaction < ApplicationRecord end def update_reactable_without_delay - Reactions::UpdateReactableWorker.new.perform(id) + Reactions::UpdateRelevantScoresWorker.new.perform(id) end def reading_time diff --git a/app/policies/follow_policy.rb b/app/policies/follow_policy.rb index 4b1256f14..e8afdf6c8 100644 --- a/app/policies/follow_policy.rb +++ b/app/policies/follow_policy.rb @@ -1,4 +1,6 @@ class FollowPolicy < ApplicationPolicy + PERMITTED_ATTRIBUTES = %i[explicit_points].freeze + def create? !user_is_banned? end @@ -8,7 +10,7 @@ class FollowPolicy < ApplicationPolicy end def permitted_attributes - %i[points] + PERMITTED_ATTRIBUTES end private diff --git a/app/views/dashboards/following_tags.html.erb b/app/views/dashboards/following_tags.html.erb index 847766a80..93546ff0d 100644 --- a/app/views/dashboards/following_tags.html.erb +++ b/app/views/dashboards/following_tags.html.erb @@ -25,12 +25,12 @@ <% tag = follow.followable %> <% if tag %> <% color = HexComparer.new([tag.bg_color_hex || "#0000000", tag.text_color_hex || "#ffffff"]).brightness(0.8) %> -
+

#<%= tag.name %> - <% if follow.points < 0 %> + <% if follow.explicit_points < 0 %> Anti-follow <% end %>

@@ -40,7 +40,7 @@

<%= form_for(follow, html: { class: "flex items-right w-100" }) do |f| %> - <%= f.number_field(:points, step: :any, required: true, class: "crayons-textfield grow-1 fs-s inline-block w-75") %> + <%= f.number_field(:explicit_points, step: :any, required: true, class: "crayons-textfield grow-1 fs-s inline-block w-75") %> <% end %>
diff --git a/app/workers/follows/update_points_worker.rb b/app/workers/follows/update_points_worker.rb new file mode 100644 index 000000000..1dd4852cf --- /dev/null +++ b/app/workers/follows/update_points_worker.rb @@ -0,0 +1,72 @@ +module Follows + class UpdatePointsWorker + include Sidekiq::Worker + sidekiq_options queue: :low_priority, retry: 10 + + def perform(article_id, user_id) + article = Article.find_by(id: article_id) + user = User.find_by(id: user_id) + return unless article && user + + adjust_other_tag_follows_of_user(user.id) + followed_tag_names = user.cached_followed_tag_names + article.decorate.cached_tag_list_array.each do |tag_name| + if followed_tag_names.include?(tag_name) + recalculate_tag_follow_points(tag_name, user) + end + end + end + + def recalculate_tag_follow_points(tag_name, user) + tag = Tag.find_by(name: tag_name) + follow = Follow.follower_tag(user.id).where(followable_id: tag.id).last + + follow.implicit_points = calculate_implicit_points(tag, user) + follow.save + end + + def calculate_implicit_points(tag, user) + last_100_reactable_ids = user.reactions.where(reactable_type: "Article", points: 0..) + .pluck(:reactable_id).last(100) + last_100_long_page_view_article_ids = user.page_views.where(time_tracked_in_seconds: 45..) + .pluck(:article_id).last(100) + articles = Article.where(id: last_100_reactable_ids + last_100_long_page_view_article_ids) + tags = articles.pluck(:cached_tag_list).flat_map { |list| list.split(", ") } + occurrences = tags.count(tag.name) + bonus = inverse_popularity_bonus(tag) + Math.log(occurrences + bonus + 1) # +1 is purelt to avoid log(0) => -infinity + end + + def adjust_other_tag_follows_of_user(user_id) + # As we bump one follow up, we should also give a slight penalty + # to other follows to ensure re-balancing of overall points + # This will help stale tags fade after a temporary interest bump + + # 0.98 is used to ensure this is "a very small amount" + # And percentage seems to makes sense, as it will be relative to + # size of current points. + + # That number could be adjusted at any point if we have reason to + # believe it is too much or too little. + Follow.follower_tag(user_id).order(Arel.sql("RANDOM()")).limit(5).each do |follow| + follow.update_column(:points, (follow.points * 0.98)) + end + end + + def inverse_popularity_bonus(tag) + # Let's give a bonus to "less popular" tags on the platform + # To help balance the weight of popular topic. + # On DEV, javascript has way more taggings than rust, for example, so we can + # help rust outweigh JS in this calculation slightly. + # The bonus will be applied to the logarithmic scale, as to blunt any outsized impact. + top_100_tag_names = cached_app_wide_top_tag_names + top_100_tag_names.index(tag.name) || (top_100_tag_names.size * 1.5) + end + + def cached_app_wide_top_tag_names + Rails.cache.fetch("top-100-tags") do + Tag.order(hotness_score: :desc).limit(100).pluck(:name) + end + end + end +end diff --git a/app/workers/reactions/update_reactable_worker.rb b/app/workers/reactions/update_relevant_scores_worker.rb similarity index 65% rename from app/workers/reactions/update_reactable_worker.rb rename to app/workers/reactions/update_relevant_scores_worker.rb index 52312042b..f3bc3f8ba 100644 --- a/app/workers/reactions/update_reactable_worker.rb +++ b/app/workers/reactions/update_relevant_scores_worker.rb @@ -1,5 +1,5 @@ module Reactions - class UpdateReactableWorker + class UpdateRelevantScoresWorker include Sidekiq::Worker sidekiq_options queue: :high_priority, retry: 10 @@ -10,6 +10,9 @@ module Reactions reaction.reactable.touch_by_reaction if reaction.reactable.respond_to?(:touch_by_reaction) reaction.reactable.sync_reactions_count if rand(6) == 1 && reaction.reactable.respond_to?(:sync_reactions_count) + return unless reaction.reactable_type == "Article" && Reaction::PUBLIC_CATEGORIES.include?(reaction.category) + + Follows::UpdatePointsWorker.perform_async(reaction.reactable_id, reaction.user_id) end end end diff --git a/db/migrate/20201119153512_add_explicit_and_implicit_follow_points.rb b/db/migrate/20201119153512_add_explicit_and_implicit_follow_points.rb new file mode 100644 index 000000000..abb372a8e --- /dev/null +++ b/db/migrate/20201119153512_add_explicit_and_implicit_follow_points.rb @@ -0,0 +1,6 @@ +class AddExplicitAndImplicitFollowPoints < ActiveRecord::Migration[6.0] + def change + add_column :follows, :explicit_points, :float, default: 1.0 # 1 is equivalent to the current default set in "score" + add_column :follows, :implicit_points, :float, default: 0.0 + end +end diff --git a/db/schema.rb b/db/schema.rb index 9a56943df..152c8ed9b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_11_14_151157) do +ActiveRecord::Schema.define(version: 2020_11_19_153512) do # These are extensions that must be enabled in order to support this database enable_extension "citext" @@ -560,10 +560,12 @@ ActiveRecord::Schema.define(version: 2020_11_14_151157) do create_table "follows", force: :cascade do |t| t.boolean "blocked", default: false, null: false t.datetime "created_at" + t.float "explicit_points", default: 1.0 t.bigint "followable_id", null: false t.string "followable_type", null: false t.bigint "follower_id", null: false t.string "follower_type", null: false + t.float "implicit_points", default: 0.0 t.float "points", default: 1.0 t.string "subscription_status", default: "all_articles", null: false t.datetime "updated_at" diff --git a/docs/technical-overview/architecture.md b/docs/technical-overview/architecture.md index 75e6b79c1..b603a4fff 100644 --- a/docs/technical-overview/architecture.md +++ b/docs/technical-overview/architecture.md @@ -188,6 +188,11 @@ article in the user's reading list. How a user keeps track of the tags, users, or articles they care about. Follows impact a user's home feed and notifications. +Follows can have a "score" which indicates how much a user wants to see the element +in their feed. Currently we only calculate these for tag follows, but it could be +expanded to users. The user can set an "explicit" score, and the system also calculates +an "implicit" score based on their activity. + ## Roles Through the "rolify" gem, users can have roles like "admin", etc. A role can diff --git a/lib/data_update_scripts/20201120001727_populate_explicit_follow_points.rb b/lib/data_update_scripts/20201120001727_populate_explicit_follow_points.rb new file mode 100644 index 000000000..ac115a6a3 --- /dev/null +++ b/lib/data_update_scripts/20201120001727_populate_explicit_follow_points.rb @@ -0,0 +1,7 @@ +module DataUpdateScripts + class PopulateExplicitFollowPoints + def run + Follow.connection.execute('UPDATE "follows" SET "explicit_points" = "points" WHERE points != 1') + end + end +end diff --git a/spec/decorators/user_decorator_spec.rb b/spec/decorators/user_decorator_spec.rb index 0714692e0..51705db77 100644 --- a/spec/decorators/user_decorator_spec.rb +++ b/spec/decorators/user_decorator_spec.rb @@ -47,7 +47,7 @@ RSpec.describe UserDecorator, type: :decorator do it "returns adjusted points for tag" do follow = saved_user.follow(tag1) - follow.update(points: 0.1) + follow.update(explicit_points: 0.1) expect(saved_user.decorate.cached_followed_tags.first.points).to eq(0.1) end end diff --git a/spec/lib/data_update_scripts/populate_explicit_follow_points_spec.rb b/spec/lib/data_update_scripts/populate_explicit_follow_points_spec.rb new file mode 100644 index 000000000..b628b7fb4 --- /dev/null +++ b/spec/lib/data_update_scripts/populate_explicit_follow_points_spec.rb @@ -0,0 +1,25 @@ +require "rails_helper" +require Rails.root.join( + "lib/data_update_scripts/20201120001727_populate_explicit_follow_points.rb", +) + +describe DataUpdateScripts::PopulateExplicitFollowPoints do + it "updates follows that had points to having explicit points", :aggregate_failures do + follow = create(:follow) + second_follow = create(:follow) + third_follow = create(:follow) + fourth_follow = create(:follow) + + follow.update_column(:points, 3) + second_follow.update_column(:points, 7) + third_follow.update_column(:points, 1) + fourth_follow.update_column(:points, 0.5) + expect(follow.explicit_points).to eq(1.0) + expect(second_follow.explicit_points).to eq(1.0) + described_class.new.run + expect(follow.reload.explicit_points).to eq(3) + expect(second_follow.reload.explicit_points).to eq(7) + expect(third_follow.reload.explicit_points).to eq(1) + expect(fourth_follow.reload.explicit_points).to eq(0.5) + end +end diff --git a/spec/models/follow_spec.rb b/spec/models/follow_spec.rb index 4830371f8..cbb76b594 100644 --- a/spec/models/follow_spec.rb +++ b/spec/models/follow_spec.rb @@ -2,6 +2,7 @@ require "rails_helper" RSpec.describe Follow, type: :model do let(:user) { create(:user) } + let(:tag) { create(:tag) } let(:user_2) { create(:user) } describe "validations" do @@ -20,6 +21,15 @@ RSpec.describe Follow, type: :model do expect(user.following?(user_2)).to eq(true) end + it "calculates points with explicit and implicit combined" do + user.follow(tag) + follow = described_class.last + follow.explicit_points = 2.0 + follow.implicit_points = 3.0 + follow.save + expect(follow.points).to eq(5.0) + end + context "when enqueuing jobs" do it "enqueues create channel job" do expect do diff --git a/spec/requests/follows_update_spec.rb b/spec/requests/follows_update_spec.rb index a68ca2299..c2a72bbdc 100644 --- a/spec/requests/follows_update_spec.rb +++ b/spec/requests/follows_update_spec.rb @@ -12,7 +12,8 @@ RSpec.describe "Following/Unfollowing", type: :request do describe "PUT follows/:id" do it "updates follow points" do user.follow(tag) - put "/follows/#{Follow.last.id}", params: { follow: { points: 3.0 } } + put "/follows/#{Follow.last.id}", params: { follow: { explicit_points: 3.0 } } + expect(Follow.last.explicit_points).to eq(3.0) expect(Follow.last.points).to eq(3.0) end diff --git a/spec/services/articles/feeds/large_forem_experimental_spec.rb b/spec/services/articles/feeds/large_forem_experimental_spec.rb index 57cb9b285..b6d59fcdc 100644 --- a/spec/services/articles/feeds/large_forem_experimental_spec.rb +++ b/spec/services/articles/feeds/large_forem_experimental_spec.rb @@ -237,7 +237,7 @@ RSpec.describe Articles::Feeds::LargeForemExperimental, type: :service do before do user.follow(tag) user.save - user.follows.last.update(points: 2) + user.follows.last.update(explicit_points: 2) end it "returns the followed tag point value" do @@ -253,7 +253,7 @@ RSpec.describe Articles::Feeds::LargeForemExperimental, type: :service do user.follow(tag) user.follow(tag2) user.save - user.follows.each { |follow| follow.update(points: 2) } + user.follows.each { |follow| follow.update(explicit_points: 2) } end it "returns the sum of followed tag point values" do diff --git a/spec/support/sidekiq_test_helpers.rb b/spec/support/sidekiq_test_helpers.rb index 102b1cae0..a6f753177 100644 --- a/spec/support/sidekiq_test_helpers.rb +++ b/spec/support/sidekiq_test_helpers.rb @@ -47,6 +47,20 @@ module SidekiqTestHelpers expect(matching_job).to be_present, "No enqueued job found with #{expected}" end + def sidekiq_assert_not_enqueued_with(job: nil, args: nil, at: nil, queue: nil) + expected = { job: job, args: args, at: at, queue: queue }.compact + expected_args = Utils.prepare_args(expected) + + yield + + # check there's at least one job with the given args + matching_job = job.jobs.detect do |queued_job| + expected_args.all? { |key, value| value == queued_job[key] } + end + + expect(matching_job).not_to be_present, "Job unexpectedly found with #{expected}" + end + # Asserts that no jobs have been enqueued. # see def sidekiq_assert_no_enqueued_jobs(only: nil, except: nil, &block) diff --git a/spec/system/dashboards/user_scrolls_down_dashboard_follows_spec.rb b/spec/system/dashboards/user_scrolls_down_dashboard_follows_spec.rb index 0e2562864..8fa783e43 100644 --- a/spec/system/dashboards/user_scrolls_down_dashboard_follows_spec.rb +++ b/spec/system/dashboards/user_scrolls_down_dashboard_follows_spec.rb @@ -45,12 +45,12 @@ RSpec.describe "Infinite scroll on dashboard", type: :system, js: true do it "updates a tag point value" do last_div = page.all('div[id^="follows"]').last within last_div do - fill_in "follow_points", with: 10.0 + fill_in "follow_explicit_points", with: 10.0 click_button "commit" end first_div = page.find('div[id^="follows"]', match: :first) within first_div do - expect(page).to have_field("follow_points", with: 10.0) + expect(page).to have_field("follow_explicit_points", with: 10.0) end end end diff --git a/spec/system/homepage/user_visits_homepage_spec.rb b/spec/system/homepage/user_visits_homepage_spec.rb index 931ec86e1..525e1d4ed 100644 --- a/spec/system/homepage/user_visits_homepage_spec.rb +++ b/spec/system/homepage/user_visits_homepage_spec.rb @@ -108,7 +108,7 @@ RSpec.describe "User visits a homepage", type: :system do find("body")["data-user"] within("#sidebar-nav-followed-tags") do - expect(all(".crayons-link--block").map(&:text)).to eq(%w[#javascript #go #ruby]) + expect(all(".crayons-link--block").map(&:text).sort).to eq(%w[#javascript #go #ruby].sort) end end end diff --git a/spec/workers/follows/update_points_worker_spec.rb b/spec/workers/follows/update_points_worker_spec.rb new file mode 100644 index 000000000..6560c9ab2 --- /dev/null +++ b/spec/workers/follows/update_points_worker_spec.rb @@ -0,0 +1,78 @@ +require "rails_helper" + +RSpec.describe Follows::UpdatePointsWorker, type: :worker do + include_examples "#enqueues_on_correct_queue", "low_priority", 1 + + describe "#perform" do + let(:worker) { subject } + + let(:user) { create(:user) } + let(:tag) { create(:tag, name: "tag") } + let(:second_tag) { create(:tag, name: "secondtag") } + let(:third_tag) { create(:tag, name: "thirdtag") } + let(:article) { create(:article, tags: [tag.name]) } + let(:second_article) { create(:article, tags: [tag.name]) } + let(:reaction) { create(:reaction, reactable: article, user: user) } + let(:page_view) { create(:page_view, user: user, article: article, time_tracked_in_seconds: 100) } + + before do + user.follow(second_tag) + user.follow(tag) + end + + it "calculates scores" do + follow = Follow.last + follow.update_column(:explicit_points, 2.2) + worker.perform(reaction.reactable_id, reaction.user_id) + follow.reload + expect(follow.implicit_points).to be > 0 + expect(follow.reload.points.round(2)).to eq (follow.implicit_points + follow.explicit_points).round(2) + end + + it "has higher score with more long page views" do + follow = Follow.last + worker.perform(reaction.reactable_id, reaction.user_id) + follow.reload + first_implicit_score = follow.implicit_points + + create(:page_view, user: user, article: second_article, time_tracked_in_seconds: 100) + + worker.perform(reaction.reactable_id, reaction.user_id) + follow.reload + expect(follow.implicit_points).to be > first_implicit_score + end + + it "has higher score with more reactions" do + follow = Follow.last + worker.perform(reaction.reactable_id, reaction.user_id) + follow.reload + first_implicit_score = follow.implicit_points + + create(:reaction, reactable: second_article, user: user) + + worker.perform(reaction.reactable_id, reaction.user_id) + follow.reload + expect(follow.implicit_points).to be > first_implicit_score + end + + it "bumps down tag follow points not included in this calc" do + follow = Follow.first + worker.perform(reaction.reactable_id, reaction.user_id) + expect(follow.reload.points.round(2)).to eq(0.98) + end + + it "applies inverse bonus to slightly penalize more popular tags" do + follow = Follow.last + tag.update_column(:hotness_score, 1000) + second_tag.update_column(:hotness_score, 100) + worker.perform(reaction.reactable_id, reaction.user_id) + follow.reload + original_points = follow.points + tag.update_column(:hotness_score, 50) + tag.reload + worker.perform(reaction.reactable_id, reaction.user_id) + + expect(follow.reload.points).to be > original_points # should be higher because tag is now less popular + end + end +end diff --git a/spec/workers/reactions/update_reactable_worker_spec.rb b/spec/workers/reactions/update_reactable_worker_spec.rb deleted file mode 100644 index 2e50569ed..000000000 --- a/spec/workers/reactions/update_reactable_worker_spec.rb +++ /dev/null @@ -1,30 +0,0 @@ -require "rails_helper" - -RSpec.describe Reactions::UpdateReactableWorker, type: :worker do - describe "#perform" do - let(:article) { create(:article) } - let(:reaction) { create(:reaction, reactable: article) } - let(:comment) { create(:comment, commentable: article) } - let(:comment_reaction) { create(:reaction, reactable: comment) } - let(:worker) { subject } - - it " updates the reactable Article" do - sidekiq_assert_enqueued_with(job: Articles::ScoreCalcWorker) do - worker.perform(reaction.id) - end - end - - it " updates the reactable Comment" do - updated_at = 1.day.ago - comment.update_columns(updated_at: updated_at) - worker.perform(comment_reaction.id) - expect(comment.reload.updated_at).to be > updated_at - end - - it " doesn't fail if a reaction doesn't exist" do - expect do - worker.perform(Reaction.maximum(:id).to_i + 1) - end.not_to raise_error - end - end -end diff --git a/spec/workers/reactions/update_relevant_scores_worker_spec.rb b/spec/workers/reactions/update_relevant_scores_worker_spec.rb new file mode 100644 index 000000000..f8c6a1e26 --- /dev/null +++ b/spec/workers/reactions/update_relevant_scores_worker_spec.rb @@ -0,0 +1,49 @@ +require "rails_helper" + +RSpec.describe Reactions::UpdateRelevantScoresWorker, type: :worker do + describe "#perform" do + let(:article) { create(:article) } + let(:reaction) { create(:reaction, reactable: article) } + let(:comment) { create(:comment, commentable: article) } + let(:comment_reaction) { create(:reaction, reactable: comment) } + let(:worker) { subject } + + it "kicks off point update if article" do + sidekiq_assert_enqueued_with(job: Follows::UpdatePointsWorker) do + worker.perform(reaction.id) + end + end + + it "does not kick off points updater if not comment reaction" do + sidekiq_assert_not_enqueued_with(job: Follows::UpdatePointsWorker) do + worker.perform(comment_reaction.id) + end + end + + it "does not kick off points updater if reaction is non-public" do + reaction.update_column(:category, "vomit") + sidekiq_assert_not_enqueued_with(job: Follows::UpdatePointsWorker) do + worker.perform(reaction.id) + end + end + + it "updates the reactable Article" do + sidekiq_assert_enqueued_with(job: Articles::ScoreCalcWorker) do + worker.perform(reaction.id) + end + end + + it "updates the reactable Comment" do + updated_at = 1.day.ago + comment.update_columns(updated_at: updated_at) + worker.perform(comment_reaction.id) + expect(comment.reload.updated_at).to be > updated_at + end + + it "doesn't fail if a reaction doesn't exist" do + expect do + worker.perform(Reaction.maximum(:id).to_i + 1) + end.not_to raise_error + end + end +end