Skip to content

feat: view trial toggle#2197

Merged
Sugandhgyl merged 32 commits intomainfrom
feat/view-trial-toggle
Apr 17, 2026
Merged

feat: view trial toggle#2197
Sugandhgyl merged 32 commits intomainfrom
feat/view-trial-toggle

Conversation

@Sugandhgyl
Copy link
Copy Markdown
Contributor

@Sugandhgyl Sugandhgyl commented Apr 13, 2026

Please ensure your pull request adheres to the following guidelines:

WHAT CHANGED

Screen.Recording.2026-04-13.at.8.00.17.PM.mov
  • 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 minor release when merged.

@Sugandhgyl Sugandhgyl force-pushed the feat/view-trial-toggle branch from 3c2b123 to be0c98c Compare April 13, 2026 14:27
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@sandsinh
Copy link
Copy Markdown
Contributor

sandsinh commented Apr 13, 2026

Rename PR title to trigger release.
Add some tests.
Move the logic inside getIsSummitPlgEnabled function, it already has the context. This will apply the logic everywhere at once, so that we do not have any broken experience by mistakenly missing any paths.

@Sugandhgyl Sugandhgyl changed the title Feat/view trial toggle Feat: view trial toggle Apr 14, 2026
@Sugandhgyl Sugandhgyl changed the title Feat: view trial toggle feat: view trial toggle Apr 14, 2026
Comment thread src/controllers/suggestions.js Outdated
@alinarublea
Copy link
Copy Markdown
Contributor

PR #2197 Review: feat: view trial toggle

Overview

This PR adds a "View as Trial" toggle for the sites-optimizer-ui. When the UI sends x-view-as-trial: true alongside x-client-type: sites-optimizer-ui, getIsSummitPlgEnabled() short-circuits to true, bypassing the actual Configuration and Entitlement lookups. The intent is to let internal users simulate the PLG/trial experience.

Critical Issues

1. "View as trial" triggers write side-effects, not just read-only filtering

The JSDoc says "treated as PLG for read-only paths", but getIsSummitPlgEnabled is used in write paths too:

  • suggestions.js:277-283 and opportunities.js:179-185: When PLG is enabled, grantSuggestionsForOpportunity() is called, which writes grant records to the database. A "view as" simulation toggle should not create real data.
  • suggestions.js:1101-1107 (auto-deploy): Blocks deployment of non-granted suggestions. If an admin toggles "view as trial" while performing real operations, they'll hit unexpected 403 Forbidden errors.

This is the biggest concern — a cosmetic UI toggle has real data mutation consequences. Consider either:

  • (a) Making isViewAsTrialRequest only affect the explicit filtering call sites (e.g., filterByGrantStatus, filterForSummitPlg), not the granting/blocking call sites, or
  • (b) Passing a readOnly flag and only short-circuiting on read paths.

2. No authorization gate on the header

Any authenticated user (not just admins or internal users) can send x-view-as-trial: true. There's no check that the requesting user is allowed to use this toggle. If this is meant for internal/admin use, it should be gated behind accessControlUtil.hasAdminAccess() or equivalent. Otherwise, any customer could send the header and get PLG-style filtering applied to their session.

Are these headers stripped or validated at the Fastly CDN layer? If not, external clients can freely set them.

Moderate Issues

3. Missing test coverage for the opportunities controller

getIsSummitPlgEnabled is called with requestContext in the opportunities controller at two locations (opportunities.js:66 and opportunities.js:179). The trial header will affect both paths, but no tests were added in test/controllers/opportunities.test.js. This is a gap — the filterForSummitPlg behavior and the grant-triggering behavior in getById should both be tested with the trial header.

4. PR description is empty

The description is the default template with no checklist items checked, no linked issues, and no explanation of what changed or why. Per project conventions, there should be a linked issue and a description of what problem this solves.

Minor Issues

5. isViewAsTrialRequest is exported but only used internally in utils.js

The function is only called inside getIsSummitPlgEnabled in the same file. It's exported and tested independently (which is fine), but it's worth noting that if consumers are not expected to call it directly, it could remain unexported. Not a blocker — the tests are valuable either way.

6. Test in suggestions.test.js relies on getIsSummitPlgEnabled short-circuit implicitly

