From 42e6433769e1a6293359f6387500310ee8060165 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Fri, 11 Oct 2024 17:20:33 -0400 Subject: [PATCH] Add `Status::Visibility` concern to hold visibility logic --- app/models/concerns/status/visibility.rb | 51 ++++++ app/models/status.rb | 27 +-- .../models/concerns/status/visibility_spec.rb | 156 ++++++++++++++++++ spec/models/status_spec.rb | 30 ---- 4 files changed, 208 insertions(+), 56 deletions(-) create mode 100644 app/models/concerns/status/visibility.rb create mode 100644 spec/models/concerns/status/visibility_spec.rb diff --git a/app/models/concerns/status/visibility.rb b/app/models/concerns/status/visibility.rb new file mode 100644 index 0000000000..c034f5802f --- /dev/null +++ b/app/models/concerns/status/visibility.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module Status::Visibility + extend ActiveSupport::Concern + + DISTRIBUTABLE_VISIBILITIES = %i(public unlisted).freeze + LIST_ELIGIBLE_VISIBILITIES = %i(public unlisted private).freeze + RESTRICTED_VISIBILITIES = %w(direct limited).freeze + + included do + enum :visibility, + { public: 0, unlisted: 1, private: 2, direct: 3, limited: 4 }, + suffix: :visibility, + validate: true + + scope :distributable_visibility, -> { where(visibility: DISTRIBUTABLE_VISIBILITIES) } + scope :list_eligible_visibility, -> { where(visibility: LIST_ELIGIBLE_VISIBILITIES) } + scope :not_direct_visibility, -> { where.not(visibility: :direct) } + + validates :visibility, exclusion: { in: RESTRICTED_VISIBILITIES }, if: :reblog? + + before_validation :set_visibility, unless: :visibility? + end + + class_methods do + def selectable_visibilities + visibilities.keys - RESTRICTED_VISIBILITIES + end + end + + def hidden? + !distributable? + end + + def distributable? + public_visibility? || unlisted_visibility? + end + + alias sign? distributable? + + private + + def set_visibility + self.visibility ||= reblog.visibility if reblog? + self.visibility ||= visibility_from_account + end + + def visibility_from_account + account.locked? ? :private : :public + end +end diff --git a/app/models/status.rb b/app/models/status.rb index c58de5114c..146b024222 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -38,6 +38,7 @@ class Status < ApplicationRecord include Status::SearchConcern include Status::SnapshotConcern include Status::ThreadingConcern + include Status::Visibility MEDIA_ATTACHMENTS_LIMIT = 4 @@ -52,8 +53,6 @@ class Status < ApplicationRecord update_index('statuses', :proper) update_index('public_statuses', :proper) - enum :visibility, { public: 0, unlisted: 1, private: 2, direct: 3, limited: 4 }, suffix: :visibility, validate: true - belongs_to :application, class_name: 'Doorkeeper::Application', optional: true belongs_to :account, inverse_of: :statuses @@ -98,7 +97,6 @@ class Status < ApplicationRecord validates_with StatusLengthValidator validates_with DisallowedHashtagsValidator validates :reblog, uniqueness: { scope: :account }, if: :reblog? - validates :visibility, exclusion: { in: %w(direct limited) }, if: :reblog? accepts_nested_attributes_for :poll @@ -125,9 +123,6 @@ class Status < ApplicationRecord scope :tagged_with_none, lambda { |tag_ids| where('NOT EXISTS (SELECT * FROM statuses_tags forbidden WHERE forbidden.status_id = statuses.id AND forbidden.tag_id IN (?))', tag_ids) } - scope :distributable_visibility, -> { where(visibility: %i(public unlisted)) } - scope :list_eligible_visibility, -> { where(visibility: %i(public unlisted private)) } - scope :not_direct_visibility, -> { where.not(visibility: :direct) } after_create_commit :trigger_create_webhooks after_update_commit :trigger_update_webhooks @@ -140,7 +135,6 @@ class Status < ApplicationRecord before_validation :prepare_contents, if: :local? before_validation :set_reblog - before_validation :set_visibility before_validation :set_conversation before_validation :set_local @@ -242,16 +236,6 @@ class Status < ApplicationRecord PreviewCardsStatus.where(status_id: id).delete_all end - def hidden? - !distributable? - end - - def distributable? - public_visibility? || unlisted_visibility? - end - - alias sign? distributable? - def with_media? ordered_media_attachments.any? end @@ -351,10 +335,6 @@ class Status < ApplicationRecord end class << self - def selectable_visibilities - visibilities.keys - %w(direct limited) - end - def favourites_map(status_ids, account_id) Favourite.select('status_id').where(status_id: status_ids).where(account_id: account_id).each_with_object({}) { |f, h| h[f.status_id] = true } end @@ -436,11 +416,6 @@ class Status < ApplicationRecord update_column(:poll_id, poll.id) if association(:poll).loaded? && poll.present? end - def set_visibility - self.visibility = reblog.visibility if reblog? && visibility.nil? - self.visibility = (account.locked? ? :private : :public) if visibility.nil? - end - def set_conversation self.thread = thread.reblog if thread&.reblog? diff --git a/spec/models/concerns/status/visibility_spec.rb b/spec/models/concerns/status/visibility_spec.rb new file mode 100644 index 0000000000..01e3d87d1c --- /dev/null +++ b/spec/models/concerns/status/visibility_spec.rb @@ -0,0 +1,156 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Status::Visibility do + describe 'Scopes' do + let!(:public_status) { Fabricate :status, visibility: :public } + let!(:unlisted_status) { Fabricate :status, visibility: :unlisted } + let!(:private_status) { Fabricate :status, visibility: :private } + let!(:direct_status) { Fabricate :status, visibility: :direct } + let!(:limited_status) { Fabricate :status, visibility: :limited } + + describe '.list_eligible_visibility' do + it 'returns appropriate records' do + expect(Status.list_eligible_visibility) + .to include( + public_status, + unlisted_status, + private_status + ) + .and not_include(direct_status) + .and not_include(limited_status) + end + end + + describe '.distributable_visibility' do + it 'returns appropriate records' do + expect(Status.distributable_visibility) + .to include( + public_status, + unlisted_status + ) + .and not_include(private_status) + .and not_include(direct_status) + .and not_include(limited_status) + end + end + end + + describe 'Callbacks' do + describe 'Setting visibility in before validation' do + subject { Fabricate.build :status, visibility: nil } + + context 'when explicit value is set' do + before { subject.visibility = :public } + + it 'does not change' do + expect { subject.valid? } + .to_not change(subject, :visibility) + end + end + + context 'when status is a reblog' do + before { subject.reblog = Fabricate(:status, visibility: :public) } + + it 'changes to match the reblog' do + expect { subject.valid? } + .to change(subject, :visibility).to('public') + end + end + + context 'when account is locked' do + before { subject.account = Fabricate.build(:account, locked: true) } + + it 'changes to private' do + expect { subject.valid? } + .to change(subject, :visibility).to('private') + end + end + + context 'when account is not locked' do + before { subject.account = Fabricate.build(:account, locked: false) } + + it 'changes to public' do + expect { subject.valid? } + .to change(subject, :visibility).to('public') + end + end + end + end + + describe '.selectable_visibilities' do + it 'returns options available for default privacy selection' do + expect(Status.selectable_visibilities) + .to match(%w(public unlisted private)) + end + end + + describe '#hidden?' do + subject { Status.new } + + context 'when visibility is private' do + before { subject.visibility = :private } + + it { is_expected.to be_hidden } + end + + context 'when visibility is direct' do + before { subject.visibility = :direct } + + it { is_expected.to be_hidden } + end + + context 'when visibility is limited' do + before { subject.visibility = :limited } + + it { is_expected.to be_hidden } + end + + context 'when visibility is public' do + before { subject.visibility = :public } + + it { is_expected.to_not be_hidden } + end + + context 'when visibility is unlisted' do + before { subject.visibility = :unlisted } + + it { is_expected.to_not be_hidden } + end + end + + describe '#distributable?' do + subject { Status.new } + + context 'when visibility is public' do + before { subject.visibility = :public } + + it { is_expected.to be_distributable } + end + + context 'when visibility is unlisted' do + before { subject.visibility = :unlisted } + + it { is_expected.to be_distributable } + end + + context 'when visibility is private' do + before { subject.visibility = :private } + + it { is_expected.to_not be_distributable } + end + + context 'when visibility is direct' do + before { subject.visibility = :direct } + + it { is_expected.to_not be_distributable } + end + + context 'when visibility is limited' do + before { subject.visibility = :limited } + + it { is_expected.to_not be_distributable } + end + end +end diff --git a/spec/models/status_spec.rb b/spec/models/status_spec.rb index 36b13df815..71d1a04594 100644 --- a/spec/models/status_spec.rb +++ b/spec/models/status_spec.rb @@ -84,36 +84,6 @@ RSpec.describe Status do end end - describe '#hidden?' do - context 'when private_visibility?' do - it 'returns true' do - subject.visibility = :private - expect(subject.hidden?).to be true - end - end - - context 'when direct_visibility?' do - it 'returns true' do - subject.visibility = :direct - expect(subject.hidden?).to be true - end - end - - context 'when public_visibility?' do - it 'returns false' do - subject.visibility = :public - expect(subject.hidden?).to be false - end - end - - context 'when unlisted_visibility?' do - it 'returns false' do - subject.visibility = :unlisted - expect(subject.hidden?).to be false - end - end - end - describe '#content' do it 'returns the text of the status if it is not a reblog' do expect(subject.content).to eql subject.text