Merge 4fa462a9f8 into 624c024766

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.

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

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?

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)

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

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

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?
 

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|

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)

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

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

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

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

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)

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)