Skip to content

fix: rejected the demo/internal domains#2613

Open
anshulk-public wants to merge 5 commits into
mainfrom
fix/plg_domain_updates
Open

fix: rejected the demo/internal domains#2613
anshulk-public wants to merge 5 commits into
mainfrom
fix/plg_domain_updates

Conversation

@anshulk-public

Copy link
Copy Markdown
Contributor

Summary

  • Adds an early-exit rejection for demo/internal sites at the top of performAsoPlgOnboarding, before any org resolution, RUM verification, or entitlement work
  • A site is considered demo/internal when its ID is listed in the ASO_PLG_INTERNAL_ORG_DEMO_SITE_IDS env var
  • On rejection, the onboarding record is set to REJECTED status with a system review entry (decision: UPHELD, reason: null) and a clear justification — no waitlist reason is set
  • Covers both the full onboarding path (new record) and the PRE_ONBOARDING fast 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 30000
  • Verify rejects onboarding for preonboarded demo/internal sites before any org resolution passes — asserts REJECTED status, system review entry, no createOrFindOrganization call, no enrollment created
  • Verify rejects onboarding for demo/internal sites before any org resolution passes — same assertions for the full onboarding path
  • Verify rejects when site id is listed in ASO_PLG_INTERNAL_ORG_DEMO_SITE_IDS passes — updated from old WAITLISTED expectation to REJECTED
  • Verify reassigns preonboarded site from internal org to customer org still passes — non-demo internal sites are unaffected

🤖 Generated with Claude Code

Please ensure your pull request adheres to the following guidelines:

  • make sure to link the related issues in this description. Or if there's no issue created, make sure you
    describe here the problem you're solving.
  • when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

If the PR is changing the API specification:

  • make sure you add a "Not implemented yet" note the endpoint description, if the implementation is not ready
    yet. Ideally, return a 501 status code with a message explaining the feature is not implemented yet.
  • make sure you add at least one example of the request and response.

If the PR is changing the API implementation or an entity exposed through the API:

  • make sure you update the API specification and the examples to reflect the changes.

If the PR is introducing a new audit type:

  • make sure you update the API specification with the type, schema of the audit result and an example

Related Issues

Thanks for contributing!

@github-actions

Copy link
Copy Markdown

This PR will trigger a patch release when merged.

@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@MysticatBot MysticatBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. [Important] CLAUDE.md "Guards and Checks (in order)" section is now stale - the new demo/internal site check at onboarding-flow.js:501 runs before handleExistingOnboardedDomain but 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 steps
Non-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 reason to a machine-readable value (e.g. 'DEMO_INTERNAL_SITE') rather than null so 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 but findById resolves 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 👎.

@MysticatBot MysticatBot added the ai-reviewed Reviewed by AI label Jun 16, 2026

@MysticatBot MysticatBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. [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 reason to a machine-readable value (e.g. 'DEMO_INTERNAL_SITE') rather than null, 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, asserting reviews.length === 2 to 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 👎.

Comment thread src/controllers/plg/plg-onboarding/onboarding-flow.js Outdated
…-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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed Reviewed by AI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants