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

Handle displaying of deleted and removed posts/comments (fixes #2624) #3286

Merged
merged 15 commits into from
Jul 20, 2023

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented Jun 23, 2023

Makes it so that deleted and removed posts/comments are only shown on the creator's profile page in the following cases:

  • When user views his own profile, he can view all posts/comments which he deleted himself (but not those which were deleted by mod actions)
  • When admin views user profile, he can see all posts/comments which were deleted by mod actions (but not those deleted by the user himself)

The existing code would also show deleted/removed posts and comments to admins/mods in post listings and search results, I dont think thats desirable.

@Nutomic Nutomic marked this pull request as draft June 23, 2023 10:55
@Nutomic Nutomic force-pushed the hide-removed-deleted-posts branch from 69a1ad0 to dde98f5 Compare June 23, 2023 12:53
@Nutomic Nutomic changed the title Hide deleted and removed posts from listings (fixes #3285) Handle displaying of deleted and removed posts/comments (fixes #2624) Jun 23, 2023
query = query
.filter(community::removed.eq(false))
.filter(post::removed.eq(false));
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Its a bit unclear what to do if the community was removed/deleted, we can revisit this if issues are reported in the future.

Copy link
Member

Choose a reason for hiding this comment

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

The difficulty with this one, and why this really needs a SQL solution (that's better than my old one), is that the posts fetched are a collection of posts that might be yours + others.

With admins and mods, we can just show all deleted and removed. When its the creator, we need to do a join + filter.

Copy link
Member Author

Choose a reason for hiding this comment

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

With the current usage this is no problem as show_removed and show_deleted are only set on the user profile, so it will only include posts authored by that user. But if it ever gets set from somewhere else that will cause problems. As an admin I also dont want to see deleted and removed posts all over the frontpage, its pretty annoying.

As a kind of solution/workaround we could add a check that show_removed/show_deleted can only be true if creator_id is Some?

Copy link
Member

Choose a reason for hiding this comment

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

As a kind of solution/workaround we could add a check that show_removed/show_deleted can only be true if creator_id is Some?

That creator_id filter is only called from the profile pages, so that means /home and /community would show deleted posts.

Copy link
Member Author

Choose a reason for hiding this comment

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

No because !self.show_removed.unwrap_or(false) with show_removed = None evaluates to !false which is true so the code in if is executed. Admittedly its confusing but its also covered by tests.

I changed the code now to avoid explicit show_removed/show_deleted parameters. If this looks better to you, I will also fix the tests.

Copy link
Member

Choose a reason for hiding this comment

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

Yes it does look better.

@Nutomic Nutomic marked this pull request as ready for review June 23, 2023 12:57
@Nutomic Nutomic force-pushed the hide-removed-deleted-posts branch from dde98f5 to 05830f5 Compare June 26, 2023 09:28
@Nutomic Nutomic force-pushed the hide-removed-deleted-posts branch from b3241cf to 15f3449 Compare June 29, 2023 10:08
Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Once you get the tests passing, feel free to merge.

query = query
.filter(community::removed.eq(false))
.filter(post::removed.eq(false));
}
Copy link
Member

Choose a reason for hiding this comment

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

Yes it does look better.

@Nutomic Nutomic requested a review from phiresky as a code owner July 14, 2023 11:43
@Nutomic
Copy link
Member Author

Nutomic commented Jul 14, 2023

  • did some cleanup for tests but not finished yet (still failing)
  • changed it so that a user can always see their own deleted posts, not only in profile
  • might be useful to show removed posts/comments in moderator view but not sure about this (Add moderator view parameter to list posts #3176)

@dessalines
Copy link
Member

Clippy failing.

@Nutomic Nutomic marked this pull request as draft July 17, 2023 11:12
@Nutomic
Copy link
Member Author

Nutomic commented Jul 17, 2023

Need to merge #3613 first.

Edit: actually not, because this PR doesnt change PostView::read. So this is ready to merge

@Nutomic Nutomic marked this pull request as ready for review July 17, 2023 12:34
@Nutomic
Copy link
Member Author

Nutomic commented Jul 19, 2023

Added another change to allow returning own deleted posts from PostView::read, which makes #3613 obsolete. I tested to confirm that its working for unauthenticated users.

@Nutomic Nutomic merged commit 047db9a into main Jul 20, 2023
0 of 2 checks passed
Nutomic added a commit that referenced this pull request Jul 21, 2023
Forgot to remove this in #3286
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