Added a way to sort comments on an article (#16686)

* Added a way to sort comments on an article * Sorting of comments now uses crayons-dropwn. Some minor fixes as per comments on PR * Add and fix test cases for sorting comments functionality * Changes in code for sort comments. Code is more aligned with forem's conventions * Added cyperss tests for sort comments on an article. Cleaned up code to better follow forem conventions * Get fresh handle of triggerButton everytime clickOustideListener is clicked. Fix Cypress test cases to reflect the earlier.
2022-04-01 19:56:18 +05:30 · 2022-04-01 19:56:18 +05:30 · 1345334b33
commit 1345334b33
parent a8ec0c3058
14 changed files with 237 additions and 19 deletions
--- a/app/assets/stylesheets/views/comments.scss
+++ b/app/assets/stylesheets/views/comments.scss
@ -275,3 +275,24 @@
 .moderator-template-button:not(.active) {
  @extend .crayons-btn--outlined;
 }
+
+#comments-sort-dropdown-container {
+  .comment-sort-option {
+    &__header {
+      color: var(--base-90);
+      font-weight: var(--fw-medium);
+    }
+
+    .comment-sort-option__header {
+      .crayons-icon {
+        position: absolute;
+        left: var(--su-4);
+        color: var(--link-branded-color);
+      }
+
+      &[aria-current='page'] {
+        font-weight: var(--fw-bold);
+      }
+    }
+  }
+}
--- a/app/controllers/stories_controller.rb
+++ b/app/controllers/stories_controller.rb
@ -256,7 +256,7 @@ class StoriesController < ApplicationController
    @discussion_lock = @article.discussion_lock
    @user = @article.user
    @organization = @article.organization
-
+    @comments_order = fetch_sort_order
    if @article.collection
      @collection = @article.collection

@ -414,4 +414,10 @@ class StoriesController < ApplicationController
      @user.profile.website_url,
    ].compact_blank
  end
+
+  def fetch_sort_order
+    return params[:comments_sort] if Comment::VALID_SORT_OPTIONS.include? params[:comments_sort]
+
+    "top"
+  end
 end
