Skip to content

General improvements to comment threads #1647

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 172 commits into
base: develop
Choose a base branch
from
Draft

Conversation

Oaphi
Copy link
Member

@Oaphi Oaphi commented Jun 8, 2025

Actually closes #1579, likely closes #544

This PR builds on the work done in #1584 by preventing the buttons for creating a new comment thread or replying in an existent one from showing up if the user can't comment (not only when it's impossible to comment [specifically, when the user is not a mod or an admin & comments on the post are disabled, or it is locked or deleted]).

Most importantly, it adds 2 new site settings for rate-limiting comments:

  • RL_NewUserCommentsOwnPosts for new user comments on their own posts;
  • RL_CommentsOwnPosts for unrestricted user comments on their own posts;

Combined with the existing settings, users will now be limited to the following number of comments per day (by default):

  • new users: 0 comments on posts of others, 30 on their own posts & answers to them;
  • unrestricted users: 50 comments on posts of others, 50 on their own posts & answers to them;

Open to discussion on whether to adjust the number of comments users can make on their own posts per day.


The PR also ensures that the add thread / reply buttons are always shown to users but disabled if they can't comment for one reason or the other. The exact reason as to why a user can't comment is moved to the button's title (instead of littering the page with huge notices):

ssr-2025-06-21_15.35.22.mp4

It also makes possible to take actions on inline comment threads (comment editing & deleting included):

2025-06-25_23-17

ssr-2025-06-26_00.32.09.mp4

Plus, locked threads in post comment threads lists now show the lock icon (as do other "restricted" states):

2025-06-25_23-47

We also no longer display the "Showing X of Y comments. See the whole thread." widget if X is equal to Y (it simply wastes space in such cases):

2025-06-26_00-18


