Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GET proposalVersionById route #1368

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions src/__tests__/proposalVersions.int.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
createBaseField,
createOpportunity,
createProposal,
createProposalVersion,
loadSystemSource,
loadTableMetrics,
} from '../database';
Expand Down Expand Up @@ -34,6 +35,67 @@ const createTestBaseFields = async () => {
};

describe('/proposalVersions', () => {
describe('GET /:proposalVersionId', () => {
it('requires authentication', async () => {
await request(app).get('/proposalVersions/1').expect(401);
});

it('returns exactly one proposal version selected by id', async () => {
const systemSource = await loadSystemSource();
await createOpportunity({ title: '🔥' });
const testUser = await loadTestUser();
await createProposal({
externalId: 'proposal-1',
opportunityId: 1,
createdBy: testUser.keycloakUserId,
});
await createApplicationForm({
opportunityId: 1,
});
const newProposalVersion = await createProposalVersion({
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@slifty slifty Dec 6, 2024

Choose a reason for hiding this comment

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

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.

proposalId: 1,
applicationFormId: 1,
sourceId: systemSource.id,
createdBy: testUser.keycloakUserId,
});

const response = await request(app)
.get(`/proposalVersions/1`)
.set(authHeader)
.expect(200);
expect(response.body).toEqual(newProposalVersion);
});

it('returns 400 bad request when id is a letter', async () => {
const result = await request(app)
.get('/proposalVersions/a')
.set(authHeader)
.expect(400);
expect(result.body).toMatchObject({
name: 'InputValidationError',
details: expect.any(Array) as unknown[],
});
});

it('returns 400 bad request when id is a number greater than 2^32-1', async () => {
const result = await request(app)
.get('/proposalVersions/555555555555555555555555555555')
.set(authHeader)
.expect(400);
expect(result.body).toMatchObject({
name: 'InputValidationError',
details: expect.any(Array) as unknown[],
});
});

it('returns 404 when id is not found', async () => {
await request(app)
.get('/proposalVersions/900000')
.set(authHeader)
.expect(404);
});
});

describe('POST /', () => {
it('requires authentication', async () => {
await request(app).post('/proposalVersions').expect(401);
Expand Down
1 change: 1 addition & 0 deletions src/database/operations/proposalVersions/index.ts
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
export * from './createProposalVersion';
export * from './loadProposalVersion';
22 changes: 22 additions & 0 deletions src/database/operations/proposalVersions/loadProposalVersion.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { db } from '../../db';
import { NotFoundError } from '../../../errors';
import type { JsonResultSet, ProposalVersion } from '../../../types';

export const loadProposalVersion = async (
id: number,
): Promise<ProposalVersion> => {
const result = await db.sql<JsonResultSet<ProposalVersion>>(
'proposalVersions.selectById',
{
id,
},
);
const proposalVersion = result.rows[0]?.object;
if (proposalVersion === undefined) {
throw new NotFoundError(`Entity not found`, {
entityType: 'ProposalVersion',
entityId: id,
});
}
return proposalVersion;
};
3 changes: 3 additions & 0 deletions src/database/queries/proposalVersions/selectById.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
SELECT proposal_version_to_json(proposal_versions.*) AS object
FROM proposal_versions
WHERE id = :id;
26 changes: 26 additions & 0 deletions src/handlers/proposalVersionsHandlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@
loadApplicationForm,
loadApplicationFormField,
loadProposal,
loadProposalVersion,
} from '../database';
import {
isAuthContext,
isTinyPgErrorWithQueryContext,
isWritableProposalVersionWithFieldValues,
isId,
} from '../types';
import {
DatabaseError,
Expand Down Expand Up @@ -212,6 +214,30 @@
});
};

const getProposalVersion = (
req: Request,
res: Response,
next: NextFunction,
): void => {
const { proposalVersionId } = req.params;
if (!isId(proposalVersionId)) {
next(new InputValidationError('Invalid request body.', isId.errors ?? []));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

return;
}
loadProposalVersion(proposalVersionId)
.then((item) => {
res.status(200).contentType('application/json').send(item);
})
.catch((error: unknown) => {
if (isTinyPgErrorWithQueryContext(error)) {
next(new DatabaseError('Error retrieving item.', error));
return;

Check warning on line 234 in src/handlers/proposalVersionsHandlers.ts

View check run for this annotation

Codecov / codecov/patch

src/handlers/proposalVersionsHandlers.ts#L233-L234

Added lines #L233 - L234 were not covered by tests
}
next(error);
});
};

export const proposalVersionsHandlers = {
postProposalVersion,
getProposalVersion,
};
45 changes: 45 additions & 0 deletions src/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -1823,6 +1823,51 @@
}
}
},
"/proposalVersions/{proposalVersionId}": {
"get": {
"operationId": "getProposalVersionById",
"summary": "Gets a specific proposalVersion.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I would say "proposal version." instead of "proposalVersion."

"tags": ["Proposals"],
"security": [
{
"auth": []
}
],
"parameters": [
{
"name": "proposalVersionId",
"description": "The PDC-generated ID of a proposalVersion.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I would say "proposal version." instead of "proposalVersion."

"in": "path",
"required": true,
"schema": {
"type": "integer"
}
}
],
"responses": {
"200": {
"description": "The requested proposalVersion.",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/ProposalVersion"
}
}
}
},
"401": {
"description": "Authentication was not provided or was invalid.",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/PdcError"
}
}
}
}
}
}
},
"/baseFields": {
"get": {
"operationId": "getBaseFields",
Expand Down
6 changes: 6 additions & 0 deletions src/routers/proposalVersionsRouter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,10 @@ proposalVersionsRouter.post(
proposalVersionsHandlers.postProposalVersion,
);

proposalVersionsRouter.get(
'/:proposalVersionId',
requireAuthentication,
proposalVersionsHandlers.getProposalVersion,
);

export { proposalVersionsRouter };
Loading