Implement My Festival favourites feature - #200
Conversation
Add FavoriteItem model with want_to_try/tasted states and tasting timestamps. Update FavoritesService to use new format with comprehensive tracking capabilities. New Features: - FavoriteItem model with JSON serialization and copyWith support - Track drinks as "want to try" or "tasted" with multiple tasting timestamps - Add/remove tasting timestamps with automatic status management - Optional notes field for each favorite item - Festival-scoped storage with graceful error handling Changes: - lib/models/favorite_item.dart: New FavoriteItem data model - lib/models/models.dart: Export FavoriteItem - lib/services/storage_service.dart: Migrate FavoritesService to Map<String, FavoriteItem> - test/models_test.dart: Add 26 tests for FavoriteItem - test/storage_service_test.dart: Update 22 tests for new FavoritesService format This implements Phase 3.1-3.2 of the My Festival feature as documented in docs/planning/festival-log/implementation-plan.md. Next phases will update BeerProvider and UI components.
Update repository interface and BeerProvider to support new My Festival features. All changes are backward compatible and compile successfully. Repository Changes: - Add new methods to DrinkRepository interface: * getFavoriteStatus() - Get drink's festival log status * markAsTasted() - Add tasting timestamp * deleteTry() - Remove tasting timestamp * getTryCount() - Get number of tastings - Implement new methods in ApiDrinkRepository - Fix getFavorites() to use new Map<String, FavoriteItem> format BeerProvider Changes: - Add getFavoriteStatus() - Get want_to_try/tasted status - Add markAsTasted() - Mark drink as tasted with timestamp - Add deleteTry() - Remove specific tasting timestamp - Add getTryCount() - Get tasting count - Add isInFestivalLog() - Check if drink is in log - All methods properly update UI state and log analytics AnalyticsService Changes: - Add logFestivalLogMarkTasted() - Track first tasting - Add logFestivalLogMultipleTasting() - Track repeat tastings - Add logFestivalLogDeleteTimestamp() - Track timestamp deletions This completes the data layer migration for My Festival. The app now: - Tracks drinks as "want to try" or "tasted" - Supports multiple tasting timestamps per drink - Maintains festival-scoped data - All existing functionality preserved - No breaking changes introduced Analysis: ✅ No issues found Next: Phase 4 will add UI components (badges, detail screen, festival log screen)
Add comprehensive tests for the 4 new BeerProvider methods introduced in Phase 3.3: - getFavoriteStatus() - returns status from repository or null - getTryCount() - returns count from repository or 0 - markAsTasted() - updates state, notifies listeners, logs analytics - deleteTry() - removes timestamps, updates state, reapplies filters Also fixes a critical bug in FavoriteItem.copyWith() where nullable parameters couldn't be explicitly set to null. Introduced Optional<T> wrapper class to handle this common Dart pattern issue. Changes: - test/beer_provider_test.dart: Added 13 tests for My Festival methods - lib/models/favorite_item.dart: Fixed copyWith with Optional wrapper - lib/services/storage_service.dart: Updated updateNotes to use Optional All 578 tests passing. Code analysis clean.
LCOV of commit
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR refactors the festival favorites feature from a simple favorite list into a comprehensive "My Festival Log" system. Users can now track drinks with statuses (want_to_try or tasted), multiple tasting timestamps, and personal notes.
Key changes:
- Introduced
FavoriteItemmodel with status tracking, timestamps, and notes support - Refactored storage from Set-based to Map-based structure with full JSON serialization
- Extended repository and provider interfaces with new methods for marking drinks as tasted, deleting specific tastings, and retrieving tasting counts
- Added analytics events for festival log interactions
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
lib/models/favorite_item.dart |
New model representing a drink in the festival log with status, tries list, notes, and timestamps |
lib/models/models.dart |
Exports the new FavoriteItem model |
lib/services/storage_service.dart |
Refactored FavoritesService from Set to Map storage with methods for marking as tasted, deleting tries, and updating notes |
lib/services/analytics_service.dart |
Added three new analytics methods for festival log interactions |
lib/domain/repositories/drink_repository.dart |
Added four new methods to the interface for festival log functionality |
lib/domain/repositories/api_drink_repository.dart |
Implemented new repository methods delegating to FavoritesService |
lib/providers/beer_provider.dart |
Added provider methods for getting favorite status, try count, marking as tasted, and deleting tries with analytics integration |
test/storage_service_test.dart |
Comprehensive tests for new favorite item functionality including tasting, deletion, and notes |
test/models_test.dart |
Full test coverage for FavoriteItem model including JSON serialization, copyWith, and equality |
test/beer_provider_test.dart |
Tests for new provider methods covering all scenarios including edge cases |
test/provider_test.mocks.dart |
Auto-generated mock implementations for new repository and analytics methods |
| // Not in log yet, add as tasted | ||
| favorites[drinkId] = FavoriteItem( | ||
| id: drinkId, | ||
| status: 'tasted', |
There was a problem hiding this comment.
The string literal 'tasted' is used directly here without type-safe constants. This is repeated in multiple places (markAsTasted in line 120) creating potential for typos. Consider using an enum or constants defined in the FavoriteItem model to ensure consistency and type safety across the codebase.
| final existing = favorites[drinkId]; | ||
| if (existing == null) return; | ||
|
|
||
| final updatedTries = existing.tries.where((t) => t != timestamp).toList(); |
There was a problem hiding this comment.
DateTime equality comparison using != may be fragile due to potential microsecond differences when timestamps are deserialized from JSON. Consider comparing timestamps using millisecondsSinceEpoch or implementing a tolerance-based comparison to ensure reliable deletion of the correct timestamp. For example:
final updatedTries = existing.tries
.where((t) => t.millisecondsSinceEpoch != timestamp.millisecondsSinceEpoch)
.toList();| final updatedTries = existing.tries.where((t) => t != timestamp).toList(); | |
| final updatedTries = existing.tries | |
| .where((t) => t.millisecondsSinceEpoch != timestamp.millisecondsSinceEpoch) | |
| .toList(); |
| test('deleteTry removes specific timestamp', () async { | ||
| final prefs = await SharedPreferences.getInstance(); | ||
| favoritesService = FavoritesService(prefs); | ||
|
|
||
| await favoritesService.markAsTasted('cbf2025', 'drink-123'); | ||
| await favoritesService.markAsTasted('cbf2025', 'drink-123'); | ||
|
|
||
| final item = favoritesService.getFavoriteItem('cbf2025', 'drink-123'); | ||
| final firstTry = item!.tries.first; | ||
|
|
||
| await favoritesService.deleteTry('cbf2025', 'drink-123', firstTry); | ||
|
|
||
| final updated = favoritesService.getFavoriteItem('cbf2025', 'drink-123'); | ||
| expect(updated!.tries.length, 1); | ||
| expect(updated.tries.contains(firstTry), isFalse); | ||
| }); |
There was a problem hiding this comment.
There's no test coverage for deleteTry after data has been persisted and deserialized from JSON. This is important because DateTime comparison issues could manifest when timestamps are round-tripped through JSON serialization. Consider adding a test that:
- Marks a drink as tasted
- Creates a new service instance with the same prefs
- Retrieves the timestamp from the reloaded data
- Attempts to delete that timestamp
This would verify that DateTime comparison works correctly after JSON serialization.
| @override | ||
| bool operator ==(Object other) => | ||
| identical(this, other) || | ||
| other is FavoriteItem && | ||
| runtimeType == other.runtimeType && | ||
| id == other.id; | ||
|
|
||
| @override | ||
| int get hashCode => id.hashCode; |
There was a problem hiding this comment.
The equality operator only compares the id field, ignoring all other fields (status, tries, notes, createdAt, updatedAt). This means two FavoriteItem instances with the same id but completely different data are considered equal. This could lead to unexpected behavior when using FavoriteItem in collections like Sets or as Map keys. Consider whether this is the intended behavior, or if equality should compare all fields. If id-only equality is intentional, add a comment explaining this design decision.
| /// Wrapper class for explicitly passing null values in copyWith methods. | ||
| class Optional<T> { | ||
| const Optional.value(this.value); | ||
|
|
||
| final T value; | ||
| } |
There was a problem hiding this comment.
The Optional class lacks documentation explaining its purpose and usage. Since this is a utility class used to distinguish between "not providing a value" and "explicitly providing null" in copyWith methods, it would benefit from a doc comment. For example:
/// Wrapper class for explicitly passing null values in copyWith methods.
///
/// Used to distinguish between omitting a parameter (keep existing value)
/// and explicitly passing null (clear the value).
class Optional<T> {
const Optional.value(this.value);
final T value;
}| final favorites = favoritesService.getFavorites('cbf2025'); | ||
|
|
||
| expect(favorites, isEmpty); | ||
| expect(favorites, isA<Map<String, dynamic>>()); |
There was a problem hiding this comment.
The type assertion isA<Map<String, dynamic>>() is incorrect. The getFavorites method returns Map<String, FavoriteItem>, not Map<String, dynamic>. This test assertion should be:
expect(favorites, isA<Map<String, FavoriteItem>>());| expect(favorites, isA<Map<String, dynamic>>()); | |
| expect(favorites, isA<Map<String, FavoriteItem>>()); |
| /// Status: 'want_to_try' or 'tasted'. | ||
| final String status; |
There was a problem hiding this comment.
The status field uses string literals ('want_to_try' and 'tasted') instead of type-safe constants or an enum. This creates potential for typos and inconsistencies. Consider defining an enum or string constants for these values to improve type safety and maintainability. For example:
enum FavoriteStatus {
wantToTry('want_to_try'),
tasted('tasted');
const FavoriteStatus(this.value);
final String value;
}Or at minimum, define string constants in the FavoriteItem class.
|
|
||
| favorites[drinkId] = FavoriteItem( | ||
| id: drinkId, | ||
| status: 'want_to_try', |
There was a problem hiding this comment.
The string literal 'want_to_try' is used directly here without type-safe constants. This is repeated in multiple places (addFavorite, toggleFavorite, deleteTry) creating potential for typos. Consider using an enum or constants defined in the FavoriteItem model to ensure consistency and type safety across the codebase.
|
@copilot open a new pull request to apply changes based on the comments in this thread, |
|
@richardthe3rd I've opened a new pull request, #201, to work on those changes. Once the pull request is ready, I'll request review from you. |
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://claude-implement-my-festival.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
- Added FavoriteStatus enum with wantToTry and tasted values - Updated FavoriteItem model to use FavoriteStatus instead of string literals - Fixed DateTime comparison in deleteTry to use millisecondsSinceEpoch - Added comprehensive documentation for FavoriteItem equality operator - Added detailed documentation for Optional class with usage examples - Fixed test assertion type from Map<String, dynamic> to Map<String, FavoriteItem> - Added test coverage for deleteTry after JSON serialization - Updated all tests to use FavoriteStatus enum Co-authored-by: richardthe3rd <573334+richardthe3rd@users.noreply.github.com>
Refactor favorite status to type-safe enum and fix DateTime comparison
|
@copilot Continue My Festival Implementation - Phase 4 (UI Layer) ✅ Phase 3 Complete - Data layer fully implemented and tested (3 commits on claude/implement-my-festival-3diaO) Next: Phase 4 - UI Implementation File: lib/widgets/drink_card.dart Add visual status indicators: 4.2: DrinkDetailScreen Tasting UI File: lib/screens/drink_detail_screen.dart Replace simple favorite button with: 4.3: FavoritesScreen → Festival Log File: lib/screens/favorites_screen.dart Redesign as unified "My Festival" view: Key Requirements |
|
@richardthe3rd I've opened a new pull request, #202, to work on those changes. Once the pull request is ready, I'll request review from you. |
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://claude-implement-my-festival.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
Co-authored-by: richardthe3rd <573334+richardthe3rd@users.noreply.github.com>
Co-authored-by: richardthe3rd <573334+richardthe3rd@users.noreply.github.com>
Co-authored-by: richardthe3rd <573334+richardthe3rd@users.noreply.github.com>
Implement My Festival UI layer (Phase 4)
|
@copilot tests failed in latest ci run. Investigate. Run locally to confirm failure and fix. |
|
@richardthe3rd I've opened a new pull request, #203, to work on those changes. Once the pull request is ready, I'll request review from you. |
…tion bar overflow Co-authored-by: richardthe3rd <573334+richardthe3rd@users.noreply.github.com>
Fix test failures: GoRouter context error and bottom action bar overflow
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://claude-implement-my-festival.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
|
@copilot problems with this as implemented. 1) heart action is confusing - what does "love" mean , 2) heart is under the tasted badge, 3) empty circle (untasted) is unclear, 4) the my festival screen should be more "diary" or "log" styled. Using days of week (since festivals are fairly short) |
|
@richardthe3rd I've opened a new pull request, #204, to work on those changes. Once the pull request is ready, I'll request review from you. |
- Replace heart icon with bookmark icon for "Want to Try" action - Move status badge to left side to avoid overlap with bookmark button - Use bookmark icon for "Want to Try" status (instead of empty circle) - Redesign My Festival screen with diary-style layout - Group tasted drinks by day (Today, Yesterday, weekday names, dates) - Show timestamps for each tasting - Visual separation between "Want to Try" and tasted sections - More compact tasted drink cards with inline timestamps - Update all tests to use bookmark icons instead of heart icons Co-authored-by: richardthe3rd <573334+richardthe3rd@users.noreply.github.com>
Refactor Festival Log UI: bookmark metaphor and diary-style layout
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://claude-implement-my-festival.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
|
@copilot the new position of the tasted icon is still not right. Now it overlaps name of drink. How about bottom right? As long as doesn't overlap info chips. Also the uncheck circle is pointless. Bookmark is enough? |
|
@richardthe3rd I've opened a new pull request, #205, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: richardthe3rd <573334+richardthe3rd@users.noreply.github.com>
Fix tasted badge positioning and remove redundant want_to_try indicator
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://claude-implement-my-festival.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
This pull request introduces a major refactor to the festival drink favorites feature, expanding it from a simple favorite list to a full "My Festival Log" system. Drinks can now be tracked with statuses, tasting timestamps, and notes. The changes affect the data model, repository interfaces, local storage, provider logic, and analytics events.
Data Model & Storage Refactor
FavoriteItemmodel to represent drinks in the festival log, including status (want_to_tryortasted), tasting timestamps, notes, and metadata. (lib/models/favorite_item.dart)FavoritesServiceto store favorite drinks as a map ofFavoriteItemobjects per festival, supporting status, multiple tastings, and notes. Methods for adding, removing, marking as tasted, deleting tastings, and updating notes were added. (lib/services/storage_service.dart)Repository Interface & Implementation
DrinkRepositoryandApiDrinkRepositoryto support new log features: getting favorite status, marking as tasted, deleting specific tastings, and retrieving tasting counts. The favorite-related methods now operate on the newFavoriteItemstructure. (lib/domain/repositories/drink_repository.dart,lib/domain/repositories/api_drink_repository.dart) [1] [2] [3] [4]Provider Logic
BeerProviderwith methods to get favorite status, check log presence, count tastings, mark as tasted, and delete specific tastings, reflecting the new log model. State and analytics are updated accordingly when these actions occur. (lib/providers/beer_provider.dart)Analytics
lib/services/analytics_service.dart)Miscellaneous
favorite_item.dartmodel for use throughout the app. (lib/models/models.dart)