-
-
Notifications
You must be signed in to change notification settings - Fork 69
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
base: develop
Are you sure you want to change the base?
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 ReportAttention: Patch coverage is
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. |
There was a problem hiding this 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.
There was a problem hiding this 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?
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. |
…audit private method
…ctly calling community_user.privilege
…ions (locking, deleting, undeleting, etc)
…reads deleted by one of them
…ch of missing cases
…?, the response will at least have a generic msg
…ally & fixed the related test
@each $key, $value in $font-sizes { | ||
.font-#{$key} { | ||
font-size: $value; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@each $side in $sides { | ||
.border-#{$side}-none { | ||
border-#{$side}: none; | ||
} | ||
} | ||
|
||
@each $align in $text-aligns { | ||
.text-#{$align} { | ||
text-align: $align; | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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])
There was a problem hiding this comment.
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>
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 |
- comments_post_error_msg; - comments_thread_error_msg;
d76a26b
to
178aaf3
Compare
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):
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.undeleted
to align with other soft-deletable models);User.active
(replaced byUser.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 theSiteSetting
model for us to be able to update site settings simply viaSiteSetting[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 tomax_comments_per_day
(proper logic encapsulation);recent_votes_count
that 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_network
tocan_push_to_network?
(to align with the naming convention for boolean checks);User#can_update
tocan_update?
(same as withcan_push_to_network
);User#can_see_deleted?
tocan_see_deleted_posts?
(the check only applies to posts);User#has_ability_on
toability_on?
(same ascan_push_to_network
);User#has_profile_on
toprofile_on?
(same ascan_push_to_network
);User#is_moderator_on
tomoderator_on?
(same ascan_push_to_network
);ModeratorController#verify_can_see_deleted
toverify_can_see_deleted_posts
(same as withcan_see_deleted?
);and a couple of shared concerns:
Inspectable
that provides a genericinspect
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