From 6e50134a42cb303e6e42f89f9ddb5aacf83e7a6d Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 25 Nov 2021 13:07:38 +0100 Subject: Add trending links (#16917) * Add trending links * Add overriding specific links trendability * Add link type to preview cards and only trend articles Change trends review notifications from being sent every 5 minutes to being sent every 2 hours Change threshold from 5 unique accounts to 15 unique accounts * Fix tests --- spec/controllers/admin/tags_controller_spec.rb | 12 ---- .../api/v1/trends/tags_controller_spec.rb | 22 +++++++ spec/controllers/api/v1/trends_controller_spec.rb | 18 ------ spec/helpers/languages_helper_spec.rb | 17 ++++++ spec/helpers/settings_helper_spec.rb | 22 ------- spec/mailers/previews/admin_mailer_preview.rb | 10 ++++ spec/models/trending_tags_spec.rb | 68 ---------------------- spec/models/trends/tags_spec.rb | 67 +++++++++++++++++++++ 8 files changed, 116 insertions(+), 120 deletions(-) create mode 100644 spec/controllers/api/v1/trends/tags_controller_spec.rb delete mode 100644 spec/controllers/api/v1/trends_controller_spec.rb create mode 100644 spec/helpers/languages_helper_spec.rb delete mode 100644 spec/helpers/settings_helper_spec.rb delete mode 100644 spec/models/trending_tags_spec.rb create mode 100644 spec/models/trends/tags_spec.rb (limited to 'spec') diff --git a/spec/controllers/admin/tags_controller_spec.rb b/spec/controllers/admin/tags_controller_spec.rb index 9145d887d..85c801a9c 100644 --- a/spec/controllers/admin/tags_controller_spec.rb +++ b/spec/controllers/admin/tags_controller_spec.rb @@ -9,18 +9,6 @@ RSpec.describe Admin::TagsController, type: :controller do sign_in Fabricate(:user, admin: true) end - describe 'GET #index' do - let!(:tag) { Fabricate(:tag) } - - before do - get :index - end - - it 'returns status 200' do - expect(response).to have_http_status(200) - end - end - describe 'GET #show' do let!(:tag) { Fabricate(:tag) } diff --git a/spec/controllers/api/v1/trends/tags_controller_spec.rb b/spec/controllers/api/v1/trends/tags_controller_spec.rb new file mode 100644 index 000000000..e2e26dcab --- /dev/null +++ b/spec/controllers/api/v1/trends/tags_controller_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Api::V1::Trends::TagsController, type: :controller do + render_views + + describe 'GET #index' do + before do + trending_tags = double() + + allow(trending_tags).to receive(:get).and_return(Fabricate.times(10, :tag)) + allow(Trends).to receive(:tags).and_return(trending_tags) + + get :index + end + + it 'returns http success' do + expect(response).to have_http_status(200) + end + end +end diff --git a/spec/controllers/api/v1/trends_controller_spec.rb b/spec/controllers/api/v1/trends_controller_spec.rb deleted file mode 100644 index 91e0d18fe..000000000 --- a/spec/controllers/api/v1/trends_controller_spec.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -RSpec.describe Api::V1::TrendsController, type: :controller do - render_views - - describe 'GET #index' do - before do - allow(TrendingTags).to receive(:get).and_return(Fabricate.times(10, :tag)) - get :index - end - - it 'returns http success' do - expect(response).to have_http_status(200) - end - end -end diff --git a/spec/helpers/languages_helper_spec.rb b/spec/helpers/languages_helper_spec.rb new file mode 100644 index 000000000..6db617824 --- /dev/null +++ b/spec/helpers/languages_helper_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe LanguagesHelper do + describe 'the HUMAN_LOCALES constant' do + it 'includes all I18n locales' do + expect(described_class::HUMAN_LOCALES.keys).to include(*I18n.available_locales) + end + end + + describe 'human_locale' do + it 'finds the human readable local description from a key' do + expect(helper.human_locale(:en)).to eq('English') + end + end +end diff --git a/spec/helpers/settings_helper_spec.rb b/spec/helpers/settings_helper_spec.rb deleted file mode 100644 index 092c37583..000000000 --- a/spec/helpers/settings_helper_spec.rb +++ /dev/null @@ -1,22 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -describe SettingsHelper do - describe 'the HUMAN_LOCALES constant' do - it 'includes all I18n locales' do - options = I18n.available_locales - - expect(described_class::HUMAN_LOCALES.keys).to include(*options) - end - end - - describe 'human_locale' do - it 'finds the human readable local description from a key' do - # Ensure the value is as we expect - expect(described_class::HUMAN_LOCALES[:en]).to eq('English') - - expect(helper.human_locale(:en)).to eq('English') - end - end -end diff --git a/spec/mailers/previews/admin_mailer_preview.rb b/spec/mailers/previews/admin_mailer_preview.rb index 561a56b78..75ffbbf40 100644 --- a/spec/mailers/previews/admin_mailer_preview.rb +++ b/spec/mailers/previews/admin_mailer_preview.rb @@ -5,4 +5,14 @@ class AdminMailerPreview < ActionMailer::Preview def new_pending_account AdminMailer.new_pending_account(Account.first, User.pending.first) end + + # Preview this email at http://localhost:3000/rails/mailers/admin_mailer/new_trending_tags + def new_trending_tags + AdminMailer.new_trending_tags(Account.first, Tag.limit(3)) + end + + # Preview this email at http://localhost:3000/rails/mailers/admin_mailer/new_trending_links + def new_trending_links + AdminMailer.new_trending_links(Account.first, PreviewCard.limit(3)) + end end diff --git a/spec/models/trending_tags_spec.rb b/spec/models/trending_tags_spec.rb deleted file mode 100644 index dfbc7d6f8..000000000 --- a/spec/models/trending_tags_spec.rb +++ /dev/null @@ -1,68 +0,0 @@ -require 'rails_helper' - -RSpec.describe TrendingTags do - describe '.record_use!' do - pending - end - - describe '.update!' do - let!(:at_time) { Time.now.utc } - let!(:tag1) { Fabricate(:tag, name: 'Catstodon', trendable: true) } - let!(:tag2) { Fabricate(:tag, name: 'DogsOfMastodon', trendable: true) } - let!(:tag3) { Fabricate(:tag, name: 'OCs', trendable: true) } - - before do - allow(Redis.current).to receive(:pfcount) do |key| - case key - when "activity:tags:#{tag1.id}:#{(at_time - 1.day).beginning_of_day.to_i}:accounts" - 2 - when "activity:tags:#{tag1.id}:#{at_time.beginning_of_day.to_i}:accounts" - 16 - when "activity:tags:#{tag2.id}:#{(at_time - 1.day).beginning_of_day.to_i}:accounts" - 0 - when "activity:tags:#{tag2.id}:#{at_time.beginning_of_day.to_i}:accounts" - 4 - when "activity:tags:#{tag3.id}:#{(at_time - 1.day).beginning_of_day.to_i}:accounts" - 13 - end - end - - Redis.current.zadd('trending_tags', 0.9, tag3.id) - Redis.current.sadd("trending_tags:used:#{at_time.beginning_of_day.to_i}", [tag1.id, tag2.id]) - - tag3.update(max_score: 0.9, max_score_at: (at_time - 1.day).beginning_of_day + 12.hours) - - described_class.update!(at_time) - end - - it 'calculates and re-calculates scores' do - expect(described_class.get(10, filtered: false)).to eq [tag1, tag3] - end - - it 'omits hashtags below threshold' do - expect(described_class.get(10, filtered: false)).to_not include(tag2) - end - - it 'decays scores' do - expect(Redis.current.zscore('trending_tags', tag3.id)).to be < 0.9 - end - end - - describe '.trending?' do - let(:tag) { Fabricate(:tag) } - - before do - 10.times { |i| Redis.current.zadd('trending_tags', i + 1, Fabricate(:tag).id) } - end - - it 'returns true if the hashtag is within limit' do - Redis.current.zadd('trending_tags', 11, tag.id) - expect(described_class.trending?(tag)).to be true - end - - it 'returns false if the hashtag is outside the limit' do - Redis.current.zadd('trending_tags', 0, tag.id) - expect(described_class.trending?(tag)).to be false - end - end -end diff --git a/spec/models/trends/tags_spec.rb b/spec/models/trends/tags_spec.rb new file mode 100644 index 000000000..4f98c6aa4 --- /dev/null +++ b/spec/models/trends/tags_spec.rb @@ -0,0 +1,67 @@ +require 'rails_helper' + +RSpec.describe Trends::Tags do + subject { described_class.new(threshold: 5, review_threshold: 10) } + + let!(:at_time) { DateTime.new(2021, 11, 14, 10, 15, 0) } + + describe '#add' do + let(:tag) { Fabricate(:tag) } + + before do + subject.add(tag, 1, at_time) + end + + it 'records history' do + expect(tag.history.get(at_time).accounts).to eq 1 + end + + it 'records use' do + expect(subject.send(:recently_used_ids, at_time)).to eq [tag.id] + end + end + + describe '#get' do + pending + end + + describe '#refresh' do + let!(:today) { at_time } + let!(:yesterday) { today - 1.day } + + let!(:tag1) { Fabricate(:tag, name: 'Catstodon', trendable: true) } + let!(:tag2) { Fabricate(:tag, name: 'DogsOfMastodon', trendable: true) } + let!(:tag3) { Fabricate(:tag, name: 'OCs', trendable: true) } + + before do + 2.times { |i| subject.add(tag1, i, yesterday) } + 13.times { |i| subject.add(tag3, i, yesterday) } + 16.times { |i| subject.add(tag1, i, today) } + 4.times { |i| subject.add(tag2, i, today) } + end + + context do + before do + subject.refresh(yesterday + 12.hours) + subject.refresh(at_time) + end + + it 'calculates and re-calculates scores' do + expect(subject.get(false, 10)).to eq [tag1, tag3] + end + + it 'omits hashtags below threshold' do + expect(subject.get(false, 10)).to_not include(tag2) + end + end + + it 'decays scores' do + subject.refresh(yesterday + 12.hours) + original_score = subject.score(tag3.id) + expect(original_score).to eq 144.0 + subject.refresh(yesterday + 12.hours + subject.options[:max_score_halflife]) + decayed_score = subject.score(tag3.id) + expect(decayed_score).to be <= original_score / 2 + end + end +end -- cgit From 013bee6afb9f76c354e5f8d096d9c85d1ac484ce Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 25 Nov 2021 23:46:30 +0100 Subject: Fix filtering DMs from non-followed users (#17042) --- app/services/notify_service.rb | 43 +++++++++++++++++++++++++++++++++++- spec/services/notify_service_spec.rb | 17 ++++++++++++-- 2 files changed, 57 insertions(+), 3 deletions(-) (limited to 'spec') diff --git a/app/services/notify_service.rb b/app/services/notify_service.rb index a1b5ca1e3..09e28b76b 100644 --- a/app/services/notify_service.rb +++ b/app/services/notify_service.rb @@ -67,8 +67,49 @@ class NotifyService < BaseService message? && @notification.target_status.direct_visibility? end + # Returns true if the sender has been mentionned by the recipient up the thread def response_to_recipient? - @notification.target_status.in_reply_to_account_id == @recipient.id && @notification.target_status.thread&.direct_visibility? + return false if @notification.target_status.in_reply_to_id.nil? + + # Using an SQL CTE to avoid unneeded back-and-forth with SQL server in case of long threads + !Status.count_by_sql([<<-SQL.squish, id: @notification.target_status.in_reply_to_id, recipient_id: @recipient.id, sender_id: @notification.from_account.id]).zero? + WITH RECURSIVE ancestors(id, in_reply_to_id, replying_to_sender) AS ( + SELECT + s.id, s.in_reply_to_id, (CASE + WHEN s.account_id = :recipient_id THEN + EXISTS ( + SELECT * + FROM mentions m + WHERE m.silent = FALSE AND m.account_id = :sender_id AND m.status_id = s.id + ) + ELSE + FALSE + END) + FROM statuses s + WHERE s.id = :id + UNION ALL + SELECT + s.id, + s.in_reply_to_id, + (CASE + WHEN s.account_id = :recipient_id THEN + EXISTS ( + SELECT * + FROM mentions m + WHERE m.silent = FALSE AND m.account_id = :sender_id AND m.status_id = s.id + ) + ELSE + FALSE + END) + FROM ancestors st + JOIN statuses s ON s.id = st.in_reply_to_id + WHERE st.replying_to_sender IS FALSE + ) + SELECT COUNT(*) + FROM ancestors st + JOIN statuses s ON s.id = st.id + WHERE st.replying_to_sender IS TRUE AND s.visibility = 3 + SQL end def from_staff? diff --git a/spec/services/notify_service_spec.rb b/spec/services/notify_service_spec.rb index f2cb22c5e..7433866b7 100644 --- a/spec/services/notify_service_spec.rb +++ b/spec/services/notify_service_spec.rb @@ -64,8 +64,9 @@ RSpec.describe NotifyService, type: :service do is_expected.to_not change(Notification, :count) end - context 'if the message chain initiated by recipient, but is not direct message' do + context 'if the message chain is initiated by recipient, but is not direct message' do let(:reply_to) { Fabricate(:status, account: recipient) } + let!(:mention) { Fabricate(:mention, account: sender, status: reply_to) } let(:activity) { Fabricate(:mention, account: recipient, status: Fabricate(:status, account: sender, visibility: :direct, thread: reply_to)) } it 'does not notify' do @@ -73,8 +74,20 @@ RSpec.describe NotifyService, type: :service do end end - context 'if the message chain initiated by recipient and is direct message' do + context 'if the message chain is initiated by recipient, but without a mention to the sender, even if the sender sends multiple messages in a row' do + let(:reply_to) { Fabricate(:status, account: recipient) } + let!(:mention) { Fabricate(:mention, account: sender, status: reply_to) } + let(:dummy_reply) { Fabricate(:status, account: sender, visibility: :direct, thread: reply_to) } + let(:activity) { Fabricate(:mention, account: recipient, status: Fabricate(:status, account: sender, visibility: :direct, thread: dummy_reply)) } + + it 'does not notify' do + is_expected.to_not change(Notification, :count) + end + end + + context 'if the message chain is initiated by the recipient with a mention to the sender' do let(:reply_to) { Fabricate(:status, account: recipient, visibility: :direct) } + let!(:mention) { Fabricate(:mention, account: sender, status: reply_to) } let(:activity) { Fabricate(:mention, account: recipient, status: Fabricate(:status, account: sender, visibility: :direct, thread: reply_to)) } it 'does notify' do -- cgit