fix(courses): re-fetch created users by email after bulk_create in migrate_edx_data

courses/management/commands/migrate_edx_data.py

@@ -255,6 +255,7 @@ def _bulk_create_users(rows, existing_emails, batch_size):
         Create users in bulk, skipping those with existing emails.
         """
         new_users = []
+        new_emails = []
         for row in rows:
             email = row.get("user_email")
             if not email or email in existing_emails:
@@ -269,10 +270,26 @@ def _bulk_create_users(rows, existing_emails, batch_size):
             )
             user.set_unusable_password()
             new_users.append(user)
-        return User.objects.bulk_create(
+            new_emails.append(email)
+
+        User.objects.bulk_create(
             new_users, batch_size=batch_size, ignore_conflicts=True
         )
 
+        if not new_emails:
+            return []
+
+        # bulk_create never populates .pk on the returned objects when
+        # ignore_conflicts=True is used - on any database backend, per
+        # Django's own documented behavior, since it can't reliably map a
+        # generated id back to a specific input object once some rows may
+        # have been silently skipped on conflict. Re-fetch by email (unique
+        # per users_user_email_unique) to get real ids for
+        # _bulk_create_legal_addresses/_bulk_create_user_profiles to key off
+        # of - without this, those two methods filter on ids that are all
+        # None, match zero rows, and silently never create anything.
+        return list(User.objects.filter(email__in=new_emails))
+
     @staticmethod
     def _bulk_create_legal_addresses(created_users, row_lookup_by_id, batch_size):
         """

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 user-migration bulk_create
+fix and repair_migrated_profiles type
+"""
 
 import pytest
 
@@ -76,6 +78,81 @@ def _break_user(user):
     UserProfile.objects.filter(user=user).delete()
 
 
+def test_bulk_create_users_returns_real_ids():
+    """_bulk_create_users must return objects with real, populated ids -
+    bulk_create(ignore_conflicts=True) never sets .pk on its own, on any
+    backend, so this only works if the returned rows are re-fetched
+    """
+    rows = [{"user_email": "new@example.com", "user_full_name": "New User"}]
+
+    created = Command._bulk_create_users(  # noqa: SLF001
+        rows, existing_emails=set(), batch_size=100
+    )
+
+    assert len(created) == 1
+    assert created[0].id is not None
+    assert User.objects.get(email="new@example.com").id == created[0].id
+
+
+def test_migrate_users_creates_legal_address_and_profile():
+    """End to end through _migrate_users: new users must actually get
+    LegalAddress and UserProfile rows, not just a User row
+    """
+    conn = FakeConnection(
+        columns=USER_COLUMNS,
+        rows=[_user_row("alice@example.com", "Alice A", country="US")],
+    )
+
+    Command()._migrate_users(conn, {})  # noqa: SLF001
+
+    user = User.objects.get(email="alice@example.com")
+    assert user.legal_address.country == "US"
+    assert UserProfile.objects.filter(user=user).exists()
+
+
+def test_migrate_users_multiple_new_users_in_one_batch():
+    """Multiple new users in a single batch must each get their own
+    LegalAddress - previously, every user.id was None, so id_row_lookup
+    collapsed every entry onto the same None key and only one user's row
+    data survived
+    """
+    conn = FakeConnection(
+        columns=USER_COLUMNS,
+        rows=[
+            _user_row("alice@example.com", "Alice A", country="US"),
+            _user_row("bob@example.com", "Bob B", country="CA"),
+        ],
+    )
+
+    Command()._migrate_users(conn, {})  # noqa: SLF001
+
+    alice = User.objects.get(email="alice@example.com")
+    bob = User.objects.get(email="bob@example.com")
+    assert alice.legal_address.country == "US"
+    assert bob.legal_address.country == "CA"
+    assert UserProfile.objects.filter(user=alice).exists()
+    assert UserProfile.objects.filter(user=bob).exists()
+
+
+def test_migrate_users_skips_existing_emails():
+    """A user who already exists in mitxonline must not get a duplicate
+    User row, and their existing LegalAddress/UserProfile must be left
+    alone
+    """
+    existing_user = UserFactory.create(email="existing@example.com")
+    LegalAddress.objects.filter(user=existing_user).delete()
+
+    conn = FakeConnection(
+        columns=USER_COLUMNS,
+        rows=[_user_row(existing_user.email, "Existing User", country="US")],
+    )
+
+    Command()._migrate_users(conn, {})  # noqa: SLF001
+
+    assert User.objects.filter(email=existing_user.email).count() == 1
+    assert not LegalAddress.objects.filter(user=existing_user).exists()
+
+
 def test_repair_creates_missing_legal_address_and_profile():
     """A user missing both rows gets both created from the matching edX row"""
     user = UserFactory.create(email="broken@example.com")