From b1682ef75568f50a927d3972162a72eca2fb3d30 Mon Sep 17 00:00:00 2001 From: sar Date: Tue, 11 Aug 2026 13:00:18 -0500 Subject: [PATCH 1/5] fix(users): backfill LegalAddress/UserProfile for migrated users, guard should_skip_onboarding migrate_edx_data's bulk_create(ignore_conflicts=True) bug (fixed separately) meant every user created by the "users" migration type ended up with no LegalAddress or UserProfile row at all - id_row_lookup was keyed on user.id=None for every newly created user, so User.objects.filter(id__in=[None]) always matched zero rows and the two bulk-create helpers silently created nothing. Add a repair_migrated_profiles migration type that re-scans the same edX table, but only acts on users already missing one of these rows - real, already-saved User objects with real ids, so the existing _bulk_create_legal_addresses/_bulk_create_user_profiles helpers work correctly the same way they already do for the enrollments migration's opportunistic repair path. Also guard User.should_skip_onboarding against a missing UserProfile - today it crashes with RelatedObjectDoesNotExist on every login for any affected user, via OpenedxAndApiGatewayLoginView.get(). --- .../management/commands/migrate_edx_data.py | 102 +++++++++- .../commands/test_migrate_edx_data.py | 187 ++++++++++++++++++ users/models.py | 3 +- users/models_test.py | 24 +++ 4 files changed, 314 insertions(+), 2 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 63b941e9d8..0b56b5b780 100644 --- a/courses/management/commands/migrate_edx_data.py +++ b/courses/management/commands/migrate_edx_data.py @@ -404,6 +404,98 @@ def _migrate_users(self, conn, options): self.stdout.write(self.style.SUCCESS(f"{user_creation_count} users created")) + def _repair_migrated_user_profiles(self, conn, options): + """ + Backfill LegalAddress/UserProfile rows for users left without them by + a historical bug in _bulk_create_users: bulk_create(ignore_conflicts=True) + never populates .pk on the objects passed into it, so every call here + used to build id_row_lookup keyed on user.id=None and then filter + User.objects.filter(id__in=[None, ...]), which always matches zero + rows - LegalAddress/UserProfile silently never got created for any + user who went through this command, regardless of the row data + Trino actually had for them. + + This re-scans the same edX table used by _migrate_users, but only + acts on users already missing one of these rows - real, already-saved + User objects with real ids, so the existing bulk-create helpers work + correctly here the same way they already do for the enrollments + migration's opportunistic repair path. + """ + limit = options.get("limit") + batch_size = options.get("batch_size", 1000) + dry_run = options.get("dry_run") + + affected_users = list( + User.objects.filter( + Q(legal_address__isnull=True) | Q(user_profile__isnull=True) + ) + ) + if limit is not None: + affected_users = affected_users[:limit] + affected_by_email = {user.email: user for user in affected_users if user.email} + + self.stdout.write( + f"{len(affected_by_email)} users found missing " + f"LegalAddress and/or UserProfile" + ) + + cur = conn.cursor() + cur.execute("SELECT * FROM edxorg_to_mitxonline_users") + columns = [desc[0] for desc in cur.description] + + # e.g. {"Male": "m", "Female": "f"} + GENDER_MAP = {label: code for code, label in GENDER_CHOICES} + + repaired_emails = set() + while True: + results = cur.fetchmany(batch_size) + if not results: + break + + rows = [dict(zip(columns, r)) for r in results] + + matched_users = [] + row_lookup_by_id = {} + for row in rows: + email = row.get("user_email") + user = affected_by_email.get(email) + if user is None or email in repaired_emails: + continue + matched_users.append(user) + row_lookup_by_id[user.id] = row + repaired_emails.add(email) + + if not matched_users: + continue + + if dry_run: + continue + + self._bulk_create_legal_addresses( + matched_users, row_lookup_by_id, batch_size + ) + self._bulk_create_user_profiles( + matched_users, row_lookup_by_id, batch_size, GENDER_MAP + ) + + unmatched = len(affected_by_email) - len(repaired_emails) + if dry_run: + self.stdout.write( + self.style.WARNING(f"[DRY RUN] Would repair {len(repaired_emails)} users") + ) + else: + self.stdout.write( + self.style.SUCCESS(f"Repaired {len(repaired_emails)} users") + ) + if unmatched: + self.stdout.write( + self.style.WARNING( + f"{unmatched} affected users had no matching row in " + f"edxorg_to_mitxonline_users - still missing " + f"LegalAddress/UserProfile, needs manual review" + ) + ) + @staticmethod def _bulk_create_enrollments( rows, @@ -1048,9 +1140,10 @@ def add_arguments(self, parser) -> None: "program_certificates", "entitlements", "future_enrollments", + "repair_migrated_profiles", ], default="course_runs", - help="Choose which migration to run: course_runs, users, course_certificates, program_certificates, entitlements, future_enrollments (default: course_runs)", + help="Choose which migration to run: course_runs, users, course_certificates, program_certificates, entitlements, future_enrollments, repair_migrated_profiles (default: course_runs)", ) parser.add_argument("--dry-run", action="store_true") parser.add_argument( @@ -1072,6 +1165,13 @@ def handle(self, *args, **options): # pylint: disable=unused-argument # noqa: A self.stdout.write("Migrating the edX users ...") self._migrate_users(conn, options) + if migrate_type == "repair_migrated_profiles": + self.stdout.write( + "Repairing LegalAddress/UserProfile rows missed by a " + "historical bug in the users migration ..." + ) + self._repair_migrated_user_profiles(conn, options) + if migrate_type == "course_certificates": self.stdout.write( "Migrating the edX course enrollments, grades and certificates ..." 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..ad7bfae8f4 --- /dev/null +++ b/courses/management/commands/test_migrate_edx_data.py @@ -0,0 +1,187 @@ +"""Tests for migrate_edx_data management command's repair_migrated_profiles type""" + +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 _break_user(user): + """Simulate the historical bug's effect: a User row with no + LegalAddress or UserProfile at all. + """ + LegalAddress.objects.filter(user=user).delete() + UserProfile.objects.filter(user=user).delete() + + +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") + _break_user(user) + + conn = FakeConnection( + columns=USER_COLUMNS, + rows=[_user_row(user.email, "Broken User", country="US")], + ) + + Command()._repair_migrated_user_profiles(conn, {}) # noqa: SLF001 + + user.refresh_from_db() + assert user.legal_address.country == "US" + assert UserProfile.objects.filter(user=user).exists() + + +def test_repair_only_backfills_missing_piece(): + """A user missing only UserProfile (LegalAddress already exists) only + gets UserProfile created - the existing helpers must not touch or + duplicate the LegalAddress that's already there + """ + user = UserFactory.create(email="half-broken@example.com") + user.legal_address.country = "CA" + user.legal_address.save() + UserProfile.objects.filter(user=user).delete() + + conn = FakeConnection( + columns=USER_COLUMNS, + rows=[_user_row(user.email, "Half Broken", country="US")], + ) + + Command()._repair_migrated_user_profiles(conn, {}) # noqa: SLF001 + + user.refresh_from_db() + assert user.legal_address.country == "CA" + assert UserProfile.objects.filter(user=user).exists() + + +def test_repair_skips_users_with_no_missing_rows(): + """A user who already has both rows is left untouched, even though a + matching edX row exists for their email + """ + user = UserFactory.create(email="fine@example.com") + user.legal_address.country = "MX" + user.legal_address.save() + + conn = FakeConnection( + columns=USER_COLUMNS, + rows=[_user_row(user.email, "Fine User", country="US")], + ) + + Command()._repair_migrated_user_profiles(conn, {}) # noqa: SLF001 + + user.refresh_from_db() + assert user.legal_address.country == "MX" + + +def test_repair_leaves_unmatched_user_broken_without_crashing(): + """An affected user with no corresponding row in edxorg_to_mitxonline_users + (e.g. their edX data was never in that table) is reported as still + missing, not silently invented from nothing + """ + user = UserFactory.create(email="no-edx-row@example.com") + _break_user(user) + + conn = FakeConnection(columns=USER_COLUMNS, rows=[]) + + Command()._repair_migrated_user_profiles(conn, {}) # noqa: SLF001 + + assert not LegalAddress.objects.filter(user=user).exists() + assert not UserProfile.objects.filter(user=user).exists() + + +def test_dry_run_does_not_write(): + """--dry-run must not create any rows, even for a clearly matched user""" + user = UserFactory.create(email="dry-run@example.com") + _break_user(user) + + conn = FakeConnection( + columns=USER_COLUMNS, + rows=[_user_row(user.email, "Dry Run User", country="US")], + ) + + Command()._repair_migrated_user_profiles(conn, {"dry_run": True}) # noqa: SLF001 + + assert not LegalAddress.objects.filter(user=user).exists() + assert not UserProfile.objects.filter(user=user).exists() + + +def test_limit_caps_number_of_users_repaired(): + """--limit caps how many affected users are considered at all, leaving + the rest broken for a later run + """ + users = [ + UserFactory.create(email=f"limited-{i}@example.com") for i in range(3) + ] + for user in users: + _break_user(user) + + conn = FakeConnection( + columns=USER_COLUMNS, + rows=[_user_row(user.email, "Limited User", country="US") for user in users], + ) + + Command()._repair_migrated_user_profiles(conn, {"limit": 1}) # noqa: SLF001 + + repaired = User.objects.filter(legal_address__isnull=False).count() + assert repaired == 1 diff --git a/users/models.py b/users/models.py index b727788994..85d77ae97e 100644 --- a/users/models.py +++ b/users/models.py @@ -368,8 +368,9 @@ def b2b_organization_sso_ids(self): @property def should_skip_onboarding(self): + user_profile = getattr(self, "user_profile", None) return ( - self.user_profile.completed_onboarding + (user_profile is not None and user_profile.completed_onboarding) or self.courserunenrollment_set(manager="all_objects").exists() ) diff --git a/users/models_test.py b/users/models_test.py index d6dcba0b29..d35e649d86 100644 --- a/users/models_test.py +++ b/users/models_test.py @@ -18,6 +18,7 @@ OPENEDX_HIGHEST_EDUCATION_MAPPINGS, LegalAddress, User, + UserProfile, ) pytestmark = pytest.mark.django_db @@ -139,6 +140,29 @@ def test_user_is_editor(is_staff, is_superuser, has_editor_group, exp_is_editor) assert user.is_editor is exp_is_editor +def test_should_skip_onboarding_with_no_user_profile(): + """A user with no UserProfile row at all (e.g. one created by the + historical migrate_edx_data bulk_create bug) must not crash accessing + should_skip_onboarding - it should fall back to the enrollment check + instead of raising RelatedObjectDoesNotExist + """ + user = UserFactory.create() + UserProfile.objects.filter(user=user).delete() + + assert user.should_skip_onboarding is False + + +def test_should_skip_onboarding_with_completed_onboarding(): + """A user with a UserProfile behaves as before: completed_onboarding + alone is enough to skip onboarding + """ + user = UserFactory.create() + user.user_profile.completed_onboarding = True + user.user_profile.save() + + assert user.should_skip_onboarding is True + + def test_legal_address_us_state(): """ Tests to make sure the us_state property is working properly. From f50c5f84e08528eaa9e506028cb402d5d9571c7a 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 18:01:37 +0000 Subject: [PATCH 2/5] [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 +++- courses/management/commands/test_migrate_edx_data.py | 4 +--- users/models.py | 5 ++--- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/courses/management/commands/migrate_edx_data.py b/courses/management/commands/migrate_edx_data.py index 0b56b5b780..71506b2b2d 100644 --- a/courses/management/commands/migrate_edx_data.py +++ b/courses/management/commands/migrate_edx_data.py @@ -481,7 +481,9 @@ def _repair_migrated_user_profiles(self, conn, options): unmatched = len(affected_by_email) - len(repaired_emails) if dry_run: self.stdout.write( - self.style.WARNING(f"[DRY RUN] Would repair {len(repaired_emails)} users") + self.style.WARNING( + f"[DRY RUN] Would repair {len(repaired_emails)} users" + ) ) else: self.stdout.write( diff --git a/courses/management/commands/test_migrate_edx_data.py b/courses/management/commands/test_migrate_edx_data.py index ad7bfae8f4..d89327661a 100644 --- a/courses/management/commands/test_migrate_edx_data.py +++ b/courses/management/commands/test_migrate_edx_data.py @@ -170,9 +170,7 @@ def test_limit_caps_number_of_users_repaired(): """--limit caps how many affected users are considered at all, leaving the rest broken for a later run """ - users = [ - UserFactory.create(email=f"limited-{i}@example.com") for i in range(3) - ] + users = [UserFactory.create(email=f"limited-{i}@example.com") for i in range(3)] for user in users: _break_user(user) diff --git a/users/models.py b/users/models.py index 85d77ae97e..debc64ba2e 100644 --- a/users/models.py +++ b/users/models.py @@ -370,9 +370,8 @@ def b2b_organization_sso_ids(self): def should_skip_onboarding(self): user_profile = getattr(self, "user_profile", None) return ( - (user_profile is not None and user_profile.completed_onboarding) - or self.courserunenrollment_set(manager="all_objects").exists() - ) + user_profile is not None and user_profile.completed_onboarding + ) or self.courserunenrollment_set(manager="all_objects").exists() @property def openedx_user_exists(self): From 3b50729da6a6eb40c675ebff6438cb452299d2f7 Mon Sep 17 00:00:00 2001 From: sar Date: Tue, 11 Aug 2026 13:12:02 -0500 Subject: [PATCH 3/5] 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/). --- .../test_migrate_edx_data.py => tests/migrate_edx_data_test.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename courses/management/{commands/test_migrate_edx_data.py => tests/migrate_edx_data_test.py} (100%) diff --git a/courses/management/commands/test_migrate_edx_data.py b/courses/management/tests/migrate_edx_data_test.py similarity index 100% rename from courses/management/commands/test_migrate_edx_data.py rename to courses/management/tests/migrate_edx_data_test.py From 2f94174134257d0c6906f426c5748d69fb71f28f Mon Sep 17 00:00:00 2001 From: sar Date: Tue, 11 Aug 2026 13:20:16 -0500 Subject: [PATCH 4/5] fix(auth): stop GatewayLoginView from crashing on a missing UserProfile should_skip_onboarding no longer raises for a user with no UserProfile, but GatewayLoginView.get() still did `profile = user.user_profile` immediately afterward whenever should_skip_onboarding is False - which is now exactly the case for these users - so the same RelatedObjectDoesNotExist just moved one line down into the view instead of being fixed. Use get_or_create so the missing row is created here instead of crashing. --- authentication/api_gateway/views.py | 3 ++- authentication/api_gateway/views_test.py | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/authentication/api_gateway/views.py b/authentication/api_gateway/views.py index a58bafd55c..7952ea0fb4 100644 --- a/authentication/api_gateway/views.py +++ b/authentication/api_gateway/views.py @@ -29,6 +29,7 @@ USER_MSG_TYPE_PROFILE_CREATED, ) from main.utils import encode_json_cookie_value, is_success_response +from users.models import UserProfile User = get_user_model() @@ -135,7 +136,7 @@ def get( params = urlencode({"next": redirect_url}) redirect_url = f"{settings.MITXONLINE_NEW_USER_LOGIN_URL}?{params}" - profile = user.user_profile + profile, _ = UserProfile.objects.get_or_create(user=user) profile.completed_onboarding = True profile.save() return redirect(redirect_url) diff --git a/authentication/api_gateway/views_test.py b/authentication/api_gateway/views_test.py index d53a3d6559..b73d56b3d9 100644 --- a/authentication/api_gateway/views_test.py +++ b/authentication/api_gateway/views_test.py @@ -101,6 +101,27 @@ def test_custom_login_view_authenticated_user_with_onboarding(settings, client): assert user.user_profile.completed_onboarding is True +def test_custom_login_view_with_no_user_profile(settings, client): + """A user with no UserProfile row at all (e.g. one created by the + historical migrate_edx_data bulk_create bug) must not crash logging in - + the view must create the missing UserProfile rather than accessing + user.user_profile directly + """ + settings.MITXONLINE_NEW_USER_LOGIN_URL = "/create-profile" + + user = UserFactory.create() + UserProfile.objects.filter(user=user).delete() + + client.force_login(user) + + qs = {"next": "/dashboard"} + response = client.get(f"{reverse('gateway-login')}?{urlencode(qs)}") + + assert response.status_code == 302 + assert response.url == "/create-profile?next=%2Fdashboard" + assert UserProfile.objects.get(user=user).completed_onboarding is True + + @pytest.mark.parametrize( "next_url", ["/dashboard", "http://openedx.odl.local/courses/abc"] ) From 5a8208c28300e6b8fce7d8c4b8b0c68a6bb3a527 Mon Sep 17 00:00:00 2001 From: sar Date: Wed, 12 Aug 2026 10:44:16 -0500 Subject: [PATCH 5/5] perf(courses): only fetch id/email for the affected-users repair query _repair_migrated_user_profiles only ever uses .id (to build user_ids/ row_lookup_by_id) and .email (to match against Trino rows) off the affected_users queryset - _bulk_create_legal_addresses/_bulk_create_user_profiles re-fetch full rows themselves via User.objects.filter(id__in=user_ids). .only() avoids loading every column for every affected user at ~63k scale. --- courses/management/commands/migrate_edx_data.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/courses/management/commands/migrate_edx_data.py b/courses/management/commands/migrate_edx_data.py index 71506b2b2d..5bd1f40bf3 100644 --- a/courses/management/commands/migrate_edx_data.py +++ b/courses/management/commands/migrate_edx_data.py @@ -428,7 +428,7 @@ def _repair_migrated_user_profiles(self, conn, options): affected_users = list( User.objects.filter( Q(legal_address__isnull=True) | Q(user_profile__isnull=True) - ) + ).only("id", "email") ) if limit is not None: affected_users = affected_users[:limit]