Enhance coverage for StatusPin model (#32515)

app/models/status_pin.rb

@@ -17,11 +17,17 @@ class StatusPin < ApplicationRecord
 
   validates_with StatusPinValidator
 
-  after_destroy :invalidate_cleanup_info
+  after_destroy :invalidate_cleanup_info, if: %i(account_matches_status_account? account_local?)
+
+  delegate :local?, to: :account, prefix: true
+
+  private
 
   def invalidate_cleanup_info
-    return unless status&.account_id == account_id && account.local?
-
     account.statuses_cleanup_policy&.invalidate_last_inspected(status, :unpin)
   end
+
+  def account_matches_status_account?
+    status&.account_id == account_id
+  end
 end

spec/models/status_pin_spec.rb

@@ -3,70 +3,100 @@
 require 'rails_helper'
 
 RSpec.describe StatusPin do
-  describe 'validations' do
+  describe 'Validations' do
-    it 'allows pins of own statuses' do
+    subject { Fabricate.build :status_pin }
-      account = Fabricate(:account)
-      status  = Fabricate(:status, account: account)
 
-      expect(described_class.new(account: account, status: status).save).to be true
+    context 'with an account pinning statuses' do
-    end
+      subject { Fabricate.build :status_pin, account: account }
 
-    it 'does not allow pins of statuses by someone else' do
+      let(:account) { Fabricate(:account) }
-      account = Fabricate(:account)
-      status  = Fabricate(:status)
 
-      expect(described_class.new(account: account, status: status).save).to be false
+      context 'with a self-owned status' do
-    end
+        let(:status) { Fabricate(:status, account: account) }
 
-    it 'does not allow pins of reblogs' do
+        it { is_expected.to allow_value(status).for(:status) }
-      account = Fabricate(:account)
-      status  = Fabricate(:status, account: account)
-      reblog  = Fabricate(:status, reblog: status)
-
-      expect(described_class.new(account: account, status: reblog).save).to be false
-    end
-
-    it 'does allow pins of direct statuses' do
-      account = Fabricate(:account)
-      status  = Fabricate(:status, account: account, visibility: :private)
-
-      expect(described_class.new(account: account, status: status).save).to be true
-    end
-
-    it 'does not allow pins of direct statuses' do
-      account = Fabricate(:account)
-      status  = Fabricate(:status, account: account, visibility: :direct)
-
-      expect(described_class.new(account: account, status: status).save).to be false
-    end
-
-    context 'with a pin limit' do
-      before { stub_const('StatusPinValidator::PIN_LIMIT', 2) }
-
-      it 'does not allow pins above the max' do
-        account = Fabricate(:account)
-
-        Fabricate.times(StatusPinValidator::PIN_LIMIT, :status_pin, account: account)
-
-        pin = described_class.new(account: account, status: Fabricate(:status, account: account))
-        expect(pin.save)
-          .to be(false)
-
-        expect(pin.errors[:base])
-          .to contain_exactly(I18n.t('statuses.pin_errors.limit'))
       end
 
-      it 'allows pins above the max for remote accounts' do
+      context 'with a status from someone else' do
-        account = Fabricate(:account, domain: 'remote.test', username: 'bob', url: 'https://remote.test/')
+        let(:status) { Fabricate(:status) }
 
-        Fabricate.times(StatusPinValidator::PIN_LIMIT, :status_pin, account: account)
+        it { is_expected.to_not allow_value(status).for(:status).against(:base) }
+      end
 
-        pin = described_class.new(account: account, status: Fabricate(:status, account: account))
+      context 'with a reblog status' do
-        expect(pin.save)
+        let(:status) { Fabricate(:status, reblog: Fabricate(:status, account: account)) }
-          .to be(true)
 
-        expect(pin.errors[:base])
+        it { is_expected.to_not allow_value(status).for(:status).against(:base) }
-          .to be_empty
+      end
+
+      context 'with a private status' do
+        let(:status) { Fabricate(:status, account: account, visibility: :private) }
+
+        it { is_expected.to allow_value(status).for(:status).against(:base) }
+      end
+
+      context 'with a direct status' do
+        let(:status) { Fabricate(:status, account: account, visibility: :direct) }
+
+        it { is_expected.to_not allow_value(status).for(:status).against(:base) }
+      end
+    end
+
+    context 'with a validator pin limit' do
+      before { stub_const('StatusPinValidator::PIN_LIMIT', 2) }
+
+      context 'with a local account at the limit' do
+        let(:account) { Fabricate :account }
+
+        before { Fabricate.times(StatusPinValidator::PIN_LIMIT, :status_pin, account: account) }
+
+        it { is_expected.to_not allow_value(account).for(:account).against(:base).with_message(I18n.t('statuses.pin_errors.limit')) }
+      end
+
+      context 'with a remote account at the limit' do
+        let(:account) { Fabricate :account, domain: 'remote.test' }
+
+        before { Fabricate.times(StatusPinValidator::PIN_LIMIT, :status_pin, account: account) }
+
+        it { is_expected.to allow_value(account).for(:account) }
+      end
+    end
+  end
+
+  describe 'Callbacks' do
+    describe 'Invalidating status via policy' do
+      subject { Fabricate :status_pin, status: Fabricate(:status, account: account), account: account }
+
+      context 'with a local account that owns the status and has a policy' do
+        let(:account) { Fabricate :account, domain: nil }
+
+        before do
+          Fabricate :account_statuses_cleanup_policy, account: account
+          account.statuses_cleanup_policy.record_last_inspected(subject.status.id + 1_024)
+        end
+
+        it 'calls the invalidator on destroy' do
+          expect { subject.destroy }
+            .to change(account.statuses_cleanup_policy, :last_inspected)
+        end
+      end
+
+      context 'with a local account that owns the status and does not have a policy' do
+        let(:account) { Fabricate :account, domain: nil }
+
+        it 'does not call the invalidator on destroy' do
+          expect { subject.destroy }
+            .to_not change(account, :updated_at)
+        end
+      end
+
+      context 'with a remote account' do
+        let(:account) { Fabricate :account, domain: 'host.example' }
+
+        it 'does not call the invalidator on destroy' do
+          expect { subject.destroy }
+            .to_not change(account, :updated_at)
+        end
       end
     end
   end