3372 - adds database changes to support managing IAA subrecipients #3522
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
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: @as1729, Action: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/server/__tests__/arpa_reporter/server/db/arpa-subrecipients.spec.js
Fixed
Show fixed
Hide fixed
packages/server/__tests__/arpa_reporter/server/db/arpa-subrecipients.spec.js
Fixed
Show fixed
Hide fixed
packages/server/__tests__/arpa_reporter/server/db/arpa-subrecipients.spec.js
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
@as1729 A few initial comments for you to consider.
Additionally, would you mind filling out the description for this PR when you get a chance? I think that would help clarify the intended changes regarding the behavior of the new and existing fields (name, uei, and tin) for reviewers, especially since #3372 is showing up as closed by #3503.
packages/server/__tests__/arpa_reporter/server/helpers/with-tenant-id.js
Outdated
Show resolved
Hide resolved
packages/server/migrations/20240918183745_drop_constraints_to_arpa_subrecipients.js
Show resolved
Hide resolved
Comment on lines
5
to
9
| exports.up = function (knex) { | ||
| return knex.schema.table('arpa_subrecipients', (table) => { | ||
| table.text('name'); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Is any kind of unique constraint/index warranted here?
There was a problem hiding this comment.
yes good-call 👍
We only want to enforce it when both uei and tin are missing so as to not interfere with previous data in the database so I'll create an index with the where-clause as described above.
tzinckgraf
reviewed
Sep 19, 2024
| } else if (fieldType === 'tin') { | ||
| query.where('arpa_subrecipients.tin', value); | ||
| } else if (fieldType === 'name') { | ||
| query.where('arpa_subrecipients.name', value); | ||
| } else { | ||
| return null; |
There was a problem hiding this comment.
It might be worth logging or printing something here or failing if a fieldType is passed in that doesn't match uei, tin or name.
We should also document the expected fieldType values in a docstring.
There was a problem hiding this comment.
good call- I updated docstrings as well as updated the method to throw an error instead of returning null as it can be confusing for the caller.
tzinckgraf
reviewed
Sep 19, 2024
| } | ||
| } | ||
| if (!recipient.tin && !recipient.uei && recipient.name) { | ||
| const existingRecipient = await findRecipient('name', recipient.name, trns); |
There was a problem hiding this comment.
Can the same name exist across multiple tenants?
There was a problem hiding this comment.
yes, findRecipient scopes by tenant by default, so we should only be looking for duplicates within a tenant.
…a wrong field-type
…t null. add unit-tests to ensure indexes work correctly
…constraint or plain index
packages/server/migrations/20240918183915_add_name_to_arpa_subrecipients.js
Show resolved
Hide resolved
| table.text('name'); | ||
| }); | ||
| await knex.raw( | ||
| 'CREATE UNIQUE INDEX idx_arpa_subrecipients_tenant_id_name_unique ON arpa_subrecipients (tenant_id, name) WHERE name is not null and uei is null and tin is null', |
There was a problem hiding this comment.
In a situation like:
nameis not nullueiis not nulltinis null
Should name still be unique to the tenant_id? Similarly, could this be simplified to (tenant_id, name) WHERE name is not null i.e. should duplicate non-null names for the same tenant allowed in any circumstance, regardless of uei or tin?
Also we may want an additional constraint to ensure that at least one of uei/tin/name is provided. If that sounds like a good idea to you, I think that could be implemented as a CHECK constraint like:
ALTER TABLE arpa_subrecipients
ADD CONSTRAINT
chk_at_least_one_of_uei_tin_name_not_null
CHECK (num_nonnulls(uei, tin, name) > 0);There was a problem hiding this comment.
i.e. should duplicate non-null names for the same tenant allowed in any circumstance, regardless of uei or tin?
Right now we're working with insufficient data here. Working assumption is that there could be subrecipients in the system that share the same name but have slightly different UEI/TIN since this was never enforced in the past.
Also we may want an additional constraint to ensure that at least one of uei/tin/name is provided. If that sounds like a good idea to you, I think that could be implemented as a CHECK constraint like:
This makes sense to add since we enforce this on the application layer, can translate to a database check as well.
Comment on lines
95
to
97
| if (!(recipient.uei || recipient.tin || recipient.name)) { | ||
| throw new Error('recipient row must include a `uei`, `tin`, or `name` field'); | ||
| } |
There was a problem hiding this comment.
I like how the createRecipient() function's various if ... throw blocks make the business logic clear to developers and provide friendly error messages for users; I'm wondering if we could preserve that clarity while making better use of the db-level constraints by checking the cause of a failure after inserting rather than checking ahead of time (which can fail to identify the problem ahead of time in a double-click situation or similar race condition).
I think something like this might work:
async function createRecipient(recipient, trns = knex) {
const tenantId = useTenantId();
try {
return await trns('arpa_subrecipients')
.insert({ ...recipient, tenant_id: tenantId })
.returning('*')
.then((rows) => rows[0]);
} catch (e) {
switch (e.constraint) {
case 'chk_at_least_one_of_uei_tin_name_not_null':
throw new Error('recipient row must include a `uei`, `tin`, or `name` field');
case 'idx_arpa_subrecipients_tenant_id_uei_unique':
throw new Error('A recipient with this UEI already exists');
case 'idx_arpa_subrecipients_tenant_id_tin_unique':
throw new Error('A recipient with this TIN already exists');
// Other `case 'name_of_constraint_or_index'` statements go here...
default:
// Something else
throw e;
}
}
}(See my previous comment regarding the chk_at_least_one_of_uei_tin_name_not_null constraint.)
There was a problem hiding this comment.
This is much cleaner since we don't need to duplicate the validation logic in both layers. Will update the code 👍
There was a problem hiding this comment.
added in the latest commit
… receive an error
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 #3372
Description
This PR adds support for the new IAA subrecipient field. The goal here is to ensure that we can add subrecipients that do not have either a UEI or TIN value. In order to do this the PR does the following:
namefield toarpa_subrecipientstable and a correspondingunique indexthat ensures that when bothueiandtinare missing, that we do not create duplicate subrecipients with the samename.createSubrecipientfunction is called.uniqueconstraint onueiandtinfields and replaces with auniqueindex that only applies when these fields areNOT NULL.Screenshots / Demo Video
State Before migrations
State After Migrations
Confirming that Rollback works
Testing
Automated and Unit Tests
Manual tests for Reviewer
Checklist