From 3c17afdf4b183e5b8d940302ff6b1802e93598ce Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 20:24:19 +0000 Subject: [PATCH] fix(filters): scope drink facets by all filters but their own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Category, style, and allergen facets each used a different, ad-hoc scoping rule, so selecting one filter didn't consistently narrow the others and could hide the very option a user had just selected. Replace all three with one rule, implemented once in _scopeFor(): a facet is derived from every structural filter except its own. A selected option is always kept in its facet list/count even when its scoped count drops to 0, so an active filter (especially an allergen exclusion, a safety control) never disappears from the UI. Also fix availableAllergens to only list allergens with a non-zero value in scope, since a value of 0 means "confirmed absent" — previously zeroed keys inflated the allergen list with permanent no-op toggles. Free-text search stays excluded from facet scoping, unchanged. Fixes #317 --- .../controllers/drink_filter_controller.dart | 118 +++-- .../drink_filter_controller_test.dart | 407 +++++++++++++++++- 2 files changed, 490 insertions(+), 35 deletions(-) diff --git a/lib/domain/controllers/drink_filter_controller.dart b/lib/domain/controllers/drink_filter_controller.dart index 2a9d7677..b2ff8a09 100644 --- a/lib/domain/controllers/drink_filter_controller.dart +++ b/lib/domain/controllers/drink_filter_controller.dart @@ -14,6 +14,36 @@ import '../services/services.dart'; /// /// All mutators are synchronous and side-effect free; callers are responsible /// for persisting and broadcasting changes. +/// +/// ## Facet scoping rule +/// +/// [availableCategories], [categoryCountsMap], [availableStyles], +/// [styleCountsMap], and [availableAllergens] are all derived by one rule: +/// **a facet is computed from the source with every *other* structural +/// filter applied — but never its own.** Structural filters are category, +/// styles, favourites-only, visibility filters, and excluded allergens. A +/// facet must not narrow itself, or selecting one of its own options would +/// hide its siblings and the list would collapse under the user's finger +/// (e.g. picking one style must not make every other style disappear from +/// the style picker). See [_scopeFor], the single helper all five getters +/// share. +/// +/// Two invariants hold for every facet: +/// 1. A currently-selected option is always listed, even when its scoped +/// count is 0 — an active filter (especially an allergen exclusion, +/// which is a safety filter) must never vanish from the UI the user +/// would use to clear it. +/// 2. [availableAllergens] lists only allergens actually present (a +/// non-zero value) in scope, not merely mentioned with a value of 0 in a +/// drink's allergens map — matching +/// [DrinkFilterService.filterByExcludedAllergens]'s own definition of +/// "absent". +/// +/// Free-text search is deliberately **excluded** from facet scoping. +/// `drinks_screen.dart` derives `hasStyleFilter` from +/// `provider.availableStyles.isNotEmpty` to decide whether to show the Style +/// button in the filter bar; scoping facets by the search query would make +/// that button appear and disappear as the user types. class DrinkFilterController { final DrinkFilterService _filterService; final DrinkSortService _sortService; @@ -55,52 +85,75 @@ class DrinkFilterController { /// Drinks after the active filters and sort have been applied. List get filteredDrinks => _filtered; - /// Unique categories present in the source drinks, sorted. + /// Unique categories present in scope (see class doc), sorted naturally. + /// The [selectedCategory], if any, is always included even if its scoped + /// count is 0 (invariant 1). List get availableCategories { - return _source.map((d) => d.category).toSet().toList()..sort(); + final categories = _scopeFor( + _Facet.category, + ).map((d) => d.category).toSet(); + if (_selectedCategory != null) categories.add(_selectedCategory!); + return categories.toList()..sort(); } - /// Unique styles in the source drinks, narrowed to the selected category when - /// one is active, sorted case-insensitively (via - /// [StringComparisonHelper.compareCaseInsensitive]) so styles order in a stable, - /// human-friendly way regardless of capitalisation. Presentation consumes - /// this directly — no sorting in the UI. + /// Unique styles in scope (see class doc), sorted case-insensitively (via + /// [StringComparisonHelper.compareCaseInsensitive]) so styles order in a + /// stable, human-friendly way regardless of capitalisation. Every + /// [selectedStyles] entry is always included even if its scoped count is 0 + /// (invariant 1). Presentation consumes this directly — no sorting in the + /// UI. List get availableStyles { - return _categoryScopedSource() - .where((d) => d.style != null && d.style!.isNotEmpty) - .map((d) => d.style!) - .toSet() - .toList() - ..sort(StringComparisonHelper.compareCaseInsensitive); + final styles = + _scopeFor(_Facet.style) + .where((d) => d.style != null && d.style!.isNotEmpty) + .map((d) => d.style!) + .toSet() + ..addAll(_selectedStyles); + return styles.toList()..sort(StringComparisonHelper.compareCaseInsensitive); } - /// Drink count per category across the full source. + /// Drink count per category, scoped per the class doc. A [selectedCategory] + /// with no matches in scope is still present, mapped to 0 (invariant 1). Map get categoryCountsMap { final counts = {}; - for (final drink in _source) { + for (final drink in _scopeFor(_Facet.category)) { counts[drink.category] = (counts[drink.category] ?? 0) + 1; } + if (_selectedCategory != null) { + counts.putIfAbsent(_selectedCategory!, () => 0); + } return counts; } - /// Drink count per style, narrowed to the selected category when one is - /// active. + /// Drink count per style, scoped per the class doc. Every entry of + /// [selectedStyles] with no matches in scope is still present, mapped to 0 + /// (invariant 1). Map get styleCountsMap { final counts = {}; - for (final drink in _categoryScopedSource()) { + for (final drink in _scopeFor(_Facet.style)) { if (drink.style != null && drink.style!.isNotEmpty) { counts[drink.style!] = (counts[drink.style!] ?? 0) + 1; } } + for (final style in _selectedStyles) { + counts.putIfAbsent(style, () => 0); + } return counts; } - /// Every allergen key present across the source drinks. + /// Allergens actually present (non-zero) on at least one drink in scope + /// (see class doc and invariant 2), plus every currently + /// [excludedAllergens] entry even if nothing in scope carries it + /// (invariant 1) — a ticked allergen exclusion is a safety filter and must + /// stay visible so the user can untick it. Set get availableAllergens { final allergens = {}; - for (final drink in _source) { - allergens.addAll(drink.allergens.keys); + for (final drink in _scopeFor(_Facet.allergen)) { + for (final entry in drink.allergens.entries) { + if (entry.value != 0) allergens.add(entry.key); + } } + allergens.addAll(_excludedAllergens); return allergens; } @@ -232,10 +285,21 @@ class DrinkFilterController { } } - /// Source narrowed to the selected category, or the full source when no - /// category is selected. - Iterable _categoryScopedSource() { - if (_selectedCategory == null) return _source; - return _source.where((d) => d.category == _selectedCategory); - } + /// Source filtered by every structural criterion *except* the one + /// belonging to [facet] — the single implementation of the facet-scoping + /// rule documented on the class. Free-text search is intentionally never + /// applied here (see class doc). + Iterable _scopeFor(_Facet facet) => _filterService.filterDrinks( + _source, + category: facet == _Facet.category ? null : _selectedCategory, + styles: facet == _Facet.style ? const {} : _selectedStyles, + favoritesOnly: _showFavoritesOnly, + visibilityFilters: _visibilityFilters, + excludedAllergens: facet == _Facet.allergen ? const {} : _excludedAllergens, + // searchQuery intentionally omitted — see class doc "Facet scoping rule". + ); } + +/// Which structural criterion a facet getter must not apply to itself. See +/// [DrinkFilterController._scopeFor]. +enum _Facet { category, style, allergen } diff --git a/test/domain/controllers/drink_filter_controller_test.dart b/test/domain/controllers/drink_filter_controller_test.dart index f86a7f17..3995483b 100644 --- a/test/domain/controllers/drink_filter_controller_test.dart +++ b/test/domain/controllers/drink_filter_controller_test.dart @@ -310,17 +310,86 @@ void main() { }); }); - group('facet getters', () { - test('availableCategories is unique and sorted', () { + // Facet scoping rule under test: each facet is derived from the source + // with every *other* structural filter applied, but never its own. See + // the class doc on DrinkFilterController for the full statement. + + group('category facet scoping', () { + test('availableCategories and categoryCountsMap span the full source ' + 'when no other filter is active', () { controller.setSource(_sampleDrinks()); expect(controller.availableCategories, ['beer', 'cider']); + expect(controller.categoryCountsMap, {'beer': 2, 'cider': 2}); }); - test('categoryCountsMap counts across the full source', () { - controller.setSource(_sampleDrinks()); + test('categories narrow when a style filter is active', () { + controller + ..setSource(_sampleDrinks()) + ..toggleStyle('IPA'); // IPA only exists on a beer drink. + expect(controller.availableCategories, ['beer']); + expect(controller.categoryCountsMap, {'beer': 1}); + }); + + test('categories narrow when favourites-only is active', () { + final drinks = _sampleDrinks(); + drinks[0] = drinks[0].copyWith( + userState: UserDrinkState.initial().copyWith(wantToTry: true), + ); // Alpha Ale (beer) is the only favourite. + controller + ..setSource(drinks) + ..setShowFavoritesOnly(value: true); + expect(controller.availableCategories, ['beer']); + expect(controller.categoryCountsMap, {'beer': 1}); + }); + + test('categories narrow when a visibility filter is active', () { + final drinks = _sampleDrinks(); + // Mark both beer drinks tasted so the entire category drops out + // under the not-tasted filter. + drinks[0] = drinks[0].copyWith( + userState: UserDrinkState.initial().copyWith( + tastingEvents: [DateTime(2026, 5, 18)], + ), + ); + drinks[1] = drinks[1].copyWith( + userState: UserDrinkState.initial().copyWith( + tastingEvents: [DateTime(2026, 5, 18)], + ), + ); + controller + ..setSource(drinks) + ..setVisibilityFilter(DrinkVisibilityFilter.notTasted, active: true); + expect(controller.availableCategories, ['cider']); + expect(controller.categoryCountsMap, {'cider': 2}); + }); + + test('categories narrow when an allergen exclusion is active', () { + controller + ..setSource([ + _drink( + id: 'a', + name: 'Gluten Pale', + category: 'beer', + allergens: {'gluten': 1}, + ), + _drink(id: 'b', name: 'Only Cider', category: 'cider'), + ]) + ..setAllergenFilter('gluten', active: true); + expect(controller.availableCategories, ['cider']); + expect(controller.categoryCountsMap, {'cider': 1}); + }); + + test('category facet does not narrow itself — selecting one category ' + 'still lists the others', () { + controller + ..setSource(_sampleDrinks()) + ..setCategory('beer'); + expect(controller.availableCategories, ['beer', 'cider']); expect(controller.categoryCountsMap, {'beer': 2, 'cider': 2}); }); + }); + group('style facet scoping', () { test('availableStyles spans all categories when none selected', () { controller.setSource(_sampleDrinks()); expect(controller.availableStyles, ['Bitter', 'Dry', 'IPA', 'Sweet']); @@ -353,17 +422,339 @@ void main() { expect(controller.styleCountsMap, {'IPA': 1, 'Bitter': 1}); }); - test('availableAllergens aggregates keys across the source', () { + test('styles narrow when favourites-only is active', () { + final drinks = _sampleDrinks(); + drinks[0] = drinks[0].copyWith( + userState: UserDrinkState.initial().copyWith(wantToTry: true), + ); // Alpha Ale (style IPA) is the only favourite. + controller + ..setSource(drinks) + ..setShowFavoritesOnly(value: true); + expect(controller.availableStyles, ['IPA']); + expect(controller.styleCountsMap, {'IPA': 1}); + }); + + test('styles narrow when a visibility filter is active', () { + final drinks = _sampleDrinks(); + // Tag every drink except Crisp Cider (style Dry) as tasted. + drinks[0] = drinks[0].copyWith( + userState: UserDrinkState.initial().copyWith( + tastingEvents: [DateTime(2026, 5, 18)], + ), + ); + drinks[1] = drinks[1].copyWith( + userState: UserDrinkState.initial().copyWith( + tastingEvents: [DateTime(2026, 5, 18)], + ), + ); + drinks[3] = drinks[3].copyWith( + userState: UserDrinkState.initial().copyWith( + tastingEvents: [DateTime(2026, 5, 18)], + ), + ); + controller + ..setSource(drinks) + ..setVisibilityFilter(DrinkVisibilityFilter.notTasted, active: true); + expect(controller.availableStyles, ['Dry']); + expect(controller.styleCountsMap, {'Dry': 1}); + }); + + test('styles narrow when an allergen exclusion is active', () { + controller + ..setSource([ + _drink( + id: 'a', + name: 'Gluten IPA', + category: 'beer', + style: 'IPA', + allergens: {'gluten': 1}, + ), + _drink( + id: 'b', + name: 'Clean Stout', + category: 'beer', + style: 'Stout', + ), + ]) + ..setAllergenFilter('gluten', active: true); + expect(controller.availableStyles, ['Stout']); + expect(controller.styleCountsMap, {'Stout': 1}); + }); + + test('style facet does not narrow itself — selecting one style still ' + 'lists the others', () { + controller + ..setSource(_sampleDrinks()) + ..toggleStyle('IPA'); + expect(controller.availableStyles, ['Bitter', 'Dry', 'IPA', 'Sweet']); + expect(controller.styleCountsMap, { + 'IPA': 1, + 'Bitter': 1, + 'Dry': 1, + 'Sweet': 1, + }); + }); + }); + + group('allergen facet scoping', () { + test('availableAllergens narrows to the selected category', () { + controller + ..setSource([ + _drink( + id: 'a', + name: 'Gluten Beer', + category: 'beer', + allergens: {'gluten': 1}, + ), + _drink( + id: 'b', + name: 'Nutty Cider', + category: 'cider', + allergens: {'nuts': 1}, + ), + ]) + ..setCategory('beer'); + expect(controller.availableAllergens, {'gluten'}); + }); + + test('availableAllergens narrows to the selected style', () { + controller + ..setSource([ + _drink( + id: 'a', + name: 'Gluten IPA', + category: 'beer', + style: 'IPA', + allergens: {'gluten': 1}, + ), + _drink( + id: 'b', + name: 'Nutty Stout', + category: 'beer', + style: 'Stout', + allergens: {'nuts': 1}, + ), + ]) + ..toggleStyle('IPA'); + expect(controller.availableAllergens, {'gluten'}); + }); + + test('availableAllergens narrows when favourites-only is active', () { + final favourite = + _drink( + id: 'a', + name: 'Gluten Beer', + category: 'beer', + allergens: {'gluten': 1}, + ).copyWith( + userState: UserDrinkState.initial().copyWith(wantToTry: true), + ); + controller + ..setSource([ + favourite, + _drink( + id: 'b', + name: 'Nutty Beer', + category: 'beer', + allergens: {'nuts': 1}, + ), + ]) + ..setShowFavoritesOnly(value: true); + expect(controller.availableAllergens, {'gluten'}); + }); + + test('allergen facet does not narrow itself — excluding one allergen ' + 'still lists the others', () { + controller + ..setSource([ + _drink( + id: 'a', + name: 'Gluten Beer', + category: 'beer', + allergens: {'gluten': 1}, + ), + _drink( + id: 'b', + name: 'Nutty Beer', + category: 'beer', + allergens: {'nuts': 1}, + ), + ]) + ..setAllergenFilter('gluten', active: true); + expect(controller.availableAllergens, {'gluten', 'nuts'}); + }); + }); + + group('facet invariant: an active filter is never hidden', () { + test('selected category with a scoped count of 0 is still listed, with ' + 'count 0', () { + controller + ..setSource(_sampleDrinks()) + ..setCategory('cider') + ..toggleStyle('IPA'); // IPA has no cider drinks. + expect(controller.selectedCategory, 'cider'); + expect(controller.availableCategories, containsAll(['beer', 'cider'])); + expect(controller.categoryCountsMap['cider'], 0); + expect(controller.categoryCountsMap['beer'], 1); + }); + + test('selected style with a scoped count of 0 is still listed, with ' + 'count 0', () { + controller + ..setSource(_sampleDrinks()) + ..setCategory('cider') + ..toggleStyle('IPA'); // IPA has no cider drinks. + expect(controller.selectedStyles, {'IPA'}); + expect( + controller.availableStyles, + containsAll(['Dry', 'Sweet', 'IPA']), + ); + expect(controller.styleCountsMap['IPA'], 0); + expect(controller.styleCountsMap['Dry'], 1); + expect(controller.styleCountsMap['Sweet'], 1); + }); + + test( + 'excluded allergen with nothing matching in scope is still listed', + () { + controller + ..setSource([_drink(id: 'a', name: 'Clean Beer', category: 'beer')]) + ..setAllergenFilter('gluten', active: true); + expect(controller.excludedAllergens, {'gluten'}); + expect(controller.availableAllergens, contains('gluten')); + }, + ); + + test('ticking a scoped-narrowed category option yields exactly the ' + 'stated count', () { + final drinks = _sampleDrinks(); + drinks[0] = drinks[0].copyWith( + userState: UserDrinkState.initial().copyWith(wantToTry: true), + ); // Alpha Ale (beer) is the only favourite. + controller + ..setSource(drinks) + ..setShowFavoritesOnly(value: true); + expect(controller.categoryCountsMap, {'beer': 1}); + + controller.setCategory('beer'); + expect(controller.filteredDrinks, hasLength(1)); + expect(controller.filteredDrinks.single.name, 'Alpha Ale'); + }); + + test('ticking a scoped-narrowed style option yields exactly the stated ' + 'count', () { + final drinks = _sampleDrinks(); + drinks[0] = drinks[0].copyWith( + userState: UserDrinkState.initial().copyWith(wantToTry: true), + ); // Alpha Ale (style IPA) is the only favourite. + controller + ..setSource(drinks) + ..setShowFavoritesOnly(value: true); + expect(controller.styleCountsMap, {'IPA': 1}); + + controller.toggleStyle('IPA'); + expect(controller.filteredDrinks, hasLength(1)); + expect(controller.filteredDrinks.single.name, 'Alpha Ale'); + }); + + test('ticking a scoped-narrowed allergen exclusion yields exactly the ' + 'stated filtered result', () { controller.setSource([ _drink( id: 'a', - name: 'A', + name: 'Gluten Beer', category: 'beer', allergens: {'gluten': 1}, ), - _drink(id: 'b', name: 'B', category: 'beer', allergens: {'nuts': 0}), + _drink(id: 'b', name: 'Clean Beer', category: 'beer'), ]); - expect(controller.availableAllergens, {'gluten', 'nuts'}); + expect(controller.availableAllergens, {'gluten'}); + + controller.setAllergenFilter('gluten', active: true); + expect(controller.filteredDrinks.map((d) => d.name), ['Clean Beer']); + }); + }); + + group('allergen presence (only non-zero counts as present)', () { + test('an allergen key present only with value 0 is not listed', () { + controller.setSource([ + _drink(id: 'a', name: 'A', category: 'beer', allergens: {'nuts': 0}), + ]); + expect(controller.availableAllergens, isNot(contains('nuts'))); + }); + + test('an allergen key is listed once some drink in scope has it ' + 'non-zero', () { + controller.setSource([ + _drink(id: 'a', name: 'A', category: 'beer', allergens: {'nuts': 0}), + _drink(id: 'b', name: 'B', category: 'beer', allergens: {'nuts': 1}), + ]); + expect(controller.availableAllergens, contains('nuts')); + }); + + test('an allergen key is listed when currently selected even if scope ' + 'only has it zeroed', () { + controller + ..setSource([ + _drink( + id: 'a', + name: 'A', + category: 'beer', + allergens: {'nuts': 0}, + ), + ]) + ..setAllergenFilter('nuts', active: true); + expect(controller.availableAllergens, contains('nuts')); + }); + + test('bool and numeric allergen values are honoured by the presence ' + 'check, not just int', () { + // Product.fromJson already normalises bool/num allergen values to + // int at parse time — confirm the facet respects that normalised + // value rather than assuming the raw map is always int-valued. + controller.setSource([ + _drink( + id: 'a', + name: 'A', + category: 'beer', + allergens: {'gluten': false, 'nuts': 2.0, 'milk': true}, + ), + ]); + expect(controller.availableAllergens, {'nuts', 'milk'}); + }); + }); + + group('search query does not affect facets', () { + test('search narrows filteredDrinks but not category/style facets', () { + controller + ..setSource(_sampleDrinks()) + ..setSearchQuery('alpha'); + expect(controller.filteredDrinks.map((d) => d.name), ['Alpha Ale']); + expect(controller.availableCategories, ['beer', 'cider']); + expect(controller.categoryCountsMap, {'beer': 2, 'cider': 2}); + expect(controller.availableStyles, ['Bitter', 'Dry', 'IPA', 'Sweet']); + expect(controller.styleCountsMap, { + 'IPA': 1, + 'Bitter': 1, + 'Dry': 1, + 'Sweet': 1, + }); + }); + + test('search narrows filteredDrinks but not the allergen facet', () { + controller + ..setSource([ + _drink( + id: 'a', + name: 'Gluten Beer', + category: 'beer', + allergens: {'gluten': 1}, + ), + _drink(id: 'b', name: 'Clean Cider', category: 'cider'), + ]) + ..setSearchQuery('clean'); + expect(controller.filteredDrinks.map((d) => d.name), ['Clean Cider']); + expect(controller.availableAllergens, {'gluten'}); }); });