Skip to content

Conversation

@adalpari
Copy link
Contributor

@adalpari adalpari commented Jan 22, 2026

Description

Add stats period selector feature that allows users to view stats for different time periods (Today, Last 7 Days, Last 30 Days, Last 6 Months, Last 12 Months, and Custom date
range). This includes a new period dropdown in the stats screen, a date range picker dialog for custom periods, and updates to the repository to fetch comparison data for
current vs previous periods.

Testing instructions

Period selector menu:

  1. Open the new stats screen
  2. Tap the period dropdown (defaults to "Today")
  • Verify dropdown shows all period options: Today, Last 7 Days, Last 30 Days, Last 6 Months, Last 12 Months, Custom
  1. Select "Last 7 Days"
  • Verify the chart updates to show 7 days of data
  • Verify the header shows current vs previous period comparison
  1. Select "Last 30 Days"
  • Verify the chart updates to show 30 days of data

Custom date range:

  1. Open the period selector dropdown
  2. Select "Custom"
  • Verify a date range picker dialog appears
  1. Select a start date and end date (must be in the past)
  2. Tap "Confirm"
  • Verify the chart updates to show data for the selected range
  • Verify future dates are not selectable
Screen_recording_20260123_162150.mp4

- Create StatsDataSource interface and implementation to abstract data fetching
- Add StatsModule for dependency injection
- Refactor StatsRepository to use the new data source
- Rename todaysstat -> todaystats package in newstats
- Add StatsRepositoryTest with comprehensive test coverage
- Create StatsDataSource interface and implementation to abstract data fetching
- Add StatsModule for dependency injection
- Refactor StatsRepository to use the new data source
- Rename todaysstat -> todaystats package in newstats
- Add StatsRepositoryTest with comprehensive test coverage
…/github.com/wordpress-mobile/WordPress-Android into feat/CMM-1143-new-stats-views-card-tests-i2

# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/newstats/datasource/StatsDataSourceImpl.kt
@dangermattic
Copy link
Collaborator

dangermattic commented Jan 22, 2026

2 Warnings
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 22, 2026

App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
FlavorJalapeno
Build TypeDebug
Versionpr22515-dd2b563
Commitdd2b563
Direct Downloadjetpack-prototype-build-pr22515-dd2b563.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 22, 2026

App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
FlavorJalapeno
Build TypeDebug
Versionpr22515-dd2b563
Commitdd2b563
Direct Downloadwordpress-prototype-build-pr22515-dd2b563.apk
Note: Google Login is not supported on these builds.

@codecov
Copy link

codecov bot commented Jan 22, 2026

Codecov Report

❌ Patch coverage is 49.45534% with 232 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.88%. Comparing base (0429fdf) to head (dd2b563).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines Patch % Lines
.../android/ui/newstats/StatsDateRangePickerDialog.kt 0.00% 72 Missing ⚠️
.../wordpress/android/ui/newstats/NewStatsActivity.kt 0.00% 54 Missing ⚠️
...roid/ui/newstats/viewsstats/ViewsStatsViewModel.kt 60.80% 31 Missing and 18 partials ⚠️
...s/android/ui/newstats/viewsstats/ViewsStatsCard.kt 0.00% 43 Missing ⚠️
...a/org/wordpress/android/ui/newstats/StatsPeriod.kt 57.14% 6 Missing ⚠️
.../android/ui/newstats/repository/StatsRepository.kt 95.65% 3 Missing and 3 partials ⚠️
...roid/ui/newstats/datasource/StatsDataSourceImpl.kt 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #22515      +/-   ##
==========================================
+ Coverage   38.85%   38.88%   +0.02%     
==========================================
  Files        2200     2202       +2     
  Lines      106799   107073     +274     
  Branches    15048    15104      +56     
==========================================
+ Hits        41497    41635     +138     
- Misses      61859    61975     +116     
- Partials     3443     3463      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Base automatically changed from feat/CMM-1143-new-stats-views-card-tests-i2 to trunk January 23, 2026 14:51
# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/newstats/datasource/StatsDataSource.kt
#	WordPress/src/main/java/org/wordpress/android/ui/newstats/datasource/StatsDataSourceImpl.kt
#	WordPress/src/main/java/org/wordpress/android/ui/newstats/repository/StatsRepository.kt
#	WordPress/src/test/java/org/wordpress/android/ui/newstats/repository/StatsRepositoryTest.kt
@adalpari
Copy link
Contributor Author

@claude

@claude
Copy link
Contributor

claude bot commented Jan 23, 2026

OUTDATED

@adalpari
Copy link
Contributor Author

1. Low Test Coverage (Priority: High)

Location: Multiple UI files Issue: Codecov reports 49.5% patch coverage with 224 lines missing coverage:

  • StatsDateRangePickerDialog.kt: 0% coverage (70 lines)
  • ViewsStatsCard.kt: 0% coverage (60 lines)
  • NewStatsActivity.kt: 0% coverage (54 lines)

Impact: The majority of the new UI code is untested, making it difficult to catch regressions.

Recommendation: While Compose UI testing can be challenging, consider:

  • Adding screenshot/snapshot tests for the key UI states
  • Testing the StatsPeriodMenu composable in isolation
  • Adding integration tests that verify the period selector interaction flow
  • Ensuring ViewModel test coverage is comprehensive (currently at 68%, which is good but can be improved)

