feat: Migrate grants_interested data to grant_followers and update PUT handler #3593
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
…T handler - Created Knex migration to migrate existing data from grants_interested to grant_followers for records with 'Interested' status. - Updated PUT /:grantId/interested/:agencyId route to ensure grant_followers table is kept up-to-date when users mark interest in grants. - Added logic to follow/unfollow grants based on interestedCode status using followGrant and unfollowGrant functions. - Ensured backward compatibility by handling follower logic after calling markGrantAsInterested.
sushilrajeeva
requested review from
TylerHendrickson,
ekhoffman and
ClaireValdivia
github-actions
bot
added
database-changes
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: @sushilrajeeva, Action: |
TylerHendrickson
requested changes
Oct 11, 2024
There was a problem hiding this comment.
@sushilrajeeva This is looking good. I have a few comments related to error handling and atomicity of database writes for you to take a look at.
Additionally, it would be preferable for this PR to add tests for the new behavior in packages/server/__tests__/api/grants.test.js.
packages/server/src/routes/grants.js
Outdated
Comment on lines
442
to
446
| // Query to check if the interestedCode corresponds to 'Interested' | ||
| const interestedStatus = await knex('interested_codes') | ||
| .select('status_code') | ||
| .where({ id: interestedCode }) | ||
| .first(); |
There was a problem hiding this comment.
Let's run this query first in order to validate that the interestedCode provided by the request is valid before doing anything else.
Suggested change
| // Query to check if the interestedCode corresponds to 'Interested' | |
| const interestedStatus = await knex('interested_codes') | |
| .select('status_code') | |
| .where({ id: interestedCode }) | |
| .first(); | |
| // Query to check if the interestedCode corresponds to 'Interested' | |
| const interestedStatus = await knex('interested_codes') | |
| .select('status_code') | |
| .where({ id: interestedCode }) | |
| .first(); | |
| if (!interestedStatus) { | |
| res.status(400).json({ error: 'Invalid interestedCode in request' }); | |
| return; | |
| } |
packages/server/src/routes/grants.js
Outdated
Comment on lines
448
to
453
| // Follow or Unfollow the grant based on the interestedCode | ||
| if (interestedStatus?.status_code === 'Interested') { | ||
| await followGrant(knex, grantId, user.id); | ||
| } else { | ||
| await unfollowGrant(knex, grantId, user.id); | ||
| } |
There was a problem hiding this comment.
This should come before the call to db.markGrantAsInterested(), and be wrapped in a transaction so that we can roll back in case db.markGrantAsInterested() fails:
// Follow or Unfollow the grant based on the interestedCode
const trx = await knex.transaction();
if (interestedStatus?.status_code === 'Interested') {
await followGrant(trx, grantId, user.id);
} else {
await unfollowGrant(trx, grantId, user.id);
}
try {
// Calling the existing function to mark the grant as interested
await db.markGrantAsInterested({
grantId,
agencyId,
userId: user.id,
interestedCode,
});
} catch (error) {
// Roll back the follow/unfollow operation since marking grant as interested failed
trx.rollback();
console.error('Error in marking as interested:', error);
throw error;
}
// Commit the follow/unfollow operation since everything succeeded
trx.commit();Note: The order of db.markGrantAsInterested() and either followGrant() or unfollowGrant() wouldn't matter if we were able to pass the transaction to db.markGrantAsInterested(), since that would allow us to commit or rollback as a unit. Modifying that function is arguably out of scope for this issue, but just to illustrate what I mean:
async function markGrantAsInterested({
trx = knex, grantId, agencyId, userId, interestedCode,
}) {
const results = await trx(TABLES.grants_interested)
.insert({
agency_id: agencyId,
grant_id: grantId,
user_id: userId,
interested_code_id: +interestedCode,
});
return results;
}There was a problem hiding this comment.
@TylerHendrickson adding transaction to rollback if markGrantAsInterested fails is a great suggestion, thanks for the detailed implementation. I have incorporated your suggestion, I will push the code with the suggested changes.
There was a problem hiding this comment.
@TylerHendrickson I have added await to the transaction.commit() and transaction.rollback().
packages/server/src/routes/grants.js
Outdated
Comment on lines
457
to
460
| } catch (error) { | ||
| console.error('Error in marking as interested:', error); | ||
| res.status(500).send({ success: false, error: 'Internal Server Error' }); | ||
| } |
There was a problem hiding this comment.
I'd prefer to remove this wrapping try/catch block since we're not inspecting it or otherwise using it to provide any additional context to the user if something were to fail. Otherwise the try/catch will suppress the error and make this harder to monitor and debug (in Datadog).
phm200
approved these changes
Oct 11, 2024
There was a problem hiding this comment.
I tested this locally and both the migration and PUT behavior met expectations.
Specifically I tested the following locally:
- w/feature flag off, set myself as interested in two grants, then ran this branch, ran migration and turned on feature flag -> was following those two grants as expected
- turned off feature flag, marked as interested in a new grant, turned on feature flag -> was following the grant as expected
- spot checked a few grants I had not marked as interested -> not following the grant as expected
- with feature flag off, marked as interested, then un-marked/deleted interested, turn on feature flag -> not following the grant as expected
|
@sushilrajeeva just want to make sure you see Ty's comments above for change requests! |
|
@TylerHendrickson I have updated the test case for the PUT function. Specifically, I modified the previous implementation by adding a GET operation after performing the PUT request. This allows me to validate whether the agency.agency_id returned is correct. If there are any additional checks or improvements you'd recommend for this test, please let me know. |
TylerHendrickson
approved these changes
Oct 17, 2024
sushilrajeeva
deleted the
feat/sushilrajeeva/data-migration-status-to-follow+note
branch
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.
…T
Ticket #2961
Description
This PR addresses the requirements outlined in the issue #2961 as follows:
grants_interestedtable to thegrant_followerstable where theinterested_codereferences records withstatus_code = 'Interested'.PUT /:grantId/interested/:agencyIdroute to keep thegrant_followerstable in sync when users update their interest status. After marking a grant as "Interested" via themarkGrantAsInterested()function, the handler now follows/unfollows grants based on theinterestedCodeprovided.Screenshots / Demo Video
No UI changes. Backend logic updates and database migrations.
Testing
Automated and Unit Tests
Manual tests for Reviewer
Checklist