Review fix: duplicate-name scan excludes only the edited child

Review round 1 (Codex gpt-5.6-terra) found: a case-only edit could
suppress the duplicate-name warning even when a DIFFERENT child
already carried the normalised name, because the edit path compared
only against the child's own old name. The scan now always runs on
the resulting state and excludes only the child being edited
(exclude_child_id), so case-only edits and stay-duplicated renames
warn like any other collision; renaming to your own name stays
warning-free when no other child holds the name. Tests: new
case-only-collision test, stay-duplicated rename assertion, and a
split self-rename test; suite at 848 passed.
This commit is contained in:
Joshua King
2026-09-15 14:08:24 -04:00
parent 61b77cc2db
commit 712527d5ba
2 changed files with 65 additions and 14 deletions
+15 -6
View File
@@ -74,16 +74,25 @@ def _validate_sort_order(value: object) -> int:
async def _warn_on_duplicate_name( async def _warn_on_duplicate_name(
database: NestQuestDatabase, display_name: str database: NestQuestDatabase,
display_name: str,
*,
exclude_child_id: int | None = None,
) -> None: ) -> None:
"""Log a warning when ``display_name`` duplicates an existing child. """Log a warning when ``display_name`` duplicates an existing child.
Case-insensitive and whitespace-normalised on both sides. The Case-insensitive and whitespace-normalised on both sides. The
duplicate is still allowed — this only makes it visible; the log duplicate is still allowed — this only makes it visible; the log
line names both rows so an accidental double-add can be found. line names both rows so an accidental double-add can be found.
``exclude_child_id`` removes the child being edited from the scan,
so an edit only warns when the result collides with a DIFFERENT
child's name — including case-only edits, where the self-comparison
would otherwise hide a real duplicate.
""" """
lowered = display_name.casefold() lowered = display_name.casefold()
for child in await ChildrenDao(database).list_all(): for child in await ChildrenDao(database).list_all():
if exclude_child_id is not None and child.id == exclude_child_id:
continue
if child.display_name.strip().casefold() == lowered: if child.display_name.strip().casefold() == lowered:
LOGGER.warning( LOGGER.warning(
"Child display name %r duplicates existing child %d " "Child display name %r duplicates existing child %d "
@@ -142,9 +151,10 @@ async def edit_child(
field raises: the DAO's update writes only provided fields and field raises: the DAO's update writes only provided fields and
cannot express NULL, so a silent no-op would mask the caller's cannot express NULL, so a silent no-op would mask the caller's
intent (see the module docstring). A provided display name is intent (see the module docstring). A provided display name is
validated like create's and, when it duplicates a DIFFERENT child, validated like create's; the duplicate scan excludes only the child
still allowed with a warning. Raises ValueError when the child being edited, so a case-only edit colliding with another child's
does not exist. normalised name still warns. Raises ValueError when the child does
not exist.
""" """
dao = ChildrenDao(database) dao = ChildrenDao(database)
existing = await dao.get(child_id) existing = await dao.get(child_id)
@@ -154,8 +164,7 @@ async def edit_child(
if display_name is not _UNSET: if display_name is not _UNSET:
name = _validate_text(display_name, "display_name", required=True) name = _validate_text(display_name, "display_name", required=True)
assert name is not None assert name is not None
if name.casefold() != existing.display_name.strip().casefold(): await _warn_on_duplicate_name(database, name, exclude_child_id=child_id)
await _warn_on_duplicate_name(database, name)
updates["display_name"] = name updates["display_name"] = name
if colour is not _UNSET: if colour is not _UNSET:
value = _validate_text(colour, "colour") value = _validate_text(colour, "colour")
+50 -8
View File
@@ -291,21 +291,63 @@ def test_edit_child_rename_to_duplicate_warns_but_different_name_not(
# Renaming Bo onto Ada's name warns (different child's name). # Renaming Bo onto Ada's name warns (different child's name).
edited = await edit_child(database, bo.id, display_name="ada") edited = await edit_child(database, bo.id, display_name="ada")
assert edited.display_name == "ada" assert edited.display_name == "ada"
assert any( warnings = [
"duplicates existing child" in r.getMessage() r for r in caplog.records if "duplicates existing child" in r.getMessage()
for r in caplog.records ]
) assert len(warnings) == 1
assert "Ada" in warnings[0].getMessage()
return edited
_with_db(tmp_path, "edit-rename-duplicate.db")(_body)
def test_edit_child_rename_to_own_name_casing_alone_warns_not(
tmp_path, caplog
) -> None:
"""Renaming a child to their own name (casing aside) is not a
duplicate when NO OTHER child carries the normalised name."""
async def _body(database):
ada = await create_child(database, "Ada")
caplog.clear() caplog.clear()
# Renaming a child to their own name (casing aside) is not a edited = await edit_child(database, ada.id, display_name="ADA")
# duplicate: no warning. assert edited.display_name == "ADA"
await edit_child(database, ada.id, display_name="ADA")
assert not any( assert not any(
"duplicates existing child" in r.getMessage() "duplicates existing child" in r.getMessage()
for r in caplog.records for r in caplog.records
) )
return edited
_with_db(tmp_path, "edit-rename-self.db")(_body)
def test_edit_child_case_only_edit_colliding_with_other_child_warns(
tmp_path, caplog
) -> None:
"""A case-only edit must not hide a real duplicate: with two Ada
profiles already present, editing either one's name to another
casing still warns (the scan excludes only the edited child)."""
async def _body(database):
first = await create_child(database, "Ada")
await create_child(database, "ada")
caplog.clear()
edited = await edit_child(database, first.id, display_name="ADA")
assert edited.display_name == "ADA"
warnings = [
r for r in caplog.records if "duplicates existing child" in r.getMessage()
]
assert len(warnings) == 1
# Editing the name to a value that STAYS duplicated (no change,
# other child still carries it) warns too: the scan is on the
# resulting state, not on whether the word moved.
caplog.clear()
await edit_child(database, first.id, display_name=" ada ")
warnings = [
r for r in caplog.records if "duplicates existing child" in r.getMessage()
]
assert len(warnings) == 1
return None return None
_with_db(tmp_path, "edit-rename-duplicate.db")(_body) _with_db(tmp_path, "edit-case-only-duplicate.db")(_body)
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------