The first new test ('filters suggestions by grant status when x-view-as-trial header is present') uses the real suggestionsController without mocking getIsSummitPlgEnabled. It works because the header short-circuits before hitting Configuration/Entitlement, but this creates a fragile coupling — if the short-circuit order changes, the test breaks in a non-obvious way.

Summary

Area Verdict
Code correctness ⚠️ Needs work — write side-effects from a read-only toggle
Security ⚠️ Needs work — no authz gate on the header
Test coverage Partial — missing opportunities controller tests
Style/conventions Minor — empty PR description

Recommendation: The core logic of isViewAsTrialRequest is clean and well-tested, but the blast radius of short-circuiting getIsSummitPlgEnabled globally needs to be scoped down to avoid unintended writes and to add an authorization check.

Copy link
Copy Markdown
Contributor

@alinarublea alinarublea left a comment

Choose a reason for hiding this comment

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

See review comment above.

@Sugandhgyl
Copy link
Copy Markdown
Contributor Author

PR #2197 Review: feat: view trial toggle

Overview

This PR adds a "View as Trial" toggle for the sites-optimizer-ui. When the UI sends x-view-as-trial: true alongside x-client-type: sites-optimizer-ui, getIsSummitPlgEnabled() short-circuits to true, bypassing the actual Configuration and Entitlement lookups. The intent is to let internal users simulate the PLG/trial experience.

Critical Issues

1. "View as trial" triggers write side-effects, not just read-only filtering

The JSDoc says "treated as PLG for read-only paths", but getIsSummitPlgEnabled is used in write paths too:

  • suggestions.js:277-283 and opportunities.js:179-185: When PLG is enabled, grantSuggestionsForOpportunity() is called, which writes grant records to the database. A "view as" simulation toggle should not create real data.
  • suggestions.js:1101-1107 (auto-deploy): Blocks deployment of non-granted suggestions. If an admin toggles "view as trial" while performing real operations, they'll hit unexpected 403 Forbidden errors.

This is the biggest concern — a cosmetic UI toggle has real data mutation consequences. Consider either:

  • (a) Making isViewAsTrialRequest only affect the explicit filtering call sites (e.g., filterByGrantStatus, filterForSummitPlg), not the granting/blocking call sites, or
  • (b) Passing a readOnly flag and only short-circuiting on read paths.

2. No authorization gate on the header

Any authenticated user (not just admins or internal users) can send x-view-as-trial: true. There's no check that the requesting user is allowed to use this toggle. If this is meant for internal/admin use, it should be gated behind accessControlUtil.hasAdminAccess() or equivalent. Otherwise, any customer could send the header and get PLG-style filtering applied to their session.

Are these headers stripped or validated at the Fastly CDN layer? If not, external clients can freely set them.

Moderate Issues

3. Missing test coverage for the opportunities controller

getIsSummitPlgEnabled is called with requestContext in the opportunities controller at two locations (opportunities.js:66 and opportunities.js:179). The trial header will affect both paths, but no tests were added in test/controllers/opportunities.test.js. This is a gap — the filterForSummitPlg behavior and the grant-triggering behavior in getById should both be tested with the trial header.

4. PR description is empty

The description is the default template with no checklist items checked, no linked issues, and no explanation of what changed or why. Per project conventions, there should be a linked issue and a description of what problem this solves.

Minor Issues

5. isViewAsTrialRequest is exported but only used internally in utils.js

The function is only called inside getIsSummitPlgEnabled in the same file. It's exported and tested independently (which is fine), but it's worth noting that if consumers are not expected to call it directly, it could remain unexported. Not a blocker — the tests are valuable either way.

6. Test in suggestions.test.js relies on getIsSummitPlgEnabled short-circuit implicitly

The first new test ('filters suggestions by grant status when x-view-as-trial header is present') uses the real suggestionsController without mocking getIsSummitPlgEnabled. It works because the header short-circuits before hitting Configuration/Entitlement, but this creates a fragile coupling — if the short-circuit order changes, the test breaks in a non-obvious way.

Summary

