Remove "create new comment thread" if it is not possible to add a comment in the first place (+ huge refactor of access control-related methods)#1584
Conversation
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:
|
Oaphi
changed the title
|
Are we also intercepting attempts to leave a new comment in an existing thread? |
Actually, we already do, we were only missing a similar check in the inline view:
|
…ost has comments disabled
…ther admin nor mod
ArtOfCode-
left a comment
ArtOfCode-
left a comment
There was a problem hiding this comment.
One comment for consideration but otherwise LGTM
There was a problem hiding this comment.
Various places in the code have had redundancy due to the somewhat misleadingly named is_moderator actually meaning "is a moderator or an admin" (since admins can do everything moderators can do). Over time I've tried to remove that redundancy by removing || is_admin from places I've worked on (since it's already covered by is_moderator), but I can now see that it is still potentially confusing so perhaps we should make the naming more clear.
I've added comments to specific places where I'd expect confusion to occur in future.
I don't know the best way to rename the functions but I think it's worth a discussion before committing to something new. The is_moderator function effectively means "has moderator powers" but I'm not sure that's a better name.
…el & switched recalc_abilities_upon_first_migration to it
|
This is approved (with some suggestions still pending), but per discussion in chat, do not merge yet. (The author would like to participate in the merge.) |
…user instead of helpers
…g at_least_moderator? on current_user instead of using an application helper
…odel for consistency
…e are now consistently prefixed with 'is_'
|
Should be good to go now, I believe I've addressed everything I wanted to |
trichoplax
left a comment
trichoplax
left a comment
There was a problem hiding this comment.
As someone who has worked on some of the affected code in the past and struggled, this feels much more comfortable to read now. Thank you!
There are a few places where user.standard? has been used to replace !user.at_least_moderator?. For consistency, would it make sense to use this in more places? I've added a few suggested places, but please note I don't have a dev environment at present so I haven't tested anything.
trichoplax
left a comment
trichoplax
left a comment
There was a problem hiding this comment.
Marking approved to avoid holding this up. Note that I've only read the code, not being able to test locally at present.
…fer to global_admin? & global_moderator? columns on the parent user (instead of columns)
Co-authored-by: trichoplax <[email protected]>
This reverts commit f77bca0.
Co-authored-by: trichoplax <[email protected]>
… on the community user model Co-authored-by: trichoplax <[email protected]>
…munity user model Co-authored-by: trichoplax <[email protected]>
Co-authored-by: trichoplax <[email protected]>
…dard?` Co-authored-by: trichoplax <[email protected]>
4 participants
closes #1579
Also introduces a couple of QoL methods on the User model:is_standardfor checking if the current user is neither an admin nor a moderator, as well as its inverseis_privileged(we should rename all of theis_checks to follow the?convention, but this is outside the scope of this PR).Also nearly fully refactors access control methods to disambiguate their usage & reduce duplication (such as "is moderator or admin" and related checks).