wip/kick #36

baaboe · 2025-07-22T15:40:08Z

baaboe commented

2025-07-22 15:40:08 +00:00

Added the member endpoint, with a get and a delete(kick) request method

baaboe added 3 commits 2025-07-22 15:40:08 +00:00

New endpoint 'members' with get and delete

ci/woodpecker/push/build-and-publish Pipeline was successful

Details

82f4388dab

path name fix

ci/woodpecker/push/build-and-publish Pipeline failed

Details

4ec36c1cda

more path name fix

ci/woodpecker/push/build-and-publish Pipeline was successful

Details

ci/woodpecker/pr/build-and-publish Pipeline was successful

Details

228bc68327

radical requested changes 2025-07-22 15:54:46 +00:00

radical left a comment

Needs changes

src/api/v1/member/mod.rs Outdated

					
				@ -0,0 +10,4 @@

				mod uuid;

				pub fn router(app_state: Arc<AppState>) -> Router<Arc<AppState>> {

radical commented

2025-07-22 15:47:59 +00:00

this router should not have a layer and merging it with an empty router is pointless here, function body should be written as:

Router::new()
        .route("/{uuid}", get(uuid::get))
        .route("/{uuid}", delete(uuid::delete))

and the function should not need the app_state parameter

this router should not have a layer and merging it with an empty router is pointless here, function body should be written as: ```rs Router::new() .route("/{uuid}", get(uuid::get)) .route("/{uuid}", delete(uuid::delete)) ``` and the function should not need the `app_state` parameter

radical marked this conversation as resolved

src/api/v1/member/uuid/mod.rs Outdated

					
				@ -0,0 +38,4 @@

				    State(app_state): State<Arc<AppState>>,

				    Path(member_uuid): Path<Uuid>,

				    Extension(CurrentUser(uuid)): Extension<CurrentUser<Uuid>>,

				) -> Result<impl IntoResponse, Error> {

radical commented

2025-07-22 15:50:23 +00:00

instead of calling app_state.pool.get().await? on multiple functions you should define it and use the same one multiple times

let mut conn = app_state.pool.get().await?;

let me = Me::get(&mut conn, uuid).await?;

let member = Member::fetch_one_with_member(&app_state, &me, member_uuid).await?;

let deleter = Member::check_membership(&mut conn, uuid, member.guild_uuid).await?;
    
deleter.check_permission(&app_state, Permissions::ManageMember).await?;

member.delete(&mut conn).await?;

instead of calling `app_state.pool.get().await?` on multiple functions you should define it and use the same one multiple times ```rs let mut conn = app_state.pool.get().await?; let me = Me::get(&mut conn, uuid).await?; let member = Member::fetch_one_with_member(&app_state, &me, member_uuid).await?; let deleter = Member::check_membership(&mut conn, uuid, member.guild_uuid).await?; deleter.check_permission(&app_state, Permissions::ManageMember).await?; member.delete(&mut conn).await?; ```

radical marked this conversation as resolved

src/api/v1/mod.rs Outdated

					
				@ -30,2 +31,3 @@

				        .nest("/auth", auth::router(app_state.clone()))

				        .nest("/channels", channels::router(app_state))

				        .nest("/channels", channels::router(app_state.clone()))

				        .nest("/member", member::router(app_state))

radical commented

2025-07-22 15:53:27 +00:00

this should be moved to the router_with_auth instead as all endpoints inside /members needs auth, also it should be /members plural and not /member singular. This also goes for the folder/module name

this should be moved to the router_with_auth instead as all endpoints inside /members needs auth, also it should be `/members` plural and not `/member` singular. This also goes for the folder/module name

baaboe commented

2025-07-22 15:59:15 +00:00

why plural, we are only accessing a single member

sauceyred commented

2025-07-22 16:06:49 +00:00

@baaboe wrote in #36 (comment):

why plural, we are only accessing a single member

Because it’s accessing a singular resource from the members resource collection. It’s a REST API thing.

@baaboe wrote in https://git.gorb.app/gorb/backend/pulls/36#issuecomment-831: > why plural, we are only accessing a single member Because it’s accessing a singular resource from the `members` resource *collection*. It’s a REST API thing.

radical commented

2025-07-22 16:21:53 +00:00

/members contains all members, even though you're only accessing one, think of it like a folder tree with /members containing all of the members of every guild out there but you only have access to a subset of them

`/members` contains *all* members, even though you're only accessing one, think of it like a folder tree with /members containing all of the members of every guild out there but you only have access to a subset of them

radical marked this conversation as resolved

baaboe added 4 commits 2025-07-22 16:27:49 +00:00

fix: Unecessary merge of routers 0468d1adca

fix: reduced numder of function calls to get conn ea33230e58

fix: memebrs not member as endpoint 31596c6bfe

fix: members in router_with_auth

ci/woodpecker/push/build-and-publish Pipeline was successful

Details

ci/woodpecker/pr/build-and-publish Pipeline failed

Details

6dd8ddb0df

radical reviewed 2025-07-22 16:28:38 +00:00

src/api/v1/member/uuid/mod.rs Outdated

radical commented

2025-07-22 16:28:38 +00:00

the get() function needs to check membership, missed it in the review. A user that is not a member of a server should not be able to view members of that server

the `get()` function needs to check membership, missed it in the review. A user that is not a member of a server should not be able to view members of that server

radical marked this conversation as resolved

baaboe added 1 commit 2025-07-22 16:33:54 +00:00

fix: Only people in a server should see its members list

ci/woodpecker/push/build-and-publish Pipeline was successful

Details

ci/woodpecker/pr/build-and-publish Pipeline was successful

Details

a3c460a611

baaboe added 1 commit 2025-07-22 16:50:29 +00:00

fix: cargo clippy --fix && cargo fmt

ci/woodpecker/push/build-and-publish Pipeline was successful

Details