Area Verdict
Code correctness ⚠️ Needs work — write side-effects from a read-only toggle
Security ⚠️ Needs work — no authz gate on the header
Test coverage Partial — missing opportunities controller tests
Style/conventions Minor — empty PR description
Recommendation: The core logic of isViewAsTrialRequest is clean and well-tested, but the blast radius of short-circuiting getIsSummitPlgEnabled globally needs to be scoped down to avoid unintended writes and to add an authorization check.

PR Review Comments Addressed

  • Added 3 tests to opportunities.test.js covering getByID PLG paths (grant triggered, grant skipped, requestContext threading)
  • Fixed fragile test in suggestions.test.js by explicitly mocking getIsSummitPlgEnabled via esmock instead of relying on the short-circuit
  • Grant writes in the read path are intentional — without them filterByGrantStatus returns empty results, breaking the simulation.
  • Auto-deploy 403 concerns — deploy button is disabled in the UI when isViewAsTrial is true

@Sugandhgyl Sugandhgyl requested a review from alinarublea April 16, 2026 15:29
Copy link
Copy Markdown
Contributor

@alinarublea alinarublea left a comment

Choose a reason for hiding this comment

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

Re-review — PR #2197: feat: view trial toggle

Previous review: Changes requested (alinarublea, 2026-04-16)
Author response: Tests added for opportunities controller, fragile test fixed, grant writes justified as intentional


Issues Addressed Since Last Review

Issue #3 (Missing opportunities controller tests)Resolved. Three tests added covering getByID PLG paths.

Issue #6 (Fragile test coupling)Resolved. The author now mocks getIsSummitPlgEnabled explicitly via esmock.


Outstanding Critical Issues

1. [Blocking] x-view-as-trial is not in the CORS Access-Control-Allow-Headers list

src/index.js:128 and src/index.js:178 define the allowed headers for CORS preflight:

Content-Type, Authorization, x-api-key, x-ims-org-id, x-client-type, x-import-api-key, x-trigger-audits, x-requested-with, origin, accept

x-view-as-trial is absent. Since the API at spacecat.experiencecloud.live is cross-origin from the sites-optimizer-ui, browsers will reject the preflight and block the request entirely. This header will never reach the server from the UI. This is a functional blocker — the feature cannot work without updating the CORS config.

2. [Security] No server-side authorization gate — unchanged

The author's response did not address this. Any authenticated user (customers, not just Adobe internal) can send x-view-as-trial: true via non-browser clients (curl, Postman, scripts). This:

  • Short-circuits getIsSummitPlgEnabled to true, bypassing Configuration and Entitlement checks
  • Triggers grantSuggestionsForOpportunity which writes real grant records and consumes token capacity
  • Applies PLG-style filtering to a site that may not be enrolled in PLG

The x-client-type check is not a security boundary — it's an unverified header. This needs a server-side gate such as accessControlUtil.hasAdminAccess().

3. [Design] Grant writes from a simulation toggle — partially addressed

The author explains: "Grant writes in the read path are intentional — without them filterByGrantStatus returns empty results, breaking the simulation."

This is a valid functional argument, but the consequence is that a "view" toggle mutates production state:

  • grantSuggestionsForOpportunity creates SuggestionGrant and Token records
  • These records persist after the toggle is turned off
  • If the site is later genuinely enrolled in PLG, these pre-existing grants could interfere with the real token cycle (handleExistingTokenCycle in grant-suggestions-handler.js:359 revokes and re-grants based on existing token state)

The auto-deploy concern — the author says "deploy button is disabled in the UI when isViewAsTrial is true." This is a client-side-only defense. The server still accepts x-view-as-trial: true + a deploy request and returns 403. If a power user or API consumer isn't using the UI, they get a confusing 403 with no indication it's caused by a simulation header. At minimum, the 403 message at suggestions.js:1105 should mention the trial simulation if isViewAsTrialRequest is active.

Recommendation

Before merging:

  1. Add x-view-as-trial to CORS allowed headers in src/index.js (lines 128 and 178) — otherwise the feature is dead on arrival
  2. Gate the toggle behind admin access or at minimum verify the user is internal (IMS org check, admin scope, etc.)
  3. Consider a cleanup mechanism for grant records created during simulation, or document that simulation grants are permanent

The test improvements are solid, and the core utility function is well-structured. The remaining concerns are about security boundaries and operational safety, not code quality.

