-
-
Notifications
You must be signed in to change notification settings - Fork 886
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
update moderator view #3820
Conversation
Note: Internally, the listing type is called ListingType.ModeratorView, but it's called "Moderator View" in the api endpoint
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 |
crates/db_schema/src/lib.rs
Outdated
@@ -115,6 +115,9 @@ pub enum ListingType { | |||
Local, | |||
/// Content only from communities you've subscribed to. | |||
Subscribed, | |||
#[serde(rename = "Moderator View")] |
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.
Get rid of this.
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.
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.
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.
Renaming is more confusing, plus that code is auto-generated and Im not sure it can handle the serde rename correctly.
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.
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.
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.
Yep, definitely remove this renaming. If necessary I can do an rc
build of lemmy-js-client. Ping me if / when you need that.
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.
@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.
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). |
Yeah it was just an idea, maybe it doesnt make sense. |
crates/db_schema/src/lib.rs
Outdated
@@ -115,6 +115,9 @@ pub enum ListingType { | |||
Local, | |||
/// Content only from communities you've subscribed to. | |||
Subscribed, | |||
#[serde(rename = "Moderator View")] |
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.
Yep, definitely remove this renaming. If necessary I can do an rc
build of lemmy-js-client. Ping me if / when you need that.
api_tests/package.json
Outdated
@@ -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", |
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.
Fine for now I spose, but I'll have to merge that in lemmy-js-client and do another release.
api_tests/src/community.spec.ts
Outdated
|
||
// 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; |
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.
Will need to change to ModeratorView
@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. |
@biosfood it looks like 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 |
I deployed a lemmy-js-client now with this addition: |
Wow. Thanks a lot for the help |
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.
No probs! Thank you for spending time on this one. Once @Nutomic approves, we'll get this merged.
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.