From 1edb527072b8004b0ac25157d49f992b697ff800 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Mon, 28 Oct 2024 03:34:58 -0400 Subject: [PATCH] Enhance coverage for `StatusPin` model (#32515) --- app/models/status_pin.rb | 12 ++- spec/models/status_pin_spec.rb | 140 ++++++++++++++++++++------------- 2 files changed, 94 insertions(+), 58 deletions(-) diff --git a/app/models/status_pin.rb b/app/models/status_pin.rb index dae4a5b4e6..83711dde42 100644 --- a/app/models/status_pin.rb +++ b/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 diff --git a/spec/models/status_pin_spec.rb b/spec/models/status_pin_spec.rb index da375009ae..1501d43cc4 100644 --- a/spec/models/status_pin_spec.rb +++ b/spec/models/status_pin_spec.rb @@ -3,70 +3,100 @@ require 'rails_helper' RSpec.describe StatusPin do - describe 'validations' do - it 'allows pins of own statuses' do - account = Fabricate(:account) - status = Fabricate(:status, account: account) + describe 'Validations' do + subject { Fabricate.build :status_pin } - expect(described_class.new(account: account, status: status).save).to be true - end + context 'with an account pinning statuses' do + subject { Fabricate.build :status_pin, account: account } - it 'does not allow pins of statuses by someone else' do - account = Fabricate(:account) - status = Fabricate(:status) + let(:account) { Fabricate(:account) } - expect(described_class.new(account: account, status: status).save).to be false - end + context 'with a self-owned status' do + let(:status) { Fabricate(:status, account: account) } - it 'does not allow pins of reblogs' do - 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')) + it { is_expected.to allow_value(status).for(:status) } end - it 'allows pins above the max for remote accounts' do - account = Fabricate(:account, domain: 'remote.test', username: 'bob', url: 'https://remote.test/') + context 'with a status from someone else' do + 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)) - expect(pin.save) - .to be(true) + context 'with a reblog status' do + let(:status) { Fabricate(:status, reblog: Fabricate(:status, account: account)) } - expect(pin.errors[:base]) - .to be_empty + it { is_expected.to_not allow_value(status).for(:status).against(:base) } + 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