From 4fa462a9f877162fe447f0c8bb4769e0037c486e Mon Sep 17 00:00:00 2001
From: Matt Jankowski <matt@jankowski.online>
Date: Sun, 24 Aug 2025 09:55:55 -0400
Subject: [PATCH] Extract form batch classes for relationships and follow
 recommendations

Related to - https://github.com/mastodon/mastodon/pull/35458 - which added a batch base class, and to - https://github.com/mastodon/mastodon/pull/35872 - which envisions a world where form objects tell the view what to do.

Background here...

- The `Form::AccountBatch` form object is used in a few different places, for somewhat varied purposes
- The user relationships view uses it on a show page, which does a PUT to update, and uses the `follow`, `unfollow`, `remove_from_followers`, and `remove_domains_from_followers` "actions" of the form model
- The admin accounts view shows a form on index view, does POST to batch action, uses the `suspend`, `approve` and `reject` actions.
- The admin relationships view (for admins only) also has a form on index and does a POST back to that same spot
- The admin/follow_recommendations page has a form on show and does PUT to update, using the `suppress_follow_recommendation` and `unsuppress_follow_recommendation` actions

So, while these are all related in that they supply an `accounts_ids` array, the actual things that they "bulk update" are Follow (from relationships), FollowRecommendationSuppression (from follow recs) and Account (from accounts). This is a little inconsistent with other of the "batch form model" classes, which tend to update the same domain concept (ie, CustomEmojiBlock, IpBlockBatch, EmailDomainBlockBatch, etc) that they are named after (and in some cases that a "filter" class is named after).

All that said, the changes here:

- Pull out `Form::FollowRecommendationBatch` and `Form::RelationshipBatch` classes, which more closely align with the domain concept they are bulk updating (open to naming suggestions here)
- Mark both of these as `persisted?` true so that their form views will infer correct HTTP action
- Add some missing coverage to existing class, and coverage for new classes as well

It would be pretty straightforward to chop this up into smaller pieces, if any of "just the coverage first", "just the boolean attribute", "one new batch class at a time", etc - were more appealing.

Possible follow-up:

- In the existing `Form::AccountBatch`, use attributes API for boolean `select_all_matching` value, remove query method
- Convert the relationships controller spec to mix of request/system specs, round out coverage
- Most (all maybe?) of the "batch actions" are doing a `POST` to a `batch` action on the controller of whatever they are related to. Might be worth looking at what the diff would look like to move these all to nested controllers where they did a `PUT to */batches#update` with restful routing, etc.
---
 .../follow_recommendations_controller.rb      |  8 +-
 app/controllers/relationships_controller.rb   |  8 +-
 app/models/form/account_batch.rb              | 56 ------------
 .../form/follow_recommendation_batch.rb       | 38 ++++++++
 app/models/form/relationship_batch.rb         | 58 +++++++++++++
 .../follow_recommendations/show.html.haml     |  2 +-
 app/views/relationships/show.html.haml        |  2 +-
 .../relationships_controller_spec.rb          |  2 +-
 ...w_recommendation_suppression_fabricator.rb |  5 ++
 spec/models/form/account_batch_spec.rb        | 33 +++++++
 .../form/follow_recommendation_batch_spec.rb  | 45 ++++++++++
 spec/models/form/relationship_batch_spec.rb   | 87 +++++++++++++++++++
 .../admin/follow_recommendations_spec.rb      |  2 +-
 spec/requests/relationships_spec.rb           |  2 +-
 14 files changed, 279 insertions(+), 69 deletions(-)
 create mode 100644 app/models/form/follow_recommendation_batch.rb
 create mode 100644 app/models/form/relationship_batch.rb
 create mode 100644 spec/fabricators/follow_recommendation_suppression_fabricator.rb
 create mode 100644 spec/models/form/follow_recommendation_batch_spec.rb
 create mode 100644 spec/models/form/relationship_batch_spec.rb

diff --git a/app/controllers/admin/follow_recommendations_controller.rb b/app/controllers/admin/follow_recommendations_controller.rb
index b060cfbe949..01719db2215 100644
--- a/app/controllers/admin/follow_recommendations_controller.rb
+++ b/app/controllers/admin/follow_recommendations_controller.rb
@@ -7,14 +7,14 @@ module Admin
     def show
       authorize :follow_recommendation, :show?
 
-      @form     = Form::AccountBatch.new
+      @form = Form::FollowRecommendationBatch.new
       @accounts = filtered_follow_recommendations.page(params[:page])
     end
 
     def update
       authorize :follow_recommendation, :show?
 
