Migrate Analytics Dashboard to domain-driven query API (Angular changes) - #36843
Conversation
Migrates the Content Analytics dashboard's Angular code off the old per-metric REST endpoints onto the new domain-driven query API (/analytics/events, /analytics/sessions, /analytics/content), per the backend redesign in dotCMS/dot-ca-event-manager#92. - Migrate all 7 DotAnalyticsService methods to the new endpoints, mapping each response back to the existing DTO shapes so no store/component code needed to change - Leave getConversionsOverview() on the old /conversion endpoint (no replacement exists) and remove the "Conversions Overview" table widget from the UI, matching the backend's decision to retire it - Fix the dotCMS Java proxy allowlist (EventAnalyticsProxyHelper) to permit the new events/sessions/content path prefixes - Change the dashboard's default landing tab from Pageview to Engagement (product decision) - Add a full-report loading overlay shown while a tab's data loads, matching dot-edit-content-layout's reload-overlay look - Fix analyticsHealthGuard from canMatch to canActivate — canMatch was re-firing the health check (and blocking navigation) on every tab click; canActivate only fires when entering/re-entering the portlet Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ST-API-for-Analytics-Dashboard-Angular-code-changes
|
Claude finished @jcastro-dotcms's task in 5m 56s —— View job Code review — Analytics Dashboard domain-driven query migrationReviewed the full diff ( New Issues
Existing (rollback risk — already flagged, non-blocking, tracked via label)
Resolved
Everything else in the migration (response→DTO mapping, |
github-actions
Bot
added
Area : Backend
|
Pull Request Unsafe to Rollback!!!
|
…ST-API-for-Analytics-Dashboard-Angular-code-changes
There was a problem hiding this comment.
Pull request overview
This PR migrates the Angular Analytics Dashboard data-access layer from legacy per-metric analytics proxy endpoints to the newer domain-driven query API, while keeping downstream store/component DTO contracts stable. It also aligns the UI with the backend retirement of the Conversions Overview resource and improves navigation/loading behavior in the dashboard.
Changes:
- Update
DotAnalyticsServiceto query/api/v1/analytics/events|sessions|contentand map the unified tabular envelope back into existing DTO shapes. - Remove the “Conversions Overview” widget end-to-end (component, store slice, types, i18n keys) and adjust the Conversions report layout accordingly.
- Update routing/UX: switch analytics health guard to
canActivate, change default dashboard landing tab to Engagement, and add a full-report loading overlay.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dotCMS/src/test/java/com/dotcms/rest/api/v1/analytics/event/EventAnalyticsProxyHelperTest.java | Adds unit coverage for allowing new domain-driven proxy paths (events, sessions, content). |
| dotCMS/src/main/webapp/WEB-INF/messages/Language.properties | Removes unused i18n keys for the retired Conversions Overview widget. |
| dotCMS/src/main/java/com/dotcms/rest/api/v1/analytics/event/EventAnalyticsProxyHelper.java | Extends proxy allowlist to permit the new domain-driven analytics resources. |
| core-web/libs/portlets/dot-analytics/portlet/src/lib/lib.routes.ts | Switches analytics guard to canActivate and changes default dashboard redirect to engagement. |
| core-web/libs/portlets/dot-analytics/portlet/src/lib/guards/analytics-health.guard.ts | Updates guard type/signature and documents why canActivate is preferred. |
| core-web/libs/portlets/dot-analytics/portlet/src/lib/guards/analytics-health.guard.spec.ts | Updates tests to call the guard with activate snapshots/state. |
| core-web/libs/portlets/dot-analytics/portlet/src/lib/dot-analytics-dashboard/reports/conversions/dot-analytics-conversions-report/dot-analytics-conversions-report.component.ts | Removes the Conversions Overview table slice/computed state usage. |
| core-web/libs/portlets/dot-analytics/portlet/src/lib/dot-analytics-dashboard/reports/conversions/dot-analytics-conversions-report/dot-analytics-conversions-report.component.html | Removes the Conversions Overview table widget from the report layout. |
| core-web/libs/portlets/dot-analytics/portlet/src/lib/dot-analytics-dashboard/reports/conversions/dot-analytics-conversions-overview-table/dot-analytics-conversions-overview-table.component.ts | Deletes the retired Conversions Overview table component. |
| core-web/libs/portlets/dot-analytics/portlet/src/lib/dot-analytics-dashboard/reports/conversions/dot-analytics-conversions-overview-table/dot-analytics-conversions-overview-table.component.spec.ts | Deletes tests for the removed Conversions Overview component. |
| core-web/libs/portlets/dot-analytics/portlet/src/lib/dot-analytics-dashboard/reports/conversions/dot-analytics-conversions-overview-table/dot-analytics-conversions-overview-table.component.scss | Deletes styles for the removed Conversions Overview component. |
| core-web/libs/portlets/dot-analytics/portlet/src/lib/dot-analytics-dashboard/reports/conversions/dot-analytics-conversions-overview-table/dot-analytics-conversions-overview-table.component.html | Deletes template for the removed Conversions Overview component. |
| core-web/libs/portlets/dot-analytics/portlet/src/lib/dot-analytics-dashboard/dot-analytics-dashboard.component.scss | Adds styles for a full-report loading overlay container/backdrop. |
| core-web/libs/portlets/dot-analytics/portlet/src/lib/dot-analytics-dashboard/dot-analytics-dashboard.component.html | Adds a full-report loading overlay driven by store loading state. |
| core-web/libs/portlets/dot-analytics/data-access/src/lib/types/index.ts | Exports new domain-driven query response types. |
| core-web/libs/portlets/dot-analytics/data-access/src/lib/types/analytics-domain-api.types.ts | Adds unified tabular envelope types for the new query resources. |
| core-web/libs/portlets/dot-analytics/data-access/src/lib/types/analytics-api.types.ts | Removes Conversions Overview types tied to the retired endpoint/widget. |
| core-web/libs/portlets/dot-analytics/data-access/src/lib/store/features/with-pageview.feature.ts | Adjusts pageview breakdown calls to match updated service/query behavior. |
| core-web/libs/portlets/dot-analytics/data-access/src/lib/store/features/with-conversions.feature.ts | Removes conversions overview state/loading method and stops triggering it. |
| core-web/libs/portlets/dot-analytics/data-access/src/lib/store/dot-analytics-dashboard.store.ts | Adds computed $isReportLoading to coordinate full-report loading overlay per tab. |
| core-web/libs/portlets/dot-analytics/data-access/src/lib/services/dot-analytics.service.ts | Migrates multiple analytics methods to new domain-driven query endpoints and adds shared param builder. |
| core-web/libs/portlets/dot-analytics/data-access/src/lib/services/dot-analytics.service.spec.ts | Updates HTTP expectations and response fixtures to the new unified envelope and endpoints. |
| core-web/libs/portlets/dot-analytics/data-access/src/lib/constants/dot-analytics.constants.ts | Adds new domain-driven endpoint constants and documents conversions overview retirement. |
| export const analyticsHealthGuard: CanActivateFn = (_route, _state) => { | ||
| const analyticsService = inject(DotAnalyticsService); | ||
| const router = inject(Router); | ||
| const activatedRoute = inject(ActivatedRoute); |
There was a problem hiding this comment.
Good catch — you're right. isEnterprise is resolved on the parent analytics route (via DotEnterpriseLicenseResolver in app.routes.ts) and Angular merges resolved data down the ancestor chain unconditionally, so it's already present on this guard's own route snapshot param. Injecting ActivatedRoute instead risked resolving to whatever route was active before this navigation, since the target route's own instance isn't created until after guards pass — exactly the case that matters most now that this guard runs on canActivate (cross-portlet entry) rather than canMatch. Fixed in 4ce129a: reading from the snapshot param, dropped the now-unused ActivatedRoute injection, and updated the guard's tests accordingly.
nx format:check (Prettier) failed the PR pipeline: this file's Tailwind classes weren't in the plugin's canonical sort order. Pure class-reorder, no structural/behavioral change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ven-query-REST-API-for-Analytics-Dashboard-Angular-code-changes' into issue-36628-Implement-domain-driven-query-REST-API-for-Analytics-Dashboard-Angular-code-changes
Per Copilot review on #36843: inject(ActivatedRoute) inside a CanActivateFn can resolve to whatever route was active *before* this navigation, since the target route's own ActivatedRoute instance isn't created until after guards pass. Read isEnterprise from the snapshot parameter instead — Angular merges resolved `data` down the ancestor chain unconditionally, so it's already there correctly (isEnterprise is resolved on the parent 'analytics' route in app.routes.ts). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per automated PR review on #36843: isUnsettled treated ComponentStatus.INIT as "loading". When currentSiteId is empty, loadAllPageviewData()/loadConversionsData()/loadEngagementData() all return without firing any request, leaving their slices at INIT forever — nothing was ever going to resolve that status, so the new full-report overlay would stay up indefinitely with no site selected. Restrict isUnsettled to LOADING only. A real load transitions to LOADING synchronously the instant it starts (patchState happens before any network round trip), so the overlay still covers the actual ~2s wait for an in-flight load; it just no longer hangs when nothing was asked to load. Also adds dot-analytics-dashboard.store.spec.ts (previously no test file existed for this store), covering the regression case directly plus the normal loading -> settled transition for $isReportLoading. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the review — addressed both new findings:
All three commits pushed to this branch. |
|
@claude regarding your feedback: All three findings from your review were verified and addressed:
The non-blocking notes (Conversions Overview removal, allowlist additivity, the All changes verified: full test suites pass, lint clean, |
|
Claude finished @jcastro-dotcms's task in 44s —— View job Verified — all three fixes confirmed correct
1. 2. 3. Test coverage — No further changes needed — closing this out. |
nicobytes
left a comment
nicobytes
left a comment
There was a problem hiding this comment.
Review: code simplification + Angular
Net positive migration (−1017 / +710). Domain-driven query API is encapsulated in DotAnalyticsService, DTO contracts stay stable, Conversions Overview removal is clean, and the canActivate + $isReportLoading fixes address real UX bugs.
Verdict: approve-with-follow-ups (non-blocking). Inline notes cover dead-code cleanup, service duplication/comment debt, and Angular guard/a11y polish.
Already solid
- Shared
#buildDomainQueryParams+toSessionEngagementData - Guard switched from
canMatch→canActivate(correct Router lifecycle) - Overlay driven by store computed
$isReportLoading(right layer) - Regression coverage for no-site overlay stuck state
Nit: PR description
Body still says getConversionsOverview() was left wired to the old endpoint — in the diff it was removed entirely. Worth updating for future readers.
| @@ -9,12 +9,17 @@ export const ANALYTICS_CONVERSION_CONTENT_ATTRIBUTION_URL = | |||
| '/api/v1/analytics/conversion/content/attribution' as const; | |||
There was a problem hiding this comment.
Simplification — dead constant
ANALYTICS_CONVERSION_CONTENT_ATTRIBUTION_URL is no longer imported anywhere after the migration to ANALYTICS_CONTENT_URL. Safe to delete this export (and the JSDoc above it) in a small follow-up so we don't leave a stale proxy path in the public constants surface.
There was a problem hiding this comment.
Confirmed dead — removed ANALYTICS_CONVERSION_CONTENT_ATTRIBUTION_URL and its JSDoc in 4adc8a9.
| @@ -81,8 +78,7 @@ const initialConversionsState: ConversionsState = { | |||
| conversionRate: createInitialRequestState(), | |||
There was a problem hiding this comment.
Simplification — dead store slice
conversionRate: RequestState<number> is still initialized/reset here, but nothing loads it and the Conversions report computes the rate locally from convertingVisitors. Leftover from before the overview removal — drop the field from ConversionsState, initialConversionsState, and this reset block.
There was a problem hiding this comment.
Confirmed dead — removed conversionRate from ConversionsState, initialConversionsState, and the no-site reset block in 4adc8a9.
| } | ||
|
|
||
| /** Response wrapper from analytics event endpoints (entity.data field) */ | ||
| export interface AnalyticsEventResponse<T> { |
There was a problem hiding this comment.
Simplification — unused legacy type
AnalyticsEventResponse<T> modeled the old entity.data envelope. The service now consumes AnalyticsQueryResponse (rows). Nothing imports this anymore — delete it (and update any stale JSDoc in this file that still references /event/total-events, /event/top-content, etc.).
There was a problem hiding this comment.
Confirmed unused — deleted AnalyticsEventResponse<T> and fixed the stale JSDoc across this file that still referenced /event/total-events, /event/top-content, /event/pageviews-by-device-browser, /session/engagement, and /conversion/content/attribution (now describing the current DotAnalyticsService methods instead of the retired proxy paths). Done in 4adc8a9.
| params: GetUniqueVisitorsParams | ||
| ): Observable<UniqueVisitorsData | UniqueVisitorsByDayData[]> { | ||
| const httpParams = this.#buildUniqueVisitorsParams(params); | ||
| const httpParams = this.#buildDomainQueryParams({ |
There was a problem hiding this comment.
Simplification — duplicated aggregate/series flow
getTotalEvents and getUniqueVisitors are the same shape: build params → GET /analytics/events → map rows[0] vs map series with dimensions=granularity. Consider a private helper (e.g. #queryEventsMetric) so the next metric doesn't copy-paste this again. Same idea applies lightly to getPageviewsByDeviceBrowser (device vs browser only renames the dimension field).
There was a problem hiding this comment.
Extracted a private #queryEventsMetric<K>(metric, params) helper — getTotalEvents/getUniqueVisitors are now one-line delegations. Structural typing worked out cleanly so no cast was needed at the call sites (K infers as the literal metric name, e.g. Record<'totalEvents', number> is structurally TotalEventsData).
Held off on getPageviewsByDeviceBrowser per your "lightly" framing — the device/browser duplication there is really just one output key name differing (2 lines), and unifying it would need a mapped-type generic ({ [P in K]: string } & { total: number }) that seemed like more type complexity than the duplication warranted. Happy to do it too if you'd rather have it unified for consistency. Done in 4adc8a9.
| * | ||
| * @param params - Date range plus optional `siteId` and `eventType` query params | ||
| */ | ||
| getTopContent(params: GetRangeSiteEventParams): Observable<TopContentData[]> { |
There was a problem hiding this comment.
Simplification — JSDoc length
The migration "why" notes on methods like getTopContent / getContentAttribution / getSessionEngagementGroupBy are useful in the PR, but as permanent service docs they're heavy (mode resolution, Postman confirmation, field renames). Prefer 1–2 lines of enduring intent (e.g. "restrict metrics to these 4 or grouped queries 400") and keep the archaeology in the PR / migration guide.
There was a problem hiding this comment.
Trimmed getTopContent/getContentAttribution/getSessionEngagementGroupBy (and lightly getTotalEvents/getUniqueVisitors, now mostly documented on the shared #queryEventsMetric helper instead) down to 1-2 lines of enduring intent — dropped the mode-resolution deep-dive, the Postman-confirmation note, and the "old caller never did this" framing, kept the hard constraints (e.g. the 400-risk on getSessionEngagementGroupBy's metric restriction, the eventType-as-dimension mode flip on getTopContent). Done in 4adc8a9.
| // unconditionally, so it's already present here correctly. | ||
| const isEnterprise = route.data?.['isEnterprise'] ?? true; | ||
|
|
||
| router.navigate(['/analytics/error'], { |
There was a problem hiding this comment.
Angular — prefer UrlTree over navigate + false
canActivate change is correct ✅. While here: Angular's preferred pattern is to return the redirect instead of side-effecting:
return router.createUrlTree(['/analytics/error'], {
queryParams: { status: healthStatus, isEnterprise }
});Avoids a separate navigate call, plays nicer with cancellation, and makes the guard test assert the tree directly. Pre-existing smell, but this file already moved — good moment to clean it up.
There was a problem hiding this comment.
Done — the guard now returns router.createUrlTree(['/analytics/error'], {...}) instead of calling router.navigate() then returning false. Updated the spec to mock createUrlTree (echoing its args back) and assert the guard's return value directly instead of asserting a navigate side-effect. Done in 4adc8a9.
| the per-widget skeletons underneath. --> | ||
| @if (store.$isReportLoading()) { | ||
| <div | ||
| class="analytics-dashboard-body__loading-overlay flex items-center justify-center" |
There was a problem hiding this comment.
Angular / a11y — loading overlay
Visual parity with edit-content's reload overlay is good. For WCAG AA, consider announcing the busy state:
role="status"+aria-live="polite"(oraria-busyon.analytics-dashboard-body)aria-hidden="true"on the decorative spinner icon- optional
aria-labelvia| dm
Non-blocking — matches the reference overlay — but worth a small polish since this is new UI.
There was a problem hiding this comment.
Added role="status", aria-live="polite", and aria-hidden="true" on the decorative spinner icon. Also added an aria-label (reusing the existing generic Loading i18n key rather than adding a new one) since without it the live region has no accessible name to announce — the icon is the only content and it's now hidden from AT. Done in 4adc8a9.
| position: relative; | ||
| } | ||
|
|
||
| .analytics-dashboard-body__loading-overlay { |
There was a problem hiding this comment.
Angular / styling — prefer Tailwind utilities
Per project standards (STYLING_STANDARDS.md / Tailwind-first), this overlay does not need custom SCSS. Move these to the template on the overlay div:
class="analytics-dashboard-body__loading-overlay absolute inset-0 z-10 flex items-center justify-center bg-white/60"Then drop this rule (and ideally the parent position: relative if you can put relative on .analytics-dashboard-body via Tailwind in the HTML too). Keep SCSS only when PrimeNG/::ng-deep forces it.
There was a problem hiding this comment.
Moved to Tailwind utilities on the template (relative on .analytics-dashboard-body, absolute inset-0 z-10 flex items-center justify-center bg-white/60 on the overlay), dropped both SCSS rules entirely — kept the BEM class names for the data-testid/selector hooks per your snippet. Done in 4adc8a9.
| let spectator: SpectatorService<InstanceType<typeof DotAnalyticsDashboardStore>>; | ||
| let store: InstanceType<typeof DotAnalyticsDashboardStore>; | ||
| let analyticsService: SpyObject<DotAnalyticsService>; | ||
| let globalStore: SpyObject<GlobalStore>; |
There was a problem hiding this comment.
TypeScript — GlobalStore is a value, not a type
'GlobalStore' refers to a value, but is being used as a type here. Did you mean 'typeof GlobalStore'?
GlobalStore is a Signal Store (runtime value), same pattern as DotAnalyticsDashboardStore above. Fix:
let globalStore: SpyObject<InstanceType<typeof GlobalStore>>;(or mirror whatever typing other portlet store specs use for GlobalStore).
There was a problem hiding this comment.
Good catch, real compile error (ts-jest transpile-only mode doesn't catch it, only a real tsc run does — same lesson from a type error caught in a build earlier in this PR). Fixed: SpyObject<InstanceType<typeof GlobalStore>>. Verified with tsc --noEmit directly this time, not just the test run. Done in 4adc8a9.
Per human review on #36843: - Remove dead ANALYTICS_CONVERSION_CONTENT_ATTRIBUTION_URL constant and unused AnalyticsEventResponse<T> type; fix stale JSDoc still referencing old /event/*, /session/*, /conversion* proxy paths - Remove dead conversionRate slice from ConversionsState — nothing loaded it; the Conversions report computes the rate locally - Extract a shared #queryEventsMetric helper in DotAnalyticsService so getTotalEvents/getUniqueVisitors stop duplicating the build-params -> GET -> map-scalar-or-series shape - Trim migration-era JSDoc (mode-resolution deep-dives, Postman confirmation notes, "old caller never did this" framing) down to 1-2 lines of enduring intent per method - analyticsHealthGuard: return router.createUrlTree(...) instead of calling router.navigate() then returning false — the Angular-preferred guard-redirect pattern - Loading overlay: add role="status" + aria-live="polite" + aria-label (reusing the existing generic "Loading" key), aria-hidden on the decorative spinner icon; move its positioning/ backdrop from custom SCSS to Tailwind utility classes per STYLING_STANDARDS.md - Fix a real TS error in the new store spec: GlobalStore is a value (signal store), not a type — SpyObject<InstanceType<typeof GlobalStore>>, not SpyObject<GlobalStore> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the thorough review, @nicobytes! All 9 inline suggestions addressed and pushed in Also fixed the PR description nit — it still said |
|
Pull Request Unsafe to Rollback!!!
|
Proposed Changes
DotAnalyticsServicemethods off the old per-metric endpoints (/event/*,/session/*,/conversion/content/attribution) onto the new domain-driven query API (/analytics/events,/analytics/sessions,/analytics/content), mapping each response back to the existing DTO shapes so no store/component code needed to change downstreamgetConversionsOverview()and the "Conversions Overview" table widget (component, store slice, types, i18n keys) from the Conversions tab entirely — the backend never built a domain-driven replacement for/analytics/conversion, since Product decided to drop that resourceEventAnalyticsProxyHelper) to permit the newevents/sessions/contentpath prefixes — without this, calls to the new endpoints 400 before reaching the upstream serviceAdditional Changes (Not Part of the Original Ticket Requirements)
#36628 is scoped to the REST API migration itself. The two items below were separate asks/fixes that came up during implementation and review of this PR — flagging them explicitly since they go beyond that ticket's requirements:
dot-edit-content-layout's reload overlayanalyticsHealthGuardfromcanMatchtocanActivate—canMatchwas re-firing the/api/v1/analytics/healthcheck (and blocking navigation on its response) on every single tab click;canActivateonly fires when entering/re-entering the Analytics Dashboard portlet, not on internal tab switchesChecklist
Additional Info
Related backend work (separate repo): dotCMS/dot-ca-event-manager#92, following the design/migration guide in that repo's
docs/analytics-frontend-migration.md.Migration was done gradually, one endpoint at a time: for each method, the exact outgoing request from the Angular dev build was captured and replayed against the old endpoint in Postman to diff the values before moving to the next.
Security note: the
EventAnalyticsProxyHelperallowlist change only adds 3 new literal path-prefix strings (events,sessions,content) to an existing exact-match/prefix allowlist — it does not loosen the matching logic itself. Covered by existing + new unit tests inEventAnalyticsProxyHelperTest.Translations: no new user-facing strings were added; only unused i18n keys tied to the removed Conversions Overview widget were deleted from the default
Language.properties(no other locale files had these keys). The new loading overlay'saria-labelreuses the existing genericLoadingkey.Review follow-ups incorporated: dead-code cleanup (unused constant/type, dead store slice), a shared
#queryEventsMetrichelper to de-duplicate the aggregate/series query pattern, trimmed migration-era JSDoc down to enduring facts, guard redirect switched to returning aUrlTreeinstead ofrouter.navigate()+false, a11y attributes on the loading overlay (role="status",aria-live="polite",aria-hiddenon the decorative spinner), and moving the overlay's styling from custom SCSS to Tailwind utility classes perSTYLING_STANDARDS.md.Screenshots
No visual changes to chart/table rendering. User-visible changes are: default tab is now Engagement instead of Pageview, the Conversions tab no longer shows the "Conversions Overview" table, and a loading spinner overlay now appears briefly while a tab's data loads.
🤖 Generated with Claude Code