From 628d5cc6e64925ff86c39bc9493db09e4ea4eebc Mon Sep 17 00:00:00 2001 From: Petru Paler Date: Fri, 2 Jan 2026 13:01:07 +0000 Subject: [PATCH] fix: subset selection validation and remove unnecessary hash computation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs fixed in animal selection with checkboxes: 1. Confirmation dialog showed wrong count (e.g., "35 animals" instead of "2 animals" when only 2 were selected). Fixed by using valid_selected count in diff.server_count instead of full filter resolution count. 2. Spurious "Selection Changed" dialogs due to race condition in async hash computation. Fixed by removing client-side hash computation entirely - it was unnecessary since the server validates selected_ids directly against the filter resolution. Changes: - validation.py: Remove hash comparison in _validate_subset(), validate IDs directly, fix server_count in diff - animal_select.py: Remove computeSelectionHash(), hidden roster_hash field, and related async fetch code - test_selection_validation.py: Add tests for subset mode validation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- src/animaltrack/selection/validation.py | 45 ++++------ .../web/templates/animal_select.py | 48 ----------- tests/test_selection_validation.py | 83 +++++++++++++++++++ 3 files changed, 101 insertions(+), 75 deletions(-) diff --git a/src/animaltrack/selection/validation.py b/src/animaltrack/selection/validation.py index 156088b..e0a7f15 100644 --- a/src/animaltrack/selection/validation.py +++ b/src/animaltrack/selection/validation.py @@ -185,39 +185,30 @@ def _validate_subset( # Find selected IDs that no longer match the filter invalid_ids = selected_set - resolved_set + # Compute valid selected IDs (those that still match the filter) + valid_selected = [sid for sid in selected_ids if sid in resolved_set] + if not invalid_ids: - # All selected IDs are valid - compute hash from selected IDs - subset_hash = compute_roster_hash(selected_ids, context.from_location_id) - - # Verify hash matches what client sent - if subset_hash == context.roster_hash: - return SelectionValidationResult( - valid=True, - resolved_ids=selected_ids, - roster_hash=context.roster_hash, - diff=None, - ) - - # Some selected IDs are no longer valid, or hash mismatch - # Compute diff: removed = invalid_ids, added = none - diff = SelectionDiff( - added=[], - removed=sorted(invalid_ids), - server_count=len(resolved_ids), - client_count=len(selected_ids), - ) - - if context.confirmed and not invalid_ids: - # Client confirmed, and all IDs are still valid + # All selected IDs are still in the filter resolution - valid + # No hash comparison needed: we validate IDs directly return SelectionValidationResult( valid=True, resolved_ids=selected_ids, - roster_hash=context.roster_hash, - diff=diff, + roster_hash=compute_roster_hash(selected_ids, context.from_location_id), + diff=None, ) - # Invalid - return with valid selected IDs (those that still match) - valid_selected = [sid for sid in selected_ids if sid in resolved_set] + # Some selected IDs are no longer valid + # Compute diff: removed = invalid_ids, added = none + # In subset mode, server_count reflects valid selected count, not full filter + diff = SelectionDiff( + added=[], + removed=sorted(invalid_ids), + server_count=len(valid_selected), + client_count=len(selected_ids), + ) + + # Return invalid with valid selected IDs (those that still match) new_hash = compute_roster_hash(valid_selected, context.from_location_id) return SelectionValidationResult( diff --git a/src/animaltrack/web/templates/animal_select.py b/src/animaltrack/web/templates/animal_select.py index 3b3a582..76c0cf9 100644 --- a/src/animaltrack/web/templates/animal_select.py +++ b/src/animaltrack/web/templates/animal_select.py @@ -98,8 +98,6 @@ def animal_checkbox_list( *resolved_id_fields, # Hidden field to indicate subset selection mode Input(type="hidden", name="subset_mode", value="true"), - # Hidden field for roster_hash - will be updated via JS - Input(type="hidden", name="roster_hash", id="roster-hash-field"), # Script for selection management selection_script(len(animals)), id="animal-selection-list", @@ -126,8 +124,6 @@ def selection_script(total_count: int) -> Div: if (countText) {{ countText.textContent = checked + ' of {total_count} selected'; }} - // Trigger hash recomputation if needed - computeSelectionHash(); }} function selectAllAnimals(selectAll) {{ @@ -137,50 +133,6 @@ def selection_script(total_count: int) -> Div: }}); updateSelectionCount(); }} - - function getSelectedIds() {{ - var checkboxes = document.querySelectorAll('#animal-selection-list input[name="selected_ids"]:checked'); - return Array.from(checkboxes).map(cb => cb.value); - }} - - function computeSelectionHash() {{ - // Get selected IDs and compute hash via API - var selectedIds = getSelectedIds(); - var fromLocationId = document.querySelector('input[name="from_location_id"]'); - var fromLoc = fromLocationId ? fromLocationId.value : ''; - - fetch('/api/compute-hash', {{ - method: 'POST', - headers: {{'Content-Type': 'application/json'}}, - body: JSON.stringify({{ - selected_ids: selectedIds, - from_location_id: fromLoc - }}) - }}) - .then(response => {{ - if (!response.ok) {{ - console.error('Hash computation failed:', response.status); - return {{}}; - }} - var contentType = response.headers.get('content-type'); - if (contentType && contentType.includes('application/json')) {{ - return response.json(); - }} - return {{}}; - }}) - .then(data => {{ - var hashField = document.getElementById('roster-hash-field'); - if (hashField && data.roster_hash) {{ - hashField.value = data.roster_hash; - }} - }}) - .catch(err => console.error('Hash computation error:', err)); - }} - - // Initialize hash on load - document.addEventListener('DOMContentLoaded', function() {{ - computeSelectionHash(); - }}); """) diff --git a/tests/test_selection_validation.py b/tests/test_selection_validation.py index e20ee62..92d95ae 100644 --- a/tests/test_selection_validation.py +++ b/tests/test_selection_validation.py @@ -456,3 +456,86 @@ class TestSelectionMismatchError: assert error.result is result assert error.result.diff is diff + + +# ============================================================================ +# Tests for validate_selection - subset mode +# ============================================================================ + + +class TestValidateSelectionSubsetMode: + """Tests for validate_selection with subset_mode=True.""" + + def test_subset_mode_returns_valid_when_all_selected_match( + self, seeded_db, animal_service, strip1_location_id + ): + """validate_selection returns valid=True when all selected IDs are in filter.""" + # Create cohort of 5 animals + ts_utc = int(time.time() * 1000) + payload = make_cohort_payload(strip1_location_id, count=5) + event = animal_service.create_cohort(payload, ts_utc, "test_user") + all_ids = event.entity_refs["animal_ids"] + + # User selects only 2 of them + selected_ids = all_ids[:2] + subset_hash = compute_roster_hash(selected_ids, None) + + ctx = SelectionContext( + filter="species:duck", + resolved_ids=all_ids, # Full filter resolution + roster_hash=subset_hash, # Hash of selected subset + ts_utc=ts_utc, + from_location_id=None, + subset_mode=True, + selected_ids=selected_ids, + ) + + result = validate_selection(seeded_db, ctx) + + assert result.valid is True + assert result.resolved_ids == selected_ids + assert result.diff is None + + def test_subset_mode_diff_server_count_is_valid_selected_count( + self, seeded_db, animal_service, strip1_location_id, strip2_location_id + ): + """In subset mode, diff.server_count should be count of valid selected IDs, not full filter.""" + # Create cohort of 5 animals + ts_create = int(time.time() * 1000) + payload = make_cohort_payload(strip1_location_id, count=5) + event = animal_service.create_cohort(payload, ts_create, "test_user") + all_ids = event.entity_refs["animal_ids"] + + # User selects 2 animals + selected_ids = all_ids[:2] + subset_hash = compute_roster_hash(selected_ids, None) + + # Move one selected animal away (makes it invalid for the filter) + ts_move = ts_create + 1000 + move_payload = AnimalMovedPayload( + resolved_ids=[selected_ids[0]], + from_location_id=strip1_location_id, + to_location_id=strip2_location_id, + ) + animal_service.move_animals(move_payload, ts_move, "test_user") + + # Now validate at ts_move - one of the selected animals is no longer at Strip 1 + ctx = SelectionContext( + filter="location:'Strip 1'", + resolved_ids=all_ids, # Full filter resolution at creation time + roster_hash=subset_hash, + ts_utc=ts_move, # Validate at move time + from_location_id=None, + subset_mode=True, + selected_ids=selected_ids, + ) + + result = validate_selection(seeded_db, ctx) + + assert result.valid is False + assert result.diff is not None + # BUG: diff.server_count is currently len(resolved_ids) = 4 (5 minus moved) + # SHOULD BE: len(valid_selected) = 1 (2 selected minus 1 moved) + assert result.diff.server_count == 1 # Only 1 valid selected animal remains + assert result.diff.client_count == 2 # User selected 2 + assert selected_ids[0] in result.diff.removed # The moved animal is invalid