Add GET proposalVersionById route #1368
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1368 +/- ##
==========================================
+ Coverage 87.34% 87.39% +0.04%
==========================================
Files 185 186 +1
Lines 2387 2412 +25
Branches 321 325 +4
==========================================
+ Hits 2085 2108 +23
- Misses 302 304 +2 ☔ View full report in Codecov by Sentry. |
|
@hminsky2002 I see you requested @slifty review but I wanted to note the following context. When examining |
There was a problem hiding this comment.
I was able to build, run, and try this code locally. It seemed to work fine. I needed a fresh authentication token to access the endpoint and when I asked for a proposal version by ID I got it back. It looked like the right stuff that I got back. Nothing in the code appears objectionable, at least nothing new objectionable.
My vote is to rebase, push, and merge when the checks pass.
| await createApplicationForm({ | ||
| opportunityId: 1, | ||
| }); | ||
| const newProposalVersion = await createProposalVersion({ |
There was a problem hiding this comment.
While this follows some of the existing conventions, and therefore I don't think my objection is reason to hold up this PR, I prefer to get the database-generated IDs explicitly rather than making the assumption that ID 1 was inserted. Something for future PRs, perhaps.
There was a problem hiding this comment.
Agreed, and the same can be said for the other IDs referenced here -- it's all being loaded right above so there's no cost to just const opportunity = createOpportunity... and then accessing opportunity.id in the appropriate places.
Also the prefix new here isn't contextually relevant so I'd just call it proposalVersion or testProposalVersion.
| "/proposalVersions/{proposalVersionId}": { | ||
| "get": { | ||
| "operationId": "getProposalVersionById", | ||
| "summary": "Gets a specific proposalVersion.", |
There was a problem hiding this comment.
Here I would say "proposal version." instead of "proposalVersion."
| "parameters": [ | ||
| { | ||
| "name": "proposalVersionId", | ||
| "description": "The PDC-generated ID of a proposalVersion.", |
There was a problem hiding this comment.
Here I would say "proposal version." instead of "proposalVersion."
| ): void => { | ||
| const { proposalVersionId } = req.params; | ||
| if (!isId(proposalVersionId)) { | ||
| next(new InputValidationError('Invalid request body.', isId.errors ?? [])); |
There was a problem hiding this comment.
This is a copy pasted error (as in, the error exists in another endpoint so I totally feel for you) -- proposalVersionId is a query parameter, not part of the request body, so this error should be updated to say invalid query parameter
Closes #1366
I'll note I did not update the changelog because I wasn't sure if this justified a change (as it wouldn't break anything), but can gladly update if requested!