-
Notifications
You must be signed in to change notification settings - Fork 108
[MBL-19567][Student] Update ToDo filter default time periods #3418
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
Conversation
Updated default time period values for ToDo filters to align with product requirements: - Past date range: Changed from ONE_WEEK to FOUR_WEEKS (4 weeks ago) - Future date range: Changed from ONE_WEEK to THIS_WEEK The Georgian calendar week calculation logic remains unchanged (locale-aware week boundaries). Changes: - ToDoFilterEntity: Updated default pastDateRange and futureDateRange - ToDoListViewModel: Updated default filter checks in trackFilterLoadingEvent() and isFilterApplied() - ToDoFilterViewModel: Updated initial selectedPastOption and selectedFutureOption values - Updated corresponding unit tests to reflect new default values Test plan: - Verify ToDo list loads with default filters (4 weeks past, this week future) - Verify filter analytics correctly identifies default vs custom filters - Verify all unit tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
This PR updates the default date range filters for the To-Do list feature from 1 week (past/future) to 4 weeks (past) and "this week" (future). The changes are well-contained and consistently applied across the codebase.
Issues Found
- Default value inconsistency in
ToDoFilterUiState.kt: The data class default parameters still useONE_WEEKfor both past and future selections, but everywhere else in the PR these have been changed toFOUR_WEEKS(past) andTHIS_WEEK(future). This inconsistency could cause bugs where the UI state doesn't match expected defaults.
Positive Aspects
✅ Consistent updates across all layers: The changes are applied consistently across ViewModels, entities, and test files
✅ Test coverage: All affected tests have been updated to reflect the new default values
✅ No breaking changes: The change only affects default values, not the functionality itself
✅ Clear intent: The PR makes the default date range more useful by showing more past items (4 weeks vs 1 week) while keeping future items focused on the current week
Recommendations
- Update the default parameters in
ToDoFilterUiState.kt(lines 25, 27) to match the new defaults - Consider if a database migration is needed - since
ToDoFilterEntityhas default values that changed, existing users with no saved filters will see the new defaults, but users with explicitly saved filters using old defaults will keep their old values (which may be the desired behavior)
Code Quality
The code follows project conventions and the changes are straightforward. No security concerns, performance issues, or architectural problems identified.
🧪 Unit Test Results✅ 📱 Student App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Fri, 28 Nov 2025 14:54:42 GMT |
📊 Code Coverage Report✅ Student
✅ Teacher
✅ Pandautils
📈 Overall Average
|
kdeakinstructure
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA 👍
adamNagy56
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA +1
Updated testDefaultValues() to expect FOUR_WEEKS (past) and THIS_WEEK (future) instead of ONE_WEEK for both.
Updated two tests to work with new default date ranges: 1. clickDateBadgeNavigatesToCalendar: - Changed assignment from 3 days to 1 day in the future - Ensures assignment is always within 'This Week' default range 2. pastDateRangeFilterShowsOlderItems: - Changed old assignment from 2 weeks to 5 weeks ago - Updated comment to reflect new 4-week default - Assignment is now outside default range as intended - Still verifies filter selection works correctly
Updated two tests to work with new default date ranges:
1. clickDateBadgeNavigatesToCalendar:
- Restored assignment to 3 days in the future (original logic)
- Added explicit filter setup to 'Next Week' before test
- Ensures assignment is always visible regardless of current day
2. pastDateRangeFilterShowsOlderItems:
- Changed old assignment from 2 weeks to 3 weeks ago
- Updated to test filter changes properly:
* Default (4 weeks): both assignments visible
* Change to Last Week (1 week): old assignment hidden
* Change to 4 Weeks Ago: old assignment visible again
- Now properly tests that filter controls item visibility
Test plan:
Expected behavior:
refs: MBL-19567
affects: Student
release note: Updated default time period for ToDo list to show items from 4 weeks ago through this week