feat(grants-collaboration): expose notes retrieval #3395
Conversation
github-actions
bot
added
enhancement
QA Summary
Test CoverageCoverage report for `packages/client`
Coverage report for `packages/server`
|
Terraform Summary
Hint: If "Terraform Format & Style" failed, run OutputValidation OutputPlan Summary
Pusher: @greg-adams, Action: |
There was a problem hiding this comment.
@greg-adams Left a few comments regarding the import strategy for calling getOrganizationNotesForGrant().
Also wanted to say that I like this testing strategy. As a non-scientific benchmark, it looks like it only adds about 11 seconds to the test suite execution in our GitHub Actions CI job (which is probably slower than most developers' machines). I personally think that's an acceptable tradeoff for the increased test isolation. One other idea I wanted to throw out in case you were interested in experimenting: given that Postgres supports nested transactions, I wonder if it would be possible to achieve similar isolation like this:
- Seed the database to whatever extent would be "common" for all tests
- Before each test, begin an outer (i.e. root-level) transaction
- Run a test
- After each test, roll back the transaction that was started in Step 2, bringing the state of the DB back to where it was after Step 1
- (Repeat 2-4 for each test)
Thoughts?
packages/server/src/db/index.js
Outdated
| function getOrganizationNotesForGrant(...args) { | ||
| return grantsCollaboration.getOrganizationNotesForGrant(knex, ...args); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think it might be cleaner to skip this db/index.js file altogether and just import directly from the lib/grantsCollaboration module in src/routes/grants.js.
| function getOrganizationNotesForGrant(...args) { | |
| return grantsCollaboration.getOrganizationNotesForGrant(knex, ...args); | |
| } |
packages/server/src/db/index.js
Outdated
| @@ -1667,6 +1670,7 @@ module.exports = { | |||
| markGrantAsInterested, | |||
| unmarkGrantAsInterested, | |||
| getGrantAssignedAgencies, | |||
| getOrganizationNotesForGrant, | |||
There was a problem hiding this comment.
(See above)
| getOrganizationNotesForGrant, |
packages/server/src/routes/grants.js
Outdated
| return; | ||
| } | ||
|
|
||
| const rows = await db.getOrganizationNotesForGrant(grantId, user.tenant_id, { afterRevision: paginateFrom, limit: limitInt }); |
There was a problem hiding this comment.
As noted in my db/index.js comments, I think it would be preferable to require('../lib/grantsCollaboration') in this file and call its exported getOrganizationNotesForGrant() function directly rather than going through the db module.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Yes - that is the ideal I wanted...sort of a global rollback that we could use after each test to only apply to changes made during the test. Knex does allow for nested transactions, but my read on it appears that nested transactions need to maintain reference to the originating transaction (to be subsequently rolled back). i.e. I thought through a few approaches (we're not the first to have this issue, see https://github.com/bas080/knest), but some sort of dependency injection seemed too big of a refactor. I could envision a change in the exported knex where we wrap |
Agree with your rationale here – I think your current approach seems good for our needs. |
Ticket #3205
Description
Created route for notes by grant Id.
Added tests for exposed api route. After discussion with @TylerHendrickson and in an effort to increase data isolation for tests, I updated grants tests to re-seed db between tests. This approach would maintain some seed data for convenience (E.g. users, grants, agencies), but create and rollback records for a particular test. This adds some time to run the tests, but seems OK at least on my machine. I'd like to just rollback records for an individual test, but rollback/transaction approaches don't seem useful here, I'm not sure if a db template would be a faster approach.
Screenshots / Demo Video
Testing
Automated and Unit Tests
Manual tests for Reviewer
Checklist