DRY up reblog vs original status check
Checking reblog vs original status was happening in multiple places across the app. For views, this logic was encapsulated in a helper method named `proper_status` but in the other layers of the app, the logic was duplicated. Because the logic is used at all layers of the app, we extracted it into a `Status#proper` method on the model and changed all uses of the logic to use this method. There is now a single source of truth for this condition. We added test coverage to untested methods that got refactored.
This commit is contained in:
		
							parent
							
								
									4e41cd9ab8
								
							
						
					
					
						commit
						d4c94fa004
					
				
					 7 changed files with 86 additions and 11 deletions
				
			
		| 
						 | 
				
			
			@ -34,10 +34,6 @@ module StreamEntriesHelper
 | 
			
		|||
    user_signed_in? && @favourited.key?(status.id) ? 'favourited' : ''
 | 
			
		||||
  end
 | 
			
		||||
 | 
			
		||||
  def proper_status(status)
 | 
			
		||||
    status.reblog? ? status.reblog : status
 | 
			
		||||
  end
 | 
			
		||||
 | 
			
		||||
  def rtl?(text)
 | 
			
		||||
    return false if text.empty?
 | 
			
		||||
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
| 
						 | 
				
			
			@ -328,7 +328,7 @@ class AtomSerializer
 | 
			
		|||
 | 
			
		||||
  def serialize_status_attributes(entry, status)
 | 
			
		||||
    append_element(entry, 'summary', status.spoiler_text) unless status.spoiler_text.blank?
 | 
			
		||||
    append_element(entry, 'content', Formatter.instance.format(status.reblog? ? status.reblog : status).to_str, type: 'html')
 | 
			
		||||
    append_element(entry, 'content', Formatter.instance.format(status.proper).to_str, type: 'html')
 | 
			
		||||
 | 
			
		||||
    status.mentions.each do |mentioned|
 | 
			
		||||
      append_element(entry, 'link', nil, rel: :mentioned, 'ostatus:object-type': TagManager::TYPES[:person], href: TagManager.instance.uri_for(mentioned.account))
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
| 
						 | 
				
			
			@ -125,11 +125,11 @@ class Account < ApplicationRecord
 | 
			
		|||
  end
 | 
			
		||||
 | 
			
		||||
  def favourited?(status)
 | 
			
		||||
    (status.reblog? ? status.reblog : status).favourites.where(account: self).count.positive?
 | 
			
		||||
    status.proper.favourites.where(account: self).count.positive?
 | 
			
		||||
  end
 | 
			
		||||
 | 
			
		||||
  def reblogged?(status)
 | 
			
		||||
    (status.reblog? ? status.reblog : status).reblogs.where(account: self).count.positive?
 | 
			
		||||
    status.proper.reblogs.where(account: self).count.positive?
 | 
			
		||||
  end
 | 
			
		||||
 | 
			
		||||
  def keypair
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
| 
						 | 
				
			
			@ -62,8 +62,12 @@ class Status < ApplicationRecord
 | 
			
		|||
    reply? ? :comment : :note
 | 
			
		||||
  end
 | 
			
		||||
 | 
			
		||||
  def proper
 | 
			
		||||
    reblog? ? reblog : self
 | 
			
		||||
  end
 | 
			
		||||
 | 
			
		||||
  def content
 | 
			
		||||
    reblog? ? reblog.text : text
 | 
			
		||||
    proper.text
 | 
			
		||||
  end
 | 
			
		||||
 | 
			
		||||
  def target
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
| 
						 | 
				
			
			@ -16,7 +16,7 @@
 | 
			
		|||
          %strong= display_name(status.account)
 | 
			
		||||
        = t('stream_entries.reblogged')
 | 
			
		||||
 | 
			
		||||
  = render partial: centered ? 'stream_entries/detailed_status' : 'stream_entries/simple_status', locals: { status: proper_status(status) }
 | 
			
		||||
  = render partial: centered ? 'stream_entries/detailed_status' : 'stream_entries/simple_status', locals: { status: status.proper }
 | 
			
		||||
 | 
			
		||||
