diff --git a/courses/management/commands/migrate_edx_data.py b/courses/management/commands/migrate_edx_data.py index 5bd1f40bf3..ff05e8b3cc 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,11 @@ 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 +408,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/tests/migrate_edx_data_test.py b/courses/management/tests/migrate_edx_data_test.py index d89327661a..0efe266965 100644 --- a/courses/management/tests/migrate_edx_data_test.py +++ b/courses/management/tests/migrate_edx_data_test.py @@ -1,4 +1,6 @@ -"""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 +185,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"