feat(user-sync): gate optional fields and opt-in prune removals

Author: raederan

src/osw/tools/user_sync/config.py

@@ -41,6 +41,8 @@ class SyncConfig:
     # reported as warnings rather than failing the run.
     include_websites: bool = False
     link_organizations: bool = False
+    # Remove disabled optional fields and protected fields from existing items.
+    prune: bool = False
     orcid_api_base: str = ORCID_API_BASE_DEFAULT
     excluded_groups: List[str] = field(default_factory=lambda: list(SYSTEM_GROUPS))
 
@@ -105,6 +107,13 @@ def build_arg_parser() -> argparse.ArgumentParser:
         action="store_true",
         help="Enable all optional data: websites and organizations.",
     )
+    parser.add_argument(
+        "--prune",
+        dest="prune",
+        action="store_true",
+        help="Remove disabled optional fields and employment_contract_status from "
+        "existing items (data cleanup). Off by default; a normal run only adds.",
+    )
     account_scope = parser.add_mutually_exclusive_group()
     account_scope.add_argument(
         "--orcid-only",
@@ -142,4 +151,5 @@ def config_from_args(argv: Optional[List[str]] = None) -> SyncConfig:
         create_redirects=args.create_redirects,
         include_websites=args.include_websites or args.with_extras,
         link_organizations=args.link_organizations or args.with_extras,
+        prune=args.prune,
     )

src/osw/tools/user_sync/interactive.py

@@ -15,6 +15,7 @@
     GAP_FILL,
     NEW,
     RECONCILED_FIELDS,
+    REMOVE,
     UNCHANGED,
     ReconcilePlan,
     UserChange,
@@ -48,7 +49,10 @@ def render_preview(plan: ReconcilePlan) -> List[str]:
         marker = _MARKERS[change.category]
         detail = ""
         if change.diffs:
