Skip to content

fix(courses): re-fetch created users by email after bulk_create in migrate_edx_data - #3843

Open
shaidar wants to merge 4 commits into
mainfrom
sar/fix-migrate-edx-data-bulk-create-pk
Open

fix(courses): re-fetch created users by email after bulk_create in migrate_edx_data#3843
shaidar wants to merge 4 commits into
mainfrom
sar/fix-migrate-edx-data-bulk-create-pk

Conversation

@shaidar

@shaidar shaidar commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

What are the relevant tickets?

N/A

Description (What does it do?)

bulk_create(..., ignore_conflicts=True) never populates .pk on the returned objects — on any database backend, this is 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. _bulk_create_users returned that PK-less list directly, so both downstream consumers — _bulk_create_legal_addresses and _bulk_create_user_profiles — filter on user.id values that are all None, match zero rows, and silently create nothing. Not a blank LegalAddress/UserProfile row — no row at all.

Confirmed root cause: PR #3158 (merged 2025-12-19) is what introduced ignore_conflicts=True here — the diff is a single, isolated one-line change (bulk_create(new_users, batch_size=batch_size)..., ignore_conflicts=True). That PR was fixing a real, separate problem (a hard crash on duplicate rows within a Trino batch), but the fix had this unnoticed side effect.

Confirmed impact in production, via direct queries:

  • 848,688 users have been SCIM-synced to Keycloak.
  • 63,852 of them have no UserProfile row at all; 63,836 have no LegalAddress row. Both counts line up almost exactly (~7.5% of the synced population), consistent with both failing together in the same batch step, every time, since the PR above merged.
  • This isn't just a data-completeness gap. Two real code paths access these relations without a defensive fallback:
    • User.should_skip_onboarding (users/models.py) does self.user_profile.completed_onboarding — checked on every login redirect in OpenedxAndApiGatewayLoginView.get(). Django's reverse-one-to-one descriptor always raises RelatedObjectDoesNotExist when the row is missing (it never just returns None), so this is a live crash risk on login for any affected user.
    • _build_user_data() (openedx/api.py) does user.legal_address.country if user.legal_address else None — the same problem; evaluating user.legal_address to check its truthiness is exactly what raises the exception, so the guard doesn't guard anything. This runs when provisioning/syncing a user's Open edX account, so affected users can't get that synced either.

How can this be tested?

Fix: re-fetch the newly created rows by email (unique via the user_email_unique Meta.constraints entry on User) immediately after the bulk_create call, so callers get real, usable ids.

This also incidentally fixes a second, smaller bug in the same code path: id_row_lookup = {user.id: row_lookup[user.email] for user in created_users ...} keyed every entry off user.id — which collapsed onto the identical None key for every user in a batch, silently dropping every user's row data except whichever one happened to be last in the dict comprehension, whenever a batch had more than one new user. That's moot once ids are real and unique.

Added courses/management/commands/test_migrate_edx_data.py covering:

  • _bulk_create_users returns objects with real, non-None ids.
  • A full _migrate_users() run actually creates LegalAddress and UserProfile rows for a new user (not just the User row).
  • Multiple new users in a single batch each get their own correct LegalAddress (the id_row_lookup collision case).
  • An already-existing user is correctly skipped and their existing records are left untouched.

Reverted the fix locally and confirmed 3 of the 4 tests fail, reproducing the exact RelatedObjectDoesNotExist seen in production, before restoring it. ruff check clean on both changed files (aside from three pre-existing, unrelated missing-docstring findings on lines this PR doesn't touch).

Additional Context

This PR only stops the bleeding for future runs of migrate_edx_data --type users. It does not backfill the ~63.8k users already affected — that's a separate, deliberately smaller remediation step (re-fetch each affected user's original Trino row data and create their missing LegalAddress/UserProfile directly), tracked separately so it can be reviewed and rolled out independently from this fix.

@github-actions

Copy link
Copy Markdown

OpenAPI Changes

Show/hide changes
## Changes for v0.yaml:
No changes detected

## Changes for v1.yaml:
No changes detected

## Changes for v2.yaml:
No changes detected

Unexpected changes? Ensure your branch is up-to-date with main (consider rebasing).

@rachellougee rachellougee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the fix

@rhysyngsun rhysyngsun left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM

@@ -0,0 +1,143 @@
"""Tests for migrate_edx_data management command's user-migration bulk_create fix"""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this test file to courses/management/tests, because otherwise django treats it as a management command that it could run. It'll fail, but it adds noise to ./manage.py -h.

shaidar and others added 3 commits August 12, 2026 11:11
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 <noreply@anthropic.com>
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/).
@shaidar
shaidar force-pushed the sar/fix-migrate-edx-data-bulk-create-pk branch from 8a045d2 to 6370016 Compare August 12, 2026 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants