fix: rejected the demo/internal domains#2613
Conversation
|
This PR will trigger a patch 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 @anshulk-public,
Verdict: Request changes - the logic is correct but the project documentation becomes stale.
Changes: adds an early-exit rejection for demo/internal sites at the top of performAsoPlgOnboarding, preventing unnecessary org resolution and entitlement work (2 files).
Must fix before merge
- [Important] CLAUDE.md "Guards and Checks (in order)" section is now stale - the new demo/internal site check at
onboarding-flow.js:501runs beforehandleExistingOnboardedDomainbut is not listed in the documented guard ordering. Update to include it as step 1:
### Guards and Checks (in order)
1. Early demo/internal site rejection - Rejects reserved demo/internal domains before any org resolution
2. handleExistingOnboardedDomain - Duplicate domain detection
3. handlePreonboardedFastPath - Fast-track for PRE_ONBOARDING records
4. Various organization and entitlement stepsNon-blocking (4): minor issues and suggestions
- suggestion: Guard the Site lookup with
if (env.ASO_PLG_INTERNAL_ORG_DEMO_SITE_IDS)to skip the DB round-trip when no demo sites are configured -src/controllers/plg/plg-onboarding/onboarding-flow.js:501 - nit: Em-dash in log message at line 509 makes grep/log-search harder - prefer a plain dash -
src/controllers/plg/plg-onboarding/onboarding-flow.js:509 - suggestion: Set
reasonto a machine-readable value (e.g.'DEMO_INTERNAL_SITE') rather thannullso consumers can identify the rejection cause programmatically without parsing the justification string -src/controllers/plg/plg-onboarding/onboarding-flow.js:514 - suggestion: Add a test where
getSiteId()returns a value butfindByIdresolves to null (site-deleted edge case) to confirm the guard passes through correctly -test/controllers/plg/plg-onboarding.test.js
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 39s | Cost: $3.14 | Commit: 1574e246f7889a6a68078bd403e203dfc22f7784
If this code review was useful, please react with 👍. Otherwise, react with 👎.
There was a problem hiding this comment.
Hey @anshulk-public,
Verdict: Request changes - one performance concern on the hot path.
Changes: adds an early-exit rejection for demo/internal sites at the top of performAsoPlgOnboarding, preventing unnecessary org resolution and entitlement work (2 files).
Must fix before merge
- [Important] Unconditional DB lookup on every onboarding request when the feature may not be configured -
src/controllers/plg/plg-onboarding/onboarding-flow.js:504(details inline)
Non-blocking (3): minor issues and suggestions
- suggestion: Set
reasonto a machine-readable value (e.g.'DEMO_INTERNAL_SITE') rather thannull, so consumers can filter/report on rejection cause programmatically without parsing the justification string -src/controllers/plg/plg-onboarding/onboarding-flow.js:515 - nit: Em-dash character in log message at line 509 makes grep/log-search harder - prefer
--or a comma -src/controllers/plg/plg-onboarding/onboarding-flow.js:509 - suggestion: Add a test variant where
getReviews()returns an existing review array, assertingreviews.length === 2to prove existing reviews are preserved by the spread -test/controllers/plg/plg-onboarding.test.js
Previously flagged, now resolved
- CLAUDE.md "Guards and Checks (in order)" section stale - prior finding was invalid; no such section exists in this repo's CLAUDE.md. Withdrawn.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 48s | Cost: $6.74 | Commit: 6a0c5a817e9b1810305ef14cdc8c10b770c8ffdd
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…-trip Skip Site.findById/findByBaseURL when ASO_PLG_INTERNAL_ORG_DEMO_SITE_IDS is empty, eliminating a wasted DB call on every onboarding request in environments where the feature is not configured. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
performAsoPlgOnboarding, before any org resolution, RUM verification, or entitlement workASO_PLG_INTERNAL_ORG_DEMO_SITE_IDSenv varREJECTEDstatus with a system review entry (decision: UPHELD,reason: null) and a clear justification — no waitlist reason is setPRE_ONBOARDINGfast path (preonboarded record with an existing site)Why
Previously, attempting to onboard a demo/internal site would fall through to the generic "domain already assigned" WAITLISTED path deep inside Step 3 or
handlePreonboardedFastPath. This was misleading — the domain isn't waitlisted pending review, it's permanently ineligible. REJECTED is the correct terminal state and prevents any downstream work (org resolution, entitlement creation, LaunchDarkly flag updates) from running unnecessarily.Test plan
npx mocha test/controllers/plg/plg-onboarding.test.js --timeout 30000rejects onboarding for preonboarded demo/internal sites before any org resolutionpasses — assertsREJECTEDstatus, system review entry, nocreateOrFindOrganizationcall, no enrollment createdrejects onboarding for demo/internal sites before any org resolutionpasses — same assertions for the full onboarding pathrejects when site id is listed in ASO_PLG_INTERNAL_ORG_DEMO_SITE_IDSpasses — updated from oldWAITLISTEDexpectation toREJECTEDreassigns preonboarded site from internal org to customer orgstill passes — non-demo internal sites are unaffected🤖 Generated with Claude Code
Please ensure your pull request adheres to the following guidelines:
describe here the problem you're solving.
If the PR is changing the API specification:
yet. Ideally, return a 501 status code with a message explaining the feature is not implemented yet.
If the PR is changing the API implementation or an entity exposed through the API:
If the PR is introducing a new audit type:
Related Issues
Thanks for contributing!