fix: updated delivery type mismatch#2707
Conversation
|
This PR will trigger a patch release when merged. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Hey @anshulk-public,
⚠ 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 - two new code paths lack test coverage.
Complexity: LOW - small diff, single-service bug fix.
Changes: Moves delivery type mismatch detection from Step 5a (before auto-correction) to Step 5f (after), and adds rumHost-based pattern matching before falling back to findDeliveryType (3 files).
Note: CI checks are currently pending - confirm they pass before merge.
Must fix before merge
- [Important] Missing test for AEM_CS publish host pattern detection -
src/controllers/plg/plg-onboarding/onboarding-flow.js:701(details inline) - [Important] Missing test for unrecognised rumHost falling through to findDeliveryType -
src/controllers/plg/plg-onboarding/onboarding-flow.js:705(details inline)
Non-blocking (3): minor issues and suggestions
- nit: Removed
expect(findDeliveryTypeStub).to.have.been.calledOnceWith(TEST_BASE_URL)assertion in the fallback test weakens the contract check - consider restoring acalledWith(TEST_BASE_URL)assertion -test/controllers/plg/plg-onboarding/site-setup.test.js:414 - suggestion: Use
EDS_HOST_PATTERN.test(rumHost)instead ofrumHost.match(EDS_HOST_PATTERN)when only a boolean is needed (avoids allocating a match array) -src/controllers/plg/plg-onboarding/onboarding-flow.js:699 - suggestion: Consider adding unit tests for
AEM_CS_PUBLISH_HOST_PATTERNregex edge cases (.netvariant, uppercase, non-matching hosts with similar prefixes) in validation's test file -src/controllers/plg/plg-onboarding/validation.js:19
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 4s | Cost: $3.70 | Commit: 6f06b7af9bacad19dfd88b543587f363f1460a72
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| // Skip for newly created sites and preset-delivery-type flows — no prior type to compare. | ||
| // Detection order: rumHost (EDS pattern → AEM_EDGE, CS publish pattern → AEM_CS), | ||
| // falling back to findDeliveryType(baseURL) only when rumHost is absent or unrecognised. | ||
| if (!presetDeliveryType && !steps.siteCreated) { |
There was a problem hiding this comment.
issue (blocking): The AEM_CS_PUBLISH_HOST_PATTERN branch is a newly introduced code path with a new regex, but has zero test coverage. The EDS pattern path has a dedicated test (alerts via Slack using rumHost EDS pattern without calling findDeliveryType) - add an equivalent for the CS publish pattern.
Suggested test: set autoResolveAuthorUrlStub to resolve { host: 'publish-p12345-e67890.adobeaemcloud.com' } with stored delivery type aem_edge, assert the alert fires with detected type aem_cs, and findDeliveryTypeStub is NOT called.
| let detectedDeliveryType; | ||
| if (rumHost) { | ||
| if (rumHost.match(EDS_HOST_PATTERN)) { | ||
| detectedDeliveryType = SiteModel.DELIVERY_TYPES.AEM_EDGE; |
There was a problem hiding this comment.
issue (blocking): When rumHost is present but matches neither EDS_HOST_PATTERN nor AEM_CS_PUBLISH_HOST_PATTERN, detectedDeliveryType remains undefined and the code falls through to findDeliveryType. This is a distinct scenario from "rumHost absent" (where RUM fails entirely) - the existing tests only cover the absent case.
Suggested test: set autoResolveAuthorUrlStub to resolve { host: 'custom.example.com' } (unrecognised host), assert findDeliveryType IS called as fallback.
Summary
autoResolveAuthorUrldetecting AEM CS from RUM bundles, or the EDS hlxConfig path settingAEM_EDGE). This produced false-positive alerts for sites that self-healed within the same request.rumHostfirst (EDS pattern →AEM_EDGE, CS publish host pattern →AEM_CS), falling back tofindDeliveryType(baseURL)only whenrumHostis absent or unrecognised. The check compares against the site's current delivery type post-correction, so the alert only fires when human intervention is actually needed.AEM_CS_PUBLISH_HOST_PATTERNadded tovalidation.jsto matchpublish-pXXX-eXXX.adobeaemcloud.comRUM hosts.findDeliveryTypefallback, and the skip condition for new sites.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!