Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions courses/management/commands/migrate_edx_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ def _migrate_users(self, conn, options):
"""
limit = options.get("limit")
batch_size = options.get("batch_size", 1000)
dry_run = options.get("dry_run")

cur = conn.cursor()

Expand Down Expand Up @@ -389,6 +390,11 @@ def _migrate_users(self, conn, options):
).values_list("email", flat=True)
)

if dry_run:
Comment on lines 391 to +393

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

new_emails = [email for email in emails if email not in existing_emails]
user_creation_count += len(new_emails)
continue

created_users = self._bulk_create_users(rows, existing_emails, batch_size)
user_creation_count += len(created_users)

Expand All @@ -402,6 +408,14 @@ def _migrate_users(self, conn, options):
created_users, id_row_lookup, batch_size, GENDER_MAP
)

if dry_run:
self.stdout.write(
self.style.WARNING(
f"[DRY RUN] Would create {user_creation_count} users"
)
)
return

self.stdout.write(self.style.SUCCESS(f"{user_creation_count} users created"))

def _repair_migrated_user_profiles(self, conn, options):
Expand Down
62 changes: 61 additions & 1 deletion courses/management/tests/migrate_edx_data_test.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
"""Tests for migrate_edx_data management command's repair_migrated_profiles type"""
"""Tests for migrate_edx_data management command's repair_migrated_profiles
type and --dry-run behavior
"""

import pytest

Expand Down Expand Up @@ -183,3 +185,61 @@ def test_limit_caps_number_of_users_repaired():

repaired = User.objects.filter(legal_address__isnull=False).count()
assert repaired == 1


def test_migrate_users_dry_run_creates_no_records(capsys):
"""--dry-run must not create any User/LegalAddress/UserProfile rows"""
existing_user = UserFactory.create(email="existing@example.com")
conn = FakeConnection(
columns=["user_email", "user_full_name"],
rows=[
("new1@example.com", "New One"),
("new2@example.com", "New Two"),
(existing_user.email, "Existing User"),
],
)

Command()._migrate_users(conn, {"dry_run": True}) # noqa: SLF001

assert User.objects.count() == 1 # only the pre-existing user
output = capsys.readouterr().out
assert "[DRY RUN] Would create 2 users" in output


def test_migrate_users_dry_run_respects_batching(capsys):
"""The dry-run count must accumulate correctly across multiple fetchmany
batches, not just within a single batch
"""
conn = FakeConnection(
columns=["user_email", "user_full_name"],
rows=[(f"new{i}@example.com", f"New {i}") for i in range(5)],
)

Command()._migrate_users(conn, {"dry_run": True, "batch_size": 2}) # noqa: SLF001

assert User.objects.count() == 0
output = capsys.readouterr().out
assert "[DRY RUN] Would create 5 users" in output


def test_migrate_users_real_run_creates_user_records():
"""Without --dry-run, matching rows actually create User rows.

Deliberately not asserting on legal_address here: _bulk_create_users
calls User.objects.bulk_create(..., ignore_conflicts=True), and Django
never populates .pk on returned objects when ignore_conflicts=True is
used, on any backend - confirmed empirically against this test DB. That
means _bulk_create_legal_addresses/_bulk_create_user_profiles, which
filter on those (always-None) ids, never actually create anything today.
That's a separate, pre-existing bug unrelated to --dry-run - flagged
separately, not fixed here.
"""
conn = FakeConnection(
columns=["user_email", "user_full_name"],
rows=[("new@example.com", "New User")],
)

Command()._migrate_users(conn, {}) # noqa: SLF001

user = User.objects.get(email="new@example.com")
assert user.name == "New User"