feat(report-issue): gate toolbar menu item behind FEATURE_FLAG_REPORT_ISSUE_ENABLED

[fmontes]

Summary

• Adds FEATURE_FLAG_REPORT_ISSUE_ENABLED (default false) to gate the "Report an Issue" entry in the toolbar user menu added by Add report issue workflow to backend and toolbar UI #35717.
• Whitelists the new key in ConfigurationResource and exposes it in the frontend FeaturedFlags enum so DotPropertiesService can read it.
• Flag check intentionally lives in DotToolbarUserComponent (not the store) so it can be cleanly removed once the feature ships — see inline comment.

Closes #35758. Follow-up to #35717.

Test plan

• With FEATURE_FLAG_REPORT_ISSUE_ENABLED unset or false: "Report an Issue" item is not visible in the user menu.
• With FEATURE_FLAG_REPORT_ISSUE_ENABLED=true: item appears and opens the report issue dialog as before.
• yarn nx test dotcms-ui --testPathPattern=dot-toolbar-user.component.spec passes.

🤖 Generated with Claude Code

[claude]

Claude finished @fmontes's task in 1m 57s —— View job

---

Review

Mostly clean follow-up. A few things worth a second look — none blocking.

1) startWith(false) produces a flash when the flag is actually true

dot-toolbar-user.component.ts:52-71

With startWith(false) on the flag stream, combineLatest emits twice when the flag resolves to true: first [vm, false] (filtered, item missing), then [vm, true] (item present). On a flag-enabled tenant the menu briefly shows without the report-issue item before it appears. Probably imperceptible in practice (HTTP completes before the user opens the menu), but if you ever see toolbar flicker on cold loads, that's where it lives. A distinctUntilChanged() on the flag stream won't help here — the value genuinely changes. The fix, if it ever matters, is to gate on the flag's first real emission instead of using startWith.

The test at dot-toolbar-user.component.spec.ts:269-279 already has to filter past the startWith(false) emission to find the menu item, which is the test-side smell of this same behavior.

2) Removing DotHttpErrorManagerService.handle() is a behavior change beyond "handle HTML errors"

dot-report-issue.component.ts (removed dotHttpErrorManagerService.handle(error).subscribe())

The commit message frames this as "remove global error handler", but the consequence is that submission failures (401, 403, 5xx, network) no longer route through the central handler at all — no global toast, no _4xx/_5xx redirect logic, no logging side-effects that DotHttpErrorManagerService normally provides. The dialog now silently shows a local errorMessage() and that's it. If that's intentional (because the dialog has its own error surface and double-reporting was the actual bug), fine — but the PR description doesn't justify it, and a 401 here will no longer kick the user to the login flow like everywhere else in the app. Worth confirming this is what you want, not just dropping a HTML-page case.

3) HTML heuristic is fragile but acceptable

dot-report-issue.component.ts:293-297

error.trimStart().startsWith('<') will also match XML (<?xml…), SVG, or any backend error string that happens to begin with <. All get the same "service unavailable" message — which is probably the right user-facing outcome anyway, but the comment "HTML responses (e.g. dotCMS 404 pages) mean the endpoint is not reachable" overstates what the check actually proves. A < response could just as easily be a 500 from the servlet container; calling it "not reachable" is misleading for diagnostics. Consider softening the message ("could not parse the server response") or the comment.

4) Constant cleanup landed

ConfigurationResource.java:75 — the previously-flagged string literal is now REPORT_ISSUE_INCLUDE_USER_PII. 👍

5) Test rebuild pattern works but is subtle

dot-toolbar-user.component.spec.ts:288-305

(dotPropertiesService.getFeatureFlagWithDefault as jest.Mock).mockReturnValue(of(false)) mutates the same jest.fn() registered via useValue, then TestBed.createComponent builds a fresh component that picks up the new return. This works because useValue shares the object reference across components — fine, but worth a one-line comment so a future reader doesn't try to "fix" it by re-providing the service.

Other observations (not issues, just noting)

• Toolbar still gated on store.vm$ emitting at least once — startWith only patches the flag stream. Whatever was true about store.vm$ timing before is still true.
• FEATURE_FLAG_REPORT_ISSUE_ENABLED defaults to false in dotmarketing-config.properties. Good — ships dark.
• New language key report-an-issue.error.unavailable is wired up but only the English bundle was touched. Translations will fall through to the key. Probably fine for a soft-launch feature flag.

Net

The flag gating logic is correct, the resilience handling is good, tests cover both states. The two things I'd actually want a response on are (1) confirming the DotHttpErrorManagerService removal is deliberate and not just collateral with the HTML fix, and (2) whether the flash from startWith(false) is acceptable on enabled tenants.

• Pull latest changes and diff against origin/main
• Read changed files
• Flag risks/bugs/design issues
• Post review
  • Branch: fmontes/feature-request-flag