--- a/app/javascript/packs/commentDropdowns.js
+++ b/app/javascript/packs/commentDropdowns.js
@ -21,7 +21,7 @@ const handleCopyPermalink = (closeDropdown) => {
 const initializeArticlePageDropdowns = () => {
  // Gather all dropdown triggers for comment options and profile previews
  const dropdownTriggers = document.querySelectorAll(
-    'button[id^=comment-dropdown-trigger], button[id^=comment-profile-preview-trigger-]',
+    'button[id^=comment-dropdown-trigger], button[id^=comment-profile-preview-trigger-], button[id^=toggle-comments-sort-dropdown]',
  );

  for (const dropdownTrigger of dropdownTriggers) {
@ -58,7 +58,11 @@ const initializeArticlePageDropdowns = () => {
        '.report-abuse-link-wrapper',
      );
      if (reportAbuseWrapper) {
-        reportAbuseWrapper.innerHTML = `<a href="${reportAbuseWrapper.dataset.path}" class="crayons-link crayons-link--block">${locale('core.report_abuse')}</a>`;
+        reportAbuseWrapper.innerHTML = `<a href="${
+          reportAbuseWrapper.dataset.path
+        }" class="crayons-link crayons-link--block">${locale(
+          'core.report_abuse',
+        )}</a>`;
      }

      // Initialize the "Copy link" functionality
--- a/app/javascript/utilities/dropdownUtils.js
+++ b/app/javascript/utilities/dropdownUtils.js
@ -165,7 +165,10 @@ export const initializeDropdown = ({

  // Close the dropdown if user has clicked outside
  const clickOutsideListener = ({ target }) => {
+    // Get fresh handle every time, resulting in more streamlined functionality for cypress
+    const triggerButton = document.getElementById(triggerElementId);
    if (
+      triggerButton &&
      target !== triggerButton &&
      !dropdownContent.contains(target) &&
      !triggerButton.contains(target)
--- a/app/models/comment.rb
+++ b/app/models/comment.rb
@ -9,6 +9,8 @@ class Comment < ApplicationRecord

  COMMENTABLE_TYPES = %w[Article PodcastEpisode].freeze

+  VALID_SORT_OPTIONS = %w[top latest oldest].freeze
+
  URI_REGEXP = %r{
    \A
    (?:https?://)?  # optional scheme
@ -87,10 +89,10 @@ class Comment < ApplicationRecord

  alias touch_by_reaction save

-  def self.tree_for(commentable, limit = 0)
+  def self.tree_for(commentable, limit = 0, order = nil)
    commentable.comments
      .includes(user: %i[setting profile])
-      .arrange(order: "score DESC")
+      .arrange(order: build_sort_query(order))
      .to_a[0..limit - 1]
      .to_h
  end
@ -172,6 +174,19 @@ class Comment < ApplicationRecord
    ancestry && Comment.exists?(id: ancestry)
  end

+  def self.build_sort_query(order)
+    case order
+    when "latest"
+      "created_at DESC"
+    when "oldest"
+      "created_at ASC"
+    else
+      "score DESC"
+    end
+  end
+
+  private_class_method :build_sort_query
+
  private

  def remove_notifications?
--- a/app/views/articles/_full_comment_area.html.erb
+++ b/app/views/articles/_full_comment_area.html.erb
@ -2,10 +2,37 @@
  <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">
-        <h2 class="crayons-subtitle-1">
-          <%= t("views.articles.comments.subtitle_html",
-                num: tag.span(t("views.articles.comments.num", num: @article.comments_count), class: "js-comments-count", data: { comments_count: @article.comments_count })) %>
-        </h2>
+        <div class="flex">
+          <h2>
+            <%= t("views.articles.comments.subtitle.#{@comments_order}_html",
+                  num: tag.span(t("views.articles.comments.num", num: @article.comments_count), class: "js-comments-count", data: { comments_count: @article.comments_count })) %>
+          </h2>
+          <button class="c-btn c-btn--ghost crayons-btn--icon-left" id="toggle-comments-sort-dropdown" aria-controls="comments-sort-dropdown-container" aria-expanded="false" aria-label="<%= t("views.articles.comments.sort_button.aria_label") %>" aria-haspopup="true">
+            <%= inline_svg_tag("expand.svg", aria_hidden: true, title: t("views.moderations.actions.admin.icon")) %>
+          </button>
+        </div>
+
+        <nav class="crayons-dropdown p-4" id="comments-sort-dropdown-container" aria-labelledby="comments-sort-title">
+            <h3 id="comments-sort-title" class="mb-3">Sort discussion: </h3>
+            <ul class="comments-sort-dropdown__list">
+              <%= render "comments/sort_option",
+                         sort_order: "top",
+                         sort_title: "Top",
+                         sort_description: "Most upvoted and relevant comments will be first",
+                         show_selected: @comments_order == "top" %>
+              <%= render "comments/sort_option",
+                         sort_order: "latest",
+                         sort_title: "Latest",
+                         sort_description: "Most recent comments will be first",
+                         show_selected: @comments_order == "latest" %>
+              <%= render "comments/sort_option",
+                         sort_order: "oldest",
+                         sort_title: "Oldest",
+                         sort_description: "The oldest comments will be first",
+                         show_selected: @comments_order == "oldest" %>
+            </ul>
+        </nav>
+
        <div id="comment-subscription" class="print-hidden">
          <div class="crayons-btn-group">
            <span class="crayons-btn crayons-btn--outlined"><%= t("views.articles.comments.subscribe") %></span>
@ -28,7 +55,7 @@

        <div class="comments" id="comment-trees-container">
          <% if @article.comments_count > 0 %>
-            <%= render partial: "articles/comment_tree", collection: Comment.tree_for(@article, @comments_to_show_count), as: :comment_node, cached: proc { |comment, _sub_comments| comment } %>
+            <%= render partial: "articles/comment_tree", collection: Comment.tree_for(@article, @comments_to_show_count, @comments_order), as: :comment_node, cached: proc { |comment, _sub_comments| comment } %>
          <% end %>
        </div>
      </div>
--- a/app/views/comments/_sort_option.html.erb
+++ b/app/views/comments/_sort_option.html.erb
@ -0,0 +1,13 @@
+<li class="comment-sort-option mt-2 pl-7">
+  <a
+    href="<%= @article.path %>?comments_sort=<%= sort_order %>"
+    class="comment-sort-option__header"
+    aria-describedby="<%= sort_order %>-description-text"
+    aria-current="<%= "page" if show_selected %>">
+    <%= crayons_icon_tag(:checkmark, title: "Selected Sort Option", aria_hidden: true) if show_selected %>
+    <%= sort_title %>
+  </a>
+  <div id="<%= sort_order %>-description-text" class="crayons-field__description">
+    <%= sort_description %>
+  </div>
+</li>
--- a/config/locales/views/articles/en.yml
+++ b/config/locales/views/articles/en.yml
@ -8,7 +8,12 @@ en:
        aria_label: Article actions
      comments:
        aria_label: Comments for post %{title} (%{num})
-        subtitle_html: Discussion %{num}
+        sort_button:
+          aria_label: Sort comments
+        subtitle:
+          top_html: Top comments %{num}
+          latest_html: Latest comments %{num}
+          oldest_html: Oldest comments %{num}
        num: "(%{num})"
        subscribe: Subscribe
      conduct:
--- a/config/locales/views/articles/fr.yml
+++ b/config/locales/views/articles/fr.yml
@ -8,7 +8,12 @@ fr:
        aria_label: Article actions
      comments:
        aria_label: Comments for post %{title} (%{num})
-        subtitle_html: Discussion %{num}
+        sort_button:
+          aria_label: Sort comments
+        subtitle:
+          top_html: Top comments %{num}
+          latest_html: Latest comments %{num}
+          oldest_html: Oldest comments %{num}
        num: "(%{num})"
        subscribe: Subscribe
      conduct:
--- a/cypress/integration/seededFlows/articleFlows/commentOnArticle.spec.js
+++ b/cypress/integration/seededFlows/articleFlows/commentOnArticle.spec.js
@ -396,7 +396,7 @@ describe('Comment on articles', () => {

  it('should add a comment', () => {
    cy.findByRole('main').within(() => {
-      cy.findByRole('heading', { name: 'Discussion (0)' });
+      cy.findByRole('heading', { name: 'Top comments (0)' });

      cy.findByRole('textbox', { name: /^Add a comment to the discussion$/i })
        .focus() // Focus activates the Submit button and mini toolbar below a comment textbox
@ -414,7 +414,7 @@ describe('Comment on articles', () => {
      }).should('have.value', '');

      cy.findByText(/^this is a comment$/i);
-      cy.findByRole('heading', { name: 'Discussion (1)' });
+      cy.findByRole('heading', { name: 'Top comments (1)' });

      // Check that the profile preview card is there and can be displayed
      cy.findByTestId('comments-container').within(() => {
@ -439,7 +439,7 @@ describe('Comment on articles', () => {
      content: 'This is a test canned response',
    }).then((_response) => {
      cy.findByRole('main').within(() => {
-        cy.findByRole('heading', { name: 'Discussion (0)' });
+        cy.findByRole('heading', { name: 'Top comments (0)' });

        cy.findByRole('textbox', {
          name: /^Add a comment to the discussion$/i,
@ -460,7 +460,7 @@ describe('Comment on articles', () => {
          name: /^Add a comment to the discussion$/i,
        }).should('have.value', '');

-        cy.findByRole('heading', { name: 'Discussion (1)' });
+        cy.findByRole('heading', { name: 'Top comments (1)' });
      });
    });
  });
--- a/cypress/integration/seededFlows/articleFlows/sortCommentsOnArticle.spec.js
+++ b/cypress/integration/seededFlows/articleFlows/sortCommentsOnArticle.spec.js
@ -0,0 +1,84 @@
+describe('Sort Comments in an Article', () => {
+  beforeEach(() => {
+    cy.testSetup();
+    cy.viewport('macbook-16');
+    cy.fixture('users/articleEditorV1User.json').as('user');
+
+    cy.get('@user').then((user) => {
+      cy.loginAndVisit(user, '/admin_mcadmin/test-article-slug');
+      // Make sure the page has loaded
+      cy.findByRole('heading', { name: 'Test article' });
+    });
+  });
+
+  it('clicking on sort comments button should open and close dropdown menu', () => {
+    cy.findByRole('button', { name: 'Sort comments' })
+      .should('have.attr', 'aria-expanded', 'false')
+      .click()
+      .should('have.attr', 'aria-expanded', 'true');
+
+    cy.findByRole('navigation', { name: 'Sort discussion:' }).within(() => {
+      cy.findByRole('link', { name: 'Top' });
+      cy.findByRole('link', { name: 'Oldest' });
+      cy.findByRole('link', { name: 'Latest' });
+    });
+  });
+
+  it('by default shows top comments', () => {
+    cy.findByRole('button', { name: 'Sort comments' }).click();
+
+    cy.findByRole('navigation', { name: 'Sort discussion:' }).within(() => {
+      cy.findByRole('link', {
+        name: 'Top',
+      }).should('have.attr', 'aria-current', 'page');
+
+      cy.findByRole('link', {
+        name: 'Top',
+      }).should('have.focus');
+    });
+
+    cy.findByRole('heading', { name: 'Top comments (1)' }).should('exist');
+  });
+
+  it('should navigate to latest', () => {
+    cy.findByRole('button', { name: 'Sort comments' }).click();
+
+    cy.findByRole('navigation', { name: 'Sort discussion:' }).within(() => {
+      cy.findByRole('link', {
+        name: 'Latest',
+      }).click();
+    });
+
+    cy.url().should('contain', '?comments_sort=latest');
+    cy.findByRole('heading', { name: 'Latest comments (1)' }).should('exist');
+
+    cy.findByRole('button', { name: 'Sort comments' }).click();
+
+    cy.findByRole('navigation', { name: 'Sort discussion:' }).within(() => {
+      cy.findByRole('link', {
+        name: 'Latest',
+      }).should('have.attr', 'aria-current', 'page');
+    });
+  });
+
+  it('should navigate to oldest', () => {
+    cy.findByRole('button', { name: 'Sort comments' }).click();
+
+    cy.findByRole('navigation', { name: 'Sort discussion:' }).within(() => {
+      cy.findByRole('link', {
+        name: 'Oldest',
+      }).click();
+    });
+
+    cy.url().should('contain', '?comments_sort=oldest');
+    cy.findByRole('heading', { name: 'Oldest comments (1)' }).should('exist');
+
+    cy.findByRole('button', { name: 'Sort comments' }).click();
+
+    cy.findByRole('navigation', { name: 'Sort discussion:' }).within(() => {
+      cy.findByRole('link', {
+        name: 'Oldest',
+      }).should('have.attr', 'aria-current', 'page');
+    });
+  });
+});
--- a/cypress/integration/seededFlows/publishingFlows/publishOrSavePost.spec.js
+++ b/cypress/integration/seededFlows/publishingFlows/publishOrSavePost.spec.js
@ -27,7 +27,7 @@ describe('Publish or save a post', () => {
      });
      //   The post should now be published
      cy.findByRole('heading', { name: 'Test title' });
-      cy.findByRole('heading', { name: 'Discussion (0)' });
+      cy.findByRole('heading', { name: 'Top comments (0)' });
      cy.findByRole('link', { name: 'Edit' });
      cy.findByRole('link', { name: 'Manage' });
      cy.findByRole('link', { name: 'Stats' });
@ -104,7 +104,7 @@ describe('Publish or save a post', () => {
      });
      //   The post should now be published
      cy.findByRole('heading', { name: 'Test title' });
-      cy.findByRole('heading', { name: 'Discussion (0)' });
+      cy.findByRole('heading', { name: 'Top comments (0)' });
      cy.findByRole('link', { name: 'Edit' });
      cy.findByRole('link', { name: 'Manage' });
      cy.findByRole('link', { name: 'Stats' });
@ -246,7 +246,7 @@ describe('Publish or save a post', () => {

      // Wait for published post page, and choose to edit
      cy.findByRole('heading', { name: 'Test title' });
-      cy.findByRole('heading', { name: 'Discussion (0)' });
+      cy.findByRole('heading', { name: 'Top comments (0)' });
      cy.findByRole('link', { name: 'Edit' }).click();

      cy.findByLabelText('Post Content').clear().type('something else');
--- a/spec/models/comment_spec.rb
+++ b/spec/models/comment_spec.rb
@ -365,6 +365,40 @@ RSpec.describe Comment, type: :model do
      comments = described_class.tree_for(article, 1)
      expect(comments).to eq(comment => { child_comment => {} })
    end
+
+    context "with sort order" do
+      let!(:new_comment) { create(:comment, commentable: article, user: user, created_at: Date.tomorrow) }
+      let!(:old_comment) { create(:comment, commentable: article, user: user, created_at: Date.yesterday) }
+
+      before { comment }
+
+      it "returns comments in the right order when order is oldest" do
+        comments = described_class.tree_for(article, 0, "oldest")
+        comments = comments.map { |key, _| key.id }
+        expect(comments).to eq([old_comment.id, other_comment.id, comment.id, new_comment.id])
+      end
+
+      it "returns comments in the right order when order is latest" do
+        comments = described_class.tree_for(article, 0, "latest")
+        comments = comments.map { |key, _| key.id }
+        expect(comments).to eq([new_comment.id, comment.id, other_comment.id, old_comment.id])
+      end
+
+      it "returns comments in the right order when order is top" do
+        comment.update_column(:score, 5)
+        highest_rated_comment = comment
+        new_comment.update_column(:score, 1)
+        lowest_rated_comment = new_comment
+        old_comment.update_column(:score, 3)
+        mid_high_rated_comment = old_comment
+        other_comment.update_column(:score, 2)
+        mid_low_rated_comment = other_comment
+        comments = described_class.tree_for(article, 0)
+
+        comments = comments.map { |key, _| key.id }
+        expect(comments).to eq([highest_rated_comment.id, mid_high_rated_comment.id, mid_low_rated_comment.id, lowest_rated_comment.id]) # rubocop:disable Layout/LineLength
+      end
+    end
  end

  context "when callbacks are triggered after create" do
--- a/spec/views/articles_spec.rb
+++ b/spec/views/articles_spec.rb
@ -9,6 +9,7 @@ RSpec.describe "articles/show", type: :view do
    assign(:user, user1)
    assign(:article, article1.decorate)
    assign(:comment, Comment.new)
+    assign(:comments_order, "top")
    without_partial_double_verification do
      allow(view).to receive(:internal_navigation?).and_return(params[:i] == "i")
    end