-
-
Notifications
You must be signed in to change notification settings - Fork 888
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
Add community reports (only the database part) #4996
base: main
Are you sure you want to change the base?
Conversation
…t_combined_view::tests::test_community_reports
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.
Looks great, thx for adding it to combined reports also!
The only thing I see, is that you don't need the community_report_view.rs
file: all the fetching is done through the report combined view.
pub original_community_icon: Option<String>, | ||
#[cfg_attr(feature = "full", ts(optional))] | ||
pub original_community_banner: Option<String>, |
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.
Not sure if the icon and banner are totally necessary, but no reason not to have them I spose.
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 I understand correctly, reports store the original content so the creator of the reported thing can't edit it to remove evidence of the offense. It's possible that a community's icon or banner is the reason for reporting, just like the url of a post.
.first(conn) | ||
.await | ||
} | ||
} |
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.
This should be removed, as now we have the ReportCombinedView
. If you need assistance with that tho, I can help, but since the combined reports have been merged now, we should make sure it includes the community reports.
EDIT: Oh I see below you did add it. In that case just remove this file, since everything is done through the combined reports endpoint now.
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.
fixed
|
||
#[tokio::test] | ||
#[serial] | ||
async fn test_community_reports() -> LemmyResult<()> { |
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.
Excellent, nice job!
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.
Looks good. Looks like only the code for api and federation are missing now. Both should be relatively easy to add based on the other report implementations (in a separate PR if you prefer).
Need to fix conflicts here |
first part of implementing #4897