[Copilot]

Pull request overview

Adds a new opt-in feature flag to control whether the “Report an Issue” menu item appears in the toolbar user menu, wiring it through backend configuration exposure and frontend consumption.

Changes:

• Introduces FEATURE_FLAG_REPORT_ISSUE_ENABLED (default false) and exposes it via the configuration whitelist.
• Exposes the new flag in the frontend FeaturedFlags enum and gates the toolbar user-menu entry in DotToolbarUserComponent.
• Updates the component test setup to provide DotPropertiesService for the new flag lookup.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
dotCMS/src/main/resources/dotmarketing-config.properties	Adds the new feature flag with a default false value.
dotCMS/src/main/java/com/dotcms/rest/api/v1/system/ConfigurationResource.java	Whitelists the new feature flag for /api/v1/configuration/config access.
dotCMS/src/main/java/com/dotcms/featureflag/FeatureFlagName.java	Declares the new backend feature flag constant.
core-web/libs/dotcms-models/src/lib/shared-models.ts	Adds the new flag to the frontend FeaturedFlags enum.
core-web/apps/dotcms-ui/src/app/view/components/dot-toolbar/components/dot-toolbar-user/dot-toolbar-user.component.ts	Fetches the flag and removes/binds the report-issue menu item accordingly.
core-web/apps/dotcms-ui/src/app/view/components/dot-toolbar/components/dot-toolbar-user/dot-toolbar-user.component.spec.ts	Mocks DotPropertiesService.getFeatureFlagWithDefault for component tests.

[fmontes]

Thanks for the review @claude — addressed in the latest force-push:

1. combineLatest regression ✅ Fixed
Wrapped the flag observable in catchError(() => of(false)) so a failed /api/v1/configuration/config call degrades to "flag off" instead of taking the whole toolbar down with it.

2. Disabled-path test coverage ✅ Fixed
Added should hide the report issue menu item when the feature flag is disabled — overrides the DotPropertiesService mock to return of(false) and asserts the item is absent from vm.items. Now both default-production and enabled states are exercised.

3. shareReplay(1) to future-proof double-subscription ⏭️ Skipped
Only one vm$ | async subscriber today. Adding it preemptively for a hypothetical second subscriber felt like YAGNI, especially since the whole flag check is meant to be ripped out after launch.

4. Extract menu-item id constant ⏭️ Skipped
The duplication already existed in the store before this PR — the cleanup belongs in its own change rather than bundled here.

[claude]

Claude finished @fmontes's task in 1m 40s —— View job

---

Verifying the latest changes

• Read the current state of the changed files on the branch
• Verify fix update with latest SVN #1 (catchError resilience) is in place
• Verify fix Test Branch and Commit #2 (disabled-path test) is in place
• Cross-check the third original review point about the REPORT_ISSUE_INCLUDE_USER_PII constant on ConfigurationResource
• Post review summary

Verification

Fix #1 — catchError resilience ✅ Confirmed at dot-toolbar-user.component.ts:51-57. The flag observable is now .pipe(catchError(() => of(false))) and the inline comment clearly states the intent ("Degrade to 'flag off' on fetch errors so the whole toolbar doesn't disappear.").

One minor thing worth surfacing: you went with catchError alone, not startWith(false) + catchError. That means errored requests are now handled, but a slow /api/v1/configuration/config will still gate the whole vm$ | async template (no avatar/menu until the call emits). In practice this is probably fine — the endpoint is fast on a healthy backend — but if you ever see toolbar flicker on cold starts, that's the knob to turn.

Fix #2 — Disabled-path test ✅ Confirmed at dot-toolbar-user.component.spec.ts:268-286. The new spec re-mocks getFeatureFlagWithDefault to of(false), rebuilds the component so the new value flows through combineLatest, and asserts the report-issue item is undefined in vm.items. The default-true setup at lines 70-75 keeps the enabled path covered. 👍

Skipped #3/#4 — YAGNI calls and scope hygiene both look fine to me. shareReplay(1) can wait until there's an actual second subscriber, and the menu-item id constant cleanup is genuinely pre-existing churn that's better in its own PR.

One copilot point that didn't get a response

The copilot reviewer's third comment about ConfigurationResource.java:75 still applies on the current HEAD — the whitelist uses the literal "REPORT_ISSUE_INCLUDE_USER_PII" even though REPORT_ISSUE_INCLUDE_USER_PII is declared as a constant one line above (L61) and is already used at L144 for the default-value lookup. It's a trivial swap to remove the duplication. Up to you whether to fold it into this PR or leave it — same scope argument as #4 applies. Fix this →

Net

The two fixes you committed look correct and the test coverage matches what was asked for. From my side this is good to go once the lint/test job comes back green.
• Branch: fmontes/feature-request-flag