From ee36382640560191a8a43e87b607ef00051e019b Mon Sep 17 00:00:00 2001
From: Sam Saffron <sam.saffron@gmail.com>
Date: Mon, 20 Apr 2020 16:08:24 +1000
Subject: [PATCH] FEATURE: improve rendering of RSS feeds

- Eliminate superfluous "author wrote" block
- Eliminate block-quote for all posts
- Move participant count and reply count to 1 line
- Prioritize name over username if forum requests
- Use fabrication in list controller spec to speed up spec
---
 app/helpers/application_helper.rb     | 10 ++++++++++
 app/views/list/list.rss.erb           | 18 +++++-------------
 app/views/topics/show.rss.erb         | 15 ++++++---------
 config/locales/server.en.yml          | 10 +++++++---
 spec/requests/list_controller_spec.rb | 27 ++++++++++++++-------------
 5 files changed, 42 insertions(+), 38 deletions(-)

diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index e9e68829ec2..598d37a9548 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -513,4 +513,14 @@ module ApplicationHelper
     !SiteSetting.invite_only &&
     !SiteSetting.enable_sso
   end
+
+  def rss_creator(user)
+    if user
+      if SiteSetting.prioritize_username_in_ux
+        "#{user.username}"
+      else
+        "#{user.name.presence || user.username }"
+      end
+    end
+  end
 end
diff --git a/app/views/list/list.rss.erb b/app/views/list/list.rss.erb
index 0895d208dab..040ece9046b 100644
--- a/app/views/list/list.rss.erb
+++ b/app/views/list/list.rss.erb
@@ -12,23 +12,15 @@
       <atom:link href="<%= @atom_link %>" rel="self" type="application/rss+xml" />
       <% @topic_list.topics.each do |topic| %>
         <% topic_url = topic.url -%>
-        <% username = topic.user ? topic.user.username : "" %>
-        <% name = topic.user ? topic.user.name : "" %>
         <item>
           <title><%= topic.title %></title>
