From f3edd7ef10a1d6d7f31e56b017c425afb65d806b Mon Sep 17 00:00:00 2001 From: sar Date: Tue, 11 Aug 2026 11:28:31 -0500 Subject: [PATCH 1/4] feat(courses): add --dry-run support to migrate_edx_data's users type _migrate_users() never checked options.get("dry_run") at all, unlike the course_certificates/entitlements migration types which already support it - so `migrate_edx_data --type users --dry-run` silently wrote real User/LegalAddress/UserProfile rows despite the flag. Mirrors the existing dry-run pattern used elsewhere in this file: count what would be created (net of existing_emails dedup) and log a [DRY RUN] summary instead of calling the bulk_create methods. Co-Authored-By: Claude Sonnet 5 --- .../management/commands/migrate_edx_data.py | 16 ++++ .../commands/test_migrate_edx_data.py | 95 +++++++++++++++++++ 2 files changed, 111 insertions(+) create mode 100644 courses/management/commands/test_migrate_edx_data.py diff --git a/courses/management/commands/migrate_edx_data.py b/courses/management/commands/migrate_edx_data.py index 5bd1f40bf3..f78f2d01bf 100644 --- a/courses/management/commands/migrate_edx_data.py +++ b/courses/management/commands/migrate_edx_data.py @@ -358,6 +358,7 @@ def _migrate_users(self, conn, options): """ limit = options.get("limit") batch_size = options.get("batch_size", 1000) + dry_run = options.get("dry_run") cur = conn.cursor() @@ -389,6 +390,13 @@ def _migrate_users(self, conn, options): ).values_list("email", flat=True) ) + if dry_run: + new_emails = [ + email for email in emails if email not in existing_emails + ] + user_creation_count += len(new_emails) + continue + created_users = self._bulk_create_users(rows, existing_emails, batch_size) user_creation_count += len(created_users) @@ -402,6 +410,14 @@ def _migrate_users(self, conn, options): created_users, id_row_lookup, batch_size, GENDER_MAP ) + if dry_run: + self.stdout.write( + self.style.WARNING( + f"[DRY RUN] Would create {user_creation_count} users" + ) + ) + return + self.stdout.write(self.style.SUCCESS(f"{user_creation_count} users created")) def _repair_migrated_user_profiles(self, conn, options): diff --git a/courses/management/commands/test_migrate_edx_data.py b/courses/management/commands/test_migrate_edx_data.py new file mode 100644 index 0000000000..531aaba187 --- /dev/null +++ b/courses/management/commands/test_migrate_edx_data.py @@ -0,0 +1,95 @@ +"""Tests for migrate_edx_data management command's --dry-run behavior""" + +import pytest + +from courses.management.commands.migrate_edx_data import Command +from users.factories import UserFactory +from users.models import User + +pytestmark = pytest.mark.django_db + + +class FakeCursor: + """Minimal stand-in for a Trino DB-API cursor.""" + + def __init__(self, columns, rows): + self.description = [(col,) for col in columns] + self._rows = rows + self._offset = 0 + + def execute(self, query): + pass + + def fetchmany(self, size): + batch = self._rows[self._offset : self._offset + size] + self._offset += size + return batch + + +class FakeConnection: + """Minimal stand-in for a Trino DB-API connection.""" + + def __init__(self, columns, rows): + self._columns = columns + self._rows = rows + + def cursor(self): + return FakeCursor(self._columns, self._rows) + + +def test_migrate_users_dry_run_creates_no_records(capsys): + """--dry-run must not create any User/LegalAddress/UserProfile rows""" + existing_user = UserFactory.create(email="existing@example.com") + conn = FakeConnection( + columns=["user_email", "user_full_name"], + rows=[ + ("new1@example.com", "New One"), + ("new2@example.com", "New Two"), + (existing_user.email, "Existing User"), + ], + ) + + Command()._migrate_users(conn, {"dry_run": True}) # noqa: SLF001 + + assert User.objects.count() == 1 # only the pre-existing user + output = capsys.readouterr().out + assert "[DRY RUN] Would create 2 users" in output + + +def test_migrate_users_dry_run_respects_batching(capsys): + """The dry-run count must accumulate correctly across multiple fetchmany + batches, not just within a single batch + """ + conn = FakeConnection( + columns=["user_email", "user_full_name"], + rows=[(f"new{i}@example.com", f"New {i}") for i in range(5)], + ) + + Command()._migrate_users(conn, {"dry_run": True, "batch_size": 2}) # noqa: SLF001 + + assert User.objects.count() == 0 + output = capsys.readouterr().out + assert "[DRY RUN] Would create 5 users" in output + + +def test_migrate_users_real_run_creates_user_records(): + """Without --dry-run, matching rows actually create User rows. + + Deliberately not asserting on legal_address here: _bulk_create_users + calls User.objects.bulk_create(..., ignore_conflicts=True), and Django + never populates .pk on returned objects when ignore_conflicts=True is + used, on any backend - confirmed empirically against this test DB. That + means _bulk_create_legal_addresses/_bulk_create_user_profiles, which + filter on those (always-None) ids, never actually create anything today. + That's a separate, pre-existing bug unrelated to --dry-run - flagged + separately, not fixed here. + """ + conn = FakeConnection( + columns=["user_email", "user_full_name"], + rows=[("new@example.com", "New User")], + ) + + Command()._migrate_users(conn, {}) # noqa: SLF001 + + user = User.objects.get(email="new@example.com") + assert user.name == "New User" From 10ccfba76b4ed861d766d0ae0fbd951374880883 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 16:29:50 +0000 Subject: [PATCH 2/4] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- courses/management/commands/migrate_edx_data.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/courses/management/commands/migrate_edx_data.py b/courses/management/commands/migrate_edx_data.py index f78f2d01bf..ff05e8b3cc 100644 --- a/courses/management/commands/migrate_edx_data.py +++ b/courses/management/commands/migrate_edx_data.py @@ -391,9 +391,7 @@ def _migrate_users(self, conn, options): ) if dry_run: - new_emails = [ - email for email in emails if email not in existing_emails - ] + new_emails = [email for email in emails if email not in existing_emails] user_creation_count += len(new_emails) continue From c2fb70bed04ac000eed1598c08316c6791e8c9ee Mon Sep 17 00:00:00 2001 From: sar Date: Tue, 11 Aug 2026 13:11:38 -0500 Subject: [PATCH 3/4] fix(courses): move migrate_edx_data test file out of management/commands Django's command auto-discovery scans every .py file directly under management/commands/ as a candidate command module, so test_migrate_edx_data.py showed up (and would fail) in ./manage.py -h. Move it to management/tests/, matching the existing convention used by every other management command test in this app (and in users/). --- .../commands/test_migrate_edx_data.py | 95 ------------------- .../management/tests/migrate_edx_data_test.py | 61 +++++++++++- 2 files changed, 60 insertions(+), 96 deletions(-) delete mode 100644 courses/management/commands/test_migrate_edx_data.py diff --git a/courses/management/commands/test_migrate_edx_data.py b/courses/management/commands/test_migrate_edx_data.py deleted file mode 100644 index 531aaba187..0000000000 --- a/courses/management/commands/test_migrate_edx_data.py +++ /dev/null @@ -1,95 +0,0 @@ -"""Tests for migrate_edx_data management command's --dry-run behavior""" - -import pytest - -from courses.management.commands.migrate_edx_data import Command -from users.factories import UserFactory -from users.models import User - -pytestmark = pytest.mark.django_db - - -class FakeCursor: - """Minimal stand-in for a Trino DB-API cursor.""" - - def __init__(self, columns, rows): - self.description = [(col,) for col in columns] - self._rows = rows - self._offset = 0 - - def execute(self, query): - pass - - def fetchmany(self, size): - batch = self._rows[self._offset : self._offset + size] - self._offset += size - return batch - - -class FakeConnection: - """Minimal stand-in for a Trino DB-API connection.""" - - def __init__(self, columns, rows): - self._columns = columns - self._rows = rows - - def cursor(self): - return FakeCursor(self._columns, self._rows) - - -def test_migrate_users_dry_run_creates_no_records(capsys): - """--dry-run must not create any User/LegalAddress/UserProfile rows""" - existing_user = UserFactory.create(email="existing@example.com") - conn = FakeConnection( - columns=["user_email", "user_full_name"], - rows=[ - ("new1@example.com", "New One"), - ("new2@example.com", "New Two"), - (existing_user.email, "Existing User"), - ], - ) - - Command()._migrate_users(conn, {"dry_run": True}) # noqa: SLF001 - - assert User.objects.count() == 1 # only the pre-existing user - output = capsys.readouterr().out - assert "[DRY RUN] Would create 2 users" in output - - -def test_migrate_users_dry_run_respects_batching(capsys): - """The dry-run count must accumulate correctly across multiple fetchmany - batches, not just within a single batch - """ - conn = FakeConnection( - columns=["user_email", "user_full_name"], - rows=[(f"new{i}@example.com", f"New {i}") for i in range(5)], - ) - - Command()._migrate_users(conn, {"dry_run": True, "batch_size": 2}) # noqa: SLF001 - - assert User.objects.count() == 0 - output = capsys.readouterr().out - assert "[DRY RUN] Would create 5 users" in output - - -def test_migrate_users_real_run_creates_user_records(): - """Without --dry-run, matching rows actually create User rows. - - Deliberately not asserting on legal_address here: _bulk_create_users - calls User.objects.bulk_create(..., ignore_conflicts=True), and Django - never populates .pk on returned objects when ignore_conflicts=True is - used, on any backend - confirmed empirically against this test DB. That - means _bulk_create_legal_addresses/_bulk_create_user_profiles, which - filter on those (always-None) ids, never actually create anything today. - That's a separate, pre-existing bug unrelated to --dry-run - flagged - separately, not fixed here. - """ - conn = FakeConnection( - columns=["user_email", "user_full_name"], - rows=[("new@example.com", "New User")], - ) - - Command()._migrate_users(conn, {}) # noqa: SLF001 - - user = User.objects.get(email="new@example.com") - assert user.name == "New User" diff --git a/courses/management/tests/migrate_edx_data_test.py b/courses/management/tests/migrate_edx_data_test.py index d89327661a..85f64b9340 100644 --- a/courses/management/tests/migrate_edx_data_test.py +++ b/courses/management/tests/migrate_edx_data_test.py @@ -1,4 +1,5 @@ -"""Tests for migrate_edx_data management command's repair_migrated_profiles type""" +"""Tests for migrate_edx_data management command's repair_migrated_profiles +type and --dry-run behavior""" import pytest @@ -183,3 +184,61 @@ def test_limit_caps_number_of_users_repaired(): repaired = User.objects.filter(legal_address__isnull=False).count() assert repaired == 1 + + +def test_migrate_users_dry_run_creates_no_records(capsys): + """--dry-run must not create any User/LegalAddress/UserProfile rows""" + existing_user = UserFactory.create(email="existing@example.com") + conn = FakeConnection( + columns=["user_email", "user_full_name"], + rows=[ + ("new1@example.com", "New One"), + ("new2@example.com", "New Two"), + (existing_user.email, "Existing User"), + ], + ) + + Command()._migrate_users(conn, {"dry_run": True}) # noqa: SLF001 + + assert User.objects.count() == 1 # only the pre-existing user + output = capsys.readouterr().out + assert "[DRY RUN] Would create 2 users" in output + + +def test_migrate_users_dry_run_respects_batching(capsys): + """The dry-run count must accumulate correctly across multiple fetchmany + batches, not just within a single batch + """ + conn = FakeConnection( + columns=["user_email", "user_full_name"], + rows=[(f"new{i}@example.com", f"New {i}") for i in range(5)], + ) + + Command()._migrate_users(conn, {"dry_run": True, "batch_size": 2}) # noqa: SLF001 + + assert User.objects.count() == 0 + output = capsys.readouterr().out + assert "[DRY RUN] Would create 5 users" in output + + +def test_migrate_users_real_run_creates_user_records(): + """Without --dry-run, matching rows actually create User rows. + + Deliberately not asserting on legal_address here: _bulk_create_users + calls User.objects.bulk_create(..., ignore_conflicts=True), and Django + never populates .pk on returned objects when ignore_conflicts=True is + used, on any backend - confirmed empirically against this test DB. That + means _bulk_create_legal_addresses/_bulk_create_user_profiles, which + filter on those (always-None) ids, never actually create anything today. + That's a separate, pre-existing bug unrelated to --dry-run - flagged + separately, not fixed here. + """ + conn = FakeConnection( + columns=["user_email", "user_full_name"], + rows=[("new@example.com", "New User")], + ) + + Command()._migrate_users(conn, {}) # noqa: SLF001 + + user = User.objects.get(email="new@example.com") + assert user.name == "New User" From cc93be81f41687d1964995de8a77ff9b0ce9f7ec Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 12 Aug 2026 16:15:56 +0000 Subject: [PATCH 4/4] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- courses/management/tests/migrate_edx_data_test.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/courses/management/tests/migrate_edx_data_test.py b/courses/management/tests/migrate_edx_data_test.py index 85f64b9340..0efe266965 100644 --- a/courses/management/tests/migrate_edx_data_test.py +++ b/courses/management/tests/migrate_edx_data_test.py @@ -1,5 +1,6 @@ """Tests for migrate_edx_data management command's repair_migrated_profiles -type and --dry-run behavior""" +type and --dry-run behavior +""" import pytest