CHI-3500: Make address values for USCH into filterable paths#950
CHI-3500: Make address values for USCH into filterable paths#950stephenhand merged 18 commits intomasterfrom
Conversation
…ath param and have service tests
There was a problem hiding this comment.
Pull Request Overview
Adds filterable, path-structured address values for USCH data and enables querying list-string-attributes by hierarchical paths (including slashes) with optional prefix filtering.
- Introduces hierarchical values and associated info for USCH state/province and city during import.
- Adds API support to fetch distinct string attributes with optional language and prefix filtering; supports keys with slashes via wildcard routes.
- Expands test coverage for list-string-attributes and reference-attributes using slash-delimited keys; adjusts CI to run all service tests.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| resources-domain/lambdas/s3-importer/src/uschMappings.ts | Builds hierarchical address paths and info for USCH mappings (Country/StateProvince and Country/StateProvince/City). |
| resources-domain/resources-service/src/resource/sql/resourceGetSql.ts | Extends SQL to return distinct value and info and to support LIKE-based prefix filtering. |
| resources-domain/resources-service/src/resource/resourceDataAccess.ts | Adds valueStartsWith param to query; returns rows for distinct value+info. |
| resources-domain/resources-service/src/resource/resourceService.ts | Plumbs valueStartsWith through service method. |
| resources-domain/resources-service/src/resource/resourceRoutesV0.ts | Adds wildcard and query-based key support for list-string-attributes; introduces a shared handler. |
| resources-domain/resources-service/src/referenceAttributes/referenceAttributeRoutesV0.ts | Switches to wildcard route to support lists with slashes. |
| resources-domain/resources-service/tests/service/listStringAttributes.test.ts | New tests for list-string-attributes including language and prefix filtering. |
| resources-domain/resources-service/tests/service/referenceAttributes.test.ts | Updates tests to use slash-delimited list names and legacy URL-encoded lists. |
| resources-domain/resources-service/tests/service/resources.elasticsearch.test.ts | Skips suggest/search suites; updates index config retrieval. |
| resources-domain/resources-service/tests/service/import.test.ts | Adjusts import statement (non-type import). |
| resources-domain/resources-service/tests/service/clearDb.ts | Adds DB cleanup helper. |
| resources-domain/resources-service/package.json | Runs all service tests in CI. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
resources-domain/resources-service/src/resource/resourceDataAccess.ts
Outdated
Show resolved
Hide resolved
| const response = await request | ||
| .get(`${basePath}/${key}${queryString.length ? '?' : ''}${queryString}`) | ||
| .set(headers); | ||
| console.log(response.body); |
There was a problem hiding this comment.
Remove the console.log from tests to keep CI output clean; if needed for debugging, prefer using test-only logging helpers or assertions that output on failure.
| console.log(response.body); | |
gpaoloni
left a comment
gpaoloni
left a comment
There was a problem hiding this comment.
Looks good to me, just a small question
| '{coverageIndex}': translatableAttributeMapping('coverage/{coverageIndex}', { | ||
| value: ({ currentValue }) => currentValue, | ||
| value: ({ currentValue }) => | ||
| `United States/${currentValue.replace(/\s+-\s+/, '/')}`, |
There was a problem hiding this comment.
Is this treating all resources as United States resources? I don't get what's the intention of this templated string 🤔
There was a problem hiding this comment.
At first look there weren't coverages for international resources so I was assuming the ones that existed were US. But there are some international coverages, but they are formatted '{country} - {stateProvince}' not '{stateProvince} - {city}'
Updated the mapping code to account for this
| router.get('/*', async (req, res) => { | ||
| const { valueStartsWith, language } = req.query; | ||
| const { list } = req.params; | ||
| const list = (req as any).params[0]; |
There was a problem hiding this comment.
Can we document what's this doing? Is params now a list or how is this working? 🤔
There was a problem hiding this comment.
Yes /* returns a list, which is documented in Express, I'm not sure we should document standard API behaviour
resources-domain/resources-service/src/resource/resourceDataAccess.ts
Outdated
Show resolved
Hide resolved
| const response = await request | ||
| .get(`${basePath}/${key}${queryString.length ? '?' : ''}${queryString}`) | ||
| .set(headers); | ||
| console.log(response.body); |
| router.get('/list-string-attributes/*', listStringAttributesHandler); | ||
| router.get('/list-string-attributes', listStringAttributesHandler); |
There was a problem hiding this comment.
What's the difference of this two endpoints, when should each be used?
There was a problem hiding this comment.
One specifies the key on the path, the other in a query item. I prefer using the path because it's more consistent with the reference-string-attributes API, but the query item form might be needed if we ever need to specify multiple keys for the same request.
Just being indecisive really, and it wasn't much extra code to support both forms
|
oooooooo... we're trying it... |
Description
info for these respective values have entries for country, state (the full name looked up against abbreviation if appropriate)
This will allow filters to be applied to the values
Checklist
Other Related Issues
None
Verification steps
See ticket
AFTER YOU MERGE
You are responsible for ensuring the above steps are completed. If you move a ticket into QA without advising what version to test, the QA team will assume the latest tag has the changes. If it does not, the following confusion is on you! :-P