null?discordId= filter behaviour not consistent for admin/manage/users

[jxckUK]

What version of Node.js are you using?

18

What operating system are you using?

Ubuntu

What version of SnailyCAD are you currently using?

v1.78.4 aae52da

Describe the Bug

When trying to apply the same filter that works on the admin/manage/units and admin/manage/citizens routes on the admin/manage/users it returns a 404 error.

I am aware that this can be achieved by v1/admin/manage/users/{discordId} but the behaviour not being consistent leads to confusion.

Expected Behavior

It should filter the users based on the filter query specified.

To Reproduce

Run a GET query against https://CADURL/v1/admin/manage/users/null?discordId=DISCORDID.

Running queries against https://CADURL/v1/admin/manage/units/null?discordId=DISCORDID or https://CADURL/v1/admin/manage/citizens/null?discordId=DISCORDID will work fine.

Original commit that adding this functionality: 43a6323

[subtosharki]

@jxckUK just to make sure, you want support for steamId and discordId on admin/manage/users?

[subtosharki]

also, from looking at the code for admin/manage/users it looks like it does not return all users, but only the first one... or there is some magic going on i dont see

[jxckUK]

@jxckUK just to make sure, you want support for steamId and discordId on admin/manage/users?

This is already supported it seems just without the null?discordId= string, should be consistent on all three routes, either way is personally fine with me?

Users should only ever return one result, it shouldn't allow multiple accounts to share the same Discord or Steam identifiers.

[subtosharki]

users is plural so i assumed it included many, my apologies im trying to get familiar with the codebase its been a while.

i see what you mean now, with admin/manage/users/id you dont have to put the id as null and specify the id version, but with the others you do.

i could go ahead and make them follow the same pattern as admin/manage/users as that is more clean imo, but that would be a semi breaking change to any third party apps.

@MarshyMelloow im down to do this fullstack but want you opinion as you are the primary maintainer

[WhitigolProd]

The path /admin/manager/users/:id would fetch the user based on the User ID instead of the Discord ID. A new route could be created to fetched based on Discord ID, without doing anything that would be a breaking change

cc @jxckUK @subtosharki

[jxckUK]

The path /admin/manager/users/:id would fetch the user based on the User ID instead of the Discord ID. A new route could be created to fetched based on Discord ID, without doing anything that would be a breaking change

cc @jxckUK @subtosharki

Hmm odd - I tested locally and that route will correctly filter on Discord ID at present without any changes so I assume it has logic to filter both without explicit strings in the request.

This is what all the routes should do and for legacy compatibility the null?discordId= string should remain a possible filter for the routes.

[MarshyMelloow]

@MarshyMelloow im down to do this fullstack but want you opinion as you are the primary maintainer

Go for it @subtosharki! The more contributions the better!

[subtosharki]

Go for it @subtosharki! The more contributions the better!

sounds good, its 11 pm so ill do it tomorrow