fix: move on-demand validation to /geo-experiments/:geoExperimentId/validate - #3045
Conversation
…alidate The endpoint validates a GeoExperiment's own suggestions, not its linked opportunity, so geoExperimentId belongs in the route (matching the sibling PATCH/DELETE/trigger-impact-measurement GeoExperiment routes) instead of the request body under /opportunities/:opportunityId/validate. The linked opportunity is now resolved internally via geoExperiment.getOpportunityId() to pick the import-worker message type. Moves the trigger logic into suggestions.js alongside its GeoExperiment siblings instead of keeping a separate controller file.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…lidate-endpoint # Conflicts: # docs/index.html
|
This PR will trigger a patch release when merged. |
There was a problem hiding this comment.
Hey @jindaliiita,
Verdict: Request changes - two behavioral contract changes need explicit confirmation before merge.
Complexity: HIGH - large diff; API route relocation + shared contract change (OpenAPI).
Changes: Relocates the on-demand validation endpoint from the opportunity resource to the geo-experiment resource, adds access control, and resolves the linked opportunity internally (13 files).
Note: Recommend a human read before merge - this change modifies shared API contracts (OpenAPI spec). The bot review is a complement to, not a replacement for, a human read here.
Must fix before merge
- [Important] S2S access contract change: old route was in
routeRequiredCapabilitieswithopportunity:write(S2S consumers could call it); new route is inINTERNAL_ROUTESonly (S2S consumers are now denied) -src/routes/required-capabilities.js:64(details inline) - [Important] Behavioral tightening: GeoExperiments with no linked opportunity now get 400, whereas the old endpoint could accept them -
src/controllers/suggestions.js:2801(details inline)
Non-blocking (5): minor issues and suggestions
- nit: error messages say "Site ID required" / "GeoExperiment ID required" when the actual issue is invalid UUID format; sibling code says "must be a valid UUID" -
src/controllers/suggestions.js:2780 - suggestion: define
geoExperimentIdpath param via$ref: './parameters.yaml#/geoExperimentId'instead of inline, matching how sibling endpoints use shared parameter definitions -docs/openapi/site-opportunities.yaml:276 - suggestion:
GeoExperimentValidationResponseschema should mark its four always-present fields asrequiredto strengthen the contract -docs/openapi/schemas.yaml:3242 - nit: add a comment on the new
INTERNAL_ROUTESentry explaining why validate is internal (mirrors sibling routes but the existing block has section comments for each group) -src/routes/required-capabilities.js:64 - nit:
Configuration.findLatest()andsqs.sendMessage()have no null/error guards, but this is a pre-existing pattern across all sibling methods, not a regression -src/controllers/suggestions.js:2819
Note: CI checks are passing.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 50s | Cost: $6.14 | Commit: 84764b5a937b60ec1899c207ba2cd531e8b826ac
If this code review was useful, please react with 👍. Otherwise, react with 👎.
siteId, geoExperimentId, opportunityId, and status are unconditionally set on every successful (202) response, so document that guarantee in the OpenAPI schema per MysticatBot review feedback on #3045.
There was a problem hiding this comment.
Hey @jindaliiita,
Verdict: Approve - both prior blocking findings resolved through convincing author responses; new commit addresses the schema suggestion.
Complexity: HIGH - large diff; API route relocation + shared contract change (OpenAPI).
Changes: Relocates the on-demand validation endpoint from the opportunity resource to the geo-experiment resource, consolidates the controller into suggestions.js, updates access control and OpenAPI docs (13 files).
Note: Recommend a human read before merge - this change modifies shared API contracts (OpenAPI spec). The bot review is a complement to, not a replacement for, a human read here.
Note: CI checks are currently pending - confirm passing before merge.
Non-blocking (2): minor issues and suggestions
- nit: error messages say "Site ID required" / "GeoExperiment ID required" when the actual validation failure is an invalid UUID format; sibling code uses "must be a valid UUID" -
src/controllers/suggestions.js:2780 - suggestion: define
geoExperimentIdpath param via$ref: './parameters.yaml#/geoExperimentId'instead of inline, matching how sibling endpoints use shared parameter definitions -docs/openapi/site-opportunities.yaml:917
Previously flagged, now resolved
- S2S access contract concern: author confirmed endpoint is UI-only, never consumed by S2S in its 4-day lifetime at the old path, consistent with all sibling geo-experiment routes in INTERNAL_ROUTES.
- Behavioral tightening (null opportunity 400): author explained the old design was semantically broken (could validate against an unlinked opportunity); the new behavior is more correct, not just stricter.
- GeoExperimentValidationResponse schema now marks all four fields as required (addressed by new commit 1f97a99).
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 3m 47s | Cost: $1.44 | Commit: 1f97a998c397583ca14ebd2499ca577d9001c382
If this code review was useful, please react with 👍. Otherwise, react with 👎.
## [1.749.1](v1.749.0...v1.749.1) (2026-08-15) ### Bug Fixes * move on-demand validation to /geo-experiments/:geoExperimentId/validate ([#3045](#3045)) ([1dd3891](1dd3891))
|
🎉 This PR is included in version 1.749.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
POST /sites/:siteId/opportunities/:opportunityId/validate(withgeoExperimentIdin the body) toPOST /sites/:siteId/geo-experiments/:geoExperimentId/validate— this validates the GeoExperiment's own suggestions, not its linked opportunity, sogeoExperimentIdbelongs in the route, matching the sibling PATCH/DELETE/trigger-impact-measurement GeoExperiment routes.geoExperiment.getOpportunityId()instead of being passed in.opportunity-validation-controller.jsintosuggestions.jsastriggerGeoExperimentValidation, alongside its GeoExperiment siblings (triggerImpactMeasurement, etc.) instead of keeping it in its own file.src/routes/index.js), capability gating (INTERNAL_ROUTESinrequired-capabilities.js,llmo/can_configure+aso/can_editinfacs-capabilities.js), and OpenAPI docs (api.yaml,site-opportunities.yaml,schemas.yaml) to match.Test plan
npx mochaontest/controllers/suggestions.test.js,test/routes/index.test.js,test/routes/required-capabilities.test.js,test/routes/facs-capabilities.test.js,test/index.test.js— 4012 passing, 0 failingnpm run linton all changed files — cleannpm test) — passes the repo's 90% thresholdnpm run docs:lint— OpenAPI spec valid, no new warningsnpm run docs:build—docs/index.htmlregenerated🤖 Generated with Claude Code