Repository navigation
Support TerminologySets - #2425
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2425 +/- ##
==========================================
+ Coverage 93.68% 93.73% +0.05%
==========================================
Files 303 312 +9
Lines 3817 3929 +112
Branches 491 533 +42
==========================================
+ Hits 3576 3683 +107
- Misses 240 245 +5
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🔎 A preview deployment of this pull request is available at https://utilities.philanthropydatacommons.org/pdc-service-preview-2425-q4xrr/ (and upstream https://pdc-service-preview-2425-q4xrr.ondigitalocean.app) assuming this PR is still open. 🔍 |
a3ce2fe to
fe7ba31
Compare
fe7ba31 to
cdebfee
Compare
|
I found this in the DO logs:
|
This is a WIP for a reason :D |
d994293 to
2b85d19
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new first-class TerminologySet entity that lets funders define custom display labels (singular/plural) for canonical PDC concepts (Opportunity, Proposal, Application Form), and wires those sets into permissions, API surface area, and opportunity/funder behavior.
Changes:
- Added
TerminologySettype + persistence (migrations, SQL JSON functions, CRUD queries/operations) and exposedGET/POST/PATCH /terminologySets. - Extended permission model to include
terminologySetas a context entity type/scope, including server-side permission checks and OpenAPI updates. - Added opportunity and funder linkage via
opportunities.terminology_set_idandfunders.default_terminology_set_id, including JSON response shaping and integration tests.
Reviewed changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/TerminologySet.ts | Adds TS type + AJV schemas for create/patch payload validation. |
| src/types/PermissionGrantEntityType.ts | Adds TERMINOLOGY_SET context type and context key mapping. |
| src/types/PermissionGrantCondition.ts | Adds empty condition set for TERMINOLOGY_SET. |
| src/types/Opportunity.ts | Adds terminologySetId + embedded terminologySet to opportunity type/schema. |
| src/types/index.ts | Re-exports TerminologySet types. |
| src/types/Funder.ts | Adds defaultTerminologySetId to funder type/schema. |
| src/test/integrationSuiteSetup.ts | Resets the new terminology set factory between integration tests. |
| src/test/factories/index.ts | Exports the new terminology set factory helpers. |
| src/test/factories/createTestTerminologySet.ts | Adds factory for creating terminology sets in integration tests. |
| src/test/factories/createTestOpportunity.ts | Updates opportunity factory defaults for new field. |
| src/test/factories/createTestFunder.ts | Updates funder factory defaults for new field. |
| src/routers/terminologySetsRouter.ts | Adds /terminologySets router with POST/GET/GET-by-id/PATCH endpoints. |
| src/routers/index.ts | Mounts the terminology sets router. |
| src/openapi/paths/terminologySets.json | Documents list/create terminology sets endpoints. |
| src/openapi/paths/terminologySet.json | Documents get-by-id and patch-by-id terminology set endpoints. |
| src/openapi/components/schemas/TerminologySetPermissionGrant.json | Adds OpenAPI schema for terminology-set-scoped permission grants. |
| src/openapi/components/schemas/TerminologySetPatch.json | Adds OpenAPI schema for patch payload. |
| src/openapi/components/schemas/TerminologySetBundle.json | Adds OpenAPI bundle schema for list responses. |
| src/openapi/components/schemas/TerminologySet.json | Adds OpenAPI schema for terminology set entity. |
| src/openapi/components/schemas/PermissionGrantEntityType.json | Adds terminologySet to OpenAPI entity type enum. |
| src/openapi/components/schemas/Opportunity.json | Documents terminologySetId + embedded terminologySet on opportunities. |
| src/openapi/components/schemas/FunderPermissionGrant.json | Updates OpenAPI funder grant scope enum to include terminologySet (and applicationForm). |
| src/openapi/components/schemas/Funder.json | Documents defaultTerminologySetId on funders. |
| src/openapi/api.json | Wires new schemas + paths into the OpenAPI root. |
| src/handlers/terminologySetsHandlers.ts | Implements HTTP handlers with auth + permission checks for terminology sets. |
| src/handlers/opportunitiesHandlers.ts | Enforces `reference |
| src/handlers/fundersHandlers.ts | Passes through defaultTerminologySetId on funder upsert. |
| src/database/queries/terminologySets/updateById.sql | Adds SQL update for terminology set patches. |
| src/database/queries/terminologySets/selectWithPagination.sql | Adds list query with permission filtering + optional funder filter. |
| src/database/queries/terminologySets/selectById.sql | Adds by-id query with permission filtering. |
| src/database/queries/terminologySets/insertOne.sql | Adds insert query for terminology sets. |
| src/database/queries/permissionGrants/updateById.sql | Adds terminology_set_id to permission grant updates. |
| src/database/queries/permissionGrants/insertOne.sql | Adds terminology_set_id to permission grant inserts. |
| src/database/queries/opportunities/insertOne.sql | Adds terminology_set_id column and funder-default pinning behavior. |
| src/database/queries/funders/insertOrUpdateOne.sql | Adds default_terminology_set_id to funder upsert. |
| src/database/queries/authorization/hasTerminologySetPermission.sql | Adds DB query wrapper for terminology set permission checks. |
| src/database/operations/terminologySets/updateTerminologySet.ts | Adds terminology set update operation wrapper. |
| src/database/operations/terminologySets/loadTerminologySetBundle.ts | Adds terminology set bundle load operation wrapper. |
| src/database/operations/terminologySets/loadTerminologySet.ts | Adds terminology set load-by-id operation wrapper. |
| src/database/operations/terminologySets/index.ts | Exports terminology set operations. |
| src/database/operations/terminologySets/createTerminologySet.ts | Adds terminology set create operation wrapper. |
| src/database/operations/permissionGrants/updatePermissionGrant.ts | Adds terminologySetId to update operation attribute list. |
| src/database/operations/permissionGrants/createPermissionGrant.ts | Adds terminologySetId to create operation attribute list. |
| src/database/operations/opportunities/createOpportunity.ts | Adds terminologySetId to opportunity create operation attribute list. |
| src/database/operations/index.ts | Exports terminology set operations from the database module. |
| src/database/operations/funders/createOrUpdateFunder.ts | Adds defaultTerminologySetId to funder upsert operation attribute list. |
| src/database/operations/authorization/index.ts | Exports hasTerminologySetPermission. |
| src/database/operations/authorization/hasTerminologySetPermission.ts | Adds operation wrapper for terminology set permission checks. |
| src/database/migrations/0116-alter-funders-add-default_terminology_set.sql | Adds funders.default_terminology_set_id with FK semantics. |
| src/database/migrations/0115-alter-opportunities-add-terminology_set.sql | Adds opportunities.terminology_set_id with composite FK to funder-scoped sets. |
| src/database/migrations/0114-alter-permission_grants-add-terminology_set.sql | Adds permission_grants.terminology_set_id and FK constraint wiring. |
| src/database/migrations/0113-add-terminology_set-to-permission_grant_entity_type_t.sql | Adds terminologySet to the DB enum type. |
| src/database/migrations/0112-add-terminology_sets.sql | Creates terminology_sets table and audits it. |
| src/database/initialization/terminology_set_to_json.sql | Adds SQL JSON serializer for terminology sets. |
| src/database/initialization/permission_grant_to_json.sql | Adds terminologySet key serialization for permission grants. |
| src/database/initialization/opportunity_to_json.sql | Adds terminology set embedding in opportunity JSON. |
| src/database/initialization/has_terminology_set_permission.sql | Adds SQL permission function for terminology sets. |
| src/database/initialization/funder_to_json.sql | Adds defaultTerminologySetId to funder JSON. |
| src/database/initialization/can_manage_permission_grant.sql | Adds manage check path for terminology-set-scoped permission grants. |
| src/tests/terminologySets.int.test.ts | Adds integration tests covering CRUD, permissions, FK scoping, and default pinning behavior. |
| src/tests/funders.int.test.ts | Updates integration tests for the new funder JSON field. |
| docs/PERMISSIONS.md | Documents terminology set permissions and funder permission scopes. |
| docs/GLOSSARY.md | Adds glossary entry for terminology sets. |
| docs/ENTITY_RELATIONSHIP_DIAGRAM.md | Updates ERD with terminology sets + new FK relationships. |
| CHANGELOG.md | Notes new entity/endpoints/permissions and opportunity/funder linkage behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2b85d19 to
c357d34
Compare
bickelj
left a comment
There was a problem hiding this comment.
When reading code (not actually trying it) I don't see anything truly alarming but I have some questions in-line.
| } | ||
| TerminologySet { | ||
| int id PK | ||
| string funderShortCode FK |
There was a problem hiding this comment.
At first glance there appears to be a circular reference (TerminologySet.funderShortCode and Funder.defaultTerminologySetId) but it is not really circular because there can be many TerminologySets created.
I wonder, though, if we even need the TerminologySet to be associated with a funderShortCode. Why should the definition of the terminology need to be associated with the funder? We have the user who created it, and perhaps the user can grant to the funder's organization the permissions to manage it. And we already have the defaultTerminologySetId on funder and also a terminologySetId on opportunity.
There was a problem hiding this comment.
Yeah this is valid third normal form for sure; it isn't circular because default is nullable.
We do need a schema-level (not just permission) association between terminology sets and funders because terminology sets are a by funders. Users are not a proxy for organization at the schema level, since a user's association with any given organization is permission based rather than schema based.
It might help to think in terms of UX:
- When a funder creates an opportunity, they can choose a terminology set to use within that opportunity.
- The dropdown would show a list of all terminology sets associated with the funder (that they have permission to view).
- The default value of that dropdown would be driven by
defaultTerminologySetId.
The user makes a selection, hits submit, and the opportunity is created.
There was a problem hiding this comment.
I've updated the API to remove any actual "use" of the default -- it is now just information for clients to leverage if they want.
There was a problem hiding this comment.
The situation I am thinking of is this:
- A funder creates a terminology set with very commonly used terminology and sets it as default for its opportunities.
- Another funder cannot see that terminology set so creates an identical terminology set and sets it as default for its opportunities.
It seems to me that terminology (language) may be used by a particular organization but is not always particular to that organization.
We tend to avoid ownership in other cases so the alternative would be to use permissions (or lack thereof) rather than ownership. Like this:
- All terminology sets are always viewable by anyone.
- Authenticated users (or perhaps only users associated with funders) can create terminology sets.
- There is no funder associated with a terminology set.
- The user who creates the terminology set can manage it. Alternative: terminology sets are immutable.
The downside to this setup is change over time: a funder could not modify its existing terminology set and have that edit apply to all the things without going and touching all the things.
Therefore, having thought through it a bit more, it is not so bad to have the funder associated, with the exception that the funder's name will be visible to those who have access to that terminology set, when a funder may want to be a secret funder.
| | edit | proposal | Create or update proposal versions for the funder's proposals | | ||
| | view | source | View the funder's sources | | ||
| | create | source | Create sources associated with the funder | | ||
| | view | terminologySet | View the funder's terminology sets | |
There was a problem hiding this comment.
I am trying to imagine a case where a funder would like some other party to be able to view the funder but then not view the preferred terminology of that funder.
There was a problem hiding this comment.
We could say that viewing a funder means viewing all of the funder's terminology sets, but it is starting to stray from the grant permission design philosophy.
Also remember this also covers the inverse (you can view the terminology set but not the funder)
There was a problem hiding this comment.
Oh, yes, that last capability makes sense.
There was a problem hiding this comment.
Except, if we are using funder_short_code in the terminology set is that attribute filtered out on GET?
There was a problem hiding this comment.
No -- that isn't the behavior across any of our FK relationships (nor should it be).
If you have the permission to view an entity you can view all of its attributes.
(now, you wouldn't see the actual OBJECT unless you have permission!)
There was a problem hiding this comment.
Rather than thinking in terms of the technical implementation (FK relationships) I'm thinking more in terms of UX: if you can view the terminology set but not the funder, we can't truly do that if the funder name is part of the terminology set.
9792718 to
bdb1f60
Compare
TerminologySets will allow users to specify alternative terms to use for certain PDC entities. This will help funders, in particular, to translate their own concepts to PDC concepts more explicitly (e.g. they might call a "Proposal" a "Funding Request"). Issue #2330 Support TerminologySets for Opportunities to support custom terms for proposal concepts
We want users to be able to manage `TerminologySets` like other entities; this adds permission support and also updates the CRUD endpoints to no longer be administrator only. Issue #2330 Support TerminologySets for Opportunities to support custom terms for proposal concepts
Opportunities are where terminology sets actually apply. This association is what will allow a client to know when to use a given terminology set. Issue #2330 Support TerminologySets for Opportunities to support custom terms for proposal concepts
Funders can select a terminology set that they want UX to use by default when creating opportunities. This is just a setting and is not enforced by API logic. Issue #2330 Support TerminologySets for Opportunities to support custom terms for proposal concepts
bdb1f60 to
70bae33
Compare
bickelj
left a comment
There was a problem hiding this comment.
My objections have been settled, having skimmed the code this looks like a great setup that will help a lot.
I was able to create a terminology set in the preview environment like this (omitting some of the terminology I didn't need):
{
"funderShortCode": "test_funder",
"name": "RFP Vocabulary",
"opportunityLabel": "Request for Proposal",
"proposalsLabel": "Funding Requests"
}I did not try the permissions part but I trust the automated tests.
This PR introduces a new entity type,
TerminologySet, which allows funders to define new labels for specific PDC entity types (Opportunity, Proposal, and Application Form) in the context of a particular opportunity.The PR includes CRUD endpoints, documentation, and permission support.
Resolves #2330