feat(courses): add --dry-run support to migrate_edx_data's users type - #3842
Open
shaidar wants to merge 4 commits into
Open
feat(courses): add --dry-run support to migrate_edx_data's users type#3842shaidar wants to merge 4 commits into
shaidar wants to merge 4 commits into
Conversation
OpenAPI ChangesShow/hide changesUnexpected changes? Ensure your branch is up-to-date with |
_migrate_users() never checked options.get("dry_run") at all, unlike
the course_certificates/entitlements migration types which already
support it - so `migrate_edx_data --type users --dry-run` silently
wrote real User/LegalAddress/UserProfile rows despite the flag.
Mirrors the existing dry-run pattern used elsewhere in this file:
count what would be created (net of existing_emails dedup) and log a
[DRY RUN] summary instead of calling the bulk_create methods.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
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
force-pushed
the
sar/migrate-edx-data-users-dry-run
branch
from
August 12, 2026 16:14
82f7416 to
c2fb70b
Compare
for more information, see https://pre-commit.ci
Comment on lines
391
to
+393
| ) | ||
|
|
||
| if dry_run: |
There was a problem hiding this comment.
Bug: The user migration dry-run can report an inflated count of new users when an incoming email matches an existing user's username but not their email.
Severity: LOW
Suggested Fix
To fix the inaccurate count, the query for existing users should collect both usernames and emails. Instead of just values_list("email", flat=True), fetch both username and email fields. Then, create a set of all existing usernames and emails to check against, ensuring that an incoming email that matches either an existing username or email is correctly excluded from the new user count in the dry-run.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: courses/management/commands/migrate_edx_data.py#L391-L393
Potential issue: In the `migrate_edx_data` management command, the dry-run logic for
user migration can produce an inaccurate count of new users. The process identifies
existing users by checking for matches in both the `username` and `email` fields but
only collects the `email` values from the matched records. If an incoming user email
from the data source matches an existing user's `username` but not their `email`, the
dry-run will incorrectly count this as a new user to be created. However, during a real
run, the `bulk_create` operation with `ignore_conflicts=True` will silently fail due to
the unique constraint on the `username`, resulting in zero users being created. This
discrepancy leads to an inflated count in the dry-run report.
Did we get this right? 👍 / 👎 to inform future reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are the relevant tickets?
N/A
Description (What does it do?)
_migrate_users()never checkedoptions.get("dry_run")at all, unlike thecourse_certificates/entitlementsmigration types in this same file, which already support it. Somigrate_edx_data --type users --dry-runsilently wrote realUser/LegalAddress/UserProfilerows despite the flag — no error, no warning, just a false sense of safety.This mirrors the existing dry-run pattern already used elsewhere in this file (e.g.
_migrate_course_certificates): inside the batch loop, count what would be created — net of the existingexisting_emailsde-dup check, so the count matches what a real run would actually produce — and log a[DRY RUN] Would create N userssummary instead of calling_bulk_create_users/_bulk_create_legal_addresses/_bulk_create_user_profiles.How can this be tested?
Added
courses/management/commands/test_migrate_edx_data.py(this command had no test coverage at all before). Uses a minimal fake Trino cursor/connection so_migrate_users()can be exercised directly without a real Trino connection. Covers:--dry-runcreates zeroUserrows and correctly excludes already-existing emails from the count.fetchmanybatches, not just within one.Userrows as before.Reverted the fix locally and confirmed both dry-run tests fail (real writes happened despite
dry_run=True) before restoring it. Ranruff checkon both changed files — clean, aside from three pre-existing, unrelated missing-docstring findings on lines this PR doesn't touch.Additional Context
Written and tested against a fresh Django-and-Postgres test environment built directly off
main(which now includes the recentECOMMERCE_DEFAULT_PAYMENT_GATEWAYfix from #3832), so no CI-blocking workarounds were needed.