From 5e977d3c4b6c6eb6c30bbc42631e3349bf29bb2a Mon Sep 17 00:00:00 2001 From: sar Date: Tue, 11 Aug 2026 11:32:28 -0500 Subject: [PATCH 1/4] fix(courses): re-fetch created users by email after bulk_create bulk_create(..., ignore_conflicts=True) never populates .pk on the returned objects, on any database backend - this is documented Django 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. _bulk_create_users returned that PK-less list directly, so every downstream consumer (_bulk_create_legal_addresses, _bulk_create_user_profiles) filtered on ids that were all None, matched zero rows, and silently created nothing - not a blank LegalAddress/UserProfile row, no row at all. Confirmed in production: PR #3158 (merged 2025-12-19) is what introduced ignore_conflicts=True here, to fix a genuine duplicate-row crash - a real problem, but the fix had this unnoticed side effect. 848,688 users have been SCIM-synced to Keycloak; 63,852 of them have no UserProfile row at all, and 63,836 have no LegalAddress row - both counts line up almost exactly, consistent with both failing together in the same batch step ever since that PR merged. This also breaks should_skip_onboarding (users/models.py) and _build_user_data (openedx/api.py), both of which access user.legal_address/ user.user_profile without a defensive fallback - meaning affected users can hit a 500 on login and can't get their Open edX account synced. Fix: re-fetch the newly-created rows by email (unique via the user_email_unique constraint) right after the bulk_create call, so callers get real, usable ids. This also incidentally fixes a second bug in the same code path: id_row_lookup keyed every entry off user.id, which collapsed onto the same None key for every user in a batch, silently dropping all but the last user's row data whenever a batch had more than one new user - moot once ids are real and unique. This does not backfill the ~63.8k users already affected - that's a separate, deliberately smaller remediation step, tracked separately. Co-Authored-By: Claude Sonnet 5 --- .../management/commands/migrate_edx_data.py | 21 ++- .../commands/test_migrate_edx_data.py | 132 ++++++++++++++++++ 2 files changed, 150 insertions(+), 3 deletions(-) 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..6719607b77 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,9 +270,23 @@ def _bulk_create_users(rows, existing_emails, batch_size): ) user.set_unusable_password() new_users.append(user) - return User.objects.bulk_create( - new_users, batch_size=batch_size, ignore_conflicts=True - ) + 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/commands/test_migrate_edx_data.py b/courses/management/commands/test_migrate_edx_data.py new file mode 100644 index 0000000000..d0dbaa1f67 --- /dev/null +++ b/courses/management/commands/test_migrate_edx_data.py @@ -0,0 +1,132 @@ +"""Tests for migrate_edx_data management command's user-migration bulk_create fix""" + +import pytest + +from courses.management.commands.migrate_edx_data import Command +from users.factories import UserFactory +from users.models import LegalAddress, User, UserProfile + +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): + """No-op - the fake cursor already has its rows in memory.""" + + def fetchmany(self, size): + """Return the next slice of rows, matching the DB-API contract.""" + 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 a fresh fake cursor over the same fixed rows.""" + return FakeCursor(self._columns, self._rows) + + +USER_COLUMNS = [ + "user_email", + "user_full_name", + "user_address_country", + "user_address_state", + "user_address_postal_code", + "user_address_street_1", + "user_address_street_2", + "user_address_city", + "user_gender", + "user_birth_year", +] + + +def _user_row(email, name, country="US"): + return (email, name, country, "MA", "02139", "1 Main St", "", "Cambridge", None, None) + + +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() From fcad5c092cf4e286b2c1148824ca4718927b307e 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:34:15 +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 +++- .../management/commands/test_migrate_edx_data.py | 13 ++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/courses/management/commands/migrate_edx_data.py b/courses/management/commands/migrate_edx_data.py index 6719607b77..ee8fb913f4 100644 --- a/courses/management/commands/migrate_edx_data.py +++ b/courses/management/commands/migrate_edx_data.py @@ -272,7 +272,9 @@ def _bulk_create_users(rows, existing_emails, batch_size): new_users.append(user) new_emails.append(email) - User.objects.bulk_create(new_users, batch_size=batch_size, ignore_conflicts=True) + User.objects.bulk_create( + new_users, batch_size=batch_size, ignore_conflicts=True + ) if not new_emails: return [] diff --git a/courses/management/commands/test_migrate_edx_data.py b/courses/management/commands/test_migrate_edx_data.py index d0dbaa1f67..8da548d65b 100644 --- a/courses/management/commands/test_migrate_edx_data.py +++ b/courses/management/commands/test_migrate_edx_data.py @@ -54,7 +54,18 @@ def cursor(self): def _user_row(email, name, country="US"): - return (email, name, country, "MA", "02139", "1 Main St", "", "Cambridge", None, None) + return ( + email, + name, + country, + "MA", + "02139", + "1 Main St", + "", + "Cambridge", + None, + None, + ) def test_bulk_create_users_returns_real_ids(): From 6370016e85d8d2c517cc0eecc5cb253c077e1301 Mon Sep 17 00:00:00 2001 From: sar Date: Tue, 11 Aug 2026 13:11:05 -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 | 143 ------------------ .../management/tests/migrate_edx_data_test.py | 78 +++++++++- 2 files changed, 77 insertions(+), 144 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 8da548d65b..0000000000 --- a/courses/management/commands/test_migrate_edx_data.py +++ /dev/null @@ -1,143 +0,0 @@ -"""Tests for migrate_edx_data management command's user-migration bulk_create fix""" - -import pytest - -from courses.management.commands.migrate_edx_data import Command -from users.factories import UserFactory -from users.models import LegalAddress, User, UserProfile - -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): - """No-op - the fake cursor already has its rows in memory.""" - - def fetchmany(self, size): - """Return the next slice of rows, matching the DB-API contract.""" - 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 a fresh fake cursor over the same fixed rows.""" - return FakeCursor(self._columns, self._rows) - - -USER_COLUMNS = [ - "user_email", - "user_full_name", - "user_address_country", - "user_address_state", - "user_address_postal_code", - "user_address_street_1", - "user_address_street_2", - "user_address_city", - "user_gender", - "user_birth_year", -] - - -def _user_row(email, name, country="US"): - return ( - email, - name, - country, - "MA", - "02139", - "1 Main St", - "", - "Cambridge", - None, - None, - ) - - -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() diff --git a/courses/management/tests/migrate_edx_data_test.py b/courses/management/tests/migrate_edx_data_test.py index d89327661a..ca6f10e44c 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 user-migration bulk_create +fix and repair_migrated_profiles type""" import pytest @@ -76,6 +77,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") From e0f33985f3c9729b0564320c4342e2945c48b269 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:13:48 +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 ca6f10e44c..41485decc2 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 user-migration bulk_create -fix and repair_migrated_profiles type""" +fix and repair_migrated_profiles type +""" import pytest