-          <dc:creator><![CDATA[<%= "@#{username}#{" #{name}" if (name.present? && SiteSetting.enable_names?)}" -%>]]></dc:creator>
+          <dc:creator><![CDATA[<%= rss_creator(topic.user) -%>]]></dc:creator>
           <category><%= topic.category.name %></category>
           <description><![CDATA[
-            <% if username.present? %>
-            <p><%= t('author_wrote', author: link_to("@#{username}", "#{Discourse.base_url}/u/#{url_encode(topic.user.username_lower)}")).html_safe %></p>
-            <% end %>
-            <blockquote>
-              <%- if first_post = topic.ordered_posts.first %>
-              <%= first_post.cooked.html_safe %>
-              <%- end %>
-            </blockquote>
-            <p><%= t 'num_posts' %> <%= topic.posts_count %></p>
-            <p><%= t 'num_participants' %> <%= topic.participant_count %></p>
+            <%- if first_post = topic.ordered_posts.first %>
+            <%= first_post.cooked.html_safe %>
+            <%- end %>
+            <p><small><%= t 'rss_num_posts', count: topic.posts_count %> - <%= t 'rss_num_participants', count: topic.participant_count %></small></p>
             <p><%= link_to t('read_full_topic'), topic_url %></p>
           ]]></description>
           <link><%= topic_url %></link>
diff --git a/app/views/topics/show.rss.erb b/app/views/topics/show.rss.erb
index 2a71f1f1635..7da14703cd8 100644
--- a/app/views/topics/show.rss.erb
+++ b/app/views/topics/show.rss.erb
@@ -15,17 +15,14 @@
       <% next unless post.user %>
       <item>
         <title><%= @topic_view.title %></title>
-        <dc:creator><![CDATA[<%= "@#{post.user.username}#{" #{post.user.name}" if (post.user.name.present? && SiteSetting.enable_names?)}" -%>]]></dc:creator>
+        <dc:creator><![CDATA[<%= rss_creator(post.user) -%>]]></dc:creator>
         <description><![CDATA[
           <% post_url = Discourse.base_url + post.url %>
-          <p><%= t('author_wrote', author: link_to("@#{post.user.username}", "#{Discourse.base_url}/u/#{url_encode(post.user.username_lower)}")).html_safe %></p>
-          <blockquote>
-            <% if post.hidden %>
-              <%= t('flagging.user_must_edit').html_safe %>
-            <% else %>
-              <%= post.cooked.html_safe %>
-            <% end %>
-          </blockquote>
+          <% if post.hidden %>
+            <%= t('flagging.user_must_edit').html_safe %>
+          <% else %>
+            <%= post.cooked.html_safe %>
+          <% end %>
           <p><%= link_to t('read_full_topic'), post_url %></p>
         ]]></description>
         <link><%= post_url %></link>
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 51daf795cf5..2e73efe5522 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -352,9 +352,13 @@ en:
   topics_in_category: "Topics in the '%{category}' category"
   rss_posts_in_topic: "RSS feed of '%{topic}'"
   rss_topics_in_category: "RSS feed of topics in the '%{category}' category"
-  author_wrote: "%{author} wrote:"
-  num_posts: "Posts:"
-  num_participants: "Participants:"
+  rss_num_posts:
+    one: "%{count} post"
+    other: "%{count} posts"
+  rss_num_participants:
+    one: "%{count} participant"
+    other: "%{count} participants"
+
   read_full_topic: "Read full topic"
   private_message_abbrev: "Msg"
 
diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb
index 9702231805f..83a46f33739 100644
--- a/spec/requests/list_controller_spec.rb
+++ b/spec/requests/list_controller_spec.rb
@@ -3,10 +3,10 @@
 require 'rails_helper'
 
 RSpec.describe ListController do
-  let(:topic) { Fabricate(:topic, user: user) }
-  let(:group) { Fabricate(:group) }
-  let(:user) { Fabricate(:user) }
-  let(:admin) { Fabricate(:admin) }
+  fab!(:user) { Fabricate(:user) }
+  fab!(:topic) { Fabricate(:topic, user: user) }
+  fab!(:group) { Fabricate(:group) }
+  fab!(:admin) { Fabricate(:admin) }
 
   before do
     admin  # to skip welcome wizard at home page `/`
@@ -73,12 +73,12 @@ RSpec.describe ListController do
     end
 
     it "shows correct title if topic list is set for homepage" do
-      get "/"
+      get "/latest"
 
       expect(response.body).to have_tag "title", text: "Discourse"
 
       SiteSetting.short_site_description = "Best community"
-      get "/"
+      get "/latest"
 
       expect(response.body).to have_tag "title", text: "Discourse - Best community"
     end
@@ -95,7 +95,7 @@ RSpec.describe ListController do
 
       get "/categories_and_latest.json"
       data = JSON.parse(response.body)
-      expect(data["topic_list"]["topics"].length).to eq(1)
+      expect(data["topic_list"]["topics"].length).to eq(2)
     end
   end
 
@@ -305,12 +305,11 @@ RSpec.describe ListController do
 
     it 'renders links correctly with subfolder' do
       set_subfolder "/forum"
-      post = Fabricate(:post, topic: topic, user: user)
+      _post = Fabricate(:post, topic: topic, user: user)
       get "/latest.rss"
       expect(response.status).to eq(200)
       expect(response.body).to_not include("/forum/forum")
       expect(response.body).to include("http://test.localhost/forum/t/#{topic.slug}")
-      expect(response.body).to include("http://test.localhost/forum/u/#{post.user.username}")
     end
 
     it 'renders top RSS' do
@@ -498,16 +497,18 @@ RSpec.describe ListController do
   end
 
   describe "topics_by" do
+    fab!(:topic2) { Fabricate(:topic, user: user) }
+    fab!(:user2) { Fabricate(:user) }
+
     before do
-      sign_in(Fabricate(:user))
-      Fabricate(:topic, user: user)
+      sign_in(user2)
     end
 
     it "should respond with a list" do
       get "/topics/created-by/#{user.username}.json"
       expect(response.status).to eq(200)
       json = JSON.parse(response.body)
-      expect(json["topic_list"]["topics"].size).to eq(1)
+      expect(json["topic_list"]["topics"].size).to eq(2)
     end
 
     it "should work with period in username" do
@@ -515,7 +516,7 @@ RSpec.describe ListController do
       get "/topics/created-by/#{user.username}", xhr: true
       expect(response.status).to eq(200)
       json = JSON.parse(response.body)
-      expect(json["topic_list"]["topics"].size).to eq(1)
+      expect(json["topic_list"]["topics"].size).to eq(2)
     end
   end