From 6555c33503ae58a327c29a54b91a5d88fc7d8b55 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Sat, 8 Jul 2023 14:03:38 -0400 Subject: [PATCH 1/4] Admin mailer parameterization (#25759) --- app/mailers/admin_mailer.rb | 45 +++++++++++-------- app/models/trends.rb | 2 +- app/models/user.rb | 2 +- app/services/appeal_service.rb | 2 +- app/services/report_service.rb | 2 +- config/i18n-tasks.yml | 1 + .../api/v1/reports_controller_spec.rb | 4 +- .../disputes/appeals_controller_spec.rb | 4 +- spec/mailers/admin_mailer_spec.rb | 8 ++-- spec/mailers/previews/admin_mailer_preview.rb | 6 +-- 10 files changed, 40 insertions(+), 36 deletions(-) diff --git a/app/mailers/admin_mailer.rb b/app/mailers/admin_mailer.rb index bc6d87ae6f..5baf9b38a5 100644 --- a/app/mailers/admin_mailer.rb +++ b/app/mailers/admin_mailer.rb @@ -6,45 +6,52 @@ class AdminMailer < ApplicationMailer helper :accounts helper :languages - def new_report(recipient, report) - @report = report - @me = recipient - @instance = Rails.configuration.x.local_domain + before_action :process_params + before_action :set_instance + + default to: -> { @me.user_email } + + def new_report(report) + @report = report locale_for_account(@me) do - mail to: @me.user_email, subject: I18n.t('admin_mailer.new_report.subject', instance: @instance, id: @report.id) + mail subject: default_i18n_subject(instance: @instance, id: @report.id) end end - def new_appeal(recipient, appeal) - @appeal = appeal - @me = recipient - @instance = Rails.configuration.x.local_domain + def new_appeal(appeal) + @appeal = appeal locale_for_account(@me) do - mail to: @me.user_email, subject: I18n.t('admin_mailer.new_appeal.subject', instance: @instance, username: @appeal.account.username) + mail subject: default_i18n_subject(instance: @instance, username: @appeal.account.username) end end - def new_pending_account(recipient, user) - @account = user.account - @me = recipient - @instance = Rails.configuration.x.local_domain + def new_pending_account(user) + @account = user.account locale_for_account(@me) do - mail to: @me.user_email, subject: I18n.t('admin_mailer.new_pending_account.subject', instance: @instance, username: @account.username) + mail subject: default_i18n_subject(instance: @instance, username: @account.username) end end - def new_trends(recipient, links, tags, statuses) + def new_trends(links, tags, statuses) @links = links @tags = tags @statuses = statuses - @me = recipient - @instance = Rails.configuration.x.local_domain locale_for_account(@me) do - mail to: @me.user_email, subject: I18n.t('admin_mailer.new_trends.subject', instance: @instance) + mail subject: default_i18n_subject(instance: @instance) end end + + private + + def process_params + @me = params[:recipient] + end + + def set_instance + @instance = Rails.configuration.x.local_domain + end end diff --git a/app/models/trends.rb b/app/models/trends.rb index d07d62b71b..7ca51e0b3d 100644 --- a/app/models/trends.rb +++ b/app/models/trends.rb @@ -35,7 +35,7 @@ module Trends return if links_requiring_review.empty? && tags_requiring_review.empty? && statuses_requiring_review.empty? User.those_who_can(:manage_taxonomies).includes(:account).find_each do |user| - AdminMailer.new_trends(user.account, links_requiring_review, tags_requiring_review, statuses_requiring_review).deliver_later! if user.allows_trends_review_emails? + AdminMailer.with(recipient: user.account).new_trends(links_requiring_review, tags_requiring_review, statuses_requiring_review).deliver_later! if user.allows_trends_review_emails? end end diff --git a/app/models/user.rb b/app/models/user.rb index 5ee14bbdaf..fa445af811 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -475,7 +475,7 @@ class User < ApplicationRecord User.those_who_can(:manage_users).includes(:account).find_each do |u| next unless u.allows_pending_account_emails? - AdminMailer.new_pending_account(u.account, self).deliver_later + AdminMailer.with(recipient: u.account).new_pending_account(self).deliver_later end end diff --git a/app/services/appeal_service.rb b/app/services/appeal_service.rb index 399a053d6a..ef052e3547 100644 --- a/app/services/appeal_service.rb +++ b/app/services/appeal_service.rb @@ -23,7 +23,7 @@ class AppealService < BaseService def notify_staff! User.those_who_can(:manage_appeals).includes(:account).each do |u| - AdminMailer.new_appeal(u.account, @appeal).deliver_later if u.allows_appeal_emails? + AdminMailer.with(recipient: u.account).new_appeal(@appeal).deliver_later if u.allows_appeal_emails? end end end diff --git a/app/services/report_service.rb b/app/services/report_service.rb index 3444e1dfaf..39ebd5cd8a 100644 --- a/app/services/report_service.rb +++ b/app/services/report_service.rb @@ -40,7 +40,7 @@ class ReportService < BaseService User.those_who_can(:manage_reports).includes(:account).each do |u| LocalNotificationWorker.perform_async(u.account_id, @report.id, 'Report', 'admin.report') - AdminMailer.new_report(u.account, @report).deliver_later if u.allows_report_emails? + AdminMailer.with(recipient: u.account).new_report(@report).deliver_later if u.allows_report_emails? end end diff --git a/config/i18n-tasks.yml b/config/i18n-tasks.yml index 035a0e999d..cb00a62d5f 100644 --- a/config/i18n-tasks.yml +++ b/config/i18n-tasks.yml @@ -63,6 +63,7 @@ ignore_unused: - 'admin_mailer.new_appeal.actions.*' - 'statuses.attached.*' - 'move_handler.carry_{mutes,blocks}_over_text' + - 'admin_mailer.*.subject' - 'notification_mailer.*' - 'imports.overwrite_preambles.{following,blocking,muting,domain_blocking,bookmarks}_html' - 'imports.preambles.{following,blocking,muting,domain_blocking,bookmarks}_html' diff --git a/spec/controllers/api/v1/reports_controller_spec.rb b/spec/controllers/api/v1/reports_controller_spec.rb index 01b7e4a71c..f923ff0794 100644 --- a/spec/controllers/api/v1/reports_controller_spec.rb +++ b/spec/controllers/api/v1/reports_controller_spec.rb @@ -23,8 +23,6 @@ RSpec.describe Api::V1::ReportsController do let(:rule_ids) { nil } before do - allow(AdminMailer).to receive(:new_report) - .and_return(instance_double(ActionMailer::MessageDelivery, deliver_later: nil)) post :create, params: { status_ids: [status.id], account_id: target_account.id, comment: 'reasons', category: category, rule_ids: rule_ids, forward: forward } end @@ -41,7 +39,7 @@ RSpec.describe Api::V1::ReportsController do end it 'sends e-mails to admins' do - expect(AdminMailer).to have_received(:new_report).with(admin.account, Report) + expect(ActionMailer::Base.deliveries.first.to).to eq([admin.email]) end context 'when a status does not belong to the reported account' do diff --git a/spec/controllers/disputes/appeals_controller_spec.rb b/spec/controllers/disputes/appeals_controller_spec.rb index a0f9c7b910..c8444a2a96 100644 --- a/spec/controllers/disputes/appeals_controller_spec.rb +++ b/spec/controllers/disputes/appeals_controller_spec.rb @@ -14,13 +14,11 @@ RSpec.describe Disputes::AppealsController do let(:strike) { Fabricate(:account_warning, target_account: current_user.account) } before do - allow(AdminMailer).to receive(:new_appeal) - .and_return(instance_double(ActionMailer::MessageDelivery, deliver_later: nil)) post :create, params: { strike_id: strike.id, appeal: { text: 'Foo' } } end it 'notifies staff about new appeal' do - expect(AdminMailer).to have_received(:new_appeal).with(admin.account, Appeal.last) + expect(ActionMailer::Base.deliveries.first.to).to eq([admin.email]) end it 'redirects back to the strike page' do diff --git a/spec/mailers/admin_mailer_spec.rb b/spec/mailers/admin_mailer_spec.rb index 8e2eec40fd..9123804a48 100644 --- a/spec/mailers/admin_mailer_spec.rb +++ b/spec/mailers/admin_mailer_spec.rb @@ -7,7 +7,7 @@ RSpec.describe AdminMailer do let(:sender) { Fabricate(:account, username: 'John') } let(:recipient) { Fabricate(:account, username: 'Mike') } let(:report) { Fabricate(:report, account: sender, target_account: recipient) } - let(:mail) { described_class.new_report(recipient, report) } + let(:mail) { described_class.with(recipient: recipient).new_report(report) } before do recipient.user.update(locale: :en) @@ -27,7 +27,7 @@ RSpec.describe AdminMailer do describe '.new_appeal' do let(:appeal) { Fabricate(:appeal) } let(:recipient) { Fabricate(:account, username: 'Kurt') } - let(:mail) { described_class.new_appeal(recipient, appeal) } + let(:mail) { described_class.with(recipient: recipient).new_appeal(appeal) } before do recipient.user.update(locale: :en) @@ -47,7 +47,7 @@ RSpec.describe AdminMailer do describe '.new_pending_account' do let(:recipient) { Fabricate(:account, username: 'Barklums') } let(:user) { Fabricate(:user) } - let(:mail) { described_class.new_pending_account(recipient, user) } + let(:mail) { described_class.with(recipient: recipient).new_pending_account(user) } before do recipient.user.update(locale: :en) @@ -69,7 +69,7 @@ RSpec.describe AdminMailer do let(:links) { [] } let(:statuses) { [] } let(:tags) { [] } - let(:mail) { described_class.new_trends(recipient, links, tags, statuses) } + let(:mail) { described_class.with(recipient: recipient).new_trends(links, tags, statuses) } before do recipient.user.update(locale: :en) diff --git a/spec/mailers/previews/admin_mailer_preview.rb b/spec/mailers/previews/admin_mailer_preview.rb index 9572768cd7..bc8f0193b9 100644 --- a/spec/mailers/previews/admin_mailer_preview.rb +++ b/spec/mailers/previews/admin_mailer_preview.rb @@ -5,16 +5,16 @@ class AdminMailerPreview < ActionMailer::Preview # Preview this email at http://localhost:3000/rails/mailers/admin_mailer/new_pending_account def new_pending_account - AdminMailer.new_pending_account(Account.first, User.pending.first) + AdminMailer.with(recipient: Account.first).new_pending_account(User.pending.first) end # Preview this email at http://localhost:3000/rails/mailers/admin_mailer/new_trends def new_trends - AdminMailer.new_trends(Account.first, PreviewCard.joins(:trend).limit(3), Tag.limit(3), Status.joins(:trend).where(reblog_of_id: nil).limit(3)) + AdminMailer.with(recipient: Account.first).new_trends(PreviewCard.joins(:trend).limit(3), Tag.limit(3), Status.joins(:trend).where(reblog_of_id: nil).limit(3)) end # Preview this email at http://localhost:3000/rails/mailers/admin_mailer/new_appeal def new_appeal - AdminMailer.new_appeal(Account.first, Appeal.first) + AdminMailer.with(recipient: Account.first).new_appeal(Appeal.first) end end From b63d71fd489b440665f2afed0d17597ad9fb220d Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Sat, 8 Jul 2023 14:04:21 -0400 Subject: [PATCH 2/4] Remove unused `NotificationMailer#digest` preview (#25719) --- spec/mailers/previews/notification_mailer_preview.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/spec/mailers/previews/notification_mailer_preview.rb b/spec/mailers/previews/notification_mailer_preview.rb index bc41662a16..214161881b 100644 --- a/spec/mailers/previews/notification_mailer_preview.rb +++ b/spec/mailers/previews/notification_mailer_preview.rb @@ -32,9 +32,4 @@ class NotificationMailerPreview < ActionMailer::Preview r = Status.where.not(reblog_of_id: nil).first NotificationMailer.reblog(r.reblog.account, Notification.find_by(activity: r)) end - - # Preview this email at http://localhost:3000/rails/mailers/notification_mailer/digest - def digest - NotificationMailer.digest(Account.first, since: 90.days.ago) - end end From 7496c1b6556303ddad063edff023d792140cf470 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 8 Jul 2023 20:05:33 +0200 Subject: [PATCH 3/4] Change label and design of sensitive and unavailable media in web UI (#25712) --- .../mastodon/components/media_gallery.jsx | 10 ++++-- app/javascript/mastodon/locales/en.json | 4 ++- .../styles/mastodon/components.scss | 33 +++++++++---------- 3 files changed, 26 insertions(+), 21 deletions(-) diff --git a/app/javascript/mastodon/components/media_gallery.jsx b/app/javascript/mastodon/components/media_gallery.jsx index 1044b729b3..e3c0065c95 100644 --- a/app/javascript/mastodon/components/media_gallery.jsx +++ b/app/javascript/mastodon/components/media_gallery.jsx @@ -321,7 +321,10 @@ class MediaGallery extends PureComponent { if (uncached) { spoilerButton = ( ); } else if (visible) { @@ -329,7 +332,10 @@ class MediaGallery extends PureComponent { } else { spoilerButton = ( ); } diff --git a/app/javascript/mastodon/locales/en.json b/app/javascript/mastodon/locales/en.json index 8705e6cd68..edecaf60f3 100644 --- a/app/javascript/mastodon/locales/en.json +++ b/app/javascript/mastodon/locales/en.json @@ -618,6 +618,8 @@ "status.history.created": "{name} created {date}", "status.history.edited": "{name} edited {date}", "status.load_more": "Load more", + "status.media.open": "Click to open", + "status.media.show": "Click to show", "status.media_hidden": "Media hidden", "status.mention": "Mention @{name}", "status.more": "More", @@ -648,7 +650,7 @@ "status.title.with_attachments": "{user} posted {attachmentCount, plural, one {an attachment} other {{attachmentCount} attachments}}", "status.translate": "Translate", "status.translated_from_with": "Translated from {lang} using {provider}", - "status.uncached_media_warning": "Not available", + "status.uncached_media_warning": "Preview not available", "status.unmute_conversation": "Unmute conversation", "status.unpin": "Unpin from profile", "subscribed_languages.lead": "Only posts in selected languages will appear on your home and list timelines after the change. Select none to receive posts in all languages.", diff --git a/app/javascript/styles/mastodon/components.scss b/app/javascript/styles/mastodon/components.scss index 434a2f542d..0d816ba8d8 100644 --- a/app/javascript/styles/mastodon/components.scss +++ b/app/javascript/styles/mastodon/components.scss @@ -4213,34 +4213,31 @@ a.status-card.compact:hover { } &__overlay { - display: block; - background: transparent; + display: flex; + align-items: center; + justify-content: center; + background: rgba($black, 0.5); width: 100%; height: 100%; + padding: 0; + margin: 0; border: 0; + border-radius: 4px; &__label { - display: inline-block; - background: rgba($base-overlay-background, 0.5); - border-radius: 8px; - padding: 8px 12px; + display: flex; + align-items: center; + justify-content: center; + gap: 8px; + flex-direction: column; color: $primary-text-color; font-weight: 500; font-size: 14px; } - &:hover, - &:focus, - &:active { - .spoiler-button__overlay__label { - background: rgba($base-overlay-background, 0.8); - } - } - - &:disabled { - .spoiler-button__overlay__label { - background: rgba($base-overlay-background, 0.5); - } + &__action { + font-weight: 400; + font-size: 13px; } } } From b945f16ddf0c87654d5dc5d0f46f475913247dab Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 8 Jul 2023 20:16:48 +0200 Subject: [PATCH 4/4] Fix trend calculation working on too many items at a time (#25835) --- app/models/trends/links.rb | 26 +++++++++++++++++++------- app/models/trends/statuses.rb | 26 +++++++++++++++++++------- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/app/models/trends/links.rb b/app/models/trends/links.rb index c94f7c0237..fcbdb1a5f5 100644 --- a/app/models/trends/links.rb +++ b/app/models/trends/links.rb @@ -3,6 +3,8 @@ class Trends::Links < Trends::Base PREFIX = 'trending_links' + BATCH_SIZE = 100 + self.default_options = { threshold: 5, review_threshold: 3, @@ -67,8 +69,21 @@ class Trends::Links < Trends::Base end def refresh(at_time = Time.now.utc) - preview_cards = PreviewCard.where(id: (recently_used_ids(at_time) + PreviewCardTrend.pluck(:preview_card_id)).uniq) - calculate_scores(preview_cards, at_time) + # First, recalculate scores for links that were trending previously. We split the queries + # to avoid having to load all of the IDs into Ruby just to send them back into Postgres + PreviewCard.where(id: PreviewCardTrend.select(:preview_card_id)).find_in_batches(batch_size: BATCH_SIZE) do |preview_cards| + calculate_scores(preview_cards, at_time) + end + + # Then, calculate scores for links that were used today. There are potentially some + # duplicate items here that we might process one more time, but that should be fine + PreviewCard.where(id: recently_used_ids(at_time)).find_in_batches(batch_size: BATCH_SIZE) do |preview_cards| + calculate_scores(preview_cards, at_time) + end + + # Now that all trends have up-to-date scores, and all the ones below the threshold have + # been removed, we can recalculate their positions + PreviewCardTrend.connection.exec_update('UPDATE preview_card_trends SET rank = t0.calculated_rank FROM (SELECT id, row_number() OVER w AS calculated_rank FROM preview_card_trends WINDOW w AS (PARTITION BY language ORDER BY score DESC)) t0 WHERE preview_card_trends.id = t0.id') end def request_review @@ -139,10 +154,7 @@ class Trends::Links < Trends::Base to_insert = items.filter { |(score, _)| score >= options[:decay_threshold] } to_delete = items.filter { |(score, _)| score < options[:decay_threshold] } - PreviewCardTrend.transaction do - PreviewCardTrend.upsert_all(to_insert.map { |(score, preview_card)| { preview_card_id: preview_card.id, score: score, language: preview_card.language, allowed: preview_card.trendable? || false } }, unique_by: :preview_card_id) if to_insert.any? - PreviewCardTrend.where(preview_card_id: to_delete.map { |(_, preview_card)| preview_card.id }).delete_all if to_delete.any? - PreviewCardTrend.connection.exec_update('UPDATE preview_card_trends SET rank = t0.calculated_rank FROM (SELECT id, row_number() OVER w AS calculated_rank FROM preview_card_trends WINDOW w AS (PARTITION BY language ORDER BY score DESC)) t0 WHERE preview_card_trends.id = t0.id') - end + PreviewCardTrend.upsert_all(to_insert.map { |(score, preview_card)| { preview_card_id: preview_card.id, score: score, language: preview_card.language, allowed: preview_card.trendable? || false } }, unique_by: :preview_card_id) if to_insert.any? + PreviewCardTrend.where(preview_card_id: to_delete.map { |(_, preview_card)| preview_card.id }).delete_all if to_delete.any? end end diff --git a/app/models/trends/statuses.rb b/app/models/trends/statuses.rb index 84bff9c027..5cd352a6f2 100644 --- a/app/models/trends/statuses.rb +++ b/app/models/trends/statuses.rb @@ -3,6 +3,8 @@ class Trends::Statuses < Trends::Base PREFIX = 'trending_statuses' + BATCH_SIZE = 100 + self.default_options = { threshold: 5, review_threshold: 3, @@ -58,8 +60,21 @@ class Trends::Statuses < Trends::Base end def refresh(at_time = Time.now.utc) - statuses = Status.where(id: (recently_used_ids(at_time) + StatusTrend.pluck(:status_id)).uniq).includes(:status_stat, :account) - calculate_scores(statuses, at_time) + # First, recalculate scores for statuses that were trending previously. We split the queries + # to avoid having to load all of the IDs into Ruby just to send them back into Postgres + Status.where(id: StatusTrend.select(:status_id)).includes(:status_stat, :account).find_in_batches(batch_size: BATCH_SIZE) do |statuses| + calculate_scores(statuses, at_time) + end + + # Then, calculate scores for statuses that were used today. There are potentially some + # duplicate items here that we might process one more time, but that should be fine + Status.where(id: recently_used_ids(at_time)).includes(:status_stat, :account).find_in_batches(batch_size: BATCH_SIZE) do |statuses| + calculate_scores(statuses, at_time) + end + + # Now that all trends have up-to-date scores, and all the ones below the threshold have + # been removed, we can recalculate their positions + StatusTrend.connection.exec_update('UPDATE status_trends SET rank = t0.calculated_rank FROM (SELECT id, row_number() OVER w AS calculated_rank FROM status_trends WINDOW w AS (PARTITION BY language ORDER BY score DESC)) t0 WHERE status_trends.id = t0.id') end def request_review @@ -117,10 +132,7 @@ class Trends::Statuses < Trends::Base to_insert = items.filter { |(score, _)| score >= options[:decay_threshold] } to_delete = items.filter { |(score, _)| score < options[:decay_threshold] } - StatusTrend.transaction do - StatusTrend.upsert_all(to_insert.map { |(score, status)| { status_id: status.id, account_id: status.account_id, score: score, language: status.language, allowed: status.trendable? || false } }, unique_by: :status_id) if to_insert.any? - StatusTrend.where(status_id: to_delete.map { |(_, status)| status.id }).delete_all if to_delete.any? - StatusTrend.connection.exec_update('UPDATE status_trends SET rank = t0.calculated_rank FROM (SELECT id, row_number() OVER w AS calculated_rank FROM status_trends WINDOW w AS (PARTITION BY language ORDER BY score DESC)) t0 WHERE status_trends.id = t0.id') - end + StatusTrend.upsert_all(to_insert.map { |(score, status)| { status_id: status.id, account_id: status.account_id, score: score, language: status.language, allowed: status.trendable? || false } }, unique_by: :status_id) if to_insert.any? + StatusTrend.where(status_id: to_delete.map { |(_, status)| status.id }).delete_all if to_delete.any? end end