Skip to content

feat(analytics): enhance DotAnalyticsService#35549

Merged
erickgonzalez merged 8 commits intomainfrom
34849-fixes
May 4, 2026
Merged

feat(analytics): enhance DotAnalyticsService#35549
erickgonzalez merged 8 commits intomainfrom
34849-fixes

Conversation

@nicobytes
Copy link
Copy Markdown
Member

@nicobytes nicobytes commented May 4, 2026

This pull request refactors the DotAnalyticsService API and its consumers to use unified parameter objects for analytics queries, improving flexibility and maintainability. The service methods now accept a single options object (including optional fields like granularity, eventType, and siteId), and the store feature is updated accordingly. Additional tests are added for new query combinations, and utility functions are introduced to build query parameters consistently.

DotAnalyticsService API refactor and enhancements:

  • Refactored all analytics service methods (getTotalEvents, getUniqueVisitors, getTopContent, getPageviewsByDeviceBrowser) to accept a single parameter object with optional fields (granularity, eventType, siteId), replacing multiple parameters and overloads. Added private utility methods to build HTTP query parameters for each API call. [1] [2] [3]

  • Updated the store feature (withPageview) to use the new parameter object format for all analytics queries, ensuring correct passing of eventType and siteId, and added type guards for array vs. object responses. [1] [2] [3] [4] [5] [6] [7]

Testing improvements:

  • Added comprehensive tests for getTotalEvents in dot-analytics.service.spec.ts, covering various combinations of query parameters and validating request construction. Introduced a helper function to match requests by URL and method. [1] [2]

Minor improvements:

  • Fixed strict typing for test variables in the service spec file. [1] [2]

This PR fixes: #34849

This PR fixes: #34849

…tching logic

- Introduced `getTotalEvents` method in `DotAnalyticsService` to fetch total events from the new analytics event API, supporting both predefined ranges and custom date ranges.
- Updated service methods to handle optional parameters like `granularity`, `eventType`, and `siteId`.
- Refactored related store logic in `with-pageview.feature.ts` to utilize the new service methods, ensuring accurate state management.
- Added comprehensive unit tests for the new functionality, ensuring robust coverage and reliability.

This update modernizes the analytics data access layer and prepares for future enhancements.
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 4, 2026

Claude finished @nicobytes's task in 2m 14s —— View job


PR Review

  • Gather context
  • Read changed files
  • Analyze and post review

5 issues worth a second look:


1. healthCheck returns AVAILABLE on network failuredot-analytics.service.ts:70

catchError(() => of(HealthStatusTypes.AVAILABLE))

If the health endpoint is unreachable or returns a 5xx, this silently reports the service as available. It should return NOT_AVAILABLE. Pre-existing, but now that the service is more central to the feature, this is worth fixing. Fix this →


2. Double getTopContent call per page loadwith-pageview.feature.ts

_loadTopPagePerformance (line 233) and _loadTopPagesTable (line 428) call the same endpoint with identical params on every loadAllPageviewData(). That's two identical HTTP requests. _loadTopPagePerformance only needs items[0] from the same data _loadTopPagesTable already fetches. These could share one request via shareReplay or be consolidated into a single rxMethod that fans out to both state slices.


3. No store-level tests for withPageviewwith-pageview.feature.ts

The new tests only cover DotAnalyticsService. The withPageview feature has six rxMethod pipelines with switchMap, tapResponse, error handling, and fillMissingApiDates — all refactored in this PR — with zero new unit tests. If the store is already covered in a different spec file, disregard; otherwise this is a meaningful gap.


4. Dual type system without a migration pathtypes/analytics-api.types.ts vs store imports

Store state uses TotalPageViewsEntity, UniqueVisitorsEntity, PageViewDeviceBrowsersEntity (domain entity types), while the service now returns TotalEventsData, UniqueVisitorsData, DeviceBrowserData (new API types). These are structurally compatible today, but the two naming conventions will diverge as both are maintained. Worth deciding now whether the old *Entity types will be deprecated in favor of the new *Data types, or whether they're intentionally separate layers.


5. day field name is misleading for monthly granularityanalytics-api.types.ts:96–99

The comment acknowledges this: "for granularity: 'month', values are bucket starts … not necessarily every calendar day." The field is still named day. fillMissingApiDates steps by month when granularity === 'month', so the fill logic is correct, but any code that reads .day and labels it as "day" in the UI will produce misleading labels for monthly views. Low risk now since monthly granularity isn't wired to a chart label, but this will bite when it is.


Otherwise the param-builder decomposition is clean, the overload signatures for GetTotalEventsWithoutGranularity / GetTotalEventsWithGranularity are correct TypeScript idiom, and the if (params.siteId) falsy guard correctly suppresses empty-string siteIds.

nicobytes added 2 commits May 4, 2026 14:09
- Introduced new methods in `DotAnalyticsService` for fetching unique visitors, top content, and page views by device/browser, utilizing the new analytics event API.
- Updated the service to handle optional parameters and improved the request handling logic.
- Enhanced unit tests for the new methods, ensuring comprehensive coverage and error handling.
- Refactored related types and store logic to align with the new API structure.

This update modernizes the analytics service and improves data retrieval capabilities.
…ling

- Updated `DotAnalyticsService` to support new methods for fetching total events and unique visitors with improved granularity handling.
- Refactored types to better represent API parameters and responses, including new types for total events and unique visitors.
- Enhanced unit tests to cover error propagation for total events, top content, and pageviews by device/browser, ensuring robust error handling.
- Cleaned up unused code in `with-pageview.feature.ts` to streamline the implementation.

This update improves the analytics service's functionality and reliability in handling API responses.
@erickgonzalez erickgonzalez enabled auto-merge May 4, 2026 19:17
nicobytes added 2 commits May 4, 2026 15:29
…roved granularity handling

- Updated `DotAnalyticsService` to replace deprecated methods with new ones for fetching total events and unique visitors, now supporting enhanced granularity options.
- Refactored types in `analytics-api.types.ts` to remove obsolete granularity parameters and align with the new API structure.
- Improved data handling in `fillMissingApiDates` utility to accommodate monthly granularity.
- Cleaned up unused code and ensured consistency in type definitions for better maintainability.

This update enhances the analytics service's functionality and prepares it for future improvements.
@erickgonzalez erickgonzalez added this pull request to the merge queue May 4, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 4, 2026
@erickgonzalez erickgonzalez added this pull request to the merge queue May 4, 2026
@ihoffmann-dot ihoffmann-dot self-assigned this May 4, 2026
Merged via the queue into main with commit c6861a7 May 4, 2026
30 checks passed
@erickgonzalez erickgonzalez deleted the 34849-fixes branch May 4, 2026 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Add dotCMS Analytics Proxy Endpoint for /v1/analytics/** to dot-ca-event-manager

4 participants