Change auto-following admin-selected accounts, show in recommendations (#16078)

app/controllers/api/v1/suggestions_controller.rb

@@ -5,20 +5,20 @@ class Api::V1::SuggestionsController < Api::BaseController
 
   before_action -> { doorkeeper_authorize! :read }
   before_action :require_user!
-  before_action :set_accounts
 
   def index
-    render json: @accounts, each_serializer: REST::AccountSerializer
+    suggestions = suggestions_source.get(current_account, limit: limit_param(DEFAULT_ACCOUNTS_LIMIT))
+    render json: suggestions.map(&:account), each_serializer: REST::AccountSerializer
   end
 
   def destroy
-    PotentialFriendshipTracker.remove(current_account.id, params[:id])
+    suggestions_source.remove(current_account, params[:id])
     render_empty
   end
 
   private
 
-  def set_accounts
-    @accounts = PotentialFriendshipTracker.get(current_account, limit_param(DEFAULT_ACCOUNTS_LIMIT))
+  def suggestions_source
+    AccountSuggestions::PastInteractionsSource.new
   end
 end

app/lib/potential_friendship_tracker.rb

@@ -27,15 +27,5 @@ class PotentialFriendshipTracker
     def remove(account_id, target_account_id)
       redis.zrem("interactions:#{account_id}", target_account_id)
     end
-
-    def get(account, limit)
-      account_ids = redis.zrevrange("interactions:#{account.id}", 0, limit)
-
-      return [] if account_ids.empty? || limit < 1
-
-      accounts = Account.searchable.where(id: account_ids).index_by(&:id)
-
-      account_ids.map { |id| accounts[id.to_i] }.compact
-    end
   end
 end

app/models/account_suggestions.rb

@@ -1,17 +1,28 @@
 # frozen_string_literal: true
 
 class AccountSuggestions
-  class Suggestion < ActiveModelSerializers::Model
-    attributes :account, :source
-  end
+  SOURCES = [
+    AccountSuggestions::SettingSource,
+    AccountSuggestions::PastInteractionsSource,
+    AccountSuggestions::GlobalSource,
+  ].freeze
 
   def self.get(account, limit)
-    suggestions = PotentialFriendshipTracker.get(account, limit).map { |target_account| Suggestion.new(account: target_account, source: :past_interaction) }
-    suggestions.concat(FollowRecommendation.get(account, limit - suggestions.size, suggestions.map { |suggestion| suggestion.account.id }).map { |target_account| Suggestion.new(account: target_account, source: :global) }) if suggestions.size < limit
-    suggestions
+    SOURCES.each_with_object([]) do |source_class, suggestions|
+      source_suggestions = source_class.new.get(
+        account,
+        skip_account_ids: suggestions.map(&:account_id),
+        limit: limit - suggestions.size
+      )
+
+      suggestions.concat(source_suggestions)
+    end
   end
 
   def self.remove(account, target_account_id)
-    PotentialFriendshipTracker.remove(account.id, target_account_id)
+    SOURCES.each do |source_class|
+      source = source_class.new
+      source.remove(account, target_account_id)
+    end
   end
 end

app/models/account_suggestions/global_source.rb

@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+class AccountSuggestions::GlobalSource < AccountSuggestions::Source
+  def key
+    :global
+  end
+
+  def get(account, skip_account_ids: [], limit: 40)
+    account_ids = account_ids_for_locale(account.user_locale) - [account.id] - skip_account_ids
+
+    as_ordered_suggestions(
+      scope(account).where(id: account_ids),
+      account_ids
+    ).take(limit)
+  end
+
+  def remove(_account, _target_account_id)
+    nil
+  end
+
+  private
+
+  def scope(account)
+    Account.searchable
+           .followable_by(account)
+           .not_excluded_by_account(account)
+           .not_domain_blocked_by_account(account)
+  end
+
+  def account_ids_for_locale(locale)
+    Redis.current.zrevrange("follow_recommendations:#{locale}", 0, -1).map(&:to_i)
+  end
+
+  def to_ordered_list_key(account)
+    account.id
+  end
+end

app/models/account_suggestions/past_interactions_source.rb

@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+class AccountSuggestions::PastInteractionsSource < AccountSuggestions::Source
+  include Redisable
+
+  def key
+    :past_interactions
+  end
+
+  def get(account, skip_account_ids: [], limit: 40)
+    account_ids = account_ids_for_account(account.id, limit + skip_account_ids.size) - skip_account_ids
+
+    as_ordered_suggestions(
+      scope.where(id: account_ids),
+      account_ids
+    ).take(limit)
+  end
+
+  def remove(account, target_account_id)
+    redis.zrem("interactions:#{account.id}", target_account_id)
+  end
+
+  private
+
+  def scope
+    Account.searchable
+  end
+
+  def account_ids_for_account(account_id, limit)
+    redis.zrevrange("interactions:#{account_id}", 0, limit).map(&:to_i)
+  end
+
+  def to_ordered_list_key(account)
+    account.id
+  end
+end

app/models/account_suggestions/setting_source.rb

@@ -0,0 +1,68 @@
+# frozen_string_literal: true
+
+class AccountSuggestions::SettingSource < AccountSuggestions::Source
+  def key
+    :staff
+  end
+
+  def get(account, skip_account_ids: [], limit: 40)
+    return [] unless setting_enabled?
+
+    as_ordered_suggestions(
+      scope(account).where(setting_to_where_condition).where.not(id: skip_account_ids),
+      usernames_and_domains
+    ).take(limit)
+  end
+
+  def remove(_account, _target_account_id)
+    nil
+  end
+
+  private
+
+  def scope(account)
+    Account.searchable
+           .followable_by(account)
+           .not_excluded_by_account(account)
+           .not_domain_blocked_by_account(account)
+           .where(locked: false)
+           .where.not(id: account.id)
+  end
+
+  def usernames_and_domains
+    @usernames_and_domains ||= setting_to_usernames_and_domains
+  end
+
+  def setting_enabled?
+    setting.present?
+  end
+
+  def setting_to_where_condition
+    usernames_and_domains.map do |(username, domain)|
+      Arel::Nodes::Grouping.new(
+        Account.arel_table[:username].lower.eq(username.downcase).and(
+          Account.arel_table[:domain].lower.eq(domain&.downcase)
+        )
+      )
+    end.reduce(:or)
+  end
+
+  def setting_to_usernames_and_domains
+    setting.split(',').map do |str|
+      username, domain = str.strip.gsub(/\A@/, '').split('@', 2)
+      domain           = nil if TagManager.instance.local_domain?(domain)
+
+      next if username.blank?
+
+      [username, domain]
+    end.compact
+  end
+
+  def setting
+    Setting.bootstrap_timeline_accounts
+  end
+
+  def to_ordered_list_key(account)
+    [account.username, account.domain]
+  end
+end

app/models/account_suggestions/source.rb

@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+class AccountSuggestions::Source
+  def key
+    raise NotImplementedError
+  end
+
+  def get(_account, **kwargs)
+    raise NotImplementedError
+  end
+
+  def remove(_account, target_account_id)
+    raise NotImplementedError
+  end
+
+  protected
+
+  def as_ordered_suggestions(scope, ordered_list)
+    return [] if ordered_list.empty?
+
+    map = scope.index_by(&method(:to_ordered_list_key))
+
+    ordered_list.map { |ordered_list_key| map[ordered_list_key] }.compact.map do |account|
+      AccountSuggestions::Suggestion.new(
+        account: account,
+        source: key
+      )
+    end
+  end
+
+  def to_ordered_list_key(_account)
+    raise NotImplementedError
+  end
+end

app/models/account_suggestions/suggestion.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AccountSuggestions::Suggestion < ActiveModelSerializers::Model
+  attributes :account, :source
+
+  delegate :id, to: :account, prefix: true
+end

app/models/follow_recommendation.rb

@@ -21,19 +21,4 @@ class FollowRecommendation < ApplicationRecord
   def readonly?
     true
   end
-
-  def self.get(account, limit, exclude_account_ids = [])
-    account_ids = Redis.current.zrevrange("follow_recommendations:#{account.user_locale}", 0, -1).map(&:to_i) - exclude_account_ids - [account.id]
-
-    return [] if account_ids.empty? || limit < 1
-
-    accounts = Account.followable_by(account)
-                      .not_excluded_by_account(account)
-                      .not_domain_blocked_by_account(account)
-                      .where(id: account_ids)
-                      .limit(limit)
-                      .index_by(&:id)
-
-    account_ids.map { |id| accounts[id] }.compact
-  end
 end

app/models/form/admin_settings.rb

@@ -16,7 +16,6 @@ class Form::AdminSettings
     open_deletion
     timeline_preview
     show_staff_badge
-    enable_bootstrap_timeline_accounts
     bootstrap_timeline_accounts
     theme
     min_invite_role
@@ -41,7 +40,6 @@ class Form::AdminSettings
     open_deletion
     timeline_preview
     show_staff_badge
-    enable_bootstrap_timeline_accounts
     activity_api_enabled
     peers_api_enabled
     show_known_fediverse_at_about_page

app/services/bootstrap_timeline_service.rb

@@ -5,48 +5,13 @@ class BootstrapTimelineService < BaseService
     @source_account = source_account
 
     autofollow_inviter!
-    autofollow_bootstrap_timeline_accounts! if Setting.enable_bootstrap_timeline_accounts
   end
 
   private
 
   def autofollow_inviter!
     return unless @source_account&.user&.invite&.autofollow?
-    FollowService.new.call(@source_account, @source_account.user.invite.user.account)
-  end
-
-  def autofollow_bootstrap_timeline_accounts!
-    bootstrap_timeline_accounts.each do |target_account|
-      begin
-        FollowService.new.call(@source_account, target_account)
-      rescue ActiveRecord::RecordNotFound, Mastodon::NotPermittedError
-        nil
-      end
-    end
-  end
-
-  def bootstrap_timeline_accounts
-    return @bootstrap_timeline_accounts if defined?(@bootstrap_timeline_accounts)
-
-    @bootstrap_timeline_accounts = bootstrap_timeline_accounts_usernames.empty? ? admin_accounts : local_unlocked_accounts(bootstrap_timeline_accounts_usernames)
-  end
-
-  def bootstrap_timeline_accounts_usernames
-    @bootstrap_timeline_accounts_usernames ||= (Setting.bootstrap_timeline_accounts || '').split(',').map { |str| str.strip.gsub(/\A@/, '') }.reject(&:blank?)
-  end
 
-  def admin_accounts
-    User.admins
-        .includes(:account)
-        .where(accounts: { locked: false })
-        .map(&:account)
-  end
-
-  def local_unlocked_accounts(usernames)
-    Account.local
-           .without_suspended
-           .where(username: usernames)
-           .where(locked: false)
-           .where(moved_to_account_id: nil)
+    FollowService.new.call(@source_account, @source_account.user.invite.user.account)
   end
 end

app/validators/existing_username_validator.rb

@@ -4,11 +4,25 @@ class ExistingUsernameValidator < ActiveModel::EachValidator
   def validate_each(record, attribute, value)
     return if value.blank?
 
-    if options[:multiple]
-      missing_usernames = value.split(',').map { |username| username.strip.gsub(/\A@/, '') }.filter_map { |username| username unless Account.find_local(username) }
-      record.errors.add(attribute, I18n.t('existing_username_validator.not_found_multiple', usernames: missing_usernames.join(', '))) if missing_usernames.any?
-    else
-      record.errors.add(attribute, I18n.t('existing_username_validator.not_found')) unless Account.find_local(value.strip.gsub(/\A@/, ''))
+    usernames_and_domains = begin
+      value.split(',').map do |str|
+        username, domain = str.strip.gsub(/\A@/, '').split('@')
+        domain = nil if TagManager.instance.local_domain?(domain)
+
+        next if username.blank?
+
+        [str, username, domain]
+      end.compact
+    end
+
+    usernames_with_no_accounts = usernames_and_domains.filter_map do |(str, username, domain)|
+      str unless Account.find_remote(username, domain)
+    end
+
+    if usernames_with_no_accounts.any? && options[:multiple]
+      record.errors.add(attribute, I18n.t('existing_username_validator.not_found_multiple', usernames: usernames_with_no_accounts.join(', ')))
+    elsif usernames_with_no_accounts.any? || usernames_and_domains.size > 1
+      record.errors.add(attribute, I18n.t('existing_username_validator.not_found'))
     end
   end
 end

app/views/admin/settings/edit.html.haml

@@ -50,10 +50,7 @@
   %hr.spacer/
 
   .fields-group
-    = f.input :enable_bootstrap_timeline_accounts, as: :boolean, wrapper: :with_label, label: t('admin.settings.enable_bootstrap_timeline_accounts.title'), hint: t('admin.settings.enable_bootstrap_timeline_accounts.desc_html')
-
-  .fields-group
-    = f.input :bootstrap_timeline_accounts, wrapper: :with_block_label, label: t('admin.settings.bootstrap_timeline_accounts.title'), hint: t('admin.settings.bootstrap_timeline_accounts.desc_html'), disabled: !Setting.enable_bootstrap_timeline_accounts
+    = f.input :bootstrap_timeline_accounts, wrapper: :with_block_label, label: t('admin.settings.bootstrap_timeline_accounts.title'), hint: t('admin.settings.bootstrap_timeline_accounts.desc_html')
 
   %hr.spacer/
 

config/locales/en.yml

@@ -564,8 +564,8 @@ en:
         desc_html: Counts of locally published posts, active users, and new registrations in weekly buckets
         title: Publish aggregate statistics about user activity in the API
       bootstrap_timeline_accounts:
-        desc_html: Separate multiple usernames by comma. Only local and unlocked accounts will work. Default when empty is all local admins.
-        title: Default follows for new users
+        desc_html: Separate multiple usernames by comma. These accounts will be guaranteed to be shown in follow recommendations
+        title: Recommend these accounts to new users
       contact_information:
         email: Business e-mail
         username: Contact username
@@ -582,9 +582,6 @@ en:
         users: To logged-in local users
       domain_blocks_rationale:
         title: Show rationale
-      enable_bootstrap_timeline_accounts:
-        desc_html: Make new users automatically follow configured accounts so their home feed doesn't start out empty
-        title: Enable default follows for new users
       hero:
         desc_html: Displayed on the frontpage. At least 600x100px recommended. When not set, falls back to server thumbnail
         title: Hero image

config/settings.yml

@@ -62,7 +62,6 @@ defaults: &defaults
     - mod
     - moderator
   disallowed_hashtags: # space separated string or list of hashtags without the hash
-  enable_bootstrap_timeline_accounts: true
   bootstrap_timeline_accounts: ''
   activity_api_enabled: true
   peers_api_enabled: true

spec/services/bootstrap_timeline_service_spec.rb

@@ -1,42 +1,4 @@
 require 'rails_helper'
 
 RSpec.describe BootstrapTimelineService, type: :service do
-  subject { described_class.new }
-
-  describe '#call' do
-    let(:source_account) { Fabricate(:account) }
-
-    context 'when setting is empty' do
-      let!(:admin) { Fabricate(:user, admin: true) }
-
-      before do
-        Setting.bootstrap_timeline_accounts = nil
-        subject.call(source_account)
-      end
-
-      it 'follows admin accounts from account' do
-        expect(source_account.following?(admin.account)).to be true
-      end
-    end
-
-    context 'when setting is set' do
-      let!(:alice) { Fabricate(:account, username: 'alice') }
-      let!(:bob)   { Fabricate(:account, username: 'bob') }
-      let!(:eve)   { Fabricate(:account, username: 'eve', suspended: true) }
-
-      before do
-        Setting.bootstrap_timeline_accounts = 'alice, @bob, eve, unknown'
-        subject.call(source_account)
-      end
-
-      it 'follows found accounts from account' do
-        expect(source_account.following?(alice)).to be true
-        expect(source_account.following?(bob)).to be true
-      end
-
-      it 'does not follow suspended account' do
-        expect(source_account.following?(eve)).to be false
-      end
-    end
-  end
 end