Include fields on GET /applicationForms/{id} optionally #157

bickelj · 2022-12-07T03:18:43Z

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

codecov · 2022-12-07T03:21:44Z

Codecov Report

Base: 95.74% // Head: 95.63% // Decreases project coverage by -0.11% ⚠️

Coverage data is based on head (f7229b8) compared to base (c9bc6d9).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head f7229b8 differs from pull request most recent head 01efd86. Consider uploading reports for the commit 01efd86 to get more accurate results

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

Impacted Files	Coverage Δ
src/handlers/applicationFormsHandlers.ts	`95.28% <100.00%> (-1.21%)`	⬇️
src/routers/applicationFormsRouter.ts	`100.00% <100.00%> (ø)`

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/handlers/applicationFormsHandlers.ts

bickelj · 2022-12-12T18:18:39Z

e6c7877 rebases, adds a test, updates doc, does a small refactor.

bickelj · 2022-12-12T18:35:56Z

6afbc57 actually adds the test.

bickelj · 2022-12-12T21:00:34Z

Code coverage? From https://github.com/PhilanthropyDataCommons/service/actions/runs/3678723239/jobs/6222324994#step:8:34

[2022-12-12T18:37:07.844Z] ['error'] There was an error running the uploader: Error uploading to https://codecov.io: Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}

Maybe another push is needed.

bickelj · 2022-12-12T21:37:14Z

b71998d is a whitespace change in an attempt to get the code coverage report to run.

bickelj · 2022-12-12T21:44:22Z

src/handlers/applicationFormsHandlers.ts

+              // add field to previousRow
+              if (currentApplicationForm.fields === undefined) {
+                currentApplicationForm.fields = [formField];
+              } else {


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);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

bickelj · 2022-12-12T21:50:07Z

src/handlers/applicationFormsHandlers.ts

+              opportunityId: formWithFieldRow.opportunityId,
+              version: formWithFieldRow.version,
+              createdAt: formWithFieldRow.createdAt,
+              fields: [],


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' } ]

bickelj · 2022-12-12T21:58:13Z

src/types/ApplicationForm.ts

