maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Changed logic for hiding and displaying low-quality marker for comments based on their score (#20581)

Browse source 
* Specs for revised comments thresholds

* Changed thresholds for low quality comments

* Added specs for comments page

* Fixed /comments and related specs

* Reorganized code for comments trees

* Specs for podcasts episodes comments

* Refactored Comments::Tree

* Refactored Comments::Tree + added specs

* Made build_sort_query private
This commit is contained in:
Anna Buianova 2024-02-05 17:27:24 +03:00 • committed by GitHub
parent 2746586eea
commit 7125a62db0
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
16 changed files with 317 additions and 121 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
app/controllers/comments_controller.rb
View file

@ -16,9 +16,11 @@ class CommentsController < ApplicationController
@root_comment = Comment.find(params[:id_code].to_i(26)) if params[:id_code].present?
# hide low quality comments w/o children
if @root_comment && @root_comment.decorate.low_quality && !@root_comment.has_children?
not_found
if @root_comment
# 404 for all low-quality for not signed in
not_found if @root_comment.score < Comment::LOW_QUALITY_THRESHOLD && !user_signed_in?
# 404 only for < -400 w/o children for signed in
not_found if @root_comment.score < Comment::HIDE_THRESHOLD && !@root_comment.has_children?
end
if @podcast

4
app/decorators/comment_decorator.rb
View file

@ -3,6 +3,10 @@ class CommentDecorator < ApplicationDecorator
score < Comment::LOW_QUALITY_THRESHOLD
end
def super_low_quality
score < Comment::HIDE_THRESHOLD
end
def published_timestamp
return "" if created_at.nil?

18
app/helpers/comments_helper.rb
View file

@ -14,20 +14,12 @@ module CommentsHelper
!(commentable.any_comments_hidden || any_hidden_negative_comments?(commentable))
end
def article_comment_tree(article, count, order)
@article_comment_tree ||= begin
collection = Comment.tree_for(article, count, order)
collection.reject! { |comment| comment.score.negative? } unless user_signed_in?
collection
end
def article_comment_tree(article, limit, order)
Comments::Tree.for_commentable(article, limit: limit, order: order, include_negative: user_signed_in?)
end
def podcast_comment_tree(episode)
@podcast_comment_tree ||= begin
collection = Comment.tree_for(episode, 12)
collection.reject! { |comment| comment.score.negative? } unless user_signed_in?
collection
end
Comments::Tree.for_commentable(episode, include_negative: user_signed_in?, limit: 12)
end
def comment_class(comment, is_view_root: false)
@ -58,10 +50,6 @@ module CommentsHelper
end
end
def tree_for(comment, sub_comments, commentable)
nested_comments(tree: { comment => sub_comments }, commentable: commentable, is_view_root: true)
end
def should_be_hidden?(comment, root_comment)
# when opened by a permalink + root comment is hidden => show root comment and its descendants
comment.hidden_by_commentable_user && comment != root_comment && !root_comment&.hidden_by_commentable_user

22
app/models/comment.rb
View file

@ -10,6 +10,7 @@ class Comment < ApplicationRecord
COMMENTABLE_TYPES = %w[Article PodcastEpisode].freeze
LOW_QUALITY_THRESHOLD = -75
HIDE_THRESHOLD = -400 # hide comments below this threshold
VALID_SORT_OPTIONS = %w[top latest oldest].freeze
@ -90,14 +91,6 @@ class Comment < ApplicationRecord
alias touch_by_reaction save
def self.tree_for(commentable, limit = 0, order = nil)
commentable.comments
.includes(user: %i[setting profile])
.arrange(order: build_sort_query(order))
.to_a[0..limit - 1]
.to_h
end
def self.title_deleted
I18n.t("models.comment.deleted")
end
@ -114,17 +107,6 @@ class Comment < ApplicationRecord
includes(user: :profile).new(params, &blk)
end
def self.build_sort_query(order)
case order
when "latest"
"created_at DESC"
when "oldest"
"created_at ASC"
else
"score DESC"
end
end
def search_id
"comment_#{id}"
end
@ -208,8 +190,6 @@ class Comment < ApplicationRecord
Comments::CalculateScoreWorker.perform_async(id)
end
private_class_method :build_sort_query
private
def remove_notifications?

34
app/queries/comments/tree.rb Normal file
View file

@ -0,0 +1,34 @@
module Comments
module Tree
module_function
def for_commentable(commentable, limit: 0, order: nil, include_negative: false)
collection = commentable.comments
.includes(user: %i[setting profile])
.arrange(order: build_sort_query(order))
.to_a[0..limit - 1]
.to_h
collection.reject! { |comment| comment.score.negative? } unless include_negative
collection
end
def for_root_comment(root_comment, include_negative: false)
sub_comments = root_comment.subtree.includes(user: %i[setting profile]).arrange[root_comment]
sub_comments.reject! { |comment| comment.score.negative? } unless include_negative
{ root_comment => sub_comments }
end
def 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
end
end

2
app/views/articles/_comment_tree.html.erb
View file

@ -1,4 +1,4 @@
<% comment, sub_comments = comment_node %>
<% unless comment.decorate.low_quality && sub_comments.empty? %>
<% unless comment.decorate.super_low_quality && sub_comments.empty? %>
<%= nested_comments(tree: { comment => sub_comments }, commentable: @article, is_view_root: true) %>
<% end %>

2
app/views/comments/_comment_proper.html.erb
View file

@ -5,7 +5,7 @@
<div class="inner-comment comment__details">
<div class="comment__content crayons-card">
<% if comment.deleted || decorated_comment.low_quality %>
<% if comment.deleted || decorated_comment.super_low_quality %>
<div class="p-6 align-center opacity-50 fs-s">
<span class="js-comment-username">
<%= t("views.comments.delete.comment_deleted") %>

10
app/views/comments/index.html.erb
View file

@ -1,5 +1,5 @@
<%= javascript_include_tag "postCommentsPage", defer: true %>
<% if @root_comment.present? && !@root_comment.decorate.low_quality %>
<% if @root_comment.present? && !@root_comment.decorate.super_low_quality %>
<% title(@root_comment.title.to_s) %>
<% else %>
<% title(t("views.comments.meta.title_root", title: @commentable.title)) %>
@ -139,12 +139,14 @@
<div class="comments" id="comment-trees-container">
<% if @root_comment.present? %>
<% cache ["comment_root-view-root_#{user_signed_in?}", @root_comment] do %>
<%= tree_for(@root_comment, @root_comment.subtree.includes(user: %i[setting profile]).arrange[@root_comment], @commentable) %>
<%= nested_comments(tree: Comments::Tree.for_root_comment(@root_comment, include_negative: user_signed_in?), commentable: @commentable, is_view_root: true) %>
<% end %>
<% else %>
<% Comment.tree_for(@commentable).each do |comment, sub_comments| %>
<% Comments::Tree.for_commentable(@commentable, include_negative: user_signed_in?).each do |comment, sub_comments| %>
<% cache ["comment_root_#{user_signed_in?}", comment] do %>
<%= tree_for(comment, sub_comments, @commentable) %>
<% unless comment.decorate.super_low_quality && sub_comments.empty? %>
<%= nested_comments(tree: { comment => sub_comments }, commentable: @commentable, is_view_root: true) %>
<% end %>
<% end %>
<% end %>
<% end %>

2
app/views/podcast_episodes/show.html.erb
View file

@ -105,7 +105,7 @@
<div class="comment-trees" id="comment-trees-container">
<% podcast_comment_tree(@episode).each do |comment, sub_comments| %>
<% cache ["comment_root_#{user_signed_in?}", comment] do %>
<%= tree_for(comment, sub_comments, @episode) %>
<%= nested_comments(tree: { comment => sub_comments }, commentable: @episode, is_view_root: true) %>
<% end %>
<% end %>
</div>

53
spec/models/comment_spec.rb
View file

@ -360,59 +360,6 @@ RSpec.describe Comment do
end
end
describe ".tree_for" do
let!(:other_comment) { create(:comment, commentable: article, user: user) }
let!(:child_comment) { create(:comment, commentable: article, parent: comment, user: user) }
before { comment.update_column(:score, 1) }
it "returns a full tree" do
comments = described_class.tree_for(article)
expect(comments).to eq(comment => { child_comment => {} }, other_comment => {})
end
it "returns part of the tree" 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
# rubocop:disable RSpec/ExampleLength
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
# rubocop:enable RSpec/ExampleLength
end
end
context "when callbacks are triggered after create" do
let(:comment) { build(:comment, user: user, commentable: article) }

87
spec/queries/comments/tree_spec.rb Normal file
View file

@ -0,0 +1,87 @@
require "rails_helper"
RSpec.describe Comments::Tree do
let(:user) { create(:user) }
let(:article) { create(:article) }
let!(:comment) { create(:comment, user: user, commentable: article) }
let!(:other_comment) { create(:comment, commentable: article, user: user, created_at: 1.hour.from_now) }
let!(:child_comment) { create(:comment, commentable: article, parent: comment, user: user) }
before { comment.update_column(:score, 1) }
describe "#for_commentable" do
it "returns a full tree" do
comments = described_class.for_commentable(article)
expect(comments).to eq(comment => { child_comment => {} }, other_comment => {})
end
it "returns part of the tree" do
comments = described_class.for_commentable(article, limit: 1)
expect(comments).to eq(comment => { child_comment => {} })
end
context "with include_negative" do
before do
other_comment.update_column(:score, -10)
end
it "returns comments with low score if include_negative is passed" do
comments = described_class.for_commentable(article, include_negative: true)
expect(comments).to eq({ comment => { child_comment => {} }, other_comment => {} })
end
it "doesn't return comments with low score if include_negative is false" do
comments = described_class.for_commentable(article)
expect(comments).to eq(comment => { child_comment => {} })
end
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.for_commentable(article, limit: 0, order: "oldest")
comments = comments.map { |key, _| key.id }
expect(comments).to eq([old_comment.id, comment.id, other_comment.id, new_comment.id])
end
it "returns comments in the right order when order is latest" do
comments = described_class.for_commentable(article, limit: 0, order: "latest")
comments = comments.map { |key, _| key.id }
expect(comments).to eq([new_comment.id, other_comment.id, 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.for_commentable(article, limit: 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
describe "#for_root_comment" do
let!(:low_comment) { create(:comment, commentable: article, parent: comment, score: -100) }
it "returns tree for a particular comment" do
comments = described_class.for_root_comment(comment)
expect(comments).to eq(comment => { child_comment => {} })
end
it "returns tree with negative comments" do
comments = described_class.for_root_comment(comment, include_negative: true)
expect(comments).to eq(comment => { child_comment => {}, low_comment => {} })
end
end
end

16
spec/requests/articles/articles_show_spec.rb
View file

@ -182,11 +182,12 @@ RSpec.describe "ArticlesShow" do
end
context "with comments" do
let!(:spam_comment) { create(:comment, score: -80, commentable: article, body_markdown: "Spam comment") }
let!(:spam_comment) { create(:comment, score: -450, commentable: article, body_markdown: "Spam comment") }
before do
create(:comment, score: 10, commentable: article, body_markdown: "Good comment")
create(:comment, score: -10, commentable: article, body_markdown: "Bad comment")
create(:comment, score: -99, commentable: article, body_markdown: "Bad comment")
create(:comment, score: -10, commentable: article, body_markdown: "Mediocre comment")
end
context "when user signed in" do
@ -199,12 +200,12 @@ RSpec.describe "ArticlesShow" do
expect(response.body).to include("Good comment")
end
it "shows comments with score from -75 (low quality threshold) to 0" do
it "shows comments with score from -400 to -75" do
get article.path
expect(response.body).to include("Bad comment")
end
it "hides comments with score < -75 and no comment deleted message" do
it "hides comments with score < -400 and no comment deleted message" do
get article.path
expect(response.body).not_to include("Spam comment")
expect(response.body).not_to include("Comment deleted")
@ -224,14 +225,11 @@ RSpec.describe "ArticlesShow" do
expect(response.body).to include("Good comment")
end
it "hides comments with score from -75 to 0" do
it "hides all negative comments", :aggregate_failures do
get article.path
expect(response.body).not_to include("Bad comment")
end
it "hides comments with score < -75" do
get article.path
expect(response.body).not_to include("Spam comment")
expect(response.body).not_to include("Mediocre comment")
end
it "doesn't show children of a low-quality comment" do

114
spec/requests/comments_spec.rb
View file

@ -31,6 +31,55 @@ RSpec.describe "Comments" do
expect(response.body).not_to include "author-payment-pointer"
end
context "when there are comments with different score" do
let!(:spam_comment) do
create(:comment, commentable: article, user: user, score: -1000, body_markdown: "spammer-comment")
end
let!(:mediocre_comment) do
create(:comment, commentable: article, user: user, score: -50, body_markdown: "mediocre-comment")
end
before do
create(:comment, commentable: article, user: user, score: -100, body_markdown: "bad-comment")
create(:comment, commentable: article, user: user, score: 10, body_markdown: "good-comment")
end
it "displays all comments except for below -400 score for signed in", :aggregate_failures do
sign_in user
get "#{article.path}/comments"
expect(response.body).to include("mediocre-comment")
expect(response.body).to include("low quality") # marker
expect(response.body).to include("bad-comment")
expect(response.body).to include("good-comment")
expect(response.body).not_to include("spammer-comment")
end
it "displays deleted message and children of a spam comment for signed in", :aggregate_failures do
create(:comment, user: user, parent: spam_comment, commentable: article,
body_markdown: "child-of-a-spam-comment")
sign_in user
get "#{article.path}/comments"
expect(response.body).not_to include("spammer-comment")
expect(response.body).to include("Comment deleted")
expect(response.body).to include("child-of-a-spam-comment")
end
it "displays only comments with positive score for signed out user", :aggregate_failures do
get "#{article.path}/comments"
expect(response.body).not_to include("mediocre-comment")
expect(response.body).not_to include("bad-comment")
expect(response.body).to include("good-comment")
expect(response.body).not_to include("spammer-comment")
end
it "doesn't display children of negative comments for signed out user" do
create(:comment, user: user, parent: mediocre_comment, commentable: article,
body_markdown: "child-of-a-negative-comment")
get "#{article.path}/comments"
expect(response.body).not_to include("child-of-a-negative-comment")
end
end
context "when the comment is a root" do
it "displays the comment hidden message if the comment is hidden" do
comment.update(hidden_by_commentable_user: true)
@ -161,7 +210,7 @@ RSpec.describe "Comments" do
end
end
context "when the comment is low quality" do
context "when the comment is low quality and below hiding threshold" do
let(:low_comment) do
create(:comment, commentable: article, user: user, score: -1000, body_markdown: "low-comment")
end
@ -172,23 +221,80 @@ RSpec.describe "Comments" do
end.to raise_error(ActiveRecord::RecordNotFound)
end
it "is displayed as deleted when has children", :aggregate_failures do
it "raises 404 when has children and not signed in" do
create(:comment, commentable: article, user: user, parent: low_comment,
body_markdown: "child of a low-quality comment")
expect do
get low_comment.path
end.to raise_error(ActiveRecord::RecordNotFound)
end
it "raises 404 when no children + user signed in" do
sign_in user
expect do
get low_comment.path
end.to raise_error(ActiveRecord::RecordNotFound)
end
it "is displayed as deleted when has children + user signed in", :aggregate_failures do
create(:comment, commentable: article, user: user, parent: low_comment,
body_markdown: "child of a low-quality comment")
sign_in user
get low_comment.path
expect(response).to have_http_status(:ok)
expect(response.body).to include("Comment deleted")
expect(response.body).to include("child of a low-quality comment")
end
it "hides negative children for signed out" do
create(:comment, commentable: article, user: user, score: -10, parent: comment,
body_markdown: "low-child of a comment")
get comment.path
expect(response.body).not_to include("low-child of a comment")
end
end
context "when the comment is low quality and above hiding threshold" do
let(:low_comment) do
create(:comment, commentable: article, user: user, score: -100, body_markdown: "low-comment")
end
it "raises 404 when no children + not signed in" do
expect do
get low_comment.path
end.to raise_error(ActiveRecord::RecordNotFound)
end
it "raises 404 when has children and not signed in" do
create(:comment, commentable: article, user: user, parent: low_comment,
body_markdown: "child of a low-quality comment")
expect do
get low_comment.path
end.to raise_error(ActiveRecord::RecordNotFound)
end
it "is displayed with a low quality marker when user signed in" do
sign_in user
get low_comment.path
expect(response).to be_successful
expect(response.body).to include("low quality")
end
end
context "when the comment is for a podcast's episode" do
it "is successful" do
podcast_comment = create(:comment, commentable: podcast_episode, user: user)
let!(:podcast_comment) { create(:comment, commentable: podcast_episode, user: user) }
it "is successful" do
get podcast_comment.path
expect(response).to have_http_status(:ok)
end
it "raises 404 when low quality" do
podcast_comment.update_column(:score, -500)
expect do
get podcast_comment.path
end.to raise_error(ActiveRecord::RecordNotFound)
end
end
context "when the article is unpublished" do

53
spec/requests/podcasts/podcast_episodes_show_spec.rb
View file

@ -2,20 +2,65 @@ require "rails_helper"
RSpec.describe "Podcast Episodes Show Spec" do
describe "GET podcast episodes show" do
let(:podcast) { create(:podcast) }
let!(:podcast_episode) { create(:podcast_episode, podcast: podcast) }
let(:user) { create(:user) }
it "renders the correct podcast episode" do
podcast = create(:podcast)
podcast_episode = create(:podcast_episode, podcast: podcast)
get "/#{podcast.slug}/#{podcast_episode.slug}"
expect(response.body).to include podcast_episode.title
end
it "does not render another podcast's episode if the wrong podcast slug is given" do
podcast = create(:podcast)
other_podcast = create(:podcast)
podcast_episode = create(:podcast_episode, podcast: podcast)
expect do
get "/#{other_podcast.slug}/#{podcast_episode.slug}"
end.to raise_error(ActiveRecord::RecordNotFound)
end
context "with comments" do
let!(:spam_comment) do
create(:comment, commentable: podcast_episode, user: user, score: -1000, body_markdown: "spammer-comment")
end
before do
create(:comment, commentable: podcast_episode, body_markdown: "episode-comment")
create(:comment, commentable: podcast_episode, user: user, score: -50, body_markdown: "mediocre-comment")
create(:comment, commentable: podcast_episode, user: user, score: -100, body_markdown: "bad-comment")
end
it "displays only good standing comments for signed out", :aggregate_failures do
get "/#{podcast.slug}/#{podcast_episode.slug}"
expect(response.body).to include("episode-comment")
expect(response.body).not_to include("spammer-comment")
expect(response.body).not_to include("mediocre-comment")
expect(response.body).not_to include("bad-comment")
end
it "displays all comments above > -400 for signed in", :aggregate_failures do
sign_in user
get "/#{podcast.slug}/#{podcast_episode.slug}"
expect(response.body).to include("episode-comment")
expect(response.body).not_to include("spammer-comment")
expect(response.body).to include("mediocre-comment")
expect(response.body).to include("bad-comment")
end
it "displays deleted message and children of a spam comment for signed in", :aggregate_failures do
create(:comment, user: user, parent: spam_comment, commentable: podcast_episode,
body_markdown: "child-of-a-spam-comment")
sign_in user
get "/#{podcast.slug}/#{podcast_episode.slug}"
expect(response.body).not_to include("spammer-comment")
expect(response.body).to include("Comment deleted")
expect(response.body).to include("child-of-a-spam-comment")
end
it "displays a low-quality marker for a low-quality comment" do
sign_in user
get "/#{podcast.slug}/#{podcast_episode.slug}"
expect(response.body).to include("low quality/non-constructive") # for bad-comment
end
end
end
end

9
spec/requests/user/user_profile_spec.rb
View file

@ -96,13 +96,15 @@ RSpec.describe "UserProfiles" do
context "when has comments" do
before do
create(:comment, user: user, body_markdown: "nice_comment")
create(:comment, user: user, score: Comment::LOW_QUALITY_THRESHOLD - 50, body_markdown: "bad_comment")
create(:comment, user: user, score: -100, body_markdown: "low_comment")
create(:comment, user: user, score: -401, body_markdown: "bad_comment")
end
it "displays only good standing comments comments", :aggregate_failures do
it "displays good standing comments", :aggregate_failures do
sign_in current_user
get user.path
expect(response.body).to include("nice_comment")
expect(response.body).not_to include("low_comment")
expect(response.body).not_to include("bad_comment")
end
@ -235,7 +237,8 @@ RSpec.describe "UserProfiles" do
suspended_user.add_role(:suspended)
create(:article, user_id: user.id, published: false, published_at: Date.tomorrow)
expect { get "/#{suspended_user.username}" }.not_to raise_error(ActiveRecord::RecordNotFound)
get "/#{suspended_user.username}"
expect(response).to be_successful
end
end

4
spec/views/comments/_comment.html.erb_spec.rb
View file

@ -2,10 +2,10 @@ require "rails_helper"
RSpec.describe "rendering locals in a partial" do
context "when comment is low-quality" do
it "renders the comment with low-quality marker", skip: "hiding low-quality for now" do
it "renders the comment with low-quality marker" do
allow(Settings::General).to receive(:mascot_image_url).and_return("https://i.imgur.com/fKYKgo4.png")
comment = create(:comment, processed_html: "hi", score: Comment::LOW_QUALITY_THRESHOLD - 100)
article = create(:article)
comment = create(:comment, processed_html: "hi", score: -100, commentable: article)
render "comments/comment",
comment: comment,