From 8479da5014718967ae070dc8cb0fc8be976db3ae Mon Sep 17 00:00:00 2001 From: sar Date: Mon, 10 Aug 2026 14:37:08 -0500 Subject: [PATCH 1/7] feat(scim): add migrate_and_sync_users orchestrator command Replaces the old two-step manual migration process (running migrate_edx_data, then an ad hoc SCIM sync script with no field-level visibility or validation) with one auditable command: - Stage 1: backfill edX user data (call_command migrate_edx_data). - Stage 2: classify every sync candidate via LearnUserAdapter's _resolve_name() tiers - report only, never mutates legal_address. Only candidates with no name data anywhere are blocked by default (override with --force); a split-from-User.name candidate syncs normally, flagged as lower-confidence in the report. - Stage 3: sync via mitol.scim.api.sync_users_to_scim_remote, with per-user structured logging instead of only aggregate error counts. - Stage 4: verify what Keycloak actually stored by diffing the response body sync_users_to_scim_remote now returns, against what was sent - no extra API calls needed. - Stage 5: write a JSON report (synced-and-verified / blocked-or-failed / verified-but-mismatched). Depends on the response-capture change in https://github.com/mitodl/ol-django/pull/544 - sync_users_to_scim_remote needs to return UserState.response_body for Stage 4 to work. Opening as a draft until that PR releases and the mitol-django-scim pin here is bumped. Co-Authored-By: Claude Sonnet 5 --- .../commands/migrate_and_sync_users.py | 229 ++++++++++++++++++ .../tests/migrate_and_sync_users_test.py | 203 ++++++++++++++++ 2 files changed, 432 insertions(+) create mode 100644 users/management/commands/migrate_and_sync_users.py create mode 100644 users/management/tests/migrate_and_sync_users_test.py diff --git a/users/management/commands/migrate_and_sync_users.py b/users/management/commands/migrate_and_sync_users.py new file mode 100644 index 0000000000..b0afa9c443 --- /dev/null +++ b/users/management/commands/migrate_and_sync_users.py @@ -0,0 +1,229 @@ +""" +Orchestrate the full edX-to-Keycloak user migration pipeline: backfill edX +data, classify each candidate user by how confident we are in their name +data, sync to Keycloak via SCIM, verify what was actually stored, and report. + +This replaces the old two-step manual process (running `migrate_edx_data`, +then an ad hoc SCIM sync script with no field-level visibility) with one +auditable command. +""" + +import json + +from django.contrib.auth import get_user_model +from django.core.management import BaseCommand, call_command +from django.db.models import Q +from mitol.scim import api as scim_api + +from users.adapters import LearnUserAdapter + +User = get_user_model() + + +class Command(BaseCommand): + """Orchestrate the edX-backfill-then-SCIM-sync user migration pipeline.""" + + help = __doc__ + + def add_arguments(self, parser): + """Define the command's CLI flags.""" + parser.add_argument( + "--skip-edx-migration", + action="store_true", + help="Skip Stage 1 (call_command('migrate_edx_data', type='users')) " + "if it's already been run separately.", + ) + parser.add_argument( + "--batch-size", + type=int, + default=250, + help="Number of users per SCIM sync batch (default: 250).", + ) + parser.add_argument( + "--limit", + type=int, + help="Limit the number of users processed (for testing purposes).", + ) + parser.add_argument( + "--force", + action="store_true", + help="Sync users with no name data anywhere in mitxonline anyway, " + "with a blank name.givenName/familyName, instead of blocking them.", + ) + parser.add_argument( + "--dry-run", + action="store_true", + help="Run backfill and classification only; do not sync or write " + "anything.", + ) + parser.add_argument( + "--report-path", + type=str, + help="Write the JSON report to this path instead of stdout.", + ) + + def handle(self, *args, **options): # noqa: ARG002 + """Run the backfill/classify/sync/verify/report pipeline.""" + batch_size = options.get("batch_size") or 250 + limit = options.get("limit") + force = options.get("force", False) + dry_run = options.get("dry_run", False) + report_path = options.get("report_path") + + if not options.get("skip_edx_migration", False): + self.stdout.write("Stage 1: backfilling edX user data...") + call_command("migrate_edx_data", type="users") + else: + self.stdout.write("Stage 1: skipped (--skip-edx-migration).") + + self.stdout.write("Stage 2: classifying sync candidates...") + candidates = list( + User.objects.filter(is_active=True) + .filter(Q(global_id="") | Q(scim_external_id=None)) + .select_related("legal_address") + .order_by("id") + ) + if limit is not None: + candidates = candidates[:limit] + + to_sync, blocked = self._classify(candidates, force=force) + self.stdout.write( + f" {len(to_sync)} ready to sync, {len(blocked)} blocked " + f"(pass --force to sync blocked users anyway with a blank name)." + ) + + if dry_run: + self.stdout.write("Dry run: stopping before Stage 3 (sync).") + self._write_report( + { + "to_sync": [self._report_row(row) for row in to_sync], + "blocked": [self._report_row(row) for row in blocked], + "verified": [], + "mismatched": [], + }, + report_path, + ) + return + + self.stdout.write(f"Stage 3: syncing {len(to_sync)} users to Keycloak...") + verified, mismatched = [], [] + for start in range(0, len(to_sync), batch_size): + batch = to_sync[start : start + batch_size] + states = scim_api.sync_users_to_scim_remote( + [row["user"] for row in batch] + ) + self.stdout.write( + f" batch {start // batch_size + 1}: " + f"{sum(1 for s in states if s.success)}/{len(states)} succeeded" + ) + + self.stdout.write("Stage 4: verifying batch...") + rows_by_user_id = {row["user"].id: row for row in batch} + for state in states: + row = rows_by_user_id[state.user.id] + if not state.success: + self.stdout.write( + self.style.ERROR( + f" FAILED user={state.user.email} error={state.error}" + ) + ) + row["outcome"] = "failed" + row["error"] = state.error + blocked.append(row) + continue + + row["outcome"] = "synced" + if state.response_body is None: + # matched an existing remote user via search, not a fresh + # create - nothing to verify against, since nothing new + # was sent + verified.append(row) + continue + + sent_name = row["given_name"], row["family_name"] + got_name = ( + state.response_body.get("name", {}).get("givenName"), + state.response_body.get("name", {}).get("familyName"), + ) + if got_name == sent_name: + verified.append(row) + else: + row["got_given_name"], row["got_family_name"] = got_name + mismatched.append(row) + self.stdout.write( + self.style.ERROR( + f" MISMATCH user={state.user.email} " + f"sent={sent_name} got={got_name}" + ) + ) + + self.stdout.write("Stage 5: report") + self.stdout.write( + self.style.SUCCESS( + f" {len(verified)} synced and verified, " + f"{len(blocked)} blocked/failed, " + f"{len(mismatched)} verified-but-mismatched" + ) + ) + self._write_report( + { + "verified": [self._report_row(row) for row in verified], + "blocked": [self._report_row(row) for row in blocked], + "mismatched": [self._report_row(row) for row in mismatched], + }, + report_path, + ) + + def _classify(self, candidates, *, force): + """Split candidates into (to_sync, blocked) using LearnUserAdapter's + _resolve_name() tiers, purely for classification/reporting - this + never writes to legal_address. + """ + to_sync, blocked = [], [] + for user in candidates: + adapter = LearnUserAdapter(user) + given_name, family_name = adapter._resolve_name() # noqa: SLF001 + legal_address_complete = bool( + user.legal_address.first_name and user.legal_address.last_name + ) + row = { + "user": user, + "given_name": given_name, + "family_name": family_name, + "tier": ( + "legal_address" + if legal_address_complete + else ("split_name" if given_name or family_name else "none") + ), + } + if row["tier"] == "none" and not force: + row["outcome"] = "blocked" + blocked.append(row) + else: + if row["tier"] == "none": + row["outcome"] = "forced-blank-name" + to_sync.append(row) + return to_sync, blocked + + @staticmethod + def _report_row(row): + return { + "user_id": row["user"].id, + "email": row["user"].email, + "tier": row["tier"], + "given_name": row["given_name"], + "family_name": row["family_name"], + "outcome": row.get("outcome"), + "error": row.get("error"), + "got_given_name": row.get("got_given_name"), + "got_family_name": row.get("got_family_name"), + } + + def _write_report(self, report, report_path): + output = json.dumps(report, indent=2, default=str) + if report_path: + with open(report_path, "w") as f: # noqa: PTH123 + f.write(output) + self.stdout.write(f"Report written to {report_path}") + else: + self.stdout.write(output) diff --git a/users/management/tests/migrate_and_sync_users_test.py b/users/management/tests/migrate_and_sync_users_test.py new file mode 100644 index 0000000000..5af05197c8 --- /dev/null +++ b/users/management/tests/migrate_and_sync_users_test.py @@ -0,0 +1,203 @@ +"""Tests for migrate_and_sync_users management command""" + +import json +from types import SimpleNamespace + +import pytest + +from users.factories import UserFactory +from users.management.commands import migrate_and_sync_users + +COMMAND = migrate_and_sync_users.Command() + + +def _state(user, *, success=True, response_body=None, error=None, external_id="ext"): + """Build a fake UserState-like object, decoupled from whatever version of + mitol-django-scim happens to be installed - the command only duck-types + on .user/.success/.response_body/.error. + """ + return SimpleNamespace( + user=user, + success=success, + external_id=external_id if success else None, + response_body=response_body, + error=error, + ) + + +def _run(tmp_path, **options): + """Run the command and return its parsed JSON report - reading the report + file, rather than scraping stdout, avoids Command()'s stdout reference + being grabbed at module-import time (before pytest's capture fixtures are + active for the current test). + """ + report_path = tmp_path / "report.json" + COMMAND.handle(report_path=str(report_path), **options) + return json.loads(report_path.read_text()) + + +@pytest.fixture(autouse=True) +def mock_edx_migration(mocker): + """Stage 1 always gets skipped/mocked in these tests - never hit Trino.""" + return mocker.patch("users.management.commands.migrate_and_sync_users.call_command") + + +@pytest.mark.django_db +def test_dry_run_classifies_without_syncing(mocker, tmp_path): + """Dry run reports classification tiers and never calls the sync API""" + mock_sync = mocker.patch( + "users.management.commands.migrate_and_sync_users.scim_api.sync_users_to_scim_remote" + ) + + tier1_user = UserFactory.create(name="Joe Smith", global_id=None) + tier1_user.legal_address.first_name = "Joe" + tier1_user.legal_address.last_name = "Smith" + tier1_user.legal_address.save() + + tier2_user = UserFactory.create(name="Jane Doe", global_id=None) + tier2_user.legal_address.first_name = "" + tier2_user.legal_address.last_name = "" + tier2_user.legal_address.save() + + tier3_user = UserFactory.create(name="", global_id=None) + tier3_user.legal_address.first_name = "" + tier3_user.legal_address.last_name = "" + tier3_user.legal_address.save() + + report = _run( + tmp_path, dry_run=True, skip_edx_migration=False, force=False, limit=None + ) + + mock_sync.assert_not_called() + + to_sync_ids = {row["user_id"] for row in report["to_sync"]} + blocked_ids = {row["user_id"] for row in report["blocked"]} + assert tier1_user.id in to_sync_ids + assert tier2_user.id in to_sync_ids + assert tier3_user.id in blocked_ids + + +@pytest.mark.django_db +def test_tier3_blocked_without_force(mocker): + """A user with no name data anywhere is excluded from the sync call by default""" + mock_sync = mocker.patch( + "users.management.commands.migrate_and_sync_users.scim_api.sync_users_to_scim_remote", + return_value=[], + ) + + tier1_user = UserFactory.create(name="Joe Smith", global_id=None) + tier1_user.legal_address.first_name = "Joe" + tier1_user.legal_address.last_name = "Smith" + tier1_user.legal_address.save() + + tier3_user = UserFactory.create(name="", global_id=None) + tier3_user.legal_address.first_name = "" + tier3_user.legal_address.last_name = "" + tier3_user.legal_address.save() + + mock_sync.side_effect = lambda users: [_state(u) for u in users] + + COMMAND.handle(dry_run=False, skip_edx_migration=False, force=False, limit=None) + + synced_users = mock_sync.call_args[0][0] + assert tier1_user in synced_users + assert tier3_user not in synced_users + + +@pytest.mark.django_db +def test_tier3_synced_with_force(mocker): + """--force syncs a no-name-data user anyway, with a blank name""" + mock_sync = mocker.patch( + "users.management.commands.migrate_and_sync_users.scim_api.sync_users_to_scim_remote" + ) + + tier3_user = UserFactory.create(name="", global_id=None) + tier3_user.legal_address.first_name = "" + tier3_user.legal_address.last_name = "" + tier3_user.legal_address.save() + + mock_sync.side_effect = lambda users: [_state(u) for u in users] + + COMMAND.handle(dry_run=False, skip_edx_migration=False, force=True, limit=None) + + synced_users = mock_sync.call_args[0][0] + assert tier3_user in synced_users + + +@pytest.mark.django_db +def test_verifies_matching_response_body(mocker, tmp_path): + """A synced user whose echoed response matches what was sent is verified""" + user = UserFactory.create(name="Joe Smith", global_id=None) + user.legal_address.first_name = "Joe" + user.legal_address.last_name = "Smith" + user.legal_address.save() + + mocker.patch( + "users.management.commands.migrate_and_sync_users.scim_api.sync_users_to_scim_remote", + return_value=[ + _state( + user, + response_body={"name": {"givenName": "Joe", "familyName": "Smith"}}, + ) + ], + ) + + report = _run( + tmp_path, dry_run=False, skip_edx_migration=False, force=False, limit=None + ) + + assert [row["user_id"] for row in report["verified"]] == [user.id] + assert report["mismatched"] == [] + + +@pytest.mark.django_db +def test_flags_mismatched_response_body(mocker, tmp_path): + """A synced user whose echoed response doesn't match what was sent is flagged, + not silently counted as a success + """ + user = UserFactory.create(name="Joe Smith", global_id=None) + user.legal_address.first_name = "Joe" + user.legal_address.last_name = "Smith" + user.legal_address.save() + + mocker.patch( + "users.management.commands.migrate_and_sync_users.scim_api.sync_users_to_scim_remote", + return_value=[ + _state( + user, + response_body={"name": {"givenName": "", "familyName": ""}}, + ) + ], + ) + + report = _run( + tmp_path, dry_run=False, skip_edx_migration=False, force=False, limit=None + ) + + assert report["verified"] == [] + assert [row["user_id"] for row in report["mismatched"]] == [user.id] + + +@pytest.mark.django_db +def test_failed_sync_is_reported_not_swallowed(mocker, tmp_path): + """A failed SCIM operation is reported distinctly, not counted as a success""" + user = UserFactory.create(name="Joe Smith", global_id=None) + user.legal_address.first_name = "Joe" + user.legal_address.last_name = "Smith" + user.legal_address.save() + + mocker.patch( + "users.management.commands.migrate_and_sync_users.scim_api.sync_users_to_scim_remote", + return_value=[ + _state(user, success=False, error={"status": "409"}), + ], + ) + + report = _run( + tmp_path, dry_run=False, skip_edx_migration=False, force=False, limit=None + ) + + assert report["verified"] == [] + blocked_row = next(row for row in report["blocked"] if row["user_id"] == user.id) + assert blocked_row["outcome"] == "failed" + assert blocked_row["error"] == {"status": "409"} From 4fba762012967527178dbb44a4902fd296028a7c Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 11 Aug 2026 14:16:13 +0000 Subject: [PATCH 2/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- users/management/commands/migrate_and_sync_users.py | 7 ++----- .../management/tests/remediate_keycloak_user_names_test.py | 3 ++- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/users/management/commands/migrate_and_sync_users.py b/users/management/commands/migrate_and_sync_users.py index b0afa9c443..a5a24eb3d3 100644 --- a/users/management/commands/migrate_and_sync_users.py +++ b/users/management/commands/migrate_and_sync_users.py @@ -53,8 +53,7 @@ def add_arguments(self, parser): parser.add_argument( "--dry-run", action="store_true", - help="Run backfill and classification only; do not sync or write " - "anything.", + help="Run backfill and classification only; do not sync or write anything.", ) parser.add_argument( "--report-path", @@ -109,9 +108,7 @@ def handle(self, *args, **options): # noqa: ARG002 verified, mismatched = [], [] for start in range(0, len(to_sync), batch_size): batch = to_sync[start : start + batch_size] - states = scim_api.sync_users_to_scim_remote( - [row["user"] for row in batch] - ) + states = scim_api.sync_users_to_scim_remote([row["user"] for row in batch]) self.stdout.write( f" batch {start // batch_size + 1}: " f"{sum(1 for s in states if s.success)}/{len(states)} succeeded" diff --git a/users/management/tests/remediate_keycloak_user_names_test.py b/users/management/tests/remediate_keycloak_user_names_test.py index 34948da4a9..55ac8aaead 100644 --- a/users/management/tests/remediate_keycloak_user_names_test.py +++ b/users/management/tests/remediate_keycloak_user_names_test.py @@ -43,7 +43,8 @@ def test_mitxonline_users_lookup_query_count_is_flat(django_assert_max_num_queri """_mitxonline_users_by_scim_id() plus constructing a LearnUserAdapter per user (as handle() does) must not issue extra queries per user - the fixed query count covers select_related(legal_address, user_profile) plus one - bulk prefetch for openedx_users, regardless of how many users there are""" + bulk prefetch for openedx_users, regardless of how many users there are + """ for i in range(5): user = UserFactory.create(scim_external_id=f"kc-{i}") user.legal_address.first_name = "Joe" From 2d56e8c46523785070bc37c0ca2ce02a96d8a33f Mon Sep 17 00:00:00 2001 From: sar Date: Tue, 11 Aug 2026 09:21:48 -0500 Subject: [PATCH 3/7] fix(scim): guard against null/omitted name in Stage 4 response body state.response_body.get("name", {}).get("givenName") only fell back to {} when "name" was absent - not when Keycloak echoed back an explicit "name": null for a blank-name user (exactly the population this command targets), which raised AttributeError and crashed the whole run mid-batch. Even when "name" was merely omitted (no crash), the resulting None vs _resolve_name()'s "" caused a false mismatch report - the same None-vs-empty-string gap found twice already in adjacent files. Normalizes both cases to "" before comparing, and confirmed via regression tests reproducing both the crash and the false mismatch. Co-Authored-By: Claude Sonnet 5 --- .../commands/migrate_and_sync_users.py | 11 ++++- .../tests/migrate_and_sync_users_test.py | 48 +++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/users/management/commands/migrate_and_sync_users.py b/users/management/commands/migrate_and_sync_users.py index a5a24eb3d3..b1bd716386 100644 --- a/users/management/commands/migrate_and_sync_users.py +++ b/users/management/commands/migrate_and_sync_users.py @@ -138,9 +138,16 @@ def handle(self, *args, **options): # noqa: ARG002 continue sent_name = row["given_name"], row["family_name"] + # Keycloak may omit "name" entirely, or echo it back as an + # explicit null, for a user synced with a blank given/family + # name (exactly the population this command targets) - + # `.get("name", {})` only covers the omitted case; a present + # `"name": null` returns None itself, and calling .get() on + # that would raise AttributeError. Normalize every case to "". + name_body = state.response_body.get("name") or {} got_name = ( - state.response_body.get("name", {}).get("givenName"), - state.response_body.get("name", {}).get("familyName"), + name_body.get("givenName") or "", + name_body.get("familyName") or "", ) if got_name == sent_name: verified.append(row) diff --git a/users/management/tests/migrate_and_sync_users_test.py b/users/management/tests/migrate_and_sync_users_test.py index 5af05197c8..5603002572 100644 --- a/users/management/tests/migrate_and_sync_users_test.py +++ b/users/management/tests/migrate_and_sync_users_test.py @@ -150,6 +150,54 @@ def test_verifies_matching_response_body(mocker, tmp_path): assert report["mismatched"] == [] +@pytest.mark.django_db +def test_verifies_blank_name_when_response_body_omits_name(mocker, tmp_path): + """A tier-3 (forced blank name) user's response body omitting "name" + entirely must still verify - sent ("", "") should match an absent name, + not be flagged as a mismatch""" + user = UserFactory.create(name="", global_id=None) + user.legal_address.first_name = "" + user.legal_address.last_name = "" + user.legal_address.save() + + mocker.patch( + "users.management.commands.migrate_and_sync_users.scim_api.sync_users_to_scim_remote", + return_value=[_state(user, response_body={})], + ) + + report = _run( + tmp_path, dry_run=False, skip_edx_migration=False, force=True, limit=None + ) + + assert [row["user_id"] for row in report["verified"]] == [user.id] + assert report["mismatched"] == [] + + +@pytest.mark.django_db +def test_verifies_blank_name_when_response_body_has_explicit_null_name( + mocker, tmp_path +): + """A response body with "name": null (present, not omitted) must not + crash - .get("name", {}) returns None itself in that case, and calling + .get() on it would raise AttributeError without a guard""" + user = UserFactory.create(name="", global_id=None) + user.legal_address.first_name = "" + user.legal_address.last_name = "" + user.legal_address.save() + + mocker.patch( + "users.management.commands.migrate_and_sync_users.scim_api.sync_users_to_scim_remote", + return_value=[_state(user, response_body={"name": None})], + ) + + report = _run( + tmp_path, dry_run=False, skip_edx_migration=False, force=True, limit=None + ) + + assert [row["user_id"] for row in report["verified"]] == [user.id] + assert report["mismatched"] == [] + + @pytest.mark.django_db def test_flags_mismatched_response_body(mocker, tmp_path): """A synced user whose echoed response doesn't match what was sent is flagged, From f0b0572fdff8eddf9644b23ab175fbe63445af5f Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 11 Aug 2026 14:22:43 +0000 Subject: [PATCH 4/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- users/management/tests/migrate_and_sync_users_test.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/users/management/tests/migrate_and_sync_users_test.py b/users/management/tests/migrate_and_sync_users_test.py index 5603002572..3260aea6e9 100644 --- a/users/management/tests/migrate_and_sync_users_test.py +++ b/users/management/tests/migrate_and_sync_users_test.py @@ -154,7 +154,8 @@ def test_verifies_matching_response_body(mocker, tmp_path): def test_verifies_blank_name_when_response_body_omits_name(mocker, tmp_path): """A tier-3 (forced blank name) user's response body omitting "name" entirely must still verify - sent ("", "") should match an absent name, - not be flagged as a mismatch""" + not be flagged as a mismatch + """ user = UserFactory.create(name="", global_id=None) user.legal_address.first_name = "" user.legal_address.last_name = "" @@ -179,7 +180,8 @@ def test_verifies_blank_name_when_response_body_has_explicit_null_name( ): """A response body with "name": null (present, not omitted) must not crash - .get("name", {}) returns None itself in that case, and calling - .get() on it would raise AttributeError without a guard""" + .get() on it would raise AttributeError without a guard + """ user = UserFactory.create(name="", global_id=None) user.legal_address.first_name = "" user.legal_address.last_name = "" From 7d660b5b7f7cff0b88e561b98ecfd367105e23fc Mon Sep 17 00:00:00 2001 From: sar Date: Tue, 11 Aug 2026 09:46:39 -0500 Subject: [PATCH 5/7] fix(scim): thread --limit through to migrate_edx_data's Stage 1 --limit only bounded which mitxonline candidates got classified/synced in Stage 2+; Stage 1's call_command("migrate_edx_data", type="users") ran unbounded regardless, so a small test run still backfilled the entire Trino dataset before test-syncing a handful of users. migrate_edx_data's _migrate_users() reads options.get("limit") and applies it directly to its Trino query, so this is a real, useful value to thread through - unlike --dry-run, which _migrate_users() never checks at all (only course_runs/entitlements do), so it can't be threaded through to make Stage 1 a true no-op. Corrected the --dry-run help text to stop claiming it prevents all writes, and extracted Stage 1 into _run_edx_backfill() to keep handle() under the statement-count lint threshold. Co-Authored-By: Claude Sonnet 5 --- .../commands/migrate_and_sync_users.py | 33 ++++++++++++++--- .../tests/migrate_and_sync_users_test.py | 37 +++++++++++++++++++ 2 files changed, 64 insertions(+), 6 deletions(-) diff --git a/users/management/commands/migrate_and_sync_users.py b/users/management/commands/migrate_and_sync_users.py index b1bd716386..611935a229 100644 --- a/users/management/commands/migrate_and_sync_users.py +++ b/users/management/commands/migrate_and_sync_users.py @@ -42,7 +42,10 @@ def add_arguments(self, parser): parser.add_argument( "--limit", type=int, - help="Limit the number of users processed (for testing purposes).", + help="Limit the number of users processed (for testing purposes). " + "Also passed through to migrate_edx_data's Trino query in Stage 1, " + "so a small test run doesn't backfill the entire edX dataset just " + "to test-sync a handful of users.", ) parser.add_argument( "--force", @@ -53,7 +56,11 @@ def add_arguments(self, parser): parser.add_argument( "--dry-run", action="store_true", - help="Run backfill and classification only; do not sync or write anything.", + help="Run classification only; do not sync to Keycloak. Note this " + "does NOT make Stage 1 a no-op - migrate_edx_data's own 'users' " + "migration type doesn't support --dry-run, so it still writes " + "User/LegalAddress/UserProfile rows unless --skip-edx-migration " + "is also passed.", ) parser.add_argument( "--report-path", @@ -69,11 +76,10 @@ def handle(self, *args, **options): # noqa: ARG002 dry_run = options.get("dry_run", False) report_path = options.get("report_path") - if not options.get("skip_edx_migration", False): - self.stdout.write("Stage 1: backfilling edX user data...") - call_command("migrate_edx_data", type="users") - else: + if options.get("skip_edx_migration", False): self.stdout.write("Stage 1: skipped (--skip-edx-migration).") + else: + self._run_edx_backfill(limit) self.stdout.write("Stage 2: classifying sync candidates...") candidates = list( @@ -178,6 +184,21 @@ def handle(self, *args, **options): # noqa: ARG002 report_path, ) + def _run_edx_backfill(self, limit): + """Run Stage 1 (migrate_edx_data's "users" migration type). + + migrate_edx_data's "users" type doesn't support --dry-run (only + course_runs/entitlements do) - it always writes, so there's no + dry_run to thread through here. --limit is threaded through so a + small test run doesn't backfill the entire edX dataset just to + test-sync a handful of users. + """ + self.stdout.write("Stage 1: backfilling edX user data...") + if limit is not None: + call_command("migrate_edx_data", type="users", limit=limit) + else: + call_command("migrate_edx_data", type="users") + def _classify(self, candidates, *, force): """Split candidates into (to_sync, blocked) using LearnUserAdapter's _resolve_name() tiers, purely for classification/reporting - this diff --git a/users/management/tests/migrate_and_sync_users_test.py b/users/management/tests/migrate_and_sync_users_test.py index 3260aea6e9..daea5c40c1 100644 --- a/users/management/tests/migrate_and_sync_users_test.py +++ b/users/management/tests/migrate_and_sync_users_test.py @@ -42,6 +42,43 @@ def mock_edx_migration(mocker): return mocker.patch("users.management.commands.migrate_and_sync_users.call_command") +@pytest.mark.django_db +def test_limit_is_threaded_through_to_migrate_edx_data(mock_edx_migration, mocker): + """--limit should also limit Stage 1's Trino query, not just which + mitxonline candidates get classified/synced - otherwise a small test run + still backfills the entire edX dataset before test-syncing a handful + of users + """ + mocker.patch( + "users.management.commands.migrate_and_sync_users.scim_api.sync_users_to_scim_remote", + return_value=[], + ) + + COMMAND.handle( + dry_run=False, skip_edx_migration=False, force=False, limit=5 + ) + + mock_edx_migration.assert_called_once_with( + "migrate_edx_data", type="users", limit=5 + ) + + +@pytest.mark.django_db +def test_no_limit_does_not_pass_limit_kwarg(mock_edx_migration, mocker): + """Without --limit, migrate_edx_data should run with its own default + (unbounded), not an explicit limit=None which would behave differently + if migrate_edx_data ever starts treating limit=None as limit=0 + """ + mocker.patch( + "users.management.commands.migrate_and_sync_users.scim_api.sync_users_to_scim_remote", + return_value=[], + ) + + COMMAND.handle(dry_run=False, skip_edx_migration=False, force=False, limit=None) + + mock_edx_migration.assert_called_once_with("migrate_edx_data", type="users") + + @pytest.mark.django_db def test_dry_run_classifies_without_syncing(mocker, tmp_path): """Dry run reports classification tiers and never calls the sync API""" From c0979f784050636efb955b2a4042d3468b7c9dbf Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 11 Aug 2026 14:47:32 +0000 Subject: [PATCH 6/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- users/management/tests/migrate_and_sync_users_test.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/users/management/tests/migrate_and_sync_users_test.py b/users/management/tests/migrate_and_sync_users_test.py index daea5c40c1..f0f726fe17 100644 --- a/users/management/tests/migrate_and_sync_users_test.py +++ b/users/management/tests/migrate_and_sync_users_test.py @@ -54,9 +54,7 @@ def test_limit_is_threaded_through_to_migrate_edx_data(mock_edx_migration, mocke return_value=[], ) - COMMAND.handle( - dry_run=False, skip_edx_migration=False, force=False, limit=5 - ) + COMMAND.handle(dry_run=False, skip_edx_migration=False, force=False, limit=5) mock_edx_migration.assert_called_once_with( "migrate_edx_data", type="users", limit=5 From 1283e403dcb5bea1eca9abc4f07ccce3c8051033 Mon Sep 17 00:00:00 2001 From: sar Date: Tue, 11 Aug 2026 09:56:36 -0500 Subject: [PATCH 7/7] docs(scim): explain the migrate_edx_data delegation in the docstring Document why Stage 1 delegates to migrate_edx_data --type users via call_command() rather than reimplementing its Trino/bulk_create logic (that command owns the Trino schema and serves five other unrelated migration types), and the concrete limits that follow from it: --dry-run can't make Stage 1 a no-op since migrate_edx_data's "users" type doesn't support it, while --limit does get threaded through. Co-Authored-By: Claude Sonnet 5 --- users/management/commands/migrate_and_sync_users.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/users/management/commands/migrate_and_sync_users.py b/users/management/commands/migrate_and_sync_users.py index 611935a229..e2e62bc054 100644 --- a/users/management/commands/migrate_and_sync_users.py +++ b/users/management/commands/migrate_and_sync_users.py @@ -6,6 +6,17 @@ This replaces the old two-step manual process (running `migrate_edx_data`, then an ad hoc SCIM sync script with no field-level visibility) with one auditable command. + +Stage 1 delegates to `migrate_edx_data --type users` via `call_command()` +rather than reimplementing its Trino/bulk_create logic here - that command +also serves five other unrelated migration types (course_runs, entitlements, +etc.) and owns the Trino schema, so duplicating its logic would mean two +places to keep in sync as that schema changes. This delegation has real +limits worth knowing: `migrate_edx_data`'s "users" type doesn't support +`--dry-run` (only its course_runs/entitlements types do), so this command's +own `--dry-run` cannot make Stage 1 a no-op - it always writes unless +`--skip-edx-migration` is also passed. `--limit` is threaded through, since +that type does respect it. """ import json