Skip to content
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

[Posts]: Hide disabled comments info better #507

Merged
merged 4 commits into from
May 10, 2023

Conversation

DonovanDMC
Copy link
Member

@DonovanDMC DonovanDMC commented Apr 16, 2023

Currently, there's a bit of a mix between making disabled comments hard to find, and making them easy to find, as well as leaking a bit of information disabling comments should hide.

This pr:

  • Hides the number of comments for non moderators (shows 0), both in the posts list, as well as in the json api
  • Removes the comment_disabled flag in api responses
  • Excludes the comment_disabled and comment_enabled post events from non-moderators, as this makes it very easy to find posts with disabled comments

In theory the comment count can still be sniffed out with a brute force id:(...) comment_count:, but I don't think there should really be any concern over that.

@DonovanDMC DonovanDMC force-pushed the better-hide-disabled-comments branch from d04e374 to 8135b62 Compare May 7, 2023 21:08
@DonovanDMC DonovanDMC force-pushed the better-hide-disabled-comments branch from 8135b62 to f462356 Compare May 7, 2023 21:08
@Earlopain
Copy link
Collaborator

Just get rid of the api field entirely. Not sure what I was thinking including that

@DonovanDMC
Copy link
Member Author

Just get rid of the api field entirely. Not sure what I was thinking including that

I had debated doing that, but didn't want to do a breaking change. Though seeing as that isn't a field really anyone should be relying on, I'm realizing that would have been fine.

@DonovanDMC
Copy link
Member Author

The test didn't fail locally
ughhh

@Earlopain
Copy link
Collaborator

I was more meaning something like

def visible_comment_count(user)
  (user.is_moderator? || !is_comment_disabled) ? comment_count : 0
end

Also not really relevant to this, but I'm trying to at least not introduce new dependencies on CurrentUser in anything but controllers. So passing the user into the method is preferable, even if the caller still uses CurrentUser in a non-desireable location. CurrentUser is only set when the controller callbacks fire which messes with a bunch of stuff in jobs, tests and the console.

@DonovanDMC DonovanDMC force-pushed the better-hide-disabled-comments branch from 37908a8 to 9c7f896 Compare May 10, 2023 08:50
@DonovanDMC
Copy link
Member Author

I was more meaning something like

def visible_comment_count(user)
  (user.is_moderator? || !is_comment_disabled) ? comment_count : 0
end

Oh, sorry. I interpreted that differently, as you saw. I've changed it to that now.

@Earlopain Earlopain merged commit 6100f8f into e621ng:master May 10, 2023
3 checks passed
@DonovanDMC DonovanDMC deleted the better-hide-disabled-comments branch May 10, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants