From d227e1d2859a4ccf1663833727c3ea41434ebe31 Mon Sep 17 00:00:00 2001 From: Joshua Wehner Date: Mon, 28 Aug 2023 16:11:20 +0200 Subject: [PATCH] Suppressing articles negative-follow tags from appearing in the "relevant" feed (#19948) * Try suppressing all negative-follow tags * userData isn't always available * Bolster test coverage for tag filter scenarios * Antitags for the non-basic 'strategy' * Rename antitags -> hidden_tags * Rename 'anti_tags' -> 'hidden_tags' as well * Use userData.followed_tags to derive hidden_tags --- .../javascripts/initializers/initScrolling.js | 15 ++++ app/controllers/search_controller.rb | 2 + app/queries/homepage/articles_query.rb | 7 +- app/services/articles/feeds/basic.rb | 3 + app/services/articles/feeds/variant_query.rb | 8 +- app/services/homepage/fetch_articles.rb | 2 + .../homeFeedFlows/filteredHiddenTag.spec.js | 88 +++++++++++++++++++ cypress/fixtures/users/antitagUser.json | 5 ++ cypress/fixtures/users/hiddenTagUser.json | 5 ++ spec/queries/homepage/articles_query_spec.rb | 32 ++++++- spec/services/articles/feeds/basic_spec.rb | 25 +++++- spec/support/seeds/seeds_e2e.rb | 23 +++++ 12 files changed, 208 insertions(+), 7 deletions(-) create mode 100644 cypress/e2e/seededFlows/homeFeedFlows/filteredHiddenTag.spec.js create mode 100644 cypress/fixtures/users/antitagUser.json create mode 100644 cypress/fixtures/users/hiddenTagUser.json diff --git a/app/assets/javascripts/initializers/initScrolling.js b/app/assets/javascripts/initializers/initScrolling.js index 530be215d..b3b99e799 100644 --- a/app/assets/javascripts/initializers/initScrolling.js +++ b/app/assets/javascripts/initializers/initScrolling.js @@ -389,6 +389,21 @@ function paginate(tag, params, requiresApproval) { var homeEl = document.getElementById('index-container'); if (homeEl.dataset.feed === 'base-feed') { searchHash.class_name = 'Article'; + const isHomePageFeed = window.location.pathname == '/'; + if (isHomePageFeed && userData()) { + const hidden_tags = JSON.parse(userData().followed_tags).reduce(function ( + array, + tag, + ) { + if (tag.points < 0) { + array.push(tag.name); + } + return array; + }, + []); + + searchHash.hidden_tags = hidden_tags; + } } else if (homeEl.dataset.feed === 'latest') { searchHash.class_name = 'Article'; searchHash.sort_by = 'published_at'; diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 7e3739b43..b55916339 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -46,6 +46,7 @@ class SearchController < ApplicationController :user_id, { tag_names: [], + hidden_tags: [], published_at: [:gte] }, ].freeze @@ -106,6 +107,7 @@ class SearchController < ApplicationController user_id: feed_params[:user_id], organization_id: feed_params[:organization_id], tags: feed_params[:tag_names], + hidden_tags: feed_params[:hidden_tags], sort_by: params[:sort_by], sort_direction: params[:sort_direction], page: params[:page], diff --git a/app/queries/homepage/articles_query.rb b/app/queries/homepage/articles_query.rb index 76d492bfa..8ed1168f8 100644 --- a/app/queries/homepage/articles_query.rb +++ b/app/queries/homepage/articles_query.rb @@ -32,6 +32,7 @@ module Homepage user_id: nil, organization_id: nil, tags: [], + hidden_tags: [], sort_by: nil, sort_direction: nil, page: 0, @@ -45,6 +46,7 @@ module Homepage @user_id = user_id @organization_id = organization_id @tags = tags.presence || [] + @hidden_tags = hidden_tags.presence || [] @sort_by = sort_by @sort_direction = sort_direction || DEFAULT_SORT_DIRECTION @@ -59,8 +61,8 @@ module Homepage private - attr_reader :relation, :approved, :published_at, :user_id, :organization_id, :tags, :sort_by, :sort_direction, - :page, :per_page + attr_reader :relation, :approved, :published_at, :user_id, :organization_id, :tags, :hidden_tags, + :sort_by, :sort_direction, :page, :per_page def filter @relation = @relation.where(approved: approved) unless approved.nil? @@ -68,6 +70,7 @@ module Homepage @relation = @relation.where(user_id: user_id) if user_id.present? @relation = @relation.where(organization_id: organization_id) if organization_id.present? @relation = @relation.cached_tagged_with_any(tags) if tags.any? + @relation = @relation.not_cached_tagged_with_any(hidden_tags) if hidden_tags.any? @relation = @relation.includes(:distinct_reaction_categories) relation diff --git a/app/services/articles/feeds/basic.rb b/app/services/articles/feeds/basic.rb index 8bf8ab153..5e89a12ba 100644 --- a/app/services/articles/feeds/basic.rb +++ b/app/services/articles/feeds/basic.rb @@ -19,6 +19,9 @@ module Articles return articles unless @user articles = articles.where.not(user_id: UserBlock.cached_blocked_ids_for_blocker(@user.id)) + if (hidden_tags = @user.cached_antifollowed_tag_names).any? + articles = articles.not_cached_tagged_with_any(hidden_tags) + end articles.sort_by.with_index do |article, index| tag_score = score_followed_tags(article) user_score = score_followed_user(article) diff --git a/app/services/articles/feeds/variant_query.rb b/app/services/articles/feeds/variant_query.rb index 7377818a5..50f9f19c9 100644 --- a/app/services/articles/feeds/variant_query.rb +++ b/app/services/articles/feeds/variant_query.rb @@ -130,11 +130,17 @@ module Articles # This sub-query allows us to take the hard work of the hand-coded unsanitized sql and # create a sub-query that we can use to help ensure that we can use all of the ActiveRecord # goodness of scopes (e.g., limited_column_select) and eager includes. - Article.joins(join_fragment) + scope = Article.joins(join_fragment) .limited_column_select .includes(top_comments: :user) .includes(:distinct_reaction_categories) .order(config.order_by.to_sql) + + if @user.present? && (hidden_tags = @user.cached_antifollowed_tag_names).any? + scope = scope.not_cached_tagged_with_any(hidden_tags) + end + + scope end alias more_comments_minimal_weight_randomized call diff --git a/app/services/homepage/fetch_articles.rb b/app/services/homepage/fetch_articles.rb index f122b3f11..894378251 100644 --- a/app/services/homepage/fetch_articles.rb +++ b/app/services/homepage/fetch_articles.rb @@ -14,6 +14,7 @@ module Homepage user_id: nil, organization_id: nil, tags: [], + hidden_tags: [], sort_by: nil, sort_direction: nil, page: 0, @@ -25,6 +26,7 @@ module Homepage user_id: user_id, organization_id: organization_id, tags: tags, + hidden_tags: hidden_tags, sort_by: sort_by, sort_direction: sort_direction, page: page, diff --git a/cypress/e2e/seededFlows/homeFeedFlows/filteredHiddenTag.spec.js b/cypress/e2e/seededFlows/homeFeedFlows/filteredHiddenTag.spec.js new file mode 100644 index 000000000..15f67f3dd --- /dev/null +++ b/cypress/e2e/seededFlows/homeFeedFlows/filteredHiddenTag.spec.js @@ -0,0 +1,88 @@ +describe('Home Feed should not filter when logged-out', () => { + beforeEach(() => { + cy.testSetup(); + + // Explicitly set the viewport to make sure we're in the full desktop view for these tests + cy.viewport('macbook-15'); + + cy.visit('/'); + }); + + it('**should** show tag1 tagged article', () => { + cy.findByRole('heading', { name: 'Tag test article' }).should('exist'); + }); +}); + +describe('Home Feed should filter tagged article when logged-in as user with hidden tags', () => { + beforeEach(() => { + cy.testSetup(); + cy.fixture('users/hiddenTagUser.json').as('user'); + + // Explicitly set the viewport to make sure we're in the full desktop view for these tests + cy.viewport('macbook-15'); + + cy.get('@user').then((user) => { + cy.loginAndVisit(user, '/'); + }); + }); + + it('should **not** show tag1 tagged article', () => { + cy.findByRole('heading', { name: 'Tag test article' }).should('not.exist'); + }); +}); + +describe('Home Feed should not filter when logged-in as other user', () => { + beforeEach(() => { + cy.testSetup(); + cy.fixture('users/adminUser.json').as('user'); + + // Explicitly set the viewport to make sure we're in the full desktop view for these tests + cy.viewport('macbook-15'); + + cy.visit('/'); + }); + + it('**should** show tag1 tagged article', () => { + cy.findByRole('heading', { name: 'Tag test article' }).should('exist'); + }); +}); + +describe('Search should **not** filter tagged article even when logged-in as user with hidden tags', () => { + beforeEach(() => { + cy.testSetup(); + cy.fixture('users/hiddenTagUser.json').as('user'); + + // Explicitly set the viewport to make sure we're in the full desktop view for these tests + cy.viewport('macbook-15'); + + cy.get('@user').then((user) => { + cy.loginAndVisit(user, '/'); + cy.findByRole('textbox', { name: /search/i }).type('Tag test'); + cy.findByRole('button', { name: /search/i }).click(); + + cy.url().should('include', '/search?q=Tag%20test'); + }); + }); + + it('**should** show tag1 tagged article', () => { + cy.findByRole('heading', { name: 'Tag test article' }).should('exist'); + }); +}); + +describe('Tag view should **not** filter tagged article even when logged-in as user with hidden tags', () => { + beforeEach(() => { + cy.testSetup(); + cy.fixture('users/hiddenTagUser.json').as('user'); + + // Explicitly set the viewport to make sure we're in the full desktop view for these tests + cy.viewport('macbook-15'); + + cy.get('@user').then((user) => { + cy.loginAndVisit(user, '/t/tag1'); + }); + }); + + it('**should** show tag1 tagged article', () => { + cy.findByRole('heading', { name: 'Tag test article' }).should('exist'); + }); +}); diff --git a/cypress/fixtures/users/antitagUser.json b/cypress/fixtures/users/antitagUser.json new file mode 100644 index 000000000..f75028b01 --- /dev/null +++ b/cypress/fixtures/users/antitagUser.json @@ -0,0 +1,5 @@ +{ + "username": "not-a-fan", + "email": "not-a-fan@forem.local", + "password": "password" +} diff --git a/cypress/fixtures/users/hiddenTagUser.json b/cypress/fixtures/users/hiddenTagUser.json new file mode 100644 index 000000000..f75028b01 --- /dev/null +++ b/cypress/fixtures/users/hiddenTagUser.json @@ -0,0 +1,5 @@ +{ + "username": "not-a-fan", + "email": "not-a-fan@forem.local", + "password": "password" +} diff --git a/spec/queries/homepage/articles_query_spec.rb b/spec/queries/homepage/articles_query_spec.rb index 2f1a12990..fc6b82810 100644 --- a/spec/queries/homepage/articles_query_spec.rb +++ b/spec/queries/homepage/articles_query_spec.rb @@ -130,7 +130,7 @@ RSpec.describe Homepage::ArticlesQuery, type: :query do article2.tag_list.add(:ruby) article2.save - expect(described_class.call(tags: %i[beginners ruby]).ids).to match_array([article1.id, article2.id]) + expect(described_class.call(tags: %i[beginners ruby]).ids).to contain_exactly(article1.id, article2.id) end it "does not return results for partial matches", :aggregate_failures do @@ -143,6 +143,36 @@ RSpec.describe Homepage::ArticlesQuery, type: :query do end end + describe "hidden_tags" do + let!(:article1) { create(:article, tags: "twice, first") } + let!(:article2) { create(:article, tags: "second, twice") } + let!(:article3) { create(:article) } + + it "removes articles matching any hidden_tags" do + results = described_class.call(hidden_tags: ["twice"]) + expect(results).to contain_exactly(article3) + + results = described_class.call(hidden_tags: ["first"]) + expect(results).not_to include(article1) + expect(results).to include(article2) + + results = described_class.call(hidden_tags: ["second"]) + expect(results).to include(article1) + expect(results).not_to include(article2) + end + + it "behaves expectedly when hidden_tags is any variety of blank" do + results = described_class.call(hidden_tags: []) + expect(results).to contain_exactly(article1, article2, article3) + + results = described_class.call(hidden_tags: nil) + expect(results).to contain_exactly(article1, article2, article3) + + results = described_class.call(hidden_tags: "") + expect(results).to contain_exactly(article1, article2, article3) + end + end + describe "pagination" do it "paginates by default" do stub_const("Homepage::ArticlesQuery::DEFAULT_PER_PAGE", 1) diff --git a/spec/services/articles/feeds/basic_spec.rb b/spec/services/articles/feeds/basic_spec.rb index 35dc8c7e9..4eefa74de 100644 --- a/spec/services/articles/feeds/basic_spec.rb +++ b/spec/services/articles/feeds/basic_spec.rb @@ -1,7 +1,6 @@ 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) } @@ -12,8 +11,10 @@ RSpec.describe Articles::Feeds::Basic, type: :service do let!(:low_scoring_article) { create(:article, score: -1000) } let!(:month_old_story) { create(:article, :past, past_published_at: 1.month.ago) } # rubocop:disable RSpec/LetSetup + let(:feed) { described_class.new(user: user, number_of_articles: 100, page: 1) } + context "without a user" do - let(:feed) { described_class.new(user: nil, number_of_articles: 100, page: 1) } + let(:user) { nil } it "returns articles with score above 0 in order of hotness score" do result = feed.feed @@ -25,7 +26,7 @@ RSpec.describe Articles::Feeds::Basic, type: :service do end context "with a user" do - let(:feed) { described_class.new(user: user, number_of_articles: 100, page: 1) } + let(:user) { create(:user) } it "returns articles with score above 0 sorted by user preference scores" do user.follow(old_story.user) @@ -44,5 +45,23 @@ RSpec.describe Articles::Feeds::Basic, type: :service do result = feed.feed expect(result).not_to include(hot_story) end + + context "when user has hidden tags" do + let!(:hidden) { create(:article, tags: "notme") } + let!(:visible) { create(:article, tags: "surewhynot") } + + before do + antitag = ActsAsTaggableOn::Tag.find_by(name: "notme") || create(:tag, name: "notme") + user + .follows_by_type("ActsAsTaggableOn::Tag") + .create! followable: antitag, explicit_points: -5.0 + end + + it "does not return articles with tags the user has hidden" do + result = feed.feed + expect(result).not_to include(hidden) + expect(result).to include(visible) + end + end end end diff --git a/spec/support/seeds/seeds_e2e.rb b/spec/support/seeds/seeds_e2e.rb index 62955d057..8047ac23d 100644 --- a/spec/support/seeds/seeds_e2e.rb +++ b/spec/support/seeds/seeds_e2e.rb @@ -967,6 +967,29 @@ Settings::General.sidebar_tags = %i[tag1] ############################################################################## +seeder.create_if_doesnt_exist(User, "email", "not-a-fan@forem.local") do + antitagger = User.create!( + name: "Doesnt Like Tag1", + email: "not-a-fan@forem.local", + username: "not-a-fan", + profile_image: Rails.root.join("app/assets/images/#{rand(1..40)}.png").open, + confirmed_at: Time.current, + registered_at: Time.current, + password: "password", + password_confirmation: "password", + saw_onboarding: true, + checked_code_of_conduct: true, + checked_terms_and_conditions: true, + ) + + antitag1 = ActsAsTaggableOn::Tag.find_by(name: "tag1") || create(:tag, name: "tag1") + antitagger + .follows_by_type("ActsAsTaggableOn::Tag") + .create! followable: antitag1, explicit_points: -5.0 +end + +############################################################################## + seeder.create_if_doesnt_exist(Article, "title", "Tag test article") do markdown = <<~MARKDOWN ---