- if include_threads
 | 
			
		||||
  = render partial: 'stream_entries/status', collection: @descendants, as: :status, locals: { is_successor: true }
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
| 
						 | 
				
			
			@ -99,11 +99,75 @@ RSpec.describe Account, type: :model do
 | 
			
		|||
  end
 | 
			
		||||
 | 
			
		||||
  describe '#favourited?' do
 | 
			
		||||
    pending
 | 
			
		||||
    let(:original_status) do
 | 
			
		||||
      author = Fabricate(:account, username: 'original')
 | 
			
		||||
      Fabricate(:status, account: author)
 | 
			
		||||
    end
 | 
			
		||||
 | 
			
		||||
    context 'when the status is a reblog of another status' do
 | 
			
		||||
      let(:original_reblog) do
 | 
			
		||||
        author = Fabricate(:account, username: 'original_reblogger')
 | 
			
		||||
        Fabricate(:status, reblog: original_status, account: author)
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'is is true when this account has favourited it' do
 | 
			
		||||
        Fabricate(:favourite, status: original_reblog, account: subject)
 | 
			
		||||
 | 
			
		||||
        expect(subject.favourited?(original_status)).to eq true
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'is false when this account has not favourited it' do
 | 
			
		||||
        expect(subject.favourited?(original_status)).to eq false
 | 
			
		||||
      end
 | 
			
		||||
    end
 | 
			
		||||
 | 
			
		||||
    context 'when the status is an original status' do
 | 
			
		||||
      it 'is is true when this account has favourited it' do
 | 
			
		||||
        Fabricate(:favourite, status: original_status, account: subject)
 | 
			
		||||
 | 
			
		||||
        expect(subject.favourited?(original_status)).to eq true
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'is false when this account has not favourited it' do
 | 
			
		||||
        expect(subject.favourited?(original_status)).to eq false
 | 
			
		||||
      end
 | 
			
		||||
    end
 | 
			
		||||
  end
 | 
			
		||||
 | 
			
		||||
  describe '#reblogged?' do
 | 
			
		||||
    pending
 | 
			
		||||
    let(:original_status) do
 | 
			
		||||
      author = Fabricate(:account, username: 'original')
 | 
			
		||||
      Fabricate(:status, account: author)
 | 
			
		||||
    end
 | 
			
		||||
 | 
			
		||||
    context 'when the status is a reblog of another status'do
 | 
			
		||||
      let(:original_reblog) do
 | 
			
		||||
        author = Fabricate(:account, username: 'original_reblogger')
 | 
			
		||||
        Fabricate(:status, reblog: original_status, account: author)
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'is true when this account has reblogged it' do
 | 
			
		||||
        Fabricate(:status, reblog: original_reblog, account: subject)
 | 
			
		||||
 | 
			
		||||
        expect(subject.reblogged?(original_reblog)).to eq true
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'is false when this account has not reblogged it' do
 | 
			
		||||
        expect(subject.reblogged?(original_reblog)).to eq false
 | 
			
		||||
      end
 | 
			
		||||
    end
 | 
			
		||||
 | 
			
		||||
    context 'when the status is an original status' do
 | 
			
		||||
      it 'is true when this account has reblogged it' do
 | 
			
		||||
        Fabricate(:status, reblog: original_status, account: subject)
 | 
			
		||||
 | 
			
		||||
        expect(subject.reblogged?(original_status)).to eq true
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'is false when this account has not reblogged it' do
 | 
			
		||||
        expect(subject.reblogged?(original_status)).to eq false
 | 
			
		||||
      end
 | 
			
		||||
    end
 | 
			
		||||
  end
 | 
			
		||||
 | 
			
		||||
  describe '.find_local' do
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
| 
						 | 
				
			
			@ -97,4 +97,15 @@ RSpec.describe Status, type: :model do
 | 
			
		|||
  describe '#favourites_count' do
 | 
			
		||||
    pending
 | 
			
		||||
  end
 | 
			
		||||
 | 
			
		||||
  describe '#proper' do
 | 
			
		||||
    it 'is itself for original statuses' do
 | 
			
		||||
      expect(subject.proper).to eq subject
 | 
			
		||||
    end
 | 
			
		||||
 | 
			
		||||
    it 'is the source status for reblogs' do
 | 
			
		||||
      subject.reblog = other
 | 
			
		||||
      expect(subject.proper).to eq other
 | 
			
		||||
    end
 | 
			
		||||
  end
 | 
			
		||||
end
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
		Loading…
	
	Add table
		
		Reference in a new issue