-      @form = Form::AccountBatch.new(form_account_batch_params.merge(current_account: current_account, action: action_from_button))
+      @form = Form::FollowRecommendationBatch.new(form_follow_recommendation_batch_params.merge(current_account: current_account, action: action_from_button))
       @form.save
     rescue ActionController::ParameterMissing
       # Do nothing
@@ -36,9 +36,9 @@ module Admin
       @follow_recommendation_filter ||= FollowRecommendationFilter.new(filter_params)
     end
 
-    def form_account_batch_params
+    def form_follow_recommendation_batch_params
       params
-        .expect(form_account_batch: [:action, account_ids: []])
+        .expect(form_follow_recommendation_batch: [:action, account_ids: []])
     end
 
     def filter_params
diff --git a/app/controllers/relationships_controller.rb b/app/controllers/relationships_controller.rb
index 7e793fc7342..b65162c25b2 100644
--- a/app/controllers/relationships_controller.rb
+++ b/app/controllers/relationships_controller.rb
@@ -10,11 +10,11 @@ class RelationshipsController < ApplicationController
   helper_method :following_relationship?, :followed_by_relationship?, :mutual_relationship?
 
   def show
-    @form = Form::AccountBatch.new
+    @form = Form::RelationshipBatch.new
   end
 
   def update
-    @form = Form::AccountBatch.new(form_account_batch_params.merge(current_account: current_account, action: action_from_button))
+    @form = Form::RelationshipBatch.new(form_relationship_batch_params.merge(current_account: current_account, action: action_from_button))
     @form.save
   rescue ActionController::ParameterMissing
     # Do nothing
@@ -34,8 +34,8 @@ class RelationshipsController < ApplicationController
     @relationships = AccountRelationshipsPresenter.new(@accounts, current_user.account_id)
   end
 
-  def form_account_batch_params
-    params.expect(form_account_batch: [:action, account_ids: []])
+  def form_relationship_batch_params
+    params.expect(form_relationship_batch: [:action, account_ids: []])
   end
 
   def following_relationship?
diff --git a/app/models/form/account_batch.rb b/app/models/form/account_batch.rb
index f3109ad62a7..72d025a578b 100644
--- a/app/models/form/account_batch.rb
+++ b/app/models/form/account_batch.rb
@@ -9,22 +9,10 @@ class Form::AccountBatch < Form::BaseBatch
 
   def save
     case action
-    when 'follow'
-      follow!
-    when 'unfollow'
-      unfollow!
-    when 'remove_from_followers'
-      remove_from_followers!
-    when 'remove_domains_from_followers'
-      remove_domains_from_followers!
     when 'approve'
       approve!
     when 'reject'
       reject!
-    when 'suppress_follow_recommendation'
-      suppress_follow_recommendation!
-    when 'unsuppress_follow_recommendation'
-      unsuppress_follow_recommendation!
     when 'suspend'
       suspend!
     end
@@ -32,36 +20,6 @@ class Form::AccountBatch < Form::BaseBatch
 
   private
 
-  def follow!
-    error = nil
-
-    accounts.each do |target_account|
-      FollowService.new.call(current_account, target_account)
-    rescue Mastodon::NotPermittedError, ActiveRecord::RecordNotFound => e
-      error ||= e
-    end
-
-    raise error if error.present?
-  end
-
-  def unfollow!
-    accounts.each do |target_account|
-      UnfollowService.new.call(current_account, target_account)
-    end
-  end
-
-  def remove_from_followers!
-    RemoveFromFollowersService.new.call(current_account, account_ids)
-  end
-
-  def remove_domains_from_followers!
-    RemoveDomainsFromFollowersService.new.call(current_account, account_domains)
-  end
-
-  def account_domains
-    accounts.group(:domain).pluck(:domain).compact
-  end
-
   def accounts
     if select_all_matching?
       query
@@ -92,20 +50,6 @@ class Form::AccountBatch < Form::BaseBatch
     end
   end
 
-  def suppress_follow_recommendation!
-    authorize(:follow_recommendation, :suppress?)
-
-    accounts.find_each do |account|
-      FollowRecommendationSuppression.create(account: account)
-    end
-  end
-
-  def unsuppress_follow_recommendation!
-    authorize(:follow_recommendation, :unsuppress?)
-
-    FollowRecommendationSuppression.where(account_id: account_ids).destroy_all
-  end
-
   def reject_account(account)
     authorize(account.user, :reject?)
     log_action(:reject, account.user)
