From a4240fd0272eb79b7d99cccfa7d14e8a1e12921d Mon Sep 17 00:00:00 2001 From: ThibG Date: Sun, 10 May 2020 09:50:54 +0200 Subject: [PATCH] Improve RSS entries for statuses (#13592) * Improve RSS entries for statuses - Render polls in both accounts and tags serializers - Refactor RSS serializers - Change title preview to include ellipsis when truncated - Change title preview to show CW instead of toot text - Add tests * Remove title from OEmbed serialization Twitter doesn't serialize title either, and tihs allows us to move the title formatting code to the RSS serializers. --- app/lib/rss/serializer.rb | 38 ++++++++++++++ app/models/status.rb | 8 --- app/serializers/oembed_serializer.rb | 2 +- app/serializers/rss/account_serializer.rb | 15 +----- app/serializers/rss/tag_serializer.rb | 15 +----- spec/lib/rss/serializer_spec.rb | 63 +++++++++++++++++++++++ spec/models/status_spec.rb | 29 ----------- 7 files changed, 106 insertions(+), 64 deletions(-) create mode 100644 app/lib/rss/serializer.rb create mode 100644 spec/lib/rss/serializer_spec.rb diff --git a/app/lib/rss/serializer.rb b/app/lib/rss/serializer.rb new file mode 100644 index 000000000..fd56c568c --- /dev/null +++ b/app/lib/rss/serializer.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +class RSS::Serializer + private + + def render_statuses(builder, statuses) + statuses.each do |status| + builder.item do |item| + item.title(status_title(status)) + .link(ActivityPub::TagManager.instance.url_for(status)) + .pub_date(status.created_at) + .description(status.spoiler_text.presence || Formatter.instance.format(status, inline_poll_options: true).to_str) + + status.media_attachments.each do |media| + item.enclosure(full_asset_url(media.file.url(:original, false)), media.file.content_type, media.file.size) + end + end + end + end + + def status_title(status) + return "#{status.account.acct} deleted status" if status.destroyed? + + preview = status.proper.spoiler_text.presence || status.proper.text + if preview.length > 30 || preview[0, 30].include?("\n") + preview = preview[0, 30] + preview = preview[0, preview.index("\n").presence || 30] + '…' + end + + preview = "#{status.proper.spoiler_text.present? ? 'CW ' : ''}“#{preview}”#{status.proper.sensitive? ? ' (sensitive)' : ''}" + + if status.reblog? + "#{status.account.acct} boosted #{status.reblog.account.acct}: #{preview}" + else + "#{status.account.acct}: #{preview}" + end + end +end diff --git a/app/models/status.rb b/app/models/status.rb index a938ff032..8c7fe8dfa 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -197,14 +197,6 @@ class Status < ApplicationRecord preview_cards.first end - def title - if destroyed? - "#{account.acct} deleted status" - else - reblog? ? "#{account.acct} shared a status by #{reblog.account.acct}" : "New status by #{account.acct}" - end - end - def hidden? !distributable? end diff --git a/app/serializers/oembed_serializer.rb b/app/serializers/oembed_serializer.rb index 01689633b..d6261d724 100644 --- a/app/serializers/oembed_serializer.rb +++ b/app/serializers/oembed_serializer.rb @@ -4,7 +4,7 @@ class OEmbedSerializer < ActiveModel::Serializer include RoutingHelper include ActionView::Helpers::TagHelper - attributes :type, :version, :title, :author_name, + attributes :type, :version, :author_name, :author_url, :provider_name, :provider_url, :cache_age, :html, :width, :height diff --git a/app/serializers/rss/account_serializer.rb b/app/serializers/rss/account_serializer.rb index ee972ff96..81e24af0d 100644 --- a/app/serializers/rss/account_serializer.rb +++ b/app/serializers/rss/account_serializer.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class RSS::AccountSerializer +class RSS::AccountSerializer < RSS::Serializer include ActionView::Helpers::NumberHelper include AccountsHelper include RoutingHelper @@ -17,18 +17,7 @@ class RSS::AccountSerializer builder.image(full_asset_url(account.avatar.url(:original))) if account.avatar? builder.cover(full_asset_url(account.header.url(:original))) if account.header? - statuses.each do |status| - builder.item do |item| - item.title(status.title) - .link(ActivityPub::TagManager.instance.url_for(status)) - .pub_date(status.created_at) - .description(status.spoiler_text.presence || Formatter.instance.format(status, inline_poll_options: true).to_str) - - status.media_attachments.each do |media| - item.enclosure(full_asset_url(media.file.url(:original, false)), media.file.content_type, media.file.size) - end - end - end + render_statuses(builder, statuses) builder.to_xml end diff --git a/app/serializers/rss/tag_serializer.rb b/app/serializers/rss/tag_serializer.rb index ea26189a2..e549ac367 100644 --- a/app/serializers/rss/tag_serializer.rb +++ b/app/serializers/rss/tag_serializer.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class RSS::TagSerializer +class RSS::TagSerializer < RSS::Serializer include ActionView::Helpers::NumberHelper include ActionView::Helpers::SanitizeHelper include RoutingHelper @@ -14,18 +14,7 @@ class RSS::TagSerializer .logo(full_pack_url('media/images/logo.svg')) .accent_color('2b90d9') - statuses.each do |status| - builder.item do |item| - item.title(status.title) - .link(ActivityPub::TagManager.instance.url_for(status)) - .pub_date(status.created_at) - .description(status.spoiler_text.presence || Formatter.instance.format(status).to_str) - - status.media_attachments.each do |media| - item.enclosure(full_asset_url(media.file.url(:original, false)), media.file.content_type, media.file.size) - end - end - end + render_statuses(builder, statuses) builder.to_xml end diff --git a/spec/lib/rss/serializer_spec.rb b/spec/lib/rss/serializer_spec.rb new file mode 100644 index 000000000..0364d13de --- /dev/null +++ b/spec/lib/rss/serializer_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe RSS::Serializer do + describe '#status_title' do + let(:text) { 'This is a toot' } + let(:spoiler) { '' } + let(:sensitive) { false } + let(:reblog) { nil } + let(:account) { Fabricate(:account) } + let(:status) { Fabricate(:status, account: account, text: text, spoiler_text: spoiler, sensitive: sensitive, reblog: reblog) } + + subject { RSS::Serializer.new.send(:status_title, status) } + + context 'if destroyed?' do + it 'returns "#{account.acct} deleted status"' do + status.destroy! + expect(subject).to eq "#{account.acct} deleted status" + end + end + + context 'on a toot with long text' do + let(:text) { "This toot's text is longer than the allowed number of characters" } + + it 'truncates toot text appropriately' do + expect(subject).to eq "#{account.acct}: “This toot's text is longer tha…”" + end + end + + context 'on a toot with long text with a newline' do + let(:text) { "This toot's text is longer\nthan the allowed number of characters" } + + it 'truncates toot text appropriately' do + expect(subject).to eq "#{account.acct}: “This toot's text is longer…”" + end + end + + context 'on a toot with a content warning' do + let(:spoiler) { 'long toot' } + + it 'displays spoiler text instead of toot content' do + expect(subject).to eq "#{account.acct}: CW “long toot”" + end + end + + context 'on a toot with sensitive media' do + let(:sensitive) { true } + + it 'displays that the media is sensitive' do + expect(subject).to eq "#{account.acct}: “This is a toot” (sensitive)" + end + end + + context 'on a reblog' do + let(:reblog) { Fabricate(:status, text: 'This is a toot') } + + it 'display that the toot is a reblog' do + expect(subject).to eq "#{account.acct} boosted #{reblog.account.acct}: “This is a toot”" + end + end + end +end diff --git a/spec/models/status_spec.rb b/spec/models/status_spec.rb index 51a10cd17..9c1eca1b5 100644 --- a/spec/models/status_spec.rb +++ b/spec/models/status_spec.rb @@ -82,35 +82,6 @@ RSpec.describe Status, type: :model do end end - describe '#title' do - # rubocop:disable Style/InterpolationCheck - - let(:account) { subject.account } - - context 'if destroyed?' do - it 'returns "#{account.acct} deleted status"' do - subject.destroy! - expect(subject.title).to eq "#{account.acct} deleted status" - end - end - - context 'unless destroyed?' do - context 'if reblog?' do - it 'returns "#{account.acct} shared a status by #{reblog.account.acct}"' do - reblog = subject.reblog = other - expect(subject.title).to eq "#{account.acct} shared a status by #{reblog.account.acct}" - end - end - - context 'unless reblog?' do - it 'returns "New status by #{account.acct}"' do - subject.reblog = nil - expect(subject.title).to eq "New status by #{account.acct}" - end - end - end - end - describe '#hidden?' do context 'if private_visibility?' do it 'returns true' do