-
Notifications
You must be signed in to change notification settings - Fork 2
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
Include fields on GET /applicationForms/{id} optionally #157
Conversation
Codecov ReportBase: 95.74% // Head: 95.63% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
- Coverage 95.74% 95.63% -0.11%
==========================================
Files 48 48
Lines 775 825 +50
Branches 143 156 +13
==========================================
+ Hits 742 789 +47
- Misses 32 35 +3
Partials 1 1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
a3074e5
to
e6c7877
Compare
e6c7877 rebases, adds a test, updates doc, does a small refactor. |
e6c7877
to
6afbc57
Compare
6afbc57 actually adds the test. |
Code coverage? From https://github.com/PhilanthropyDataCommons/service/actions/runs/3678723239/jobs/6222324994#step:8:34
Maybe another push is needed. |
6afbc57
to
b71998d
Compare
b71998d is a whitespace change in an attempt to get the code coverage report to run. |
// add field to previousRow | ||
if (currentApplicationForm.fields === undefined) { | ||
currentApplicationForm.fields = [formField]; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suppose I remove the above unreachable code (because it is always defined, you can see it defined on lines 67 and 96 here) in both places, now the compiler gets confused because it thinks it could possibly be undefined (even though it is always defined):
src/handlers/applicationFormsHandlers.ts:79:15 - error TS2532: Object is possibly 'undefined'.
79 currentApplicationForm.fields.push(formField);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~src/handlers/applicationFormsHandlers.ts:98:15 - error TS2532: Object is possibly 'undefined'.
98 currentApplicationForm.fields.push(formField);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
opportunityId: formWithFieldRow.opportunityId, | ||
version: formWithFieldRow.version, | ||
createdAt: formWithFieldRow.createdAt, | ||
fields: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suppose I remove this line 96 and 67 to force that possibility of undefined fields
, well then a bigger issue arises: the response body might not have the fields
key at all, which is going to be a problem if you want to consistently be able to parse the response using a schema. It makes the most sense to always have the empty list than to sometimes have it and other times not have it.
FAIL src/__tests__/applicationForm.int.test.ts (9.288 s)
● /applicationForms › GET / › returns all application forms present in the database
expected [
{
createdAt: '2022-07-20T12:00:00.000Z',
id: 1,
opportunityId: 1,
version: 1,
fields: []
},
{
createdAt: '2022-08-20T12:00:00.000Z',
id: 2,
opportunityId: 1,
version: 2,
fields: []
},
{
createdAt: '2022-09-20T12:00:00.000Z',
id: 3,
opportunityId: 2,
version: 1,
fields: []
}
] response body, got [
{
id: 1,
opportunityId: 1,
version: 1,
createdAt: '2022-07-20T12:00:00.000Z'
},
{
id: 2,
opportunityId: 1,
version: 2,
createdAt: '2022-08-20T12:00:00.000Z'
},
{
id: 3,
opportunityId: 2,
version: 1,
createdAt: '2022-09-20T12:00:00.000Z'
}
]
src/types/ApplicationForm.ts
Outdated
@@ -17,6 +17,18 @@ export interface ApplicationForm { | |||
readonly createdAt: Date; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing might be to make fields mandatory here in the original type, then it really cannot be undefined under any circumstances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing that causes validation failures that make me think an empty array does not pass validation unless there is something else I'm missing.
I'm not sure how much more time it's worth to get that additional 0.08% coverage. This might be as good as it gets. In any case I hope to hear @slifty's comments on this approach. |
@slifty Suggested make this an option and via query parameter. |
b71998d
to
4b9c3e8
Compare
Feedback on #210 applies here too: rework it to use one query per table, have the code join the results, etc. |
e5f0d6a
to
41589e8
Compare
41589e8 rebases on main. |
41589e8
to
3b0f4f7
Compare
3b0f4f7 rebases on main, improves test coverage, removed a no-longer-used sql file, and reverted some unneeded whitespace or import order changes to make the diff easier to read. |
It is unclear what makes codecov get an update vs not get an update. Perhaps a new commit? Suppose I squash all the commits, trying that because it needs to happen anyway. |
Use query parameter `includeFields=true` to include application form fields in the response from `GET /applicationForms/{id}`, otherwise a shallow response with only direct attributes on application form will be returned. The implementation is more akin to the one for /proposals in PR #210. This commit combines four commits from 2022-12-06, 2022-12-12, 2022-12-12, 2022-12-16, 2023-02-09, respectively, and cleanup on 2023-02-13. Some history of changes should be available at #157 Issue #132 Provide visibility of application form fields
3b0f4f7
to
cada08c
Compare
.contentType('application/json') | ||
.send(applicationForm); | ||
}).catch((error: unknown) => { | ||
throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error does not get caught below in 135-144.
Use query parameter `includeFields=true` to include application form fields in the response from `GET /applicationForms/{id}`, otherwise a shallow response with only direct attributes on application form will be returned. The implementation is more akin to the one for /proposals in PR #210. This commit combines four commits from 2022-12-06, 2022-12-12, 2022-12-12, 2022-12-16, 2023-02-09, respectively, and cleanup on 2023-02-13. Some history of changes should be available at #157 Issue #132 Provide visibility of application form fields
cada08c
to
46828c9
Compare
46828c9 has some additional test coverage, renames the integration tests for applicationForms (to be plural), and fixes an issue revealed by trying to get that last little bit of test coverage. |
Use query parameter `includeFields=true` to include application form fields in the response from `GET /applicationForms/{id}`, otherwise a shallow response with only direct attributes on application form will be returned. The implementation is more akin to the one for /proposals in PR #210. This commit combines four commits from 2022-12-06, 2022-12-12, 2022-12-12, 2022-12-16, 2023-02-09, respectively, and cleanup on 2023-02-13. Some history of changes should be available at #157 Issue #132 Provide visibility of application form fields
46828c9
to
f7229b8
Compare
f7229b8 has one more test. |
Use query parameter `includeFields=true` to include application form fields in the response from `GET /applicationForms/{id}`, otherwise a shallow response with only direct attributes on application form will be returned. The implementation is more akin to the one for /proposals in PR #210. This commit combines four commits from 2022-12-06, 2022-12-12, 2022-12-12, 2022-12-16, 2023-02-09, respectively, and cleanup on 2023-02-13. Some history of changes should be available at #157 Issue #132 Provide visibility of application form fields
f7229b8
to
01efd86
Compare
01efd86 has one (hopefully last?) test. |
Use query parameter
includeFields=true
to include application formfields in the response from
GET /applicationForms/{id}
, otherwise ashallow response with only direct attributes on application form will
be returned.
The implementation is more akin to the one for /proposals in PR #210.
This commit combines four commits from 2022-12-06, 2022-12-12,
2022-12-12, 2022-12-16, 2023-02-09, respectively, and cleanup on
2023-02-13. Some history of changes should be available at
#157
Issue #132 Provide visibility of application form fields