Skip to content

Implement GET /claims#82

Open
SidneyNemzer wants to merge 5 commits intoBuildTheEarth:api/v2from
SidneyNemzer:api-v2/claims
Open

Implement GET /claims#82
SidneyNemzer wants to merge 5 commits intoBuildTheEarth:api/v2from
SidneyNemzer:api-v2/claims

Conversation

@SidneyNemzer
Copy link
Contributor

This PR implements GET /claims, with some other minor changes.

Want to confirm how auth should work for this route, it looks like v1 didn't check auth for this route, although v2 requires build team token auth by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Potentially add pagination and sorting like in /applications (not done)

@Nudelsuppe42
Copy link
Contributor

Also, if its easier for you, you can now also branch inside this repo without needing your fork if thats easier for you.

@Nudelsuppe42 Nudelsuppe42 linked an issue Mar 1, 2026 that may be closed by this pull request
@Nudelsuppe42
Copy link
Contributor

Ok the more i think about it, the more i agree that we shouldnt duplicate routes the way i intended.
So i think /claims shouldnt require auth, but if present should take the id of the token as the default team filter? Is that good? That way everyone can access data & teams can do at the same time.
But that will definitley require pagination as we dont want to send out 100MB+ every time

@SidneyNemzer
Copy link
Contributor Author

Also, if its easier for you, you can now also branch inside this repo without needing your fork if thats easier for you.

Ah yeah good point! I'll do that for my next PRs

i think /claims shouldnt require auth, but if present should take the id of the token as the default team filter?

Yes as long as you don't want to list claims for every team. I'm not super familiar with how they're used so maybe it always makes sense to list claims for one team at a time. I'll make that change for this route and add pagination.

@SidneyNemzer
Copy link
Contributor Author

Pushed a commit to make auth optional for the route, defaulting team to req.token.id. Let me know what you think.

It would be nice to make @OptionalAuth() type safe so the controller knows req.token is possibly undefined, any ideas there? My best idea is to make the auth a param of the controller but not sure if that makes sense for Nest.

I'll add pagination when I get a chance later this week.

@Nudelsuppe42
Copy link
Contributor

I think what you did is good (that also how the docs say you should handle it). Not sure about type saftey though.
Just dont know about how the swagger docs will behave, worst case would be to add a note in the description or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

feat(api/v2): ⭐ Implement claims routes

2 participants