🤖 Generated with Claude Code

@Sugandhgyl
Copy link
Copy Markdown
Contributor Author

Sugandhgyl commented Apr 16, 2026

Outstanding Critical Issues

1. [Blocking] x-view-as-trial is not in the CORS Access-Control-Allow-Headers list

src/index.js:128 and src/index.js:178 define the allowed headers for CORS preflight:

Content-Type, Authorization, x-api-key, x-ims-org-id, x-client-type, x-import-api-key, x-trigger-audits, x-requested-with, origin, accept

x-view-as-trial is absent. Since the API at spacecat.experiencecloud.live is cross-origin from the sites-optimizer-ui, browsers will reject the preflight and block the request entirely. This header will never reach the server from the UI. This is a functional blocker — the feature cannot work without updating the CORS config.

2. [Security] No server-side authorization gate — unchanged

The author's response did not address this. Any authenticated user (customers, not just Adobe internal) can send x-view-as-trial: true via non-browser clients (curl, Postman, scripts). This:

  • Short-circuits getIsSummitPlgEnabled to true, bypassing Configuration and Entitlement checks
  • Triggers grantSuggestionsForOpportunity which writes real grant records and consumes token capacity
  • Applies PLG-style filtering to a site that may not be enrolled in PLG

The x-client-type check is not a security boundary — it's an unverified header. This needs a server-side gate such as accessControlUtil.hasAdminAccess().

3. [Design] Grant writes from a simulation toggle — partially addressed

The author explains: "Grant writes in the read path are intentional — without them filterByGrantStatus returns empty results, breaking the simulation."

This is a valid functional argument, but the consequence is that a "view" toggle mutates production state:

  • grantSuggestionsForOpportunity creates SuggestionGrant and Token records
  • These records persist after the toggle is turned off
  • If the site is later genuinely enrolled in PLG, these pre-existing grants could interfere with the real token cycle (handleExistingTokenCycle in grant-suggestions-handler.js:359 revokes and re-grants based on existing token state)

The auto-deploy concern — the author says "deploy button is disabled in the UI when isViewAsTrial is true." This is a client-side-only defense. The server still accepts x-view-as-trial: true + a deploy request and returns 403. If a power user or API consumer isn't using the UI, they get a confusing 403 with no indication it's caused by a simulation header. At minimum, the 403 message at suggestions.js:1105 should mention the trial simulation if isViewAsTrialRequest is active.

Recommendation

Before merging:

  1. Add x-view-as-trial to CORS allowed headers in src/index.js (lines 128 and 178) — otherwise the feature is dead on arrival
  2. Gate the toggle behind admin access or at minimum verify the user is internal (IMS org check, admin scope, etc.)
  3. Consider a cleanup mechanism for grant records created during simulation, or document that simulation grants are permanent

The test improvements are solid, and the core utility function is well-structured. The remaining concerns are about security boundaries and operational safety, not code quality.

PR Review Comments Addressed

  • CORS — Added x-view-as-trial to both Access-Control-Allow-Headers locations in src/index.js (preflight OPTIONS response + CORS handler)

  • 403 message — Updated [src/controllers/suggestions.js] to append (trial simulation mode is active — disable the "View as Trial" toggle to deploy) when isViewAsTrialRequest is detected. Real PLG users are unaffected since they never send that header

  • Security gate (Fix 2) — Reverted, not needed given the existing SitesInternal restriction already gates the toggle in ASO UI.

  • test/index.test.js — Updated the OPTIONS preflight assertion to include x-view-as-trial in access-control-allow-headers, matching the source change. Can't execute locally due to a pre-existing broken node-html-parser dependency that prevents the file from loading at all — CI should catch it.

  • test/controllers/suggestions.test.js — Added 'includes trial simulation hint in 403 message when x-view-as-trial header is present' — passes trial headers in the request, asserts the 403 body includes 'trial simulation' and 'View as Trial'. Also tightened the existing test to assert it does not include 'trial simulation' for real PLG users. Both pass.

Copy link
Copy Markdown
Contributor

@alinarublea alinarublea left a comment

Choose a reason for hiding this comment

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

Re-review #3 — PR #2197: feat: view trial toggle