-            detail = " (" + ", ".join(d.name for d in change.diffs) + ")"
+            names = [
+                ("-" + d.name if d.status == REMOVE else d.name) for d in change.diffs
+            ]
+            detail = " (" + ", ".join(names) + ")"
         placeholder = " [placeholder name]" if change.proposed.placeholder_name else ""
         lines.append(
             f"  {marker} {change.proposed.username} {change.category}{detail}{placeholder}"
@@ -178,7 +182,7 @@ def resolve_plan(
                 ResolvedChange(change, "update", {d.name for d in change.diffs})
             )
         else:  # CONFLICT
-            accepted = {d.name for d in change.diffs if d.status == GAP_FILL}
+            accepted = {d.name for d in change.diffs if d.status in (GAP_FILL, REMOVE)}
             if conflict_mode == "all":
                 accepted |= {d.name for d in change.diffs if d.status == CONFLICT}
             elif conflict_mode == "review":

src/osw/tools/user_sync/mapping.py

@@ -113,9 +113,14 @@ def _derive_names(
 def map_user(
     mw_user: MwUser,
     profile: Optional[OrcidProfile] = None,
-    link_organizations: bool = True,
+    include_websites: bool = False,
+    link_organizations: bool = False,
 ) -> Tuple[ProposedUser, List[ProposedOrganization]]:
-    """Build a ProposedUser (and any linked organizations) from the sources."""
+    """Build a ProposedUser (and any linked organizations) from the sources.
+
+    Email and names are always taken from the profile (core). Websites and
+    organizations are populated only when their flag is enabled.
+    """
     orcid_uri = f"https://orcid.org/{mw_user.orcid_id}" if mw_user.orcid_id else None
     first, surname, label, placeholder = _derive_names(profile, mw_user.name)
 
@@ -139,7 +144,7 @@ def map_user(
         label=label,
         orcid=orcid_uri,
         emails=list(profile.emails) if profile else [],
-        websites=list(profile.urls) if profile else [],
+        websites=list(profile.urls) if (profile and include_websites) else [],
         organizations=org_titles,
         placeholder_name=placeholder,
     )

src/osw/tools/user_sync/reconcile.py

@@ -1,29 +1,34 @@
 """Reconcile proposed User items against the ones already stored.
 
 Pure logic: given proposed users and an index of existing items, classify each
-into NEW, GAP_FILL, CONFLICT or UNCHANGED and record the per-field differences.
+into NEW, GAP_FILL, CONFLICT or UNCHANGED and record the per-field differences
+(including REMOVE diffs for disabled optional fields and protected fields).
 The interactive layer consumes this plan; nothing here does IO.
 """
 
 from __future__ import annotations
 
 from collections import Counter
 from dataclasses import dataclass, field
-from typing import Any, Dict, List, Optional
+from typing import Any, Dict, FrozenSet, List, Optional
 
+from .config import PROTECTED_FIELDS
 from .existing import ExistingUsers
 from .mapping import ProposedUser
 
 NEW = "new"
 GAP_FILL = "gap_fill"
 CONFLICT = "conflict"
+REMOVE = "remove"
 UNCHANGED = "unchanged"
 
 # username is the match key and is never reconciled as a value.
 SCALAR_FIELDS = ("label", "first_name", "surname", "orcid")
 SET_FIELDS = ("emails", "websites", "organizations")
 URL_FIELDS = ("orcid", "websites")
 RECONCILED_FIELDS = SCALAR_FIELDS + SET_FIELDS
+# Fields that are only written when opted in; removed from existing when disabled.
+OPTIONAL_FIELDS = ("websites", "organizations")
 
 
 def _is_empty(value: Any) -> bool:
@@ -76,6 +81,12 @@ def _existing_orgs(entity: Any) -> Any:
         return None
 
 
+def _existing_has_iri(entity: Any, name: str) -> bool:
+    """True if the loaded entity carries a relation IRI for ``name``."""
+    iris = getattr(entity, "__iris__", None)
+    return isinstance(iris, dict) and bool(iris.get(name))
+
+
 def existing_fields(entity: Any) -> Dict[str, Any]:
     """Normalized comparable field view of a loaded User entity."""
     return {
@@ -109,7 +120,7 @@ class FieldDiff:
     name: str
     existing: Any
     proposed: Any
-    status: str  # GAP_FILL or CONFLICT
+    status: str  # GAP_FILL, CONFLICT or REMOVE
 
 
 @dataclass
@@ -131,20 +142,37 @@ def _compare(existing_val: Any, proposed_val: Any) -> Optional[str]:
     return UNCHANGED if existing_val == proposed_val else CONFLICT
 
 
-def reconcile_user(proposed: ProposedUser, existing: Optional[Any]) -> UserChange:
+def reconcile_user(
+    proposed: ProposedUser,
+    existing: Optional[Any],
+    enabled_fields: FrozenSet[str] = frozenset(),
+    prune: bool = False,
+) -> UserChange:
     if existing is None:
         return UserChange(proposed=proposed, existing=None, category=NEW)
     ef = existing_fields(existing)
     pf = proposed_fields(proposed)
     diffs: List[FieldDiff] = []
-    has_conflict = has_gap = False
+    has_conflict = has_change = False
     for name in RECONCILED_FIELDS:
+        if name in OPTIONAL_FIELDS and name not in enabled_fields:
+            # Disabled optional field: remove any existing value only when pruning.
+            if prune and not _is_empty(ef[name]):
+                diffs.append(FieldDiff(name, ef[name], None, REMOVE))
+                has_change = True
+            continue
         status = _compare(ef[name], pf[name])
         if status in (GAP_FILL, CONFLICT):
             diffs.append(FieldDiff(name, ef[name], pf[name], status))
             has_conflict = has_conflict or status == CONFLICT
-            has_gap = has_gap or status == GAP_FILL
-    category = CONFLICT if has_conflict else GAP_FILL if has_gap else UNCHANGED
+            has_change = has_change or status == GAP_FILL
+    # Protected fields are stripped from existing items only when pruning.
+    if prune:
+        for name in PROTECTED_FIELDS:
+            if _existing_has_iri(existing, name):
+                diffs.append(FieldDiff(name, "set", None, REMOVE))
+                has_change = True
+    category = CONFLICT if has_conflict else GAP_FILL if has_change else UNCHANGED
     return UserChange(
         proposed=proposed, existing=existing, category=category, diffs=diffs
     )
@@ -171,7 +199,13 @@ def counts(self) -> Dict[str, int]:
 
 
 def reconcile(
-    proposed_users: List[ProposedUser], existing: ExistingUsers
+    proposed_users: List[ProposedUser],
+    existing: ExistingUsers,
+    enabled_fields: FrozenSet[str] = frozenset(),
+    prune: bool = False,
 ) -> ReconcilePlan:
-    changes = [reconcile_user(p, existing.get(p.username)) for p in proposed_users]
+    changes = [
+        reconcile_user(p, existing.get(p.username), enabled_fields, prune)
+        for p in proposed_users
+    ]
     return ReconcilePlan(changes=changes)

src/osw/tools/user_sync/sync.py

@@ -77,7 +77,10 @@ def _build_proposals(config: SyncConfig, osw: Any, session: Any):
             except OrcidRateLimitError:
                 profile = None
         proposed, orgs = map_user(
-            mw_user, profile, link_organizations=config.link_organizations
+            mw_user,
+            profile,
+            include_websites=config.include_websites,
+            link_organizations=config.link_organizations,
         )
         proposed_users.append(proposed)
         for org in orgs:
@@ -173,7 +176,12 @@ def run_user_sync(
 
     proposed_users, org_map = _build_proposals(config, osw, session)
     existing = load_existing_users(osw)
-    plan = reconcile(proposed_users, existing)
+    plan = reconcile(
+        proposed_users,
+        existing,
+        enabled_fields=frozenset(config.enabled_optional_fields()),
+        prune=config.prune,
+    )
 
     summary_lines = []
     if config.link_organizations and org_map:

tests/tools/test_user_sync_config.py

@@ -17,6 +17,7 @@ def test_defaults():
     # Websites and organizations are opt-in (off by default); email is standard.
     assert cfg.include_websites is False
     assert cfg.link_organizations is False
+    assert cfg.prune is False  # removals are opt-in
     assert cfg.enabled_optional_fields() == set()
     assert cfg.exclude_system_usernames is True
     assert "bot" in cfg.excluded_groups
@@ -42,6 +43,11 @@ def test_with_extras_enables_all():
     assert cfg.enabled_optional_fields() == {"websites", "organizations"}
 
 
+def test_prune_flag():
+    assert config_from_args([]).prune is False
+    assert config_from_args(["--prune"]).prune is True
+
+
 def test_config_from_args_parses_flags():
     cfg = config_from_args([
         "--domain",

tests/tools/test_user_sync_interactive.py

@@ -13,6 +13,7 @@
     CONFLICT,
     GAP_FILL,
     NEW,
+    REMOVE,
     UNCHANGED,
     FieldDiff,
     ReconcilePlan,
@@ -190,6 +191,41 @@ def test_final_confirm_no():
     assert res.creates()[0].change.proposed.username == "new1"
 
 
+def test_removals_auto_apply_and_marked_in_preview():
+    plan = ReconcilePlan(
+        changes=[
+            _change(
+                GAP_FILL,
+                "u1",
+                [FieldDiff("organizations", {"x"}, None, REMOVE)],
+            )
+        ]
+    )
+    assert any("-organizations" in ln for ln in render_preview(plan))
+    prompter, _ = _prompter(["y"])  # only the final confirm
+    res = resolve_plan(plan, prompter)
+    assert res.updates()[0].apply_fields == {"organizations"}
+    assert res.accepted is True
+
+
+def test_conflict_user_still_auto_applies_removal():
+    plan = ReconcilePlan(
+        changes=[
+            _change(
+                CONFLICT,
+                "u1",
+                [
+                    FieldDiff("surname", "Old", "New", CONFLICT),
+                    FieldDiff("organizations", {"x"}, None, REMOVE),
+                ],
+            )
+        ]
+    )
+    prompter, _ = _prompter(["k", "y"])  # keep existing on conflict, final yes
+    res = resolve_plan(plan, prompter)
+    assert res.updates()[0].apply_fields == {"organizations"}
+
+
 def test_prompter_reprompts_on_invalid():
     prompter, io = _prompter(["z", "k"])
     assert prompter.ask("q", ("k", "n")) == "k"

tests/tools/test_user_sync_mapping.py

@@ -29,7 +29,9 @@ def test_map_orcid_user_full_profile():
         urls=["https://example.org"],
         affiliations=[OrcidAffiliation(organization="Example University")],
     )
-    proposed, orgs = map_user(mw, profile)
+    proposed, orgs = map_user(
+        mw, profile, include_websites=True, link_organizations=True
+    )
     assert isinstance(proposed, ProposedUser)
     assert proposed.username == ORCID
     assert proposed.orcid == f"https://orcid.org/{ORCID}"
@@ -99,7 +101,7 @@ def test_duplicate_affiliations_deduped():
             OrcidAffiliation(organization="example   university"),
         ]
     )
-    proposed, orgs = map_user(mw, profile)
+    proposed, orgs = map_user(mw, profile, link_organizations=True)
     assert len(orgs) == 1
     assert len(proposed.organizations) == 1
 
@@ -110,3 +112,17 @@ def test_link_organizations_disabled():
     proposed, orgs = map_user(mw, profile, link_organizations=False)
     assert orgs == []
     assert proposed.organizations == []
+
+
+def test_email_always_included_but_extras_gated_by_default():
+    mw = MwUser(name=ORCID)
+    profile = _profile(
+        emails=["l@example.org"],
+        urls=["https://example.org"],
+        affiliations=[OrcidAffiliation(organization="Example U")],
+    )
+    proposed, orgs = map_user(mw, profile)  # defaults: websites/orgs off
+    assert proposed.emails == ["l@example.org"]
+    assert proposed.websites == []
+    assert proposed.organizations == []
+    assert orgs == []

tests/tools/test_user_sync_reconcile.py

@@ -9,6 +9,7 @@
     CONFLICT,
     GAP_FILL,
     NEW,
+    REMOVE,
     UNCHANGED,
     reconcile,
     reconcile_user,
@@ -109,10 +110,51 @@ def test_set_order_independent():
     change = reconcile_user(
         _proposed(organizations=["Item:OSWb", "Item:OSWa"]),
         _entity(organization=["Item:OSWa", "Item:OSWb"]),
+        enabled_fields=frozenset({"organizations"}),
     )
     assert change.category == UNCHANGED
 
 
+def test_disabled_optional_field_removed_when_pruning():
+    # organizations disabled + existing value + prune -> REMOVE
+    change = reconcile_user(
+        _proposed(), _entity(organization=["Item:OSWa"]), prune=True
+    )
+    assert change.category == GAP_FILL
+    removes = [d for d in change.diffs if d.status == REMOVE]
+    assert [d.name for d in removes] == ["organizations"]
+    assert removes[0].proposed is None
+
+
+def test_prune_off_keeps_disabled_and_protected():
+    entity = _entity()
+    entity.__iris__ = {
+        "organization": ["Item:OSWa"],
+        "employment_contract_status": "Item:OSWx",
+    }
+    change = reconcile_user(_proposed(), entity)  # prune off (default)
+    assert change.category == UNCHANGED
+    assert change.diffs == []
+
+
+def test_enabled_optional_field_not_removed():
+    change = reconcile_user(
+        _proposed(organizations=["Item:OSWa"]),
+        _entity(organization=["Item:OSWa"]),
+        enabled_fields=frozenset({"organizations"}),
+    )
+    assert change.category == UNCHANGED
+
+
+def test_protected_field_removed_when_pruning():
+    entity = _entity()
+    entity.__iris__ = {"employment_contract_status": "Item:OSWx"}
+    change = reconcile_user(_proposed(), entity, prune=True)
+    removes = [d.name for d in change.diffs if d.status == REMOVE]
+    assert "employment_contract_status" in removes
+    assert change.category == GAP_FILL
+
+
 def test_missing_label_is_gap_fill():
     change = reconcile_user(_proposed(label="Lukas Koschmieder"), _entity(label=None))
     assert change.category == GAP_FILL