feat(i18n): add locale query param to opportunity and suggestion GET endpoints | LLMO-5430#2649
Conversation
…endpoints | LLMO-5430
Add ?locale=<code> support to all opportunity and suggestion read endpoints so
that AI-generated content (title/description on opportunities; title/rationale/
description/actionItems/persona inside suggestion data) can be returned in the
user's selected language once audit workers begin storing translations.
How it works:
- Audit workers store translations in data.i18n[locale] alongside English content
- GET endpoints accept ?locale=fr_fr (or any xx_xx BCP-47 code)
- OpportunityDto promotes i18n[locale].{title,description} to the top-level fields
- SuggestionDto merges i18n[locale] on top of data and strips the i18n key
- Both DTOs strip data.i18n from the response to keep the shape stable
- Missing or absent translations fall back to the original English values
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bb567f2 to
6167fa0
Compare
There was a problem hiding this comment.
Hey @Sihalnic-Alin,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - one behavioral concern in the SuggestionDto null-data path.
Changes: adds locale query parameter to all opportunity and suggestion GET endpoints with i18n field projection and an ALLOWED_I18N_FIELDS whitelist (16 files).
Note: CI checks are currently failing (infrastructure issue in Node setup, not related to this code) - resolve before merge.
Must fix before merge
- [Important] SuggestionDto converts null
getData()to empty object, changing downstream code path behavior -src/dto/suggestion.js:101(details inline)
Non-blocking (4): minor issues and suggestions
- nit: Locale validation fires before required-param checks (siteId UUID) in all 8 handlers, producing confusing "Invalid locale format" errors when the real problem is a bad URL - consider reordering after required-param validation -
src/controllers/opportunities.js:100 - nit: OpportunityDto uses an IIFE + eslint-disable for i18n stripping while SuggestionDto uses a clean one-line destructure; unify to the simpler pattern -
src/dto/opportunity.js:55 - nit:
ALLOWED_I18N_FIELDStest re-declares the exact array and asserts equality (tautological - the "ignores disallowed fields" behavioral test already covers the same concern) -test/dto/suggestion.test.js:44 - suggestion: PR description says suggestions localize
expectedOutcomebutALLOWED_I18N_FIELDSusesaiRationale/aiSuggestioninstead - update description to match the code
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 56s | Cost: $6.18 | Commit: 92ba4431a45792c5ca29ccdbf5baba62c3ca8845
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| // Apply locale projection and strip the internal i18n key from the response | ||
| // eslint-disable-next-line no-unused-vars | ||
| const { i18n, ...baseData } = rawData ?? {}; | ||
| let data = baseData; |
There was a problem hiding this comment.
issue (blocking): The destructuring const { i18n, ...baseData } = rawData ?? {} converts a null return from getData() into {}. Previously data = suggestion.getData() preserved null through the DTO.
In the minimal/summary views, extractMinimalData guards with if (!data) return null - a null data short-circuits correctly, but {} passes this guard and enters the projection loop. While the end behavior appears identical today (the loop finds no fields and returns null), buildAggregationKeyFromSuggestion({}) vs buildAggregationKeyFromSuggestion(null) is an unverified path that could diverge.
Fix (one line) - preserve null semantics:
const { i18n, ...baseData } = rawData ?? {};
let data = rawData == null ? null : baseData;This keeps the i18n stripping intact while preserving the null signal for downstream consumers.
…endpoints | LLMO-5430 (#2651) > **Ported from #2649** (authored by @Sihalnic-Alin). That PR's branch lives on a fork, so the `MYSTICAT_DATA_SERVICE_REPO_READ_TOKEN` secret needed to install the private `mysticat-data-service` dependency isn't exposed and `npm ci` fails with exit 128. This branch is the identical change pushed to an org branch so CI runs with secrets. Original commit authorship preserved. --- ## Changes Made Adds a `?locale=` query parameter to all opportunity and suggestion GET endpoints so that AI-generated content can be returned in the user's selected language once audit workers begin storing translations alongside English content. - **`OpportunityDto`**: `toJSON(oppty, locale)` reads `data.i18n[locale]` and promotes matching `title`/`description` values to the top-level response fields; strips `data.i18n` from the response so the shape stays stable - **`SuggestionDto`**: `toJSON(suggestion, view, opportunity, locale)` merges `data.i18n[locale]` on top of the `data` object (covering `title`, `rationale`, `description`, `expectedOutcome`, `actionItems`, `persona`); strips `data.i18n` from the response - **`OpportunitiesController`**: all three GET handlers (`getAllForSite`, `getByStatus`, `getByID`) extract `locale` from query params and pass it to the DTO - **`SuggestionsController`**: all five GET handlers (`getAllForOpportunity`, `getAllForOpportunityPaged`, `getByStatus`, `getByStatusPaged`, `getByID`) extract `locale` and pass it to the DTO - **OpenAPI** (`parameters.yaml`, `site-opportunities.yaml`): new `locale` query parameter defined with regex `^[a-z]{2}_[a-z]{2}$` and referenced on all GET opportunity/suggestion endpoints - **Tests**: new `test/dto/opportunity.test.js` (8 cases) + 5 locale projection cases added to `test/dto/suggestion.test.js` **Storage contract for audit workers** (Phase 2 — separate PR per worker): Translations should be stored in `data.i18n[locale]` when creating/patching opportunities and suggestions: ```json // opportunity.data { "i18n": { "fr_fr": { "title": "...", "description": "..." } } } // suggestion.data { "i18n": { "fr_fr": { "title": "...", "rationale": "...", "actionItems": [...] } } } ``` Missing or absent translations fall back to the original English values — fully non-breaking. > **Companion FE PR**: adobe/project-elmo-ui#2124 (localizes static UI strings; Phase 3 — passing `?locale=` from the UI — is a follow-up once audit workers populate translations) ### Related Issues Relates to [LLMO-5430](https://jira.corp.adobe.com/browse/LLMO-5430) ## Testing the PR changes 1. Run unit tests: `npm test` — all 497 tests pass 2. Start the service locally (`cp .env.example .env && npm start`) 3. PATCH an opportunity to inject a test translation: ```bash curl -X PATCH \ -H "x-api-key: api_key_for_user_requests" \ -H "Content-Type: application/json" \ -d '{"data": {"i18n": {"fr_fr": {"title": "Titre de test en français"}}}}' \ "http://localhost:3002/sites/<siteId>/opportunities/<opportunityId>" ``` 4. Fetch with `?locale=fr_fr` and verify `title` returns the French value: ```bash curl -H "x-api-key: api_key_for_user_requests" \ "http://localhost:3002/sites/<siteId>/opportunities/<opportunityId>?locale=fr_fr" ``` 5. Fetch without `?locale` and verify the original English `title` is returned (non-breaking) 6. Verify `data.i18n` is absent from both responses ## Screenshots/Videos No screenshots available. ## Additional Notes N/A --------- Co-authored-by: Sihlanic-Alin <alin.sihlanic@yahoo.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Sihlanic Alin <34941191+Sihalnic-Alin@users.noreply.github.com>
Changes Made
Adds a
?locale=query parameter to all opportunity and suggestion GET endpoints so that AI-generated content can be returned in the user's selected language once audit workers begin storing translations alongside English content.OpportunityDto:toJSON(oppty, locale)readsdata.i18n[locale]and promotes matchingtitle/descriptionvalues to the top-level response fields; stripsdata.i18nfrom the response so the shape stays stableSuggestionDto:toJSON(suggestion, view, opportunity, locale)mergesdata.i18n[locale]on top of thedataobject (coveringtitle,rationale,description,expectedOutcome,actionItems,persona); stripsdata.i18nfrom the responseOpportunitiesController: all three GET handlers (getAllForSite,getByStatus,getByID) extractlocalefrom query params and pass it to the DTOSuggestionsController: all five GET handlers (getAllForOpportunity,getAllForOpportunityPaged,getByStatus,getByStatusPaged,getByID) extractlocaleand pass it to the DTOparameters.yaml,site-opportunities.yaml): newlocalequery parameter defined with regex^[a-z]{2}_[a-z]{2}$and referenced on all GET opportunity/suggestion endpointstest/dto/opportunity.test.js(8 cases) + 5 locale projection cases added totest/dto/suggestion.test.jsStorage contract for audit workers (Phase 2 — separate PR per worker):
Translations should be stored in
data.i18n[locale]when creating/patching opportunities and suggestions:Missing or absent translations fall back to the original English values — fully non-breaking.
Related Issues
Relates to LLMO-5430
Testing the PR changes
npm test— all 497 tests passcp .env.example .env && npm start)?locale=fr_frand verifytitlereturns the French value:?localeand verify the original Englishtitleis returned (non-breaking)data.i18nis absent from both responsesScreenshots/Videos
No screenshots available.
Additional Notes
N/A