From 6d6a429af8fe4bd92ed497f401676353fdc603e0 Mon Sep 17 00:00:00 2001 From: Eugen Date: Fri, 7 Apr 2017 05:56:56 +0200 Subject: Rewrite Atom generation from stream entries to use Ox instead of Nokogiri (#1124) * Rewrite Atom generation from stream entries to use Ox instead of Nokogiri::Builder StreamEntry is now limited to only statuses, which allows some optimization. Removed extra queries on AccountsController#show. AtomSerializer instead of AtomBuilderHelper used in AccountsController#show, StreamEntriesController#show, StreamEntryRenderer and PubSubHubbub::DistributionWorker PubSubHubbub::DistributionWorker moves n+1 DomainBlock query to PubSubHubbub::DeliveryWorker instead. All Salmon slaps that aren't based on StreamEntry still use AtomBuilderHelper and Nokogiri * All Salmon slaps now use Ox instead of Nokogiri. No touch from status on account --- app/models/stream_entry.rb | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) (limited to 'app/models') diff --git a/app/models/stream_entry.rb b/app/models/stream_entry.rb index ae7ae446e..8aff5ae06 100644 --- a/app/models/stream_entry.rb +++ b/app/models/stream_entry.rb @@ -5,25 +5,21 @@ class StreamEntry < ApplicationRecord belongs_to :account, inverse_of: :stream_entries belongs_to :activity, polymorphic: true - belongs_to :status, foreign_type: 'Status', foreign_key: 'activity_id', inverse_of: :stream_entry validates :account, :activity, presence: true - STATUS_INCLUDES = [:account, :stream_entry, :media_attachments, :tags, mentions: :account, reblog: [:stream_entry, :account, mentions: :account], thread: [:stream_entry, :account]].freeze + STATUS_INCLUDES = [:account, :stream_entry, :media_attachments, :tags, mentions: :account, reblog: [:stream_entry, :account, :media_attachments, :tags, mentions: :account], thread: [:stream_entry, :account]].freeze + default_scope { where(activity_type: 'Status') } scope :with_includes, -> { includes(:account, status: STATUS_INCLUDES) } def object_type - if orphaned? - :activity - else - targeted? ? :activity : activity.object_type - end + orphaned? || targeted? ? :activity : status.object_type end def verb - orphaned? ? :delete : activity.verb + orphaned? ? :delete : status.verb end def targeted? @@ -31,15 +27,15 @@ class StreamEntry < ApplicationRecord end def target - orphaned? ? nil : activity.target + orphaned? ? nil : status.target end def title - orphaned? ? nil : activity.title + orphaned? ? nil : status.title end def content - orphaned? ? nil : activity.content + orphaned? ? nil : status.content end def threaded? @@ -47,20 +43,16 @@ class StreamEntry < ApplicationRecord end def thread - orphaned? ? nil : activity.thread + orphaned? ? nil : status.thread end def mentions - activity.respond_to?(:mentions) ? activity.mentions.map(&:account) : [] - end - - def activity - !new_record? ? send(activity_type.underscore) || super : super + orphaned? ? [] : status.mentions.map(&:account) end private def orphaned? - activity.nil? + status.nil? end end -- cgit From d4c94fa004117fdb7226b1b846a12d12dc0542d9 Mon Sep 17 00:00:00 2001 From: Joël Quenneville Date: Fri, 7 Apr 2017 14:18:30 -0400 Subject: 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. --- app/helpers/stream_entries_helper.rb | 4 -- app/lib/atom_serializer.rb | 2 +- app/models/account.rb | 4 +- app/models/status.rb | 6 ++- app/views/stream_entries/_status.html.haml | 2 +- spec/models/account_spec.rb | 68 +++++++++++++++++++++++++++++- spec/models/status_spec.rb | 11 +++++ 7 files changed, 86 insertions(+), 11 deletions(-) (limited to 'app/models') diff --git a/app/helpers/stream_entries_helper.rb b/app/helpers/stream_entries_helper.rb index a26e912a3..38e63ed8d 100644 --- a/app/helpers/stream_entries_helper.rb +++ b/app/helpers/stream_entries_helper.rb @@ -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? diff --git a/app/lib/atom_serializer.rb b/app/lib/atom_serializer.rb index be1cced8b..b9dcee6b3 100644 --- a/app/lib/atom_serializer.rb +++ b/app/lib/atom_serializer.rb @@ -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)) diff --git a/app/models/account.rb b/app/models/account.rb index 6968607a2..cbba8b5b6 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -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 diff --git a/app/models/status.rb b/app/models/status.rb index 6948ad77c..7e3dd3e28 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -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 diff --git a/app/views/stream_entries/_status.html.haml b/app/views/stream_entries/_status.html.haml index cdd0dde3b..434c5c8da 100644 --- a/app/views/stream_entries/_status.html.haml +++ b/app/views/stream_entries/_status.html.haml @@ -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 } diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index d7f59adb8..93a45459d 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -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 diff --git a/spec/models/status_spec.rb b/spec/models/status_spec.rb index b9d079521..db244ebe7 100644 --- a/spec/models/status_spec.rb +++ b/spec/models/status_spec.rb @@ -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 -- cgit