fix(user-sync): prune a single-valued key stored with an empty value

- a stored [] on employment_contract_status is now removed by --prune

Author: LukasGold

src/osw/tools/user_sync/prune.py

@@ -17,15 +17,29 @@
 
 from osw.wtsite import WtPage
 
+_EMPTY = (None, [], "", {})
 
-def _is_multi_valued(key: str) -> bool:
+
+def is_multi_valued(key: str) -> bool:
     """True if the User field is a list or a set, False for any other key."""
     model_field = User.__fields__.get(key)
     return model_field is not None and model_field.shape != SHAPE_SINGLETON
 
 
+def needs_clearing(key: str, value: Any) -> bool:
+    """True if a present stored value of ``key`` must be cleared by pruning.
+
+    A list or set field needs it only when it is not empty. Any other key needs
+    it whenever it is present, also with an empty value such as ``[]``, which is
+    not a valid value for a single-valued field.
+    """
+    if is_multi_valued(key):
+        return value not in _EMPTY
+    return True
+
+
 def clear_jsondata_keys(osw: Any, title: str, keys: Iterable[str]) -> bool:
-    """Clear each present, non-empty key in the page's jsondata slot.
+    """Clear each key in the page's jsondata slot that needs_clearing selects.
 
     A list or set field becomes ``[]``. Any other key is deleted. The page is
     read with a new WtPage, which downloads the current revision and bypasses the
@@ -40,8 +54,8 @@ def clear_jsondata_keys(osw: Any, title: str, keys: Iterable[str]) -> bool:
         return False
     changed = False
     for key in keys:
-        if key in content and content[key] not in (None, [], "", {}):
-            if _is_multi_valued(key):
+        if key in content and needs_clearing(key, content[key]):
+            if is_multi_valued(key):
                 content[key] = []
             else:
                 del content[key]

src/osw/tools/user_sync/reconcile.py

@@ -17,6 +17,7 @@
 from .config import PROTECTED_FIELDS
 from .existing import ExistingUsers
 from .mapping import ProposedUser
+from .prune import needs_clearing
 
 NEW = "new"
 GAP_FILL = "gap_fill"
@@ -100,7 +101,9 @@ def _has_protected_value(
     """
     if stored_jsondata is None:
         return _existing_has_iri(entity, name)
-    return not _is_empty(stored_jsondata.get(name))
+    if name not in stored_jsondata:
+        return False
+    return needs_clearing(name, stored_jsondata[name])
 
 
 def existing_fields(entity: Any) -> Dict[str, Any]:

tests/tools/test_user_sync_reconcile.py

@@ -270,3 +270,18 @@ def test_placeholder_user_without_username_still_gets_it():
     )
     assert change.category == GAP_FILL
     assert [d.name for d in change.diffs] == ["username"]
+
+
+def test_protected_field_stored_as_empty_list_is_removed_when_pruning():
+    # [] is not a valid value for the single-valued field, so it is removed.
+    entity = _entity()
+    entity.__iris__ = {"employment_contract_status": "Item:OSWdefault"}
+    change = reconcile_user(
+        _proposed(),
+        entity,
+        prune=True,
+        stored_jsondata={"username": ORCID, "employment_contract_status": []},
+    )
+    assert [(d.name, d.status) for d in change.diffs] == [
+        ("employment_contract_status", REMOVE)
+    ]

tests/tools/test_user_sync_store.py

@@ -365,3 +365,19 @@ def test_clear_jsondata_keys_does_not_add_absent_keys_or_write(monkeypatch):
     assert written is False
     assert page.content == {"website": [], "username": "Alice"}
     assert page.comments == []
+
+
+def test_clear_jsondata_keys_deletes_an_empty_single_valued_key(monkeypatch):
+    page = FakePrunePage({
+        "website": [],
+        "employment_contract_status": [],
+        "username": "Alice",
+    })
+    _patch_page(monkeypatch, page)
+    written = clear_jsondata_keys(
+        SimpleNamespace(site=None),
+        ITEM_TITLE,
+        ["website", "employment_contract_status"],
+    )
+    assert written is True
+    assert page.content == {"website": [], "username": "Alice"}