General improvements to comment threads#1647
Conversation
…ments a user has made in 24H (excl. own posts & responses)
…ments the user can make daily
…ew comments on a given post
… as in the comments controller
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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. |
trichoplax
left a comment
There was a problem hiding this comment.
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.
cellio
left a comment
There was a problem hiding this comment.
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?
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 |
|
Switching back to draft as we've decided to apply global rate limits regardless of whether the user is the post owner. |
74015c6 to
6d201dc
Compare
…t mod-only error msg
…olves the size problem)
|
Important note: |
|
Probably won't move it from the draft status for a couple more days (want to improve coverage after the latest changes first), but I believe it's ready otherwise. Made re-review requests as there's been quite a few changes since the initial approvals (mostly what's listed in the finish line comment) |
I'll test the things in the finish-line comment. I assume someone else will review the code changes. |
…ate_access action callback & added tests for it
|
Testing notes based on checklist:
Confirmed working:
|
99afba6 to
26e791b
Compare
|
That fixed it, thanks! (Tested Chrome and Firefox, though it doesn't sound like this was browser-dependent anyway.) |
Actually closes #1579, likely closes #544, definitely closes #1670
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_NewUserCommentsOwnPostsfor new user comments on their own posts;RL_CommentsOwnPostsfor 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):
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):
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):
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):
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 byname);Comment.by(user)(only include ones made by theuser);Flag.by(user)(same asComment.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 theuser);Post.bad(only include posts with score < 0.5);Post.by(user)(same asComment.by);Post.good(only include posts with score > 0.5);Post.in(category)(only include posts made in thecategory);Post.on(community)(only include posts made on thecommunity);Post.parent_by(user)(only include posts that have a parent made by theuser);Post.problematic(only include posts with score < 0.25 or that are deleted);PostHistory.by(same asComment.by);PostHistory.of_type(name)(same asAuditLog.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 asComment.by);SuggestedEdit.rejected(only include those that are decided as rejected);Vote.by(user)(same asComment.by);Vote.for(user)(only include those where the target is theuser);concern (shared) scopes
Lockable.locked(only include ones that are locked);Lockable.unlocked(the inverse ofLockable.locked);SoftDeletable.deleted(only include ones that are marked as deleted);SortDeletable.undeleted(the inverse ofSoftDeletable.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 byCommunityUser.undeletedto align with other soft-deletable models);User.active(replaced byUser.undeletedto 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 theSiteSettingmodel for us to be able to update site settings simply viaSiteSetting[name] = valuewithout having to explicitly handle value type conversion & cache updates.Tangentially, it also adds two new methods:
max_votes_per_daythat functions identically tomax_comments_per_day(proper logic encapsulation);recent_votes_countthat functions similarly torecent_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_networktocan_push_to_network?(to align with the naming convention for boolean checks);User#can_updatetocan_update?(same as withcan_push_to_network);User#can_see_deleted?tocan_see_deleted_posts?(the check only applies to posts);User#has_ability_ontoability_on?(same ascan_push_to_network);User#has_profile_ontoprofile_on?(same ascan_push_to_network);User#is_moderator_ontomoderator_on?(same ascan_push_to_network);ModeratorController#verify_can_see_deletedtoverify_can_see_deleted_posts(same as withcan_see_deleted?);and a couple of shared concerns:
Inspectablethat provides a genericinspectmethod that can be used on any model;Lockablefor logic shared by all models representing records that can be locked;SoftDeletablefor logic shared by all models representing soft-deletable records;Timestampedfor logic shared by all models representing timestamped records;UserRateLimitsthat houses methods related to various rate limits (f.e., comments & votes per day);UsernameValidationsthat encapsulates username-related validations (user model is too large);Some extras:
Layout/SpaceInsideArrayLiteralBracketsis 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_intest helper method for asserting that response has the:foundHTTP status code & redirects the requestor to sign in;Related meta issue: meta:294088