Implement application routes - #134
Open
kyanvde wants to merge 22 commits into
Open
Conversation
The lockfile still carried jest 30.x entries that no workspace resolves to anymore, so a fresh `yarn install` for api-v2 rewrote them to match the pinned `@types/jest@^29`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`src/.prettierrc` was an empty object, which took precedence over the shared `@repo/prettier-config` for everything below it. Every file in src was therefore checked against Prettier defaults instead of the repo style, and all 52 of them failed that check. Removing the file lets src fall back to the shared config again. The rest of this commit is the resulting reformat: tabs, single quotes, 120 columns. No behaviour changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`delete` used `deleteMany` scoped to the authenticated team, so deleting a
question that does not exist, or that belongs to another team, quietly
reported success and returned `{ count: 0 }`. The route already documented
a 404 for that case but could never produce one.
It now throws NotFoundException when nothing was deleted and returns no
body. The documented status of the success case is corrected to the 200
the route actually returns.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds the three write routes of #59: POST /applications/questions PUT /applications/questions (bulk upsert) PUT /applications/questions/:id Every entry of the bulk upsert is sent in full: entries carrying an ID replace the matching question, entries without one are created. Questions of the team that are not part of the payload are left untouched, and the whole batch runs in a single transaction. All writes are scoped to the authenticated team through `updateMany` on `{ id, buildTeamId }`, and the bulk route rejects IDs owned by another team with a 403 rather than overwriting them, which is what the v1 implementation did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Returns the application questions of a single team. The route is public, because the application form has to render before the applicant is part of the team, which matches how v1 exposed the same data. The team is resolved by ID, or by slug when `?slug=true` is passed, and an unknown team yields a 404 so that "no such team" stays distinguishable from "team without questions". Results default to sorting by `sort`, the field that drives the order of the form itself. It lives on its own controller since a Nest controller cannot escape its own path prefix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds the last group of routes of #59: GET /applications/templates POST /applications/templates PUT /applications/templates/:id DELETE /applications/templates/:id Templates are the canned replies teams send when reviewing an application. Like the question routes, every operation is scoped to the authenticated team, and touching a template of another team yields a 404 instead of silently succeeding. Note that ApplicationResponseTemplate spells its foreign key `buildteamId`, unlike ApplicationQuestion which uses `buildTeamId`. The new module is registered in front of ApplicationsModule, because routes resolve in module registration order and /applications/:id would otherwise match /applications/templates first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The nested application routes only resolve because their modules are registered ahead of ApplicationsModule. Nothing enforced that ordering, and getting it wrong is silent: /applications/questions simply starts being handled by /applications/:id. This boots the real AppModule against a stubbed Prisma and drives it over HTTP, asserting that each nested route reaches its own controller, that /applications/:id still works, and that the public team route is reachable without a token while the authenticated one is not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Documents the commands that are not obvious from package.json, the split between the v1 Express API and the v2 Nest API, the conventions a new v2 endpoint has to follow, and the setup steps the test suite depends on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 88 out of 90 changed files in this pull request and generated 5 comments.
Suppressed comments (4)
apps/api-v2/src/sections/status/status.controller.ts:27
- Use strict equality (
===) for string comparisons to avoid unintended coercions.
apps/api-v2/src/sections/status/dto/statusComponent.dto.ts:28 StatusService.getComponents()returnsstatusandtypeas numbers, butStatusComponentDtodocuments them as a nested object and a string. This makes the Swagger schema inconsistent with the actual runtime response.
apps/api-v2/src/sections/status/dto/incident.dto.ts:26StatusService.getIncidents()mapsstatus,created_at, andoccurred_atto primitive values, butIncidentDtodocuments them as nested objects. This makes the Swagger schema inconsistent with the actual response shape.
apps/api-v2/src/sections/status/dto/incident.dto.ts:16- Fix spelling in the
ApiPropertyexample string: "Unavailibility" → "Unavailability".
`GET /applications/:id` and `PUT /applications/:id` looked applications up by ID alone. Any authenticated team could therefore read another team's application, and worse, review one: flipping its status, reason, reviewer or claim, as long as the ID was known. Both now resolve through `buildteamId` as well, matching what `findAll` and `create` already did and what every question and template route does. `review` also moves from `update` to `updateMany` plus an explicit count check. Prisma throws P2025 when `update` matches nothing, and the global exception filter turns any non-HttpException into a 500, so an unknown ID returned 500 despite the route documenting 404. It now returns the documented 404, which is also the right answer for an application owned by another team, since it does not leak whether the ID exists. Raised by Copilot on #134. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Filter values were coerced by declared type and then spread straight into a Prisma `where` clause, but nothing rejected a value that did not fit. A malformed date or an unknown enum member reached the driver and failed there, and since a Prisma error is not an HttpException the global filter reported it as 500. `?createdAt=yesterday` and `?status=NOPE` were both 500s on every list route. Invalid values are now rejected up front as 400. Numbers and booleans were previously dropped silently, which quietly returned an unfiltered page instead of saying the query was wrong, so those are rejected too. Adds Date coercion and enum membership checking to the decorator, and declares the columns that need them: createdAt/reviewedAt on applications, type on questions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two problems with `PUT /applications/questions`, both mine from earlier in this branch. It answered 403 when an ID belonged to another team, which confirms that the ID exists. Every other route here returns 404 for exactly that reason, so this one now does too. It also accepted an unbounded array and ran every entry inside a single transaction, so one request could hold row locks for as long as it took to apply an arbitrarily long payload. Capped at MAX_BULK_QUESTIONS, checked before the transaction opens. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`CreateApplicationDto` documents createdAt as "if not provided, defaults to the current time", and `whitelist` keeps it because it is a declared property, so it passed validation and was then overwritten with the current time regardless. Callers importing historical applications got a silently different timestamp back and no error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GET /:teamId/applications/questions renders the public application form, but inherited the default page size of 20. A team with more questions than that silently served a form missing its tail unless the client happened to page through. Defaults to 100 per page, capped at 200. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merging api/v2 took its yarn.lock wholesale, which silently reverted the earlier sync and brought the stale jest 30.x entries back. Regenerated with `yarn install`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nudelsuppe42
left a comment
Nudelsuppe42
left a comment
Contributor
There was a problem hiding this comment.
lgtm, but:
while upsert exists, there is no option or route to override the current config of application questions (upsert+delete).
Its debatable if we need that, but thats how the frontend v1 did it (get all questions, let the user edit and delete any, push the entire data back)
Contributor
There was a problem hiding this comment.
I am very sure we were able to only use one controller for both /:id/... and /...?
Why do we have both?
Contributor
Author
There was a problem hiding this comment.
You are able to have both as long as you register them in the right order. There is a test that tests specifically for this correct order so that if it gets messed up in the future it will be detected.
Contributor
There was a problem hiding this comment.
Thats not what I meant xD, I am very sure we just had one controller and one implementation for both /:buildteam/applications and /applications (it wasn't applications, but a different table).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #59