feat(grants-collaboration): create follow/unfollow funcs #3337
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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: @amyewang, Action: |
TylerHendrickson
requested changes
Aug 5, 2024
There was a problem hiding this comment.
@amyewang Overall functionality looks great, including the use of (sub) transactions to ensure a rollback is possible after encountering a unique constraint violation in followGrant().
I have a suggested modification to your tests to ensure that both error-handling scenarios (catch-and-rollback vs catch-and-throw) are behaving as we expect them to. Let me know if you have any questions 🙂
Comment on lines
121
to
127
| it('throws an error when trying to follow a grant twice', async () => { | ||
| try { | ||
| await followGrant(knex, fixtures.grants.earFellowship.grant_id, fixtures.users.adminUser.id); | ||
| } catch (err) { | ||
| expect(err.code).to.equal('23505'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I suspect that this test isn't actually doing anything – given that the followGrant() function is catching and suppressing the unique constraint violation error, presumably this catch (err) block in the test is never executing. In other words, this test is probably covering line 7 of packages/server/src/lib/grantsCollaboration/followers.js, but not line 9.
As an alternative, I think it would be good to test the following behaviors:
- That an error is not thrown when trying to follow a grant twice.
- That an error is thrown when the
followGrant()function encounters an error other thanunique constraint violation(e.g. a bad foreign key user ID).
Suggested change
| it('throws an error when trying to follow a grant twice', async () => { | |
| try { | |
| await followGrant(knex, fixtures.grants.earFellowship.grant_id, fixtures.users.adminUser.id); | |
| } catch (err) { | |
| expect(err.code).to.equal('23505'); | |
| } | |
| }); | |
| it('suppresses unique constraint violations when trying to follow a grant twice', async () => { | |
| await expect(followGrant(knex, fixtures.grants.earFellowship.grant_id, fixtures.users.adminUser.id)).to.not.be.rejected; | |
| }); | |
| it('does not suppress non-unique constraint violation errors', async () => { | |
| // First ensure the test user id is invalid | |
| await expect(knex('users').where({ id: 999999999 })).to.eventually.be.an('array').that.is.empty; | |
| await expect(followGrant(knex, fixtures.grants.earFellowship.grant_id, 999999999)).to.be.rejected; | |
| }); |
Edit: Updated the above tests to be compatible with errors thrown asynchronously. Note that in order to get this to work, you'll need to update this file's test suite to use chai-as-promised. To do that, you'll need to alter the require()s at the top of the file like so:
const { expect, use } = require('chai');
const chaiAsPromised = require('chai-as-promised');
const knex = require('../../src/db/connection');
const fixtures = require('../db/seeds/fixtures');
const { saveNoteRevision, getOrganizationNotesForGrant } = require('../../src/lib/grantsCollaboration/notes');
const { followGrant, unfollowGrant } = require('../../src/lib/grantsCollaboration/followers');
use(chaiAsPromised);There was a problem hiding this comment.
ah, that makes sense. thanks for noting the chai requirement!
TylerHendrickson
approved these changes
Aug 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket #3203
Description
Stacked on top of #3336
Part 2 of #3203
Created
followGrantandunfollowGrant. I was not totally sure how to handle the following note in the ticket:So I'm not sure if my solution was what was expected - if the error is specifically a unique constraint violation, I rollback, otherwise I throw the error.
Added more tests to
packages/server/__tests__/lib/grants-collaboration.test.js.Screenshots / Demo Video
Testing
Automated and Unit Tests
Manual tests for Reviewer
Checklist