fix: subset selection validation and remove unnecessary hash computation
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 <noreply@anthropic.com>
This commit is contained in:
@@ -185,39 +185,30 @@ def _validate_subset(
|
|||||||
# Find selected IDs that no longer match the filter
|
# Find selected IDs that no longer match the filter
|
||||||
invalid_ids = selected_set - resolved_set
|
invalid_ids = selected_set - resolved_set
|
||||||
|
|
||||||
if not invalid_ids:
|
# Compute valid selected IDs (those that still match the filter)
|
||||||
# All selected IDs are valid - compute hash from selected IDs
|
valid_selected = [sid for sid in selected_ids if sid in resolved_set]
|
||||||
subset_hash = compute_roster_hash(selected_ids, context.from_location_id)
|
|
||||||
|
|
||||||
# Verify hash matches what client sent
|
if not invalid_ids:
|
||||||
if subset_hash == context.roster_hash:
|
# All selected IDs are still in the filter resolution - valid
|
||||||
|
# No hash comparison needed: we validate IDs directly
|
||||||
return SelectionValidationResult(
|
return SelectionValidationResult(
|
||||||
valid=True,
|
valid=True,
|
||||||
resolved_ids=selected_ids,
|
resolved_ids=selected_ids,
|
||||||
roster_hash=context.roster_hash,
|
roster_hash=compute_roster_hash(selected_ids, context.from_location_id),
|
||||||
diff=None,
|
diff=None,
|
||||||
)
|
)
|
||||||
|
|
||||||
# Some selected IDs are no longer valid, or hash mismatch
|
# Some selected IDs are no longer valid
|
||||||
# Compute diff: removed = invalid_ids, added = none
|
# Compute diff: removed = invalid_ids, added = none
|
||||||
|
# In subset mode, server_count reflects valid selected count, not full filter
|
||||||
diff = SelectionDiff(
|
diff = SelectionDiff(
|
||||||
added=[],
|
added=[],
|
||||||
removed=sorted(invalid_ids),
|
removed=sorted(invalid_ids),
|
||||||
server_count=len(resolved_ids),
|
server_count=len(valid_selected),
|
||||||
client_count=len(selected_ids),
|
client_count=len(selected_ids),
|
||||||
)
|
)
|
||||||
|
|
||||||
if context.confirmed and not invalid_ids:
|
# Return invalid with valid selected IDs (those that still match)
|
||||||
# Client confirmed, and all IDs are still valid
|
|
||||||
return SelectionValidationResult(
|
|
||||||
valid=True,
|
|
||||||
resolved_ids=selected_ids,
|
|
||||||
roster_hash=context.roster_hash,
|
|
||||||
diff=diff,
|
|
||||||
)
|
|
||||||
|
|
||||||
# Invalid - return with valid selected IDs (those that still match)
|
|
||||||
valid_selected = [sid for sid in selected_ids if sid in resolved_set]
|
|
||||||
new_hash = compute_roster_hash(valid_selected, context.from_location_id)
|
new_hash = compute_roster_hash(valid_selected, context.from_location_id)
|
||||||
|
|
||||||
return SelectionValidationResult(
|
return SelectionValidationResult(
|
||||||
|
|||||||
@@ -98,8 +98,6 @@ def animal_checkbox_list(
|
|||||||
*resolved_id_fields,
|
*resolved_id_fields,
|
||||||
# Hidden field to indicate subset selection mode
|
# Hidden field to indicate subset selection mode
|
||||||
Input(type="hidden", name="subset_mode", value="true"),
|
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
|
# Script for selection management
|
||||||
selection_script(len(animals)),
|
selection_script(len(animals)),
|
||||||
id="animal-selection-list",
|
id="animal-selection-list",
|
||||||
@@ -126,8 +124,6 @@ def selection_script(total_count: int) -> Div:
|
|||||||
if (countText) {{
|
if (countText) {{
|
||||||
countText.textContent = checked + ' of {total_count} selected';
|
countText.textContent = checked + ' of {total_count} selected';
|
||||||
}}
|
}}
|
||||||
// Trigger hash recomputation if needed
|
|
||||||
computeSelectionHash();
|
|
||||||
}}
|
}}
|
||||||
|
|
||||||
function selectAllAnimals(selectAll) {{
|
function selectAllAnimals(selectAll) {{
|
||||||
@@ -137,50 +133,6 @@ def selection_script(total_count: int) -> Div:
|
|||||||
}});
|
}});
|
||||||
updateSelectionCount();
|
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();
|
|
||||||
}});
|
|
||||||
""")
|
""")
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -456,3 +456,86 @@ class TestSelectionMismatchError:
|
|||||||
|
|
||||||
assert error.result is result
|
assert error.result is result
|
||||||
assert error.result.diff is diff
|
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
|
||||||
|
|||||||
Reference in New Issue
Block a user