-
-
Notifications
You must be signed in to change notification settings - Fork 863
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
Api edit separation. #997
Api edit separation. #997
Conversation
Okay this is finally ready to go, damn it took a lot of work. Check #686 for everything this added, but it was new endpoints for:
As well as a lot of general API cleanup, which I updated the docs for too. |
There is a lot of copy pasted code in the API. A lot of that should be really easy to extract into helper functions. Please do that, cause otherwise this will be impossible to maintain. The backend code looks generally fine otherwise, and this is definitely a better approach than before. Tests are also passing fine. Havent looked at the frontend (neither tested it). |
- This adds a form_id to CreateComment, EditComment, and CommentResponse - This is so any front end clients can add a randomly generated string, and know which comment they submitted, is the one they're getting back. - This gets rid of all the weird complicated logic in handleFinished(), and should stop the comment forms getting cleared once and for all.
server/src/api/comment.rs
Outdated
.await??; | ||
if !mods_and_admins.contains(&user_id) { | ||
.await?; | ||
if !is_mod_or_admin { | ||
return Err(APIError::err("not_an_admin").into()); | ||
} |
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 isnt what I meant, I was talking about a function that also handles the if condition. So all you would need to put here is is_mod_or_admin(user_id)?
. If its a mod or admin, it does nothing, otherwise it returns an error.
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.
I'm not sure what you mean. I can't use a ?
, I have to match no matter what, so I can return the APIError
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 is what I mean:
async fn is_mod_or_admin(pool: &DbPool, user_id: i32, community_id: i32) -> Result<(), LemmyError> {
let is_mod_or_admin = blocking(pool, move |conn| {
Community::is_mod_or_admin(conn, user_id, community_id)
})
.await?;
if !is_mod_or_admin {
return Err(APIError::err("not_an_admin").into());
}
return Ok(())
}
Then for the actual check in an API call, you just need to do is_mod_or_admin(pool, user_id, community_id).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.
Okay I added helpers like this for is_mod_or_admin
, and another for is_admin
- Extracted methods for is_mod_or_admin, and is_admin. - Removed admins from GetPostResponse and GetCommunityResponse. - Some cleanup.
Great, thanks :) |
Fixes #686