diff --git a/courses/management/commands/migrate_edx_data.py b/courses/management/commands/migrate_edx_data.py index 5bd1f40bf3..ee8fb913f4 100644 --- a/courses/management/commands/migrate_edx_data.py +++ b/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): """ diff --git a/courses/management/tests/migrate_edx_data_test.py b/courses/management/tests/migrate_edx_data_test.py index d89327661a..41485decc2 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 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")