-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CMM-1164 stats add stats period selector #22515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
- 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
Generated by 🚫 Danger |
|
| App Name | Jetpack | |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22515-dd2b563 | |
| Commit | dd2b563 | |
| Direct Download | jetpack-prototype-build-pr22515-dd2b563.apk |
|
| App Name | WordPress | |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22515-dd2b563 | |
| Commit | dd2b563 | |
| Direct Download | wordpress-prototype-build-pr22515-dd2b563.apk |
Codecov Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
# 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
|
OUTDATED |
We are not handling UI tests at the moment... |
These are false positives. They have already been solved... |
|
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
🟡 Architecture & Design Issues1. Date Range Validation Missing (Priority: High)Location: 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 if (startDate.isBefore(endDate) || startDate.isEqual(endDate)) {
onDateRangeSelected(startDate, endDate)
} else {
// Show error or swap dates
}2. Custom Period Label Not User-Friendly (Priority: Medium)Location: 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 3. State Management Coupling (Priority: Medium)Location: The Impact: Selected period might not survive configuration changes (screen rotation). Recommendation: Move 🟡 Code Quality Issues4. Inconsistent Date Handling (Priority: Medium)Location: The codebase mixes Impact: Increased complexity and potential timezone/parsing bugs. Recommendation: Standardize on 5. Magic String Patterns (Priority: Low)Location: 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: 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. 🟢 PerformanceThe parallel fetching of current and previous period data (StatsRepository.kt:301-318) is well-implemented using 🟢 SecurityNo security concerns identified. The code properly:
🟡 TestingThe ViewModel and Repository have good test coverage. However, as you noted, UI tests are not included. The Compose UI components ( Note: I understand UI tests are not being handled at this time per your earlier comment. 🟢 DocumentationString resources are properly added and well-named. Code is generally self-documenting with clear naming conventions. 📊 SummaryCritical Issues: 1 (date range validation) 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. |
Done!
|
Done! |
|





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:
Custom date range:
Screen_recording_20260123_162150.mp4