We are not handling UI tests at the moment...

@adalpari
Copy link
Contributor Author

  1. Long Function Needs Refactoring
  1. Magic Numbers Should Be Constants

These are false positives. They have already been solved...

@adalpari
Copy link
Contributor Author

@claude

@claude
Copy link
Contributor

claude bot commented Jan 23, 2026

Claude finished @adalpari's task in 1m 28s —— View job


Code Review Complete ✓

I've performed a comprehensive review of this PR. Overall, the implementation is well-structured with good test coverage for the repository and ViewModel layers. Here are my findings:


🟢 Strengths

  1. Well-tested business logic: The ViewsStatsViewModel and StatsRepository have comprehensive unit tests (68%+ coverage on ViewModel, good repository coverage)
  2. Clean architecture: Proper separation of concerns with sealed classes for UI states and result types
  3. Parallel data fetching: The repository efficiently fetches current and previous period data in parallel using coroutines
  4. Good error handling: Proper error states and fallbacks throughout the codebase
  5. Type-safe period handling: Sealed class StatsPeriod provides excellent type safety

🟡 Architecture & Design Issues

1. Date Range Validation Missing (Priority: High)

Location: StatsDateRangePickerDialog.kt:66-77, NewStatsActivity.kt:97-99

The date range picker doesn't validate that start date is before end date. Users could select an invalid range (e.g., Jan 20 to Jan 15).

Impact: Could lead to API errors or incorrect data display.

Recommendation: Add validation before calling onDateRangeSelected:

if (startDate.isBefore(endDate) || startDate.isEqual(endDate)) {
    onDateRangeSelected(startDate, endDate)
} else {
    // Show error or swap dates
}

Fix this →

2. Custom Period Label Not User-Friendly (Priority: Medium)

Location: StatsPeriod.kt:16-17, NewStatsActivity.kt:268

When a custom period is selected, the menu shows generic "Custom" text instead of the actual date range selected.

Impact: Poor UX - users can't see what custom range they selected without opening the dropdown.

Recommendation: Consider displaying the actual date range or adding a method to StatsPeriod.Custom to generate a display label like "Jan 14 - Jan 20".

3. State Management Coupling (Priority: Medium)

Location: NewStatsActivity.kt:92-102

The selectedPeriod state is managed in the Activity and passed down to the ViewModel. This creates tight coupling and makes the state harder to restore on configuration changes.

Impact: Selected period might not survive configuration changes (screen rotation).

Recommendation: Move selectedPeriod state management into the ViewModel with savedStateHandle for proper state restoration.


🟡 Code Quality Issues

4. Inconsistent Date Handling (Priority: Medium)

Location: StatsRepository.kt:47-51, ViewsStatsViewModel.kt:264-269

The codebase mixes Calendar, SimpleDateFormat, and LocalDate for date handling, which is error-prone and inconsistent.

Impact: Increased complexity and potential timezone/parsing bugs.

Recommendation: Standardize on java.time (LocalDate/LocalDateTime) throughout, as it's more modern and less error-prone.

5. Magic String Patterns (Priority: Low)

Location: ViewsStatsViewModel.kt:31-33

Regex patterns are defined as private constants but could be shared across the stats module if other classes need similar date parsing.

Recommendation: Consider extracting to a shared constants file if date parsing is needed elsewhere.

6. Average Calculation Edge Case (Priority: Low)

Location: ViewsStatsViewModel.kt:133-143

The average calculation logs a warning when there are no data points but views > 0. However, this doesn't handle the case gracefully for the user.

Recommendation: Consider whether this scenario should show an error state or different UI indication to users.


🟢 Performance

The parallel fetching of current and previous period data (StatsRepository.kt:301-318) is well-implemented using async/await. No performance concerns.


🟢 Security

No security concerns identified. The code properly:

  • Validates access tokens before making API calls
  • Uses proper parameter binding (no SQL injection risks)
  • Doesn't expose sensitive data in logs

🟡 Testing

The ViewModel and Repository have good test coverage. However, as you noted, UI tests are not included. The Compose UI components (StatsDateRangePickerDialog, ViewsStatsCard, period selector menu) are currently untested.

Note: I understand UI tests are not being handled at this time per your earlier comment.


🟢 Documentation

String resources are properly added and well-named. Code is generally self-documenting with clear naming conventions.


📊 Summary

Critical Issues: 1 (date range validation)
Important Issues: 2 (custom period label, state management)
Minor Issues: 3 (date handling consistency, magic strings, edge case handling)

The implementation is solid overall. The most important fix is adding date range validation to prevent invalid user input. The state management coupling and custom period label UX issues should be addressed before final release.


@adalpari
Copy link
Contributor Author

  1. Date Range Validation Missing (Priority: High)

Done!

  1. State Management Coupling (Priority: Medium)
    Done!

@adalpari
Copy link
Contributor Author

  1. Inconsistent Date Handling (Priority: Medium)

Done!

@adalpari adalpari requested a review from nbradbury January 23, 2026 16:14
@adalpari adalpari marked this pull request as ready for review January 23, 2026 16:14
@sonarqubecloud
Copy link

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants