From e5e36a2e2dc25481cbc6433c9ffde9bcbe20fe80 Mon Sep 17 00:00:00 2001
From: Fernando Valverde <fernando@fdo.cr>
Date: Mon, 3 Oct 2022 07:58:34 -0600
Subject: [PATCH] Comment fragment link when signed out (#18517)

* Comment fragment link when signed out

* Optional chaining for article path

* Fix comment liquid tag rendering

* Extract signed in/out logic into helper

* Debugging specs

* Fix specs

* Use named empty anchor to link comments with id_code
---
 app/helpers/comments_helper.rb                | 11 +++++++++++
 app/lib/url.rb                                | 12 ++++++++++++
 .../articles/_full_comment_area.html.erb      |  2 +-
 app/views/comments/_comment.html.erb          |  1 +
 app/views/comments/_comment_date.erb          |  2 +-
 app/views/comments/_comment_header.html.erb   |  2 +-
 .../user_views_article_comments_spec.rb       | 19 ++++++++++++++++++-
 7 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/app/helpers/comments_helper.rb b/app/helpers/comments_helper.rb
index 4da608367..9b802c13b 100644
--- a/app/helpers/comments_helper.rb
+++ b/app/helpers/comments_helper.rb
@@ -64,6 +64,17 @@ module CommentsHelper
     end
   end
 
+  def contextual_comment_url(comment, article: nil)
+    # Liquid tag parsing doesn't have Devise/Warden (request middleware)
+    return URL.comment(comment) if request.env["warden"].nil?
+
+    # Logged in users should get the comment permalink
+    return URL.comment(comment) if user_signed_in?
+
+    # Logged out users should get the article URL with the comment anchor
+    URL.fragment_comment(comment, path: article&.path)
+  end
+
   private
 
   def nested_comments(tree:, commentable:, is_view_root: false)
diff --git a/app/lib/url.rb b/app/lib/url.rb
index 90896e890..28f382a95 100644
--- a/app/lib/url.rb
+++ b/app/lib/url.rb
@@ -45,6 +45,18 @@ module URL
     url(comment.path)
   end
 
+  # Creates a fragment URL for a comment on an article page
+  # if an article path is available
+  #
+  # @param comment [Comment] the comment to create the URL for
+  # @param path [String, nil] the path of the article to anchor the
+  #   comment link instead of using the comment's permalink
+  def self.fragment_comment(comment, path:)
+    return comment(comment) if path.nil?
+
+    url("#{path}#comment-#{comment.id_code}")
+  end
+
   # Creates a reaction URL
   #
   # A reaction URL is the URL of its reactable.
diff --git a/app/views/articles/_full_comment_area.html.erb b/app/views/articles/_full_comment_area.html.erb
index 61b139ca2..a87976789 100644
--- a/app/views/articles/_full_comment_area.html.erb
+++ b/app/views/articles/_full_comment_area.html.erb
@@ -1,4 +1,4 @@
-<% cache("whole-comment-area-#{@article.id}-#{@article.last_comment_at}-#{@article.show_comments}-#{@discussion_lock&.updated_at}-#{@comments_order}", expires_in: 2.hours) do %>
+<% cache("whole-comment-area-#{@article.id}-#{@article.last_comment_at}-#{@article.show_comments}-#{@discussion_lock&.updated_at}-#{@comments_order}-#{user_signed_in?}", expires_in: 2.hours) do %>
   <section id="comments" data-follow-button-container="true" data-updated-at="<%= Time.current %>" class="text-padding mb-4 border-t-1 border-0 border-solid border-base-10">
     <% if @article.show_comments %>
       <header class="relative flex justify-between items-center mb-6">
diff --git a/app/views/comments/_comment.html.erb b/app/views/comments/_comment.html.erb
index 4f4593be2..1d4261c35 100644
--- a/app/views/comments/_comment.html.erb
+++ b/app/views/comments/_comment.html.erb
@@ -24,6 +24,7 @@
     data-path="<%= commentable&.path %>/comments/<%= comment.id_code_generated %>"
     data-comment-author-id="<%= comment_user_id_unless_deleted comment %>"
     data-content-user-id="<%= comment_user_id_unless_deleted comment %>">
+    <a name="comment-<%= comment.id_code %>" style="position: absolute; top: -8px;">&nbsp;</a>
     <%= render("comments/comment_proper",
                comment: comment,
                commentable: commentable,
diff --git a/app/views/comments/_comment_date.erb b/app/views/comments/_comment_date.erb
index cb7159af6..729de1100 100644
--- a/app/views/comments/_comment_date.erb
+++ b/app/views/comments/_comment_date.erb
@@ -1,6 +1,6 @@
 <span class="color-base-30 px-2 m:pl-0" role="presentation">&bull;</span>
 
-<a href="<%= URL.comment(decorated_comment) %>" class="comment-date crayons-link crayons-link--secondary fs-s">
+<a href="<%= contextual_comment_url(decorated_comment, article: @article) %>" class="comment-date crayons-link crayons-link--secondary fs-s">
   <time datetime="<%= decorated_comment.published_timestamp %>" class=<%= decorated_comment.created_at.year == Time.current.year ? "date-no-year" : "date-short-year" %>>
     <%= decorated_comment.readable_publish_date %>
   </time>
diff --git a/app/views/comments/_comment_header.html.erb b/app/views/comments/_comment_header.html.erb
index d229b23b2..50fed7cd9 100644
--- a/app/views/comments/_comment_header.html.erb
+++ b/app/views/comments/_comment_header.html.erb
@@ -29,7 +29,7 @@
     </button>
     <div id="comment-dropdown-<%= comment.id %>" class="crayons-dropdown right-1 s:right-0 s:left-auto fs-base dropdown">
       <ul class="m-0">
-        <li><a href="<%= URL.comment(comment) %>" class="crayons-link crayons-link--block permalink-copybtn" aria-label="<%= t("views.comments.menu.copy.aria_label", user: comment.user.name) %>" data-no-instant><%= t("views.comments.menu.copy.text") %></a></li>
+        <li><a href="<%= contextual_comment_url(comment, article: @article) %>" class="crayons-link crayons-link--block permalink-copybtn" aria-label="<%= t("views.comments.menu.copy.aria_label", user: comment.user.name) %>" data-no-instant><%= t("views.comments.menu.copy.text") %></a></li>
         <li class="comment-actions hidden" data-user-id="<%= comment.user_id %>" data-action="settings-button" data-path="<%= URL.comment(comment) %>/settings" aria-label="<%= t("views.comments.menu.settings.aria_label", user: comment.user.name) %>"></li>
         <% action = comment.hidden_by_commentable_user ? "unhide" : "hide" %>
         <li class="comment-actions hidden" data-action="hide-button" data-commentable-user-id="<%= commentable&.user_id %>" data-user-id="<%= comment.user_id %>">
diff --git a/spec/system/comments/user_views_article_comments_spec.rb b/spec/system/comments/user_views_article_comments_spec.rb
index bbf6a327c..863322712 100644
--- a/spec/system/comments/user_views_article_comments_spec.rb
+++ b/spec/system/comments/user_views_article_comments_spec.rb
@@ -32,7 +32,7 @@ RSpec.describe "Visiting article comments", type: :system, js: true do
   end
 
   context "when root is specified" do
-    before { visit "#{article.path}/comments/#{comment.id.to_s(26)}" }
+    before { visit "#{article.path}/comments/#{comment.id.to_s(26)}" } # rubocop:disable Rails/ToSWithArgument
 
     it "displays related comments" do
       expect(page).to have_selector(".single-comment-node", visible: :visible, count: 4)
@@ -47,4 +47,21 @@ RSpec.describe "Visiting article comments", type: :system, js: true do
                                                                                               count: 1)
     end
   end
+
+  context "when looking into comment links" do
+    it "uses the permalink for signed in users" do
+      sign_in user
+      visit article.path
+      date_link = find("#comment-node-#{grandchild_comment.id} a.comment-date")
+      expect(date_link[:href]).to end_with(grandchild_comment.path)
+    end
+
+    it "uses an anchor tag instead of permalink for signed out users" do
+      sign_out user
+      visit article.path
+      date_link = find("#comment-node-#{grandchild_comment.id} a.comment-date")
+      expected_path = "#{article.path}#comment-#{grandchild_comment.id_code}"
+      expect(date_link[:href]).to end_with(expected_path)
+    end
+  end
 end