diff --git a/app/models/form/follow_recommendation_batch.rb b/app/models/form/follow_recommendation_batch.rb
new file mode 100644
index 00000000000..45fec0734b2
--- /dev/null
+++ b/app/models/form/follow_recommendation_batch.rb
@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+class Form::FollowRecommendationBatch < Form::BaseBatch
+  attr_accessor :account_ids
+
+  def save
+    case action
+    when 'suppress_follow_recommendation'
+      suppress!
+    when 'unsuppress_follow_recommendation'
+      unsuppress!
+    end
+  end
+
+  def persisted?
+    true
+  end
+
+  private
+
+  def suppress!
+    authorize(:follow_recommendation, :suppress?)
+
+    accounts.find_each do |account|
+      FollowRecommendationSuppression.create(account: account)
+    end
+  end
+
+  def unsuppress!
+    authorize(:follow_recommendation, :unsuppress?)
+
+    FollowRecommendationSuppression.where(account_id: account_ids).destroy_all
+  end
+
+  def accounts
+    Account.where(id: account_ids)
+  end
+end
diff --git a/app/models/form/relationship_batch.rb b/app/models/form/relationship_batch.rb
new file mode 100644
index 00000000000..1d34a12288b
--- /dev/null
+++ b/app/models/form/relationship_batch.rb
@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+
+class Form::RelationshipBatch < Form::BaseBatch
+  attr_accessor :account_ids
+
+  def save
+    case action
+    when 'follow'
+      follow!
+    when 'unfollow'
+      unfollow!
+    when 'remove_from_followers'
+      remove_from_followers!
+    when 'remove_domains_from_followers'
+      remove_domains_from_followers!
+    end
+  end
+
+  def persisted?
+    true
+  end
+
+  private
+
+  def follow!
+    error = nil
+
+    accounts.each do |target_account|
+      FollowService.new.call(current_account, target_account)
+    rescue Mastodon::NotPermittedError, ActiveRecord::RecordNotFound => e
+      error ||= e
+    end
+
+    raise error if error.present?
+  end
+
+  def unfollow!
+    accounts.each do |target_account|
+      UnfollowService.new.call(current_account, target_account)
+    end
+  end
+
+  def remove_from_followers!
+    RemoveFromFollowersService.new.call(current_account, account_ids)
+  end
+
+  def remove_domains_from_followers!
+    RemoveDomainsFromFollowersService.new.call(current_account, account_domains)
+  end
+
+  def account_domains
+    accounts.group(:domain).pluck(:domain).compact
+  end
+
+  def accounts
+    Account.where(id: account_ids)
+  end
+end
diff --git a/app/views/admin/follow_recommendations/show.html.haml b/app/views/admin/follow_recommendations/show.html.haml
index 6c2627743b4..73d943614d5 100644
--- a/app/views/admin/follow_recommendations/show.html.haml
+++ b/app/views/admin/follow_recommendations/show.html.haml
@@ -21,7 +21,7 @@
         %li= filter_link_to t('admin.accounts.moderation.active'), status: nil
         %li= filter_link_to t('admin.follow_recommendations.suppressed'), status: 'suppressed'
 
-= form_with model: @form, url: admin_follow_recommendations_path, method: :patch do |f|
+= form_with model: @form, url: admin_follow_recommendations_path do |f|
   - RelationshipFilter::KEYS.each do |key|
     = hidden_field_tag key, params[key] if params[key].present?
 
diff --git a/app/views/relationships/show.html.haml b/app/views/relationships/show.html.haml
index 7528e7386b1..ae9fcb69f5f 100644
--- a/app/views/relationships/show.html.haml
+++ b/app/views/relationships/show.html.haml
@@ -31,7 +31,7 @@
       %li= filter_link_to t('relationships.most_recent'), order: nil
       %li= filter_link_to t('relationships.last_active'), order: 'active'
 
-= form_with model: @form, url: relationships_path, method: :patch do |f|
+= form_with model: @form, url: relationships_path do |f|
   = hidden_field_tag :page, params[:page] || 1
 
   - RelationshipFilter::KEYS.each do |key|
diff --git a/spec/controllers/relationships_controller_spec.rb b/spec/controllers/relationships_controller_spec.rb
index 633d72fbba3..55b5be35fe9 100644
--- a/spec/controllers/relationships_controller_spec.rb
+++ b/spec/controllers/relationships_controller_spec.rb
@@ -53,7 +53,7 @@ RSpec.describe RelationshipsController do
     end
 
     context 'when select parameter is provided' do