Previous reviews: Changes requested (2x), citing CORS blocker, security gate, write side-effects
Latest commit: f9b9986 — "Fix PR review comments"


Issues Addressed Since Last Review

CORS blockerResolved. x-view-as-trial added to both Access-Control-Allow-Headers locations in src/index.js (lines 128 and 191). Test updated in test/index.test.js. The feature will now work from the browser.

403 message clarityResolved. suggestions.js:1105-1108 now appends (trial simulation mode is active - disable the View as Trial toggle to deploy) when isViewAsTrialRequest is true. Real PLG users are unaffected since they don't send the header. Test added to verify both cases (with and without the hint).

Test coverageSolid. 24 new test assertions across suggestions, opportunities, and utils covering:

  • Grant triggering and filtering with trial header
  • End-to-end grant + filter flow
  • getByStatus with trial header
  • Trial hint in 403 message (positive and negative)
  • isViewAsTrialRequest edge cases (wrong client, absent header, false value, undefined context)

Re-assessment of Security Gate Concern

On reflection, my previous security concern was overstated. Tracing every call site where getIsSummitPlgEnabled returns true:

Call site Effect More or less access?
filterForSummitPlg (opportunities) Filters to only broken-backlinks, cwv, alt-text Less — hides opportunities
filterByGrantStatus (suggestions) Filters to only granted suggestions Less — hides suggestions
grantSuggestionsForOpportunity Creates grant/token records Write side-effect (see below)
Auto-deploy guard (suggestions:1102) Blocks deploy of non-granted suggestions Less — blocks actions
getByID guard (suggestions:531) Returns 404 for non-granted suggestions Less — hides data

A malicious user sending x-view-as-trial: true would see less data and have fewer capabilities — not more. This is not a privilege escalation vector. The only effect is data pollution from grant writes, which the author has justified as necessary for the simulation to work.

The author's response that the SitesInternal restriction in the ASO UI already gates who can access the toggle is a reasonable layered defense argument. Combined with the fact that the header is self-penalizing (restricts, doesn't expand), the risk is acceptably low.


One Remaining Observation (Non-blocking)

Grant record persistence — When a user toggles "View as Trial" on and then off, the SuggestionGrant and Token records created during the simulation remain in the database. If the site is later genuinely enrolled in PLG, handleExistingTokenCycle in grant-suggestions-handler.js will encounter these pre-existing records. This should be fine (the function revokes and re-grants), but it's worth documenting in the feature's runbook that simulation-created grants are permanent and will be reconciled on real enrollment.


Summary

Category Status
CORS Fixed
403 UX Fixed
Security Acceptable risk — header is self-penalizing
Test coverage Comprehensive
CI All checks passing

Recommendation: Ready to merge. Clean up the PR description and squash the 31 commits before merging.

🤖 Generated with Claude Code

@alinarublea alinarublea dismissed stale reviews from themself April 17, 2026 07:08

Issues addressed — CORS fix, 403 message improvement, security risk re-assessed as acceptable.

Copy link
Copy Markdown
Contributor

@alinarublea alinarublea left a comment

Choose a reason for hiding this comment

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

All previously raised issues have been addressed:

  • CORS: x-view-as-trial added to allowed headers ✅
  • 403 UX: Trial simulation hint added to the error message ✅
  • Security: Re-assessed — the header is self-penalizing (restricts access, doesn't expand it), making the risk acceptably low ✅
  • Tests: Comprehensive coverage across all affected controllers ✅
  • CI: All checks passing ✅

See detailed re-review comment above. Recommend squashing the 31 commits before merge.

🤖 Generated with Claude Code

@Sugandhgyl Sugandhgyl merged commit c99aea7 into main Apr 17, 2026
18 checks passed
@Sugandhgyl Sugandhgyl deleted the feat/view-trial-toggle branch April 17, 2026 07:30
solaris007 pushed a commit that referenced this pull request Apr 17, 2026
# [1.446.0](v1.445.3...v1.446.0) (2026-04-17)

### Features

* view trial toggle ([#2197](#2197)) ([c99aea7](c99aea7))
@solaris007
Copy link
Copy Markdown
Member

🎉 This PR is included in version 1.446.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants