feat: request access Semrush api integration - #3039
Conversation
# Conflicts: # docs/index.html
|
This PR will trigger a minor release when merged. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Hey @vivesing,
Verdict: Request changes - two blocking issues around response contract and missing timeout.
Complexity: HIGH - large diff; API surface + external integration.
Changes: Extends the Semrush onboarding endpoint from a Slack-notification-only flow into actual workspace provisioning via Semrush's Adobe IMS Workspace Provisioning API (12 files).
Must fix before merge
- [Important] 2xx with unparseable body returns undefined fields, violating the OpenAPI contract that declares workspaceId and role as required -
src/support/onboarding/workspace-provisioning.js:65(details inline) - [Important] No request timeout on outbound fetch to Semrush, inconsistent with the codebase's 15s timeout pattern -
src/support/onboarding/workspace-provisioning.js:49(details inline) - [Important] 409 (user already a member) triggers a Slack failure alert and error response, but this is a success from the user's perspective -
src/controllers/onboarding.js:74(details inline)
Non-blocking (4): minor issues and suggestions
- suggestion: Map known upstream status codes (409/422/403) to caller-facing messages instead of the generic "Failed to provision Semrush workspace access" for all failures -
src/controllers/onboarding.js:95 - suggestion: Upstream error status codes pass through verbatim to the caller; consider mapping to a bounded set so a future Semrush 429 does not leak through undocumented -
src/controllers/onboarding.js:66 - nit: OpenAPI spec does not document the 503 that surfaces when SEMRUSH_PROJECTS_BASE_URL is unset -
docs/openapi/onboarding-api.yaml - nit: Lockfile includes peer-flag churn on ~25 packages without a corresponding package.json change or PR-body explanation -
package-lock.json
Note: Recommend a human read before merge - possible divergence from the documented design (heuristic, flagged by the conventions reviewer). The workspace provisioning flow is not documented in the referenced Semrush IMS integration proposal.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 27s | Cost: $9.03 | Commit: 572806729bd6ef941bd0c62c57b5a28bc4e91725
If this code review was useful, please react with 👍. Otherwise, react with 👎.
|
Fixes applied
Updated OpenAPI (OnboardingProvisioningResponse gained optional alreadyMember; removed the now-unreachable 409 response doc; 502 doc mentions timeout/invalid-response causes) and added corresponding unit tests in both workspace-provisioning.test.js and onboarding.test.js (13 new/changed test cases total, all passing). |
There was a problem hiding this comment.
Hey @vivesing,
Verdict: Approve - all prior blocking findings addressed, no new blocking issues.
Complexity: HIGH - large diff; API surface + dependency churn.
Changes: Extends the Semrush onboarding endpoint from Slack-notification-only into actual workspace provisioning via Semrush's Adobe IMS Workspace Provisioning API (12 files).
Note: Recommend a human read before merge - this change modifies a shared API contract (OpenAPI spec). The bot review is a complement to, not a replacement for, a human read here.
Non-blocking (1): minor issues and suggestions
- nit: JSDoc @returns declares email and organizationId as string but runtime uses optional chaining (body?.email) and can return undefined when Semrush omits them -
src/support/onboarding/workspace-provisioning.js:76
Previously flagged, now resolved
- 2xx with unparseable body now validated: throws 502 on missing workspace_id/role
- Fetch timeout added via AbortSignal.timeout(DEFAULT_TIMEOUT_MS), reusing the shared 15s constant
- 409 from Semrush now treated as success: returns 200 with alreadyMember flag, no Slack alert
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 5s | Cost: $7.03 | Commit: 63b9140112ae93227ae389e697d6a97ddbb43213
If this code review was useful, please react with 👍. Otherwise, react with 👎.
# Conflicts: # docs/index.html
Code review —
|
Summary
Extends
POST /v2/orgs/:spaceCatId/semrush-onboardingfrom a Slack-notification-only endpoint into one that actually provisions the caller's Semrush workspace access via Semrush's new Adobe IMS Workspace Provisioning API. Slack is now used only as a failure alert (manual follow-up), not a success notification.✅ Semrush api to add user is available on prod
POST https://www.semrush.com/enterprise/users/api/v1/adobe-ims/workspace-members
Changes
Controller
src/controllers/onboarding.jsresolveSemrushImsTokenhelper (x-promise-tokenheader → IMS exchange). Returns401if the header is missing/invalid.provisionWorkspaceMember(env, imsToken)to grant the caller admin access to their org's Semrush workspace.200 { provisioned: true, workspaceId, role }— no Slack call.409 Conflictalready covers repeat-call de-duplication.New support module
src/support/onboarding/workspace-provisioning.js(new)provisionWorkspaceMember(env, imsToken)—POST {SEMRUSH_PROJECTS_BASE_URL}/enterprise/users/api/v1/adobe-ims/workspace-members, sending the IMS token on both theAuthorization: Bearerheader and the{ token }JSON body.SerenityTransportErrorcarrying the upstream status (400/401/403/409/422/500) on a non-2xx response, or502on a netwSlack notifier (repurposed) - **`src/support/onboarding/slack-no
notifyOnboarding→notifyProvisioningFailure.:warning:failure alert, with an optionalreasonline.Shared transport
baseUrl()helper (resolves/validatesSEMRUSH_PROJECTS_BASE_URL) for reuse by the new onboarding module.OpenAPI
docs/openapi/onboarding-api.yamlx-promise-tokenheader parameter.200schema withOnboardingProvisioningResponse; added401/409/422responses;updated
400/403/502descriptions pented error table.docs/openapi/schemas.yamlOnboardingNotificationProvisioningResponse(provisioned,workspaceId,role).docs/index.html— regenerated vianpm run docs:build.Tests
test/controllers/onboarding.test.js— rewritten: missing-promise-token → 401; successful provisioning → no Slack call; Semrush failure → Slack alert sent + mapped status; unexpected/network errors → 500/502 + alert; email fallback/unknowncases.test/support/onboarding/slack-ned for thenotifyProvisioningFailure` rename and the new failure-alert message shape.Not in scope / follow-ups
SEMRUSH_USERS_BASE_URLvsSEMRUSH_PROJECTS_BASE_URL: currently readingSEMRUSH_PROJECTS_BASE_URLdirectly (adobe-hackathon.semrush.com), per confirmation that this will be repointed towww.semrush.com.