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

Add Modlog Filters #2313

Merged
merged 6 commits into from
Aug 16, 2022
Merged

Add Modlog Filters #2313

merged 6 commits into from
Aug 16, 2022

Conversation

makotech222
Copy link
Collaborator

@makotech222 makotech222 commented Jun 13, 2022

Addresses issue: LemmyNet/lemmy-ui#555

In summary, this PR does the following:

  • (minor) fixes running on windows build
  • Adds three new filter inputs to the Modlog: Action, Mod, and User
  • Adds new setting to Site: Hide modlog mod names (default true). Hides the mod names on the mod log page (front and backend)
  • Adds a generic dropdown component for general use (derived from the navbar user dropdown, could eventually be refactored to use new component)

Associated PRs:

image
image

Site:
image

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.

Check out the search perform impl, this should look very similar to that.

crates/api_common/src/site.rs Outdated Show resolved Hide resolved
crates/api/src/site/mod_log.rs Outdated Show resolved Hide resolved
crates/db_views_actor/src/person_view.rs Outdated Show resolved Hide resolved
@Nutomic
Copy link
Member

Nutomic commented Jun 17, 2022

About general Rust usage, cargo clippy should give you some hints. Also make sure that CI passes. You can look at .drone.yml for the exact commands it uses, so you can run them locally.

@makotech222 makotech222 marked this pull request as draft June 17, 2022 19:01
@makotech222 makotech222 force-pushed the modlog-filters branch 4 times, most recently from 61b4ac5 to 7d1a93e Compare June 19, 2022 00:16
@makotech222
Copy link
Collaborator Author

makotech222 commented Jun 19, 2022

@dessalines Okay i'm in a pretty good state now. I have two questions before I convert back to PR: Is there an established way to get just admins+mods from the api? And also, Should mods be able to view modlog names like admins would?

I'm currently just getting all users for the 'filter by mod' feature, which isn't entirely correct, but works if you search and select a mod user. Also not sure if mods should be able to view another mods actions that may be in a different community.

@Nutomic
Copy link
Member

Nutomic commented Jun 21, 2022

What do you mean "get admins and mods from api"? If you want to check that a given person is a mod/admin, use is_mod_or_admin() function from crates/api_common/src/utils.rs. I think mods should be able to see names of other mods from the same community in the modlog, but not when the modlog item is in another community.

crates/api/src/site/mod_log.rs Outdated Show resolved Hide resolved
crates/api/src/site/mod_log.rs Outdated Show resolved Hide resolved
crates/api/src/site/mod_log.rs Outdated Show resolved Hide resolved
crates/api_common/src/site.rs Outdated Show resolved Hide resolved
crates/db_views_moderator/src/admin_purge_comment_view.rs Outdated Show resolved Hide resolved
crates/db_views_moderator/src/mod_add_community_view.rs Outdated Show resolved Hide resolved
crates/db_views_moderator/src/structs.rs Show resolved Hide resolved
@makotech222 makotech222 force-pushed the modlog-filters branch 2 times, most recently from 8a202ef to 369d42a Compare July 12, 2022 01:48
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.

Its looking pretty close! I'll take a look at the front end ones now. lmk when you're ready for me to test these.

crates/db_views_moderator/src/admin_purge_post_view.rs Outdated Show resolved Hide resolved
crates/db_views_moderator/src/admin_purge_person_view.rs Outdated Show resolved Hide resolved
crates/api/src/site/mod_log.rs Outdated Show resolved Hide resolved
crates/api/src/site/mod_log.rs Outdated Show resolved Hide resolved
crates/api/src/site/mod_log.rs Outdated Show resolved Hide resolved
crates/api/src/site/mod_log.rs Outdated Show resolved Hide resolved
crates/api/src/site/mod_log.rs Outdated Show resolved Hide resolved
@makotech222 makotech222 marked this pull request as ready for review July 26, 2022 02:56
@makotech222
Copy link
Collaborator Author

Okay give this all another look.

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.

We try to review these as quickly as we can, so I apologize if I'm slow there, but it was 14 days since your last update.

crates/api/src/site/mod_log.rs Outdated Show resolved Hide resolved
crates/api/src/site/mod_log.rs Outdated Show resolved Hide resolved
crates/api/src/site/mod_log.rs Outdated Show resolved Hide resolved
@makotech222
Copy link
Collaborator Author

Okay resolved those issues

crates/api/src/site/mod_log.rs Outdated Show resolved Hide resolved
let is_admin = is_admin(local_user_view.as_ref().expect("")).is_ok();
let local_person_id = match local_user_view {
Some(s) => s.person.id,
None => PersonId(-1),
Copy link
Member

Choose a reason for hiding this comment

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

Why are you setting these to -1? They should be left as options so you can catch the None case. Also this is a better syntax:

let local_person_id = local_user_view.map(|l| l.person.id);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is only used to clean up the syntax around calling 'is_mod_or_admin' which requires a PersonId.

crates/api/src/site/mod_log.rs Show resolved Hide resolved
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.

Looks good, but I'll have @Nutomic take a look too.

crates/api/src/site/mod_log.rs Show resolved Hide resolved
crates/api/src/site/mod_log.rs Outdated Show resolved Hide resolved
@Nutomic
Copy link
Member

Nutomic commented Jul 28, 2022

Other than my last comment it looks good to me. Once that is changed we can merge.

@dessalines
Copy link
Member

@makotech222 Have you had time to look at these? The merge conflicts will start to pile up the longer this waits here.

@makotech222
Copy link
Collaborator Author

Yep on my things to do list for this weekend

@makotech222
Copy link
Collaborator Author

Okay should be good now

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.

Nutomic should be able to take a look when they get back from vacay.

@Nutomic Nutomic enabled auto-merge (squash) August 16, 2022 11:50
@Nutomic Nutomic disabled auto-merge August 16, 2022 11:51
@Nutomic Nutomic merged commit 21455d6 into LemmyNet:main Aug 16, 2022
@Nutomic
Copy link
Member

Nutomic commented Aug 16, 2022

Thank you for the contribution :)

@makotech222 makotech222 deleted the modlog-filters branch February 25, 2023 15:21
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

3 participants