-      subject { patch :update, params: { form_account_batch: { account_ids: [alice.id] }, remove_domains_from_followers: '' } }
+      subject { patch :update, params: { form_relationship_batch: { account_ids: [alice.id] }, remove_domains_from_followers: '' } }
 
       it 'soft-blocks followers from selected domains' do
         alice.follow!(user.account)
diff --git a/spec/fabricators/follow_recommendation_suppression_fabricator.rb b/spec/fabricators/follow_recommendation_suppression_fabricator.rb
new file mode 100644
index 00000000000..f545f9f3807
--- /dev/null
+++ b/spec/fabricators/follow_recommendation_suppression_fabricator.rb
@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+Fabricator(:follow_recommendation_suppression) do
+  account { Fabricate.build(:account) }
+end
diff --git a/spec/models/form/account_batch_spec.rb b/spec/models/form/account_batch_spec.rb
index 8db9a2a2545..f5303f0ed3b 100644
--- a/spec/models/form/account_batch_spec.rb
+++ b/spec/models/form/account_batch_spec.rb
@@ -11,6 +11,7 @@ RSpec.describe Form::AccountBatch do
     let(:account)     { Fabricate(:admin_user).account }
     let(:account_ids) { [] }
     let(:query)       { Account.none }
+    let(:select_all_matching) { false }
 
     before do
       account_batch.assign_attributes(
@@ -77,5 +78,37 @@ RSpec.describe Form::AccountBatch do
         [target_account.reload, target_account2.reload].map(&:suspended?)
       end
     end
+
+    context 'when action is "approve"' do
+      let(:action) { 'approve' }
+      let(:account_ids) { [unapproved_user_account.id] }
+      let(:unapproved_user_account) { Fabricate :account }
+
+      before { unapproved_user_account.user.update!(approved: false) }
+
+      it 'approves the user of the supplied accounts' do
+        expect { subject }
+          .to change { unapproved_user_account.reload.user.approved? }.to(true)
+      end
+    end
+
+    context 'when action is "reject"' do
+      let(:action) { 'reject' }
+      let(:account_ids) { [reject_user_account.id] }
+      let(:reject_user_account) { Fabricate :account }
+
+      before { reject_user_account.user.update!(approved: false) }
+
+      it 'rejects the user of the supplied accounts' do
+        expect { subject }
+          .to change { reject_user_account.reload.suspended? }.to(true)
+      end
+    end
+
+    context 'when action is "unknown"' do
+      let(:action) { 'unknown' }
+
+      it { is_expected.to be_nil }
+    end
   end
 end
diff --git a/spec/models/form/follow_recommendation_batch_spec.rb b/spec/models/form/follow_recommendation_batch_spec.rb
new file mode 100644
index 00000000000..2988236fb12
--- /dev/null
+++ b/spec/models/form/follow_recommendation_batch_spec.rb
@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Form::FollowRecommendationBatch do
+  describe '#persisted?' do
+    it { is_expected.to be_persisted }
+  end
+
+  describe '#save' do
+    subject { described_class.new(action:, account_ids:, current_account: admin).save }
+
+    let(:account_ids) { [account.id] }
+    let(:account) { Fabricate :account }
+    let(:admin) { Fabricate :account, user: Fabricate(:admin_user) }
+
+    context 'when action is suppress_follow_recommendation' do
+      let(:action) { 'suppress_follow_recommendation' }
+
+      it 'adds a suppression for the accounts' do
+        expect { subject }
+          .to change(FollowRecommendationSuppression, :count).by(1)
+          .and change { account.reload.follow_recommendation_suppression }.from(be_nil).to(be_present)
+      end
+    end
+
+    context 'when action is unsuppress_follow_recommendation' do
+      let(:action) { 'unsuppress_follow_recommendation' }
+
+      before { Fabricate :follow_recommendation_suppression, account: }
+
+      it 'removes a suppression for the accounts' do
+        expect { subject }
+          .to change(FollowRecommendationSuppression, :count).by(-1)
+          .and change { account.reload.follow_recommendation_suppression }.from(be_present).to(be_nil)
+      end
+    end
+
+    context 'when action is unknown' do
+      let(:action) { 'unknown' }
+
+      it { is_expected.to be_nil }
+    end
+  end
+end
diff --git a/spec/models/form/relationship_batch_spec.rb b/spec/models/form/relationship_batch_spec.rb
new file mode 100644
index 00000000000..882703f1501
--- /dev/null
+++ b/spec/models/form/relationship_batch_spec.rb
@@ -0,0 +1,87 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Form::RelationshipBatch do
+  describe '#persisted?' do
+    it { is_expected.to be_persisted }
+  end
+
+  describe '#save' do
+    subject { described_class.new(action:, account_ids:, current_account:).save }
+
+    let(:account_ids) { [account.id] }
+    let(:account) { Fabricate :account }
+    let(:current_account) { Fabricate :account }
+
+    context 'when action is follow' do
+      let(:action) { 'follow' }
+      let(:account_ids) { [account.id] }
+
+      it 'adds a follow for the accounts' do
+        expect { subject }
+          .to change(Follow, :count).by(1)
+          .and change { current_account.reload.active_relationships.exists?(target_account: account) }.from(false).to(true)
+      end
+
+      context 'when account cannot be followed' do
+        let(:account) { Fabricate :account, domain: 'test.example' }
+
+        it 'does not save follows and re-raises error' do
+          expect { subject }
+            .to raise_error(Mastodon::NotPermittedError)
+            .and not_change(Follow, :count)
+        end
+      end
+    end
+
+    context 'when action is unfollow' do
+      let(:action) { 'unfollow' }
+
+      before { Fabricate :follow, account: current_account, target_account: account }
+
+      it 'removes a follow for the accounts' do
+        expect { subject }
+          .to change(Follow, :count).by(-1)
+          .and change { current_account.reload.active_relationships.exists?(target_account: account) }.from(true).to(false)
+      end
+    end
+
+    context 'when action is remove_from_followers' do
+      let(:action) { 'remove_from_followers' }
+
+      before { Fabricate :follow, account: account, target_account: current_account }
+
+      it 'removes followers from the accounts' do
+        expect { subject }
+          .to change(Follow, :count).by(-1)
+          .and change { current_account.reload.passive_relationships.exists?(account: account) }.from(true).to(false)
+      end
+    end
+
+    context 'when action is remove_domains_from_followers' do
+      let(:action) { 'remove_domains_from_followers' }
+
+      let(:account) { Fabricate :account, domain: 'host.example' }
+      let(:account_other) { Fabricate :account, domain: 'host.example' }
+
+      before do
+        Fabricate :follow, account: account, target_account: current_account
+        Fabricate :follow, account: account_other, target_account: current_account
+      end
+
+      it 'removes all followers from domains of the accounts' do
+        expect { subject }
+          .to change(Follow, :count).by(-2)
+          .and change { current_account.reload.passive_relationships.exists?(account: account) }.from(true).to(false)
+          .and change { current_account.reload.passive_relationships.exists?(account: account_other) }.from(true).to(false)
+      end
+    end
+
+    context 'when action is unknown' do
+      let(:action) { 'unknown' }
+
+      it { is_expected.to be_nil }
+    end
+  end
+end
diff --git a/spec/requests/admin/follow_recommendations_spec.rb b/spec/requests/admin/follow_recommendations_spec.rb
index 146c26448cd..0d76fe4336c 100644
--- a/spec/requests/admin/follow_recommendations_spec.rb
+++ b/spec/requests/admin/follow_recommendations_spec.rb
@@ -7,7 +7,7 @@ RSpec.describe 'Admin Follow Recommendations' do
     before { sign_in Fabricate(:admin_user) }
 
     it 'gracefully handles invalid nested params' do
-      put admin_follow_recommendations_path(form_account_batch: 'invalid')
+      put admin_follow_recommendations_path(form_follow_recommendation_batch: 'invalid')
 
       expect(response)
         .to redirect_to(admin_follow_recommendations_path)
diff --git a/spec/requests/relationships_spec.rb b/spec/requests/relationships_spec.rb
index ee6b321c46a..3d5d76ed8ca 100644
--- a/spec/requests/relationships_spec.rb
+++ b/spec/requests/relationships_spec.rb
@@ -7,7 +7,7 @@ RSpec.describe 'Relationships' do
     before { sign_in Fabricate(:user) }
 
     it 'gracefully handles invalid nested params' do
-      put relationships_path(form_account_batch: 'invalid')
+      put relationships_path(form_relationship_batch: 'invalid')
 
       expect(response)
         .to redirect_to(relationships_path)