From 10ae969139c67c9b350365b993e2f893659a37c6 Mon Sep 17 00:00:00 2001 From: benjamin wil Date: Sat, 16 Jul 2022 21:41:49 -0700 Subject: [PATCH] Refactor RSS view renderer In issue #1815, it was reported that Gollum RSS feeds attempt to put entire commit messages into a feed item's `` element. Given that commit messages can be many paragraphs long, this is not an acceptable way to render a feed item title. If a commit has many lines or paragraphs, we now put those inside the `<description>` element as of this commit. While I was editing this view, I decided it would be a good time to increase test coverage, as this view renderer was not under test at all. I also fixed a typo ("Commited" should say "Committed"). There are implicit dependencies on other gems that provide the `Gollum::Git::Commit`, `Gollum::Git::Actor`, and `Rugged::Commit` classes here, so mocking out simple versions of their interfaces seemed like the path of least resistance to setting up controllable tests. I also removed some now-duplicated assertions from `test/test_app.rb`. --- lib/gollum/views/rss.rb | 79 +++++++++++++++++++++++-------- test/test_app.rb | 28 ++++------- test/test_rss_view.rb | 101 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 37 deletions(-) create mode 100644 test/test_rss_view.rb diff --git a/lib/gollum/views/rss.rb b/lib/gollum/views/rss.rb index 73ad9d2c..293a5efa 100644 --- a/lib/gollum/views/rss.rb +++ b/lib/gollum/views/rss.rb @@ -1,43 +1,84 @@ require 'rss' class RSSView - include Precious::Views::AppHelpers include Precious::Views::RouteHelpers - + attr_reader :base_url - + def initialize(base_url, wiki_title, url, changes) @base_url = base_url @wiki_title = wiki_title @url = url @changes = changes end - + def render - latest_changes = "#{@url}#{latest_changes_path}" RSS::Maker.make('2.0') do |maker| maker.channel.author = 'Gollum Wiki' - maker.channel.updated = @changes.first.authored_date - maker.channel.title = "#{@wiki_title} Latest Changes" maker.channel.description = "Latest Changes in #{@wiki_title}" - maker.channel.link = latest_changes + maker.channel.link = latest_changes_url + maker.channel.title = "#{@wiki_title} Latest Changes" + maker.channel.updated = @changes.first.authored_date + @changes.each do |change| maker.items.new_item do |item| - item.link = latest_changes - item.title = change.message + item.description = feed_item_description(change) + item.link = latest_changes_url + item.title = feed_item_title(change) item.updated = change.authored_date - id = change.id - files = change.stats.files.map do |files| - [files[:old_file], files[:new_file]].compact.map do |file| - f = extract_page_dir(file) - "<li><a href=\"#{@url}#{page_route(f)}/#{id}\">#{f}</a></li>" - end - end - item.description = "Commited by: <a href=\"mailto:#{change.author.email}\">#{change.author.name}</a><br/>Commit ID: #{id[0..6]}<br/><br/>Affected files:<ul>#{files.join}</ul>" end end end.to_s end -end \ No newline at end of file + private + + def feed_item_commit_body(change) + body = change.message.lines[1..-1].join + body = body.split(/\n{2}/).map { |paragraph| "<p>#{paragraph}</p>" }.join + body.gsub!(/\n/, ' ') + body + end + + def feed_item_description(change) + ERB.new(<<~HTML_PARTIAL) + <%= feed_item_commit_body(change) %> + Committed by: <a href="mailto:<%= change.author.email %>"> + <%= change.author.name %> + </a><br /> + Commit ID: <%= change.id[0..6] %><br /><br /> + <%= feed_item_files(change) %> + HTML_PARTIAL + .result(binding) + end + + def feed_item_files(change) + file_list = change.stats.files.map { |change_files| + [ + change_files[:old_file], + change_files[:new_file] + ].compact + } + + ERB.new(<<~HTML_PARTIAL) + Affected files: <ul> + <% file_list.each do |change_files| %> + <% change_files.each do |file| %> + <% file_href = "%s%s/%s" % [@url, page_route(file), change.id] %> + <li><a href="<%= file_href %>"><%= file %></a></li> + <% end %> + <% end %> + </ul> + HTML_PARTIAL + .result(binding) + end + + def feed_item_title(change) + change.message.lines.first.strip + end + + def latest_changes_url + "%s%s" % [@url, latest_changes_path] + end +end diff --git a/test/test_app.rb b/test/test_app.rb index ec0670bb..a0808791 100644 --- a/test/test_app.rb +++ b/test/test_app.rb @@ -63,18 +63,10 @@ context "Frontend" do end test 'rss feed' do - channel_title = <<EOF -<title>Gollum Wiki Latest Changes -EOF - item = <Commited by: <a href="mailto:dawa.ometto@phil.uu.nl">Dawa Ometto</a><br/>Commit ID: 02796b1<br/><br/>Affected files:<ul><li><a href="http://example.org/custom.css/02796b1450691f90db5d6dc6a816a4980ce80d07">custom.css</a></li><li><a href="http://example.org/custom.js/02796b1450691f90db5d6dc6a816a4980ce80d07">custom.js</a></li></ul> -EOF get '/gollum/feed/' + assert last_response.ok? assert_equal 'application/rss+xml', last_response.headers['Content-Type'] - assert last_response.body.start_with?('", feed + assert_match //m, feed + assert_match /(.*)<\/channel>/m, feed + + # Assert that we have feed metadata. + # + assert_match "Wiki Name Latest Changes", feed + assert_match "https://example.com/gollum/latest_changes", feed + assert_match "Latest Changes in Wiki Name", feed + assert_match /(.*)<\/pubDate>/, feed + + # Assert that we have an item in our feed. + # + assert_match /(.*)<\/item>/m, feed + + # And it has a title. + # + assert_match "Multi-line commit message", feed + + # Assert that the description contains expected content. + # + assert_match /(.*)<\/description>/m, feed + assert_match /<p> This commit(.*)<\/p>/, feed + assert_match /<p>Git's documentation(.*)<\/p>/, feed + + # Assert that the description contains information about the commit. + # i.e.: + # + # committer name + # + # Commit ID: f0f0f0f0 + # + assert_match /Committed by: /, feed + assert_match /\<a href=\"mailto:email@example.com\"\>/, feed + assert_match /\>\n committer name\n\<\/a>/, feed + assert_match "Commit ID: f0f0f0f", feed + + # Assert that affected files include links to commits, i.e.: + # + # new + # + assert_match /Affected files: /, feed + assert_match /\<a href=\"https:\/\/example.com\/old\/f0f0f0f0\"/, + feed + assert_match /f0f0f0f0">new<\/a>/, feed + end +end