Backfill missing LegalAddress/UserProfile for migrated users; guard should_skip_onboarding

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)

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"]
 )

courses/management/commands/migrate_edx_data.py

@@ -404,6 +404,100 @@ 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)
+            ).only("id", "email")
+        )
+        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 +1142,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 +1167,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 ..."

courses/management/tests/migrate_edx_data_test.py

@@ -0,0 +1,185 @@
+"""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

users/models.py

@@ -368,10 +368,10 @@ 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
-            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):