It also adds several scopes (and subsequently increases coverage across the project to make sure the new scopes don't break anything important):

model scopes

  • AuditLog.of_type(name) (only include logs of a specific type by name);
  • Comment.by(user) (only include ones made by the user);
  • Flag.by(user) (same as Comment.by);
  • Flag.declined (only include those that are handled as declined);
  • Flag.helpful (only include those that are handled as helpful);
  • ModWarning.active (only include warnings that are currently active);
  • ModWarning.to(user) (only include warnings issued to the user);
  • Post.bad (only include posts with score < 0.5);
  • Post.by(user) (same as Comment.by);
  • Post.good (only include posts with score > 0.5);
  • Post.in(category) (only include posts made in the category);
  • Post.on(community) (only include posts made on the community);
  • Post.parent_by(user) (only include posts that have a parent made by the user);
  • Post.problematic (only include posts with score < 0.25 or that are deleted);
  • PostHistory.by (same as Comment.by);
  • PostHistory.of_type(name) (same as AuditLog.of_type);
  • PostHistory.on_undeleted (only include events made on undeleted posts);
  • SuggestedEdit.approved (only include those that are decided as approved);
  • SuggestedEdit.by(user) (same as Comment.by);
  • SuggestedEdit.rejected (only include those that are decided as rejected);
  • Vote.by(user) (same as Comment.by);
  • Vote.for(user) (only include those where the target is the user);

concern (shared) scopes

  • Lockable.locked (only include ones that are locked);
  • Lockable.unlocked (the inverse of Lockable.locked);
  • SoftDeletable.deleted (only include ones that are marked as deleted);
  • SortDeletable.undeleted (the inverse of SoftDeletable.deleted);
  • Timestamped.newest_first (order by created_at DESC);
  • Timestamped.oldest_first (order by created_at ASC);
  • Timestamped.recent (only include ones made in the last 24 hours);

as well as removes some existing scopes:

  • CommunityUser.active (replaced by CommunityUser.undeleted to align with other soft-deletable models);
  • User.active (replaced by User.undeleted to align with other soft-deletable models);

It also removes check_edits_limit! as it's been unused since 412b490 (the check has been done inline for at least 4 years - we might want to abstract it into a similar helper).

Additionally, it adds a QoL []= class method to the SiteSetting model for us to be able to update site settings simply via SiteSetting[name] = value without having to explicitly handle value type conversion & cache updates.

Tangentially, it also adds two new methods:

  • max_votes_per_day that functions identically to max_comments_per_day (proper logic encapsulation);
  • recent_votes_count that functions similarly to recent_comments_count (unsure why we exclude self votes on parents but not votes on own posts there);

as well as renames some existing ones:

  • User#can_push_to_network to can_push_to_network? (to align with the naming convention for boolean checks);
  • User#can_update to can_update? (same as with can_push_to_network);
  • User#can_see_deleted? to can_see_deleted_posts? (the check only applies to posts);
  • User#has_ability_on to ability_on? (same as can_push_to_network);
  • User#has_profile_on to profile_on? (same as can_push_to_network);
  • User#is_moderator_on to moderator_on? (same as can_push_to_network);
  • ModeratorController#verify_can_see_deleted to verify_can_see_deleted_posts (same as with can_see_deleted?);

and a couple of shared concerns:

  • Inspectable that provides a generic inspect method that can be used on any model;
  • Lockable for logic shared by all models representing records that can be locked;
  • SoftDeletable for logic shared by all models representing soft-deletable records;
  • Timestamped for logic shared by all models representing timestamped records;
  • UserRateLimits that houses methods related to various rate limits (f.e., comments & votes per day);
  • UsernameValidations that encapsulates username-related validations (user model is too large);

Some extras:

  • Layout/SpaceInsideArrayLiteralBrackets is now enabled to match our style guide for Ruby;
  • assert_json_response_message(expected) test helper method for asserting that response body contains a given message;
  • assert_redirected_to_sign_in test helper method for asserting that response has the :found HTTP status code & redirects the requestor to sign in;

Related meta issue: meta:294088

@Oaphi Oaphi requested review from cellio, trichoplax, ArtOfCode- and a team June 8, 2025 09:42
@Oaphi Oaphi self-assigned this Jun 8, 2025
Copy link

codecov bot commented Jun 8, 2025

Codecov Report

Attention: Patch coverage is 99.70501% with 1 line in your changes missing coverage. Please review.

Project coverage is 69.14%. Comparing base (e0b731f) to head (5d92a41).
Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
app/helpers/comments_helper.rb 96.55% 1 Missing ⚠️
Additional details and impacted files
Components Coverage Δ
controllers 64.60% <100.00%> (+3.04%) ⬆️
helpers 70.46% <96.87%> (+1.40%) ⬆️
jobs 28.00% <ø> (ø)
models 85.09% <100.00%> (+3.26%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Oaphi Oaphi changed the title 0valt/1579/comments Prevent create comment / thread buttons from showing up if the user can't comment Jun 8, 2025
@Oaphi
Copy link
Member Author

Oaphi commented Jun 8, 2025

Leaving the coverage miss be as the logic there hasn't changed at all and both cases are covered already except for the audit log creation part, which definitely hasn't changed.

@Oaphi Oaphi marked this pull request as ready for review June 8, 2025 09:50
Copy link
Contributor

@trichoplax trichoplax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me (I've separately made a minor comment that should not hold up merging). I'm not adding approval because I'm not at a development machine for a few days so haven't tested.

Copy link
Member

@cellio cellio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a user can't comment (for the reasons in this PR), how is the message delivered to the user? I see the helper changes and the refactoring, but I don't see a view change involving the message. What am I missing?

@Oaphi
Copy link
Member Author

Oaphi commented Jun 10, 2025

If a user can't comment (for the reasons in this PR), how is the message delivered to the user?

Because it isn't since the button will no longer be present if the user can't comment due to either lacking the privilege or being rate-limited in addition to the current reasons. Showing a notice as with the locked posts / posts with comments disabled will likely be a bit too noisy. If my memory serves me right, we decided to switch to always show the button but disable it instead with a tooltip explaining the reason - if that's the case, we can address the issue when we get to it.

Open to ideas for an interim solution until the PR is merged

@Oaphi Oaphi marked this pull request as draft June 10, 2025 04:18
@Oaphi
Copy link
Member Author

Oaphi commented Jun 10, 2025

Switching back to draft as we've decided to apply global rate limits regardless of whether the user is the post owner.

Oaphi added 19 commits June 27, 2025 20:32
…?, the response will at least have a generic msg
Comment on lines +3 to +6
@each $key, $value in $font-sizes {
.font-#{$key} {
font-size: $value;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have font size utilities available from Co-Design - are they not working for something here? If that's the case we should log an issue for it there and re-consider how they work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, unfortunately, they are not - as you can see in the typography section, these utilities are in absolute units and do not match our usage in QPixel (the latter is the primary reason for the additions, though), so I ended up gathering font-size values scattered across QPixel's rulesets in one map.

These utilities should be moved to Co-Design, however, I feel like doing so now will end up stalling the PR for quite a while, so I'd keep them here until we can spare some bandwidth to move them over

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer we file an issue for moving this to Co-Design and not try to do it as part of this PR, which is already large. I don't want to risk more delays.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, not suggesting we add more work here. Let's file it for later.

Comment on lines +366 to +376
@each $side in $sides {
.border-#{$side}-none {
border-#{$side}: none;
}
}

@each $align in $text-aligns {
.text-#{$align} {
text-align: $align;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, these feel like changes that should be part of Co-Design. Perhaps not work for this PR, but we should log the missing utilities so that they can be added later.

Copy link
Member Author

@Oaphi Oaphi Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

text-(left|right|center) can probably go - I missed that the text alignment utils are described in the typography section of Co-Design (that said, I heavily prefer the shorter WindiCSS/UnoCSS style text-center and the like to has-text-align-center, which is extremely verbose for what it does [but that's an overall issue with our utilities that needs to be discussed separately])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly for border-(top|right|bottom|left)-none, although I was aware of the utilities in that case, it's just that has-border-top-style-none is even more verbose than the text align one.

Co-authored-by: ArtOfCode <ArtOfCode-@users.noreply.github.com>
@Oaphi
Copy link
Member Author

Oaphi commented Jun 30, 2025

Note to folks stumbling on the PR: it's 99% done, feel free to review even though it's not marked as ready yet to avoid wasting time, the PR is massive. I only plan on adding a couple of tests and translation strings

@Oaphi Oaphi force-pushed the 0valt/1579/comments branch from d76a26b to 178aaf3 Compare June 30, 2025 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants