Add option to deep GET /proposals/{proposalId}

[bickelj]

When optional query parameter includeFieldsAndValues is set to true on GET /proposals/{proposalId}, include all proposal versions, associated values, and associated application form fields in the response.

By returning this almost-fully-deep object tree, it is more convenient for the caller and more efficient in terms of request, query, and response counts. The assumption is that the purpose of the PDC is to see and compare which fields are used for what purpose for proposals.

Issue #101 Implement GET /proposals/{proposalId} endpoint

(Alternative approach vs #198)

[codecov]

Codecov Report

Base: 96.44% // Head: 95.74% // Decreases project coverage by -0.71% ⚠️

Coverage data is based on head (551be26) compared to base (9614dc6).
Patch coverage: 89.18% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #210      +/-   ##
==========================================
- Coverage   96.44%   95.74%   -0.71%     
==========================================
  Files          47       48       +1     
  Lines         704      775      +71     
  Branches      125      143      +18     
==========================================
+ Hits          679      742      +63     
- Misses         24       32       +8     
  Partials        1        1              

Impacted Files	Coverage Δ
src/handlers/proposalsHandlers.ts	93.33% <87.09%> (-6.67%)	⬇️
src/errors/NotFoundError.ts	100.00% <100.00%> (ø)
src/errors/index.ts	100.00% <100.00%> (ø)
src/middleware/errorHandler.ts	98.50% <100.00%> (+0.14%)	⬆️
src/types/ProposalFieldValue.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.

[bickelj]

fcd5ea6 rebased on main, moved a comment, clarified a comment, made SQL more consistent (caps AS).

[bickelj]

I will try to improve the ugliness in mergeApplicationFormFields.

[bickelj]

abc0291 uses map instead of for, eliminating the gymnastics around convincing TypeScript that elements are truly defined.

[bickelj]

6ce9d01 improves test coverage by removing a couple of redundant lines and adding some unit tests.

[bickelj]

c9e413b improves test coverage and improves a couple of comments.

[bickelj]

42a1e98 improves test coverage and rebases on main.

[slifty]

Some initial feedback -- I have some more but wanted to start with this to see what you think first.

I believe that things could be a bit simpler -- we don't need the decorator / merge functions to be in terms of the PROPOSAL but rather in terms of the object actually being decorated (e.g. the version, the field value, etc). This then saves a whole lot of internal checks and assumptions.

I think this would result in much more concise code with less need for checking / thrown errors while ALSO making those decorator functions universal / relevant to future deep object queries (and the deep query endpoint implementations will just be a matter of getting the data and calling the decorators in the right order!)

[bickelj]

Going way back to the higher level questions: @slifty what do you think of the parameter name includeFieldsAndValues? This differs, for example, from the parameter includeFields proposed for a different endpoint. The discussion is really over in #174.

[bickelj]

c602b6a has most of the suggested changes. I spent way too long trying to figure out how to convince jest to only throw an error on the second call to db.sql rather than on the first (the naive "chain another .mockImplementationOnce call" did not work for me) but just pushed without those two more lines being covered. This commit should be very close to the test coverage goal regardless.

[bickelj]

ca13026 cleans up src/ProposalFieldValue.ts a bit, also based on previous feedback. I think it's time to ring the "good enough" bell. What do you think @slifty ?

[bickelj]

0d56542 rebased on main.

[bickelj]

9b3bdb1 rebased on main.

[slifty]

This looks really great thank you so much for all the iteration!

[slifty]

Ack so sorry -- so the code looks excellent, but I'm now seeing test coverage isn't 100% (unless I'm being lied to by codecov which has happened before)

That said, it occurs to me that we might consider helper functions that invoke the query and parse the result, which are then tested in isolation and reused across our endpoints rather than having to test every single endpoint... If you prefer we could just open an issue to take on that debt separately -- it would just mean coverage goes down for the short term but we commit to bringing it back up with that refactor (I can take on the refactor).

I'm imagining something like this, but more database specific: https://github.com/PermanentOrg/permanent-sdk/blob/main/src/utils/typedJsonParse.ts

[bickelj]

Since I could not get that extra one percent in 4 hours I judged it not worth it because we have 80% coverage already, this also has >80% coverage on new code, and does not reduce the coverage by 2%.

The utility/helper function idea might be a more straightforward way to get the coverage rather than mocking only the second db.sql call (which I could not get working after many hours of trying).

[slifty]

Yeah absolutely -- I'll update back to approve and create an issue! Thanks for considering it (and for the very legit pushback)

[bickelj]

551be26 squashed the two commits into one.