Merge duplicate user accounts by email — management command, auth backend, and registration guard - #5
Open
SpiderQubit with Copilot wants to merge 3 commits into
Open
Merge duplicate user accounts by email — management command, auth backend, and registration guard#5SpiderQubit with Copilot wants to merge 3 commits into
SpiderQubit with Copilot wants to merge 3 commits into
Conversation
Copilot created this pull request from a session on behalf of
SpiderQubit
April 15, 2026 15:56
View session
SpiderQubit
force-pushed
the
copilot/merge-users-by-email
branch
3 times, most recently
from
April 18, 2026 12:09
341fcd4 to
f77bd93
Compare
SpiderQubit
marked this pull request as ready for review
April 29, 2026 08:19
SpiderQubit
marked this pull request as draft
April 29, 2026 08:19
SpiderQubit
added
bug
Owner
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds email-or-username authentication, a management command to merge duplicate user accounts by email (with optional notification), enforces unique active emails at registration, updates auth backends ordering, and includes tests covering the new backend and merge command dry-run behavior. ChangesAuthentication and Registration
User Merge Management Command
Sequence Diagram(s)sequenceDiagram
participant Admin as "Admin (runs command)"
participant Cmd as "merge_users_by_email\n(Management Command)"
participant DB as "Database / User & Content Models"
participant Email as "Email backend (send_mail)"
Admin->>Cmd: run command (--dry-run|--send-emails|--no-input)
Cmd->>DB: query users with non-empty emails grouped by email
alt no duplicates
DB-->>Cmd: empty result
Cmd-->>Admin: print "no duplicates" and exit
else duplicates found
DB-->>Cmd: groups with user lists
Cmd->>Admin: print plan (dry-run)
opt not dry-run
Cmd->>DB: for each group select target, update related blocks/courses (prefix names), regenerate slugs, reassign owner, save
Cmd->>DB: deactivate old user accounts
Cmd->>Email: if --send-emails, send grouped notifications
Email-->>Cmd: success/failure per recipient
Cmd-->>Admin: summary of deactivated accounts and email results
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
rodatraden/tests.py (1)
50-88: ⚡ Quick winConsider adding test for inactive user rejection.
The backend relies on
user_can_authenticate()to reject deactivated (merged) users. Adding a test case that verifies inactive users cannot authenticate would strengthen coverage of this critical post-merge behavior.Suggested test
def test_inactive_user_fails(self): """Inactive (deactivated) users should not be able to authenticate.""" from rodatraden.backends import EmailOrUsernameBackend backend = EmailOrUsernameBackend() user = User.objects.create_user('inactiveuser', 'inactive@example.com', 'testpass') user.is_active = False user.save() result = backend.authenticate(None, username='inactiveuser', password='testpass') self.assertIsNone(result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rodatraden/tests.py` around lines 50 - 88, Add a test that verifies EmailOrUsernameBackend rejects inactive users: create a user via User.objects.create_user (e.g., username 'inactiveuser'), set user.is_active = False and save, then call EmailOrUsernameBackend().authenticate(None, username='inactiveuser', password='testpass') and assert the result is None; this ensures the backend’s use of user_can_authenticate()/is_active is covered (name the test method test_inactive_user_fails).rodatraden/backends.py (1)
45-49: 💤 Low valueConsider filtering to active users in the email fallback query.
When multiple users share the same email, the fallback selects by
-last_login. If a deactivated (merged) user logged in more recently, they'll be selected first but then rejected byuser_can_authenticate(). This works but is inefficient. Additionally, whenlast_loginisNULLfor multiple users, ordering behavior may vary across databases.Consider filtering to active users upfront for clarity and consistency:
Suggested improvement
try: user = UserModel.objects.get(username=username) except UserModel.DoesNotExist: user = UserModel.objects.filter( - email__iexact=username - ).order_by('-last_login').first() + email__iexact=username, + is_active=True + ).order_by('-last_login').first() if user is None: return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rodatraden/backends.py` around lines 45 - 49, The email-fallback query selects the most-recent user by last_login but can pick deactivated users and behaves nondeterministically when last_login is NULL; update the UserModel email lookup (the call that does UserModel.objects.filter(email__iexact=username).order_by('-last_login').first()) to pre-filter only active users (e.g., .filter(is_active=True) or your project’s active/merged flag) and add a stable secondary ordering (e.g., .order_by('-last_login', 'pk')) so selection is deterministic; keep the existing user_can_authenticate() check as a safeguard.tf/settings-template.py (1)
88-92: 💤 Low valueStale comment: "For CAS authentication" no longer applies.
The comment on line 88 references CAS authentication, but this configuration now uses
EmailOrUsernameBackend. Consider updating the comment to reflect the current authentication setup.Suggested fix
-# For CAS authentication +# Authentication backends (email/username login first, then Django default) AUTHENTICATION_BACKENDS = ( 'rodatraden.backends.EmailOrUsernameBackend', 'django.contrib.auth.backends.ModelBackend', )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tf/settings-template.py` around lines 88 - 92, The comment "For CAS authentication" above AUTHENTICATION_BACKENDS is stale; update or remove it to accurately describe the current backends. Locate the AUTHENTICATION_BACKENDS tuple and either change the comment to something like "Authentication backends: email/username lookup and default model backend" or delete the comment entirely so it reflects the use of 'rodatraden.backends.EmailOrUsernameBackend' and 'django.contrib.auth.backends.ModelBackend'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rodatraden/backends.py`:
- Around line 45-49: The email-fallback query selects the most-recent user by
last_login but can pick deactivated users and behaves nondeterministically when
last_login is NULL; update the UserModel email lookup (the call that does
UserModel.objects.filter(email__iexact=username).order_by('-last_login').first())
to pre-filter only active users (e.g., .filter(is_active=True) or your project’s
active/merged flag) and add a stable secondary ordering (e.g.,
.order_by('-last_login', 'pk')) so selection is deterministic; keep the existing
user_can_authenticate() check as a safeguard.
In `@rodatraden/tests.py`:
- Around line 50-88: Add a test that verifies EmailOrUsernameBackend rejects
inactive users: create a user via User.objects.create_user (e.g., username
'inactiveuser'), set user.is_active = False and save, then call
EmailOrUsernameBackend().authenticate(None, username='inactiveuser',
password='testpass') and assert the result is None; this ensures the backend’s
use of user_can_authenticate()/is_active is covered (name the test method
test_inactive_user_fails).
In `@tf/settings-template.py`:
- Around line 88-92: The comment "For CAS authentication" above
AUTHENTICATION_BACKENDS is stale; update or remove it to accurately describe the
current backends. Locate the AUTHENTICATION_BACKENDS tuple and either change the
comment to something like "Authentication backends: email/username lookup and
default model backend" or delete the comment entirely so it reflects the use of
'rodatraden.backends.EmailOrUsernameBackend' and
'django.contrib.auth.backends.ModelBackend'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 22034eaa-fc57-472b-9bc5-2a80dced0d51
📥 Commits
Reviewing files that changed from the base of the PR and between f29ed5b and 2008c8ca2aa54009a218ad8bba3abd7bc4f08385.
📒 Files selected for processing (6)
rodatraden/backends.pyrodatraden/management/commands/merge_users_by_email.pyrodatraden/tests.pyrodatraden/views.pytf/forms.pytf/settings-template.py
SpiderQubit
force-pushed
the
copilot/merge-users-by-email
branch
from
May 5, 2026 20:37
2008c8c to
34692d5
Compare
SpiderQubit
marked this pull request as ready for review
May 6, 2026 10:09
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tf/settings-template.py (1)
88-92: 💤 Low valueStale comment should be updated.
The comment "For CAS authentication" at line 88 is now misleading since the backend has been changed to
EmailOrUsernameBackend. Consider updating the comment to reflect the current purpose.📝 Suggested comment update
-# For CAS authentication +# Authentication backends (email/username login, then standard model backend) AUTHENTICATION_BACKENDS = ( 'rodatraden.backends.EmailOrUsernameBackend', 'django.contrib.auth.backends.ModelBackend', )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tf/settings-template.py` around lines 88 - 92, The comment above AUTHENTICATION_BACKENDS is outdated and mentions "For CAS authentication" while the tuple now uses 'rodatraden.backends.EmailOrUsernameBackend' and 'django.contrib.auth.backends.ModelBackend'; update the comment to reflect the actual purpose (e.g., to indicate custom email-or-username authentication and fallback to the default model backend) so it accurately documents AUTHENTICATION_BACKENDS and EmailOrUsernameBackend.rodatraden/management/commands/merge_users_by_email.py (1)
30-30: 💤 Low valueUnused import.
The
slugifyimport is not used in this file. The code usesget_unique_slugfromrodatraden.modelsinstead.🧹 Remove unused import
-from django.utils.text import slugify🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rodatraden/management/commands/merge_users_by_email.py` at line 30, Remove the unused import "slugify" from the top of the file where it's currently imported; the code uses get_unique_slug from rodatraden.models instead, so delete the import statement referencing slugify to eliminate the unused import warning and keep imports minimal.rodatraden/tests.py (1)
50-88: ⚡ Quick winLGTM on the backend tests.
Good coverage of the core authentication scenarios. The static analysis warnings about hardcoded passwords are false positives for test code.
Consider adding test coverage for:
- Case-insensitive email matching (
TEST@example.comvstest@example.com)MultipleObjectsReturnedscenario (multiple users with same email)- Inactive user authentication (should fail via
user_can_authenticate)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rodatraden/tests.py` around lines 50 - 88, Add three tests to EmailOrUsernameBackendTests: (1) test_email_case_insensitive — create a user with email "test@example.com" and assert EmailOrUsernameBackend().authenticate(None, username="TEST@example.com", password="testpass") returns the user (case-insensitive match); (2) test_multiple_users_same_email — create two users with the same email and assert that EmailOrUsernameBackend().authenticate(...) either raises django.core.exceptions.MultipleObjectsReturned (wrap in assertRaises) or returns None depending on the backend semantics you expect (choose the assertion consistent with current backend behavior) to cover the MultipleObjectsReturned scenario; (3) test_inactive_user_fails — create a user, set user.is_active = False and save, then assert EmailOrUsernameBackend().authenticate(None, username='testuser', password='testpass') returns None to verify user_can_authenticate is respected. Use the same EmailOrUsernameBackend symbol so tests match existing style.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rodatraden/management/commands/merge_users_by_email.py`:
- Around line 103-108: The duplicate detection is using case-sensitive email
grouping while registration uses case-insensitive comparisons; update the
grouping to normalize emails (e.g., annotate with Lower('email') or equivalent)
so duplicates differing only by case are detected, then iterate using that
normalized key (reference the duplicate_emails aggregation and the loop that
sets email and calls User.objects.filter) and fetch the users with a
case-insensitive lookup (User.objects.filter(email__iexact=...)) or by filtering
on the annotated normalized value to ensure consistent behavior with
clean_email.
---
Nitpick comments:
In `@rodatraden/management/commands/merge_users_by_email.py`:
- Line 30: Remove the unused import "slugify" from the top of the file where
it's currently imported; the code uses get_unique_slug from rodatraden.models
instead, so delete the import statement referencing slugify to eliminate the
unused import warning and keep imports minimal.
In `@rodatraden/tests.py`:
- Around line 50-88: Add three tests to EmailOrUsernameBackendTests: (1)
test_email_case_insensitive — create a user with email "test@example.com" and
assert EmailOrUsernameBackend().authenticate(None, username="TEST@example.com",
password="testpass") returns the user (case-insensitive match); (2)
test_multiple_users_same_email — create two users with the same email and assert
that EmailOrUsernameBackend().authenticate(...) either raises
django.core.exceptions.MultipleObjectsReturned (wrap in assertRaises) or returns
None depending on the backend semantics you expect (choose the assertion
consistent with current backend behavior) to cover the MultipleObjectsReturned
scenario; (3) test_inactive_user_fails — create a user, set user.is_active =
False and save, then assert EmailOrUsernameBackend().authenticate(None,
username='testuser', password='testpass') returns None to verify
user_can_authenticate is respected. Use the same EmailOrUsernameBackend symbol
so tests match existing style.
In `@tf/settings-template.py`:
- Around line 88-92: The comment above AUTHENTICATION_BACKENDS is outdated and
mentions "For CAS authentication" while the tuple now uses
'rodatraden.backends.EmailOrUsernameBackend' and
'django.contrib.auth.backends.ModelBackend'; update the comment to reflect the
actual purpose (e.g., to indicate custom email-or-username authentication and
fallback to the default model backend) so it accurately documents
AUTHENTICATION_BACKENDS and EmailOrUsernameBackend.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4470c263-9663-496d-923e-83a64a66c730
📥 Commits
Reviewing files that changed from the base of the PR and between 2008c8ca2aa54009a218ad8bba3abd7bc4f08385 and 34692d5.
📒 Files selected for processing (6)
rodatraden/backends.pyrodatraden/management/commands/merge_users_by_email.pyrodatraden/tests.pyrodatraden/views.pytf/forms.pytf/settings-template.py
Comment on lines
+103
to
+108
| for entry in duplicate_emails: | ||
| email = entry['email'] | ||
| users = list( | ||
| User.objects.filter(email=email) | ||
| .order_by('-last_login', '-date_joined') | ||
| ) |
There was a problem hiding this comment.
Email grouping uses case-sensitive comparison, but registration validation uses case-insensitive.
The duplicate detection at line 106 uses the default case-sensitive email field comparison, but the registration form's clean_email uses email__iexact. This could miss duplicates where emails differ only by case (e.g., Test@example.com vs test@example.com).
Consider using case-insensitive grouping for consistency:
🔧 Suggested fix using annotation
+from django.db.models.functions import Lower
+
# In handle method, change the duplicate_emails query:
- duplicate_emails = (
- User.objects
- .exclude(email='')
- .exclude(email__isnull=True)
- .values('email')
- .annotate(user_count=Count('id'))
- .filter(user_count__gt=1)
- .order_by('-user_count')
- )
+ duplicate_emails = (
+ User.objects
+ .exclude(email='')
+ .exclude(email__isnull=True)
+ .annotate(email_lower=Lower('email'))
+ .values('email_lower')
+ .annotate(user_count=Count('id'))
+ .filter(user_count__gt=1)
+ .order_by('-user_count')
+ )
# And update the user lookup at line 106:
- users = list(
- User.objects.filter(email=email)
- .order_by('-last_login', '-date_joined')
- )
+ users = list(
+ User.objects.filter(email__iexact=entry['email_lower'])
+ .order_by('-last_login', '-date_joined')
+ )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rodatraden/management/commands/merge_users_by_email.py` around lines 103 -
108, The duplicate detection is using case-sensitive email grouping while
registration uses case-insensitive comparisons; update the grouping to normalize
emails (e.g., annotate with Lower('email') or equivalent) so duplicates
differing only by case are detected, then iterate using that normalized key
(reference the duplicate_emails aggregation and the loop that sets email and
calls User.objects.filter) and fetch the users with a case-insensitive lookup
(User.objects.filter(email__iexact=...)) or by filtering on the annotated
normalized value to ensure consistent behavior with clean_email.
…end, and duplicate email prevention - Management command to merge duplicate users sharing the same email - Renames migrated blocks/private courses with username prefix to avoid slug collisions - Deactivates old accounts instead of deleting them - Supports --dry-run, --send-emails, and --no-input flags - EmailOrUsernameBackend allows login by email or username - Registration form rejects duplicate emails - Initial migration for rodatraden models - Tests for backend and command Agent-Logs-Url: https://github.com/it-amanuens/rodatraden/sessions/08553e33-ca08-4e85-8480-a792e7b01761 Co-authored-by: SpiderQubit <25958778+SpiderQubit@users.noreply.github.com>
…d, exclude inactive users in registration check, narrow exception handling in email sending Agent-Logs-Url: https://github.com/it-amanuens/rodatraden/sessions/08553e33-ca08-4e85-8480-a792e7b01761 Co-authored-by: SpiderQubit <25958778+SpiderQubit@users.noreply.github.com>
SpiderQubit
force-pushed
the
copilot/merge-users-by-email
branch
from
July 18, 2026 09:48
34692d5 to
5b08175
Compare
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.
Overview
This PR fixes duplicate-account issues caused by users creating multiple accounts with the same email.
It merges duplicate accounts safely, improves login behavior, and prevents new duplicates.
Changes
merge_users_by_emailto merge users sharing an emailBlock,PrivateCourse)is_active=False) instead of deleting themEmailOrUsernameBackendso users can log in with either username or emailOperations
python manage.py merge_users_by_email --dry-runpython manage.py merge_users_by_emailpython manage.py merge_users_by_email --send-emailsAUTHENTICATION_BACKENDS)Why this matters
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Behavior