From 472c3d29228333df379420e6f58d8c064ae3805f Mon Sep 17 00:00:00 2001 From: Ben Halpern Date: Mon, 18 Jan 2021 11:08:23 -0500 Subject: [PATCH] Fix and refactor hide user content (#12307) --- app/controllers/async_info_controller.rb | 2 +- app/controllers/stories_controller.rb | 6 +++--- .../contentDisplayPolicy/hideBlockedContent.js | 2 +- app/models/user.rb | 4 ---- app/models/user_block.rb | 15 +++++++++++++++ app/services/articles/feeds/basic.rb | 1 + .../articles/feeds/large_forem_experimental.rb | 1 + spec/models/user_block_spec.rb | 16 +++++++++++++++- spec/services/articles/feeds/basic_spec.rb | 9 ++++++++- .../feeds/large_forem_experimental_spec.rb | 8 +++++++- 10 files changed, 52 insertions(+), 12 deletions(-) diff --git a/app/controllers/async_info_controller.rb b/app/controllers/async_info_controller.rb index ed418dd83..c0b933e06 100644 --- a/app/controllers/async_info_controller.rb +++ b/app/controllers/async_info_controller.rb @@ -55,7 +55,7 @@ class AsyncInfoController < ApplicationController methods: [:points]), followed_podcast_ids: @user.cached_following_podcasts_ids, reading_list_ids: @user.cached_reading_list_article_ids, - blocked_user_ids: @user.all_blocking.pluck(:blocked_id), + blocked_user_ids: UserBlock.cached_blocked_ids_for_blocker(@user.id), saw_onboarding: @user.saw_onboarding, checked_code_of_conduct: @user.checked_code_of_conduct, checked_terms_and_conditions: @user.checked_terms_and_conditions, diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index f8006a128..740a7d54c 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -168,13 +168,11 @@ class StoriesController < ApplicationController def handle_base_index @home_page = true - assign_feed_stories + assign_feed_stories unless user_signed_in? # Feed fetched async for signed-in users assign_hero_html assign_podcasts get_latest_campaign_articles if Campaign.current.show_in_sidebar? @article_index = true - @featured_story = (featured_story || Article.new)&.decorate - @stories = ArticleDecorator.decorate_collection(@stories) set_surrogate_key_header "main_app_home_page" set_cache_control_headers(600, stale_while_revalidate: 30, @@ -279,6 +277,8 @@ class StoriesController < ApplicationController @default_home_feed = true @featured_story, @stories = feed.default_home_feed_and_featured_story(user_signed_in: user_signed_in?) end + @featured_story = (featured_story || Article.new)&.decorate + @stories = ArticleDecorator.decorate_collection(@stories) end def assign_article_show_variables diff --git a/app/javascript/contentDisplayPolicy/hideBlockedContent.js b/app/javascript/contentDisplayPolicy/hideBlockedContent.js index a3fe970ce..6910cfb72 100644 --- a/app/javascript/contentDisplayPolicy/hideBlockedContent.js +++ b/app/javascript/contentDisplayPolicy/hideBlockedContent.js @@ -13,7 +13,7 @@ export default function hideBlockedContent() { }); divsToHide.forEach((div) => { - if (div.className.includes('single-article')) { + if (div.className.includes('crayons-story')) { div.style.display = 'none'; } else if (div.className.includes('single-comment-node')) { const divInnerComment = div.getElementsByClassName('inner-comment')[0]; diff --git a/app/models/user.rb b/app/models/user.rb index 4515d33d7..19b7144cc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -435,10 +435,6 @@ class User < ApplicationRecord def block; end - def all_blocking - UserBlock.where(blocker_id: id) - end - def all_blocked_by UserBlock.where(blocked_id: id) end diff --git a/app/models/user_block.rb b/app/models/user_block.rb index cc0869399..f7df8b781 100644 --- a/app/models/user_block.rb +++ b/app/models/user_block.rb @@ -10,10 +10,21 @@ class UserBlock < ApplicationRecord counter_culture :blocker, column_name: "blocking_others_count" counter_culture :blocked, column_name: "blocked_by_count" + after_create :bust_blocker_cache + before_destroy :bust_blocker_cache + + BLOCKED_IDS_CACHE_KEY = "blocked_ids_for_blocker/".freeze + class << self def blocking?(blocker_id, blocked_id) exists?(blocker_id: blocker_id, blocked_id: blocked_id) end + + def cached_blocked_ids_for_blocker(blocker_id) + Rails.cache.fetch("#{BLOCKED_IDS_CACHE_KEY}#{blocker_id}", expires_in: 48.hours) do + where(blocker_id: blocker_id).pluck(:blocked_id) + end + end end private @@ -21,4 +32,8 @@ class UserBlock < ApplicationRecord def blocker_cannot_be_same_as_blocked errors.add(:blocker_id, "can't be the same as the blocked_id") if blocker_id == blocked_id end + + def bust_blocker_cache + Rails.cache.delete("#{BLOCKED_IDS_CACHE_KEY}#{blocker_id}") + end end diff --git a/app/services/articles/feeds/basic.rb b/app/services/articles/feeds/basic.rb index 819464fe0..f1c73d695 100644 --- a/app/services/articles/feeds/basic.rb +++ b/app/services/articles/feeds/basic.rb @@ -16,6 +16,7 @@ module Articles .limited_column_select.includes(top_comments: :user) return articles unless @user + 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| diff --git a/app/services/articles/feeds/large_forem_experimental.rb b/app/services/articles/feeds/large_forem_experimental.rb index d4868e8c4..fd9d2026e 100644 --- a/app/services/articles/feeds/large_forem_experimental.rb +++ b/app/services/articles/feeds/large_forem_experimental.rb @@ -128,6 +128,7 @@ module Articles def globally_hot_articles(user_signed_in) if user_signed_in hot_stories = experimental_hot_story_grab + hot_stories = hot_stories.where.not(user_id: UserBlock.cached_blocked_ids_for_blocker(@user.id)) featured_story = hot_stories.where.not(main_image: nil).first new_stories = Article.published .where("score > ?", -15) diff --git a/spec/models/user_block_spec.rb b/spec/models/user_block_spec.rb index 7fafb87ac..ebdde52f9 100644 --- a/spec/models/user_block_spec.rb +++ b/spec/models/user_block_spec.rb @@ -1,7 +1,8 @@ require "rails_helper" RSpec.describe UserBlock, type: :model do - let(:blocker) { build(:user) } + let(:blocker) { create(:user) } + let(:blocked_user) { create(:user) } describe "validations" do it { is_expected.to validate_inclusion_of(:config).in_array(%w[default]) } @@ -11,5 +12,18 @@ RSpec.describe UserBlock, type: :model do expect(user_block).not_to be_valid expect(user_block.errors.full_messages).to include("Blocker can't be the same as the blocked_id") end + + it "returns ids blocked by user" do + create(:user_block, blocker: blocker, blocked: blocked_user, config: "default") + expect(described_class.cached_blocked_ids_for_blocker(blocker)).to eq([blocked_user.id]) + end + + it "busts user block cache" do + allow(Rails.cache).to receive(:delete).and_call_original + block = create(:user_block, blocker: blocker, blocked: blocked_user, config: "default") + expect(Rails.cache).to have_received(:delete).with("blocked_ids_for_blocker/#{blocker.id}").once + block.destroy + expect(Rails.cache).to have_received(:delete).with("blocked_ids_for_blocker/#{blocker.id}").twice + end end end diff --git a/spec/services/articles/feeds/basic_spec.rb b/spec/services/articles/feeds/basic_spec.rb index 9152689b8..1ff9fad79 100644 --- a/spec/services/articles/feeds/basic_spec.rb +++ b/spec/services/articles/feeds/basic_spec.rb @@ -2,9 +2,10 @@ require "rails_helper" RSpec.describe Articles::Feeds::Basic, type: :service do let(:user) { create(:user) } + let(:second_user) { create(:user) } let(:unique_tag_name) { "foo" } let!(:article) { create(:article, hotness_score: 10) } - let!(:hot_story) { create(:article, hotness_score: 1000, score: 1000, published_at: 3.hours.ago) } + let!(:hot_story) { create(:article, hotness_score: 1000, score: 1000, published_at: 3.hours.ago, user_id: second_user.id) } let!(:old_story) { create(:article, hotness_score: 500, published_at: 3.days.ago, tags: unique_tag_name) } let!(:low_scoring_article) { create(:article, score: -1000) } let!(:month_old_story) { create(:article, published_at: 1.month.ago) } # rubocop:disable RSpec/LetSetup @@ -36,5 +37,11 @@ RSpec.describe Articles::Feeds::Basic, type: :service do expect(result.third).to eq article expect(result).not_to include(low_scoring_article) end + + it "does not load blocked articles" do + create(:user_block, blocker: user, blocked: second_user, config: "default") + result = feed.feed + expect(result).not_to include(hot_story) + end end end diff --git a/spec/services/articles/feeds/large_forem_experimental_spec.rb b/spec/services/articles/feeds/large_forem_experimental_spec.rb index 5c18545c0..1942794b9 100644 --- a/spec/services/articles/feeds/large_forem_experimental_spec.rb +++ b/spec/services/articles/feeds/large_forem_experimental_spec.rb @@ -2,9 +2,10 @@ require "rails_helper" RSpec.describe Articles::Feeds::LargeForemExperimental, type: :service do let(:user) { create(:user) } + let(:second_user) { create(:user)} let!(:feed) { described_class.new(user: user, number_of_articles: 100, page: 1) } let!(:article) { create(:article) } - let!(:hot_story) { create(:article, hotness_score: 1000, score: 1000, published_at: 3.hours.ago) } + let!(:hot_story) { create(:article, hotness_score: 1000, score: 1000, published_at: 3.hours.ago, user_id: second_user.id) } let!(:old_story) { create(:article, published_at: 3.days.ago) } let!(:low_scoring_article) { create(:article, score: -1000) } let!(:month_old_story) { create(:article, published_at: 1.month.ago) } @@ -76,6 +77,11 @@ RSpec.describe Articles::Feeds::LargeForemExperimental, type: :service do expect(stories).to include(article) expect(stories).to include(hot_story) end + + it "does not load blocked articles" do + create(:user_block, blocker: user, blocked: second_user, config: "default") + expect(result).not_to include(hot_story) + end end context "when ranking is true" do