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"] ) diff --git a/courses/management/commands/migrate_edx_data.py b/courses/management/commands/migrate_edx_data.py index 63b941e9d8..5bd1f40bf3 100644 --- a/courses/management/commands/migrate_edx_data.py +++ b/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 ..." diff --git a/courses/management/tests/migrate_edx_data_test.py b/courses/management/tests/migrate_edx_data_test.py new file mode 100644 index 0000000000..d89327661a --- /dev/null +++ b/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 diff --git a/users/models.py b/users/models.py index b727788994..debc64ba2e 100644 --- a/users/models.py +++ b/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): 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.