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

update moderator view #3820

Merged
merged 29 commits into from
Aug 31, 2023
Merged

update moderator view #3820

merged 29 commits into from
Aug 31, 2023

Conversation

biosfood
Copy link
Contributor

@biosfood biosfood commented Aug 4, 2023

In the PR LemmyNet/lemmy-ui#1993, many code owners commented that the moderator view should be implemented as a listing type instead of a separate query parameter.

  • use moderator view as a listing type
  • support for moderator view when listing comments

@biosfood
Copy link
Contributor Author

biosfood commented Aug 5, 2023

It would be great to have a new lemmy-js-client release for the changes as well to fix the ugly url in package.json

@@ -115,6 +115,9 @@ pub enum ListingType {
Local,
/// Content only from communities you've subscribed to.
Subscribed,
#[serde(rename = "Moderator View")]
Copy link
Member

Choose a reason for hiding this comment

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

Get rid of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In LemmyNet/lemmy-js-client#172, I added the listing type as "Moderator View". Since a space in the enum name in the rust code would be impossible / very ugly, I used the rename attribute.

Should the listing type be renamed to "ModeratorView" or just "Moderator"?

Just removing this line will lead to the listing type "Moderator View" not being recognized as ModeratorView.

Copy link
Member

Choose a reason for hiding this comment

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

Renaming is more confusing, plus that code is auto-generated and Im not sure it can handle the serde rename correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, this will need another change to "ModeratorView" in https://github.com/LemmyNet/lemmy-js-client/blob/main/src/types/ListingType.ts then. Should I open another PR there or will the file automatically be updated when these changes here are merged?

Sorry for the inconvenience, I tried to be clever and update lemmy-js-client first so that the api tests could be rewritten to fit the new implementation without needing to wait for another PR in the middle of development. I didn't give much thought to the space character in the string until trying to make the backend work.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, definitely remove this renaming. If necessary I can do an rc build of lemmy-js-client. Ping me if / when you need that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dessalines If you could change "Moderator View" to "ModeratorView", somewhat reverting the changes in 981c866930de1552338f81d43b5c01431f480759 and create a new official release (candidate), that would be great.

@Nutomic
Copy link
Member

Nutomic commented Aug 7, 2023

We can also consider showing removed/deleted posts in the moderator view.

@biosfood
Copy link
Contributor Author

biosfood commented Aug 7, 2023

We can also consider showing removed/deleted posts in the moderator view.

It would certainly be possible but I would question the utility of that feature. If a post has been deleted, it does no longer need to be looked at by a moderator, right? (Maybe in an appeal to revoke the removal but that is not relevant here).

@Nutomic
Copy link
Member

Nutomic commented Aug 7, 2023

Yeah it was just an idea, maybe it doesnt make sense.

@biosfood biosfood marked this pull request as ready for review August 11, 2023 13:16
@@ -115,6 +115,9 @@ pub enum ListingType {
Local,
/// Content only from communities you've subscribed to.
Subscribed,
#[serde(rename = "Moderator View")]
Copy link
Member

Choose a reason for hiding this comment

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

Yep, definitely remove this renaming. If necessary I can do an rc build of lemmy-js-client. Ping me if / when you need that.

@@ -19,7 +19,7 @@
"eslint": "^8.40.0",
"eslint-plugin-prettier": "^4.0.0",
"jest": "^29.5.0",
"lemmy-js-client": "0.18.3-rc.3",
"lemmy-js-client": "https://github.com/LemmyNet/lemmy-js-client#f165ef6538f6f471a923d412665d01d0e46de9b3",
Copy link
Member

Choose a reason for hiding this comment

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

Fine for now I spose, but I'll have to merge that in lemmy-js-client and do another release.


// in moderator view, alpha should not see otherPost, wich was posted on a community alpha doesn't moderate
posts = (await getPosts(alpha, true)).posts;
posts = (await getPosts(alpha, "Moderator View")).posts;
Copy link
Member

Choose a reason for hiding this comment

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

Will need to change to ModeratorView

@biosfood
Copy link
Contributor Author

@dessalines If you could change "Moderator View" to "ModeratorView", somewhat reverting the changes in 981c866930de1552338f81d43b5c01431f480759 and create a new official release (candidate), that would be great.

@dessalines
Copy link
Member

dessalines commented Aug 29, 2023

@biosfood it looks like ModeratorView never got added to the listing_type_enum in postgres as part of this PR, which is required for me to generate it correctly.

It's kind of complicated (postgres doesn't let you easily add and remove enums) so I'll add it now to this PR.

Also, in the future, use a feature branch rather than biosfood:main. Otherwise you won't be able to add other features easily.

@dessalines
Copy link
Member

I deployed a lemmy-js-client now with this addition: 0.19.0-rc.3

@biosfood
Copy link
Contributor Author

Wow. Thanks a lot for the help

@dessalines dessalines requested a review from Nutomic August 29, 2023 22:46
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.

No probs! Thank you for spending time on this one. Once @Nutomic approves, we'll get this merged.

@Nutomic Nutomic merged commit 384e55f into LemmyNet:main Aug 31, 2023
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.

3 participants