@@ -17,6 +17,18 @@ export interface ApplicationForm {
  readonly createdAt: Date;


One more thing might be to make fields mandatory here in the original type, then it really cannot be undefined under any circumstances.

Doing that causes validation failures that make me think an empty array does not pass validation unless there is something else I'm missing.

bickelj · 2022-12-12T22:25:52Z

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.

bickelj · 2022-12-13T18:15:11Z

@slifty Suggested make this an option and via query parameter.

bickelj · 2022-12-16T19:35:44Z

@slifty @reefdog better?

bickelj · 2023-02-09T16:13:03Z

Feedback on #210 applies here too: rework it to use one query per table, have the code join the results, etc.

bickelj · 2023-02-10T15:27:31Z

41589e8 rebases on main.

bickelj · 2023-02-13T15:50:20Z

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.

bickelj · 2023-02-13T15:58:35Z

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

bickelj · 2023-02-13T19:57:15Z

src/handlers/applicationFormsHandlers.ts

+            .contentType('application/json')
+            .send(applicationForm);
+        }).catch((error: unknown) => {
+          throw error;


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

bickelj · 2023-02-13T20:15:52Z

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

bickelj · 2023-02-13T20:30:24Z

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

bickelj · 2023-02-13T20:41:10Z

01efd86 has one (hopefully last?) test.

bickelj requested a review from slifty December 7, 2022 03:18

bickelj commented Dec 8, 2022

View reviewed changes

src/handlers/applicationFormsHandlers.ts Outdated Show resolved Hide resolved

bickelj force-pushed the 132-visibility-of-application-form-fields branch from a3074e5 to e6c7877 Compare December 12, 2022 18:18

bickelj force-pushed the 132-visibility-of-application-form-fields branch from e6c7877 to 6afbc57 Compare December 12, 2022 18:35

bickelj force-pushed the 132-visibility-of-application-form-fields branch from 6afbc57 to b71998d Compare December 12, 2022 21:36

bickelj commented Dec 12, 2022

View reviewed changes

bickelj mentioned this pull request Dec 14, 2022

Add ability to retrieve deep associations #174

Open

bickelj force-pushed the 132-visibility-of-application-form-fields branch from b71998d to 4b9c3e8 Compare December 16, 2022 19:33

bickelj mentioned this pull request Dec 30, 2022

Implement GET /proposals/{proposalId} endpoint #101

Closed

bickelj force-pushed the 132-visibility-of-application-form-fields branch 2 times, most recently from e5f0d6a to 41589e8 Compare February 10, 2023 15:27

bickelj force-pushed the 132-visibility-of-application-form-fields branch from 41589e8 to 3b0f4f7 Compare February 13, 2023 15:46

bickelj force-pushed the 132-visibility-of-application-form-fields branch from 3b0f4f7 to cada08c Compare February 13, 2023 16:10

bickelj changed the title ~~Include fields on GET /applicationForms~~ Include fields on GET /applicationForms/{id} optionally Feb 13, 2023

bickelj commented Feb 13, 2023

View reviewed changes

bickelj force-pushed the 132-visibility-of-application-form-fields branch from cada08c to 46828c9 Compare February 13, 2023 20:12

bickelj force-pushed the 132-visibility-of-application-form-fields branch from 46828c9 to f7229b8 Compare February 13, 2023 20:27

bickelj force-pushed the 132-visibility-of-application-form-fields branch from f7229b8 to 01efd86 Compare February 13, 2023 20:40

bickelj merged commit 19167f6 into main Feb 13, 2023

bickelj deleted the 132-visibility-of-application-form-fields branch February 13, 2023 20:56

bickelj mentioned this pull request Feb 27, 2023

Provide visibility of application form fields #132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include fields on GET /applicationForms/{id} optionally #157

Include fields on GET /applicationForms/{id} optionally #157

bickelj commented Dec 7, 2022 •

edited

codecov bot commented Dec 7, 2022 •

edited

bickelj commented Dec 12, 2022

bickelj commented Dec 12, 2022

bickelj commented Dec 12, 2022

bickelj commented Dec 12, 2022

bickelj Dec 12, 2022

bickelj Dec 12, 2022

bickelj Dec 12, 2022

bickelj Dec 12, 2022

bickelj commented Dec 12, 2022

bickelj commented Dec 13, 2022

bickelj commented Dec 16, 2022 •

edited

bickelj commented Feb 9, 2023

bickelj commented Feb 10, 2023

bickelj commented Feb 13, 2023

bickelj commented Feb 13, 2023

bickelj Feb 13, 2023

bickelj commented Feb 13, 2023

bickelj commented Feb 13, 2023

bickelj commented Feb 13, 2023

		@@ -17,6 +17,18 @@ export interface ApplicationForm {
		readonly createdAt: Date;

Include fields on GET /applicationForms/{id} optionally #157

Include fields on GET /applicationForms/{id} optionally #157

Conversation

bickelj commented Dec 7, 2022 • edited

codecov bot commented Dec 7, 2022 • edited

Codecov Report

bickelj commented Dec 12, 2022

bickelj commented Dec 12, 2022

bickelj commented Dec 12, 2022

bickelj commented Dec 12, 2022

bickelj Dec 12, 2022

Choose a reason for hiding this comment

bickelj Dec 12, 2022

Choose a reason for hiding this comment

bickelj Dec 12, 2022

Choose a reason for hiding this comment

bickelj Dec 12, 2022

Choose a reason for hiding this comment

bickelj commented Dec 12, 2022

bickelj commented Dec 13, 2022

bickelj commented Dec 16, 2022 • edited

bickelj commented Feb 9, 2023

bickelj commented Feb 10, 2023

bickelj commented Feb 13, 2023

bickelj commented Feb 13, 2023

bickelj Feb 13, 2023

Choose a reason for hiding this comment

bickelj commented Feb 13, 2023

bickelj commented Feb 13, 2023

bickelj commented Feb 13, 2023

bickelj commented Dec 7, 2022 •

edited

codecov bot commented Dec 7, 2022 •

edited

bickelj commented Dec 16, 2022 •

edited