Fix case-sensitive email duplicates and add merge script
- Normalize emails to lowercase on user creation - Use case-insensitive lookup in get_user_by_email - Add merge_duplicate_email_users script to merge existing duplicates - Add unit and integration tests to prevent regression - Update CLAUDE.md with tmux-hosts production troubleshooting
This commit is contained in:
parent
df5c656eb0
commit
9330782999
6 changed files with 358 additions and 2 deletions
10
CLAUDE.md
10
CLAUDE.md
|
|
@ -34,3 +34,13 @@ When adding new columns or modifying the database schema:
|
||||||
3. **Generate migration**: `alembic -c development.ini revision --autogenerate -m "description"`
|
3. **Generate migration**: `alembic -c development.ini revision --autogenerate -m "description"`
|
||||||
4. **Clean up migration**: Remove extra autogenerated changes, keep only the new field
|
4. **Clean up migration**: Remove extra autogenerated changes, keep only the new field
|
||||||
5. **Run migration**: `alembic -c development.ini upgrade head`
|
5. **Run migration**: `alembic -c development.ini upgrade head`
|
||||||
|
|
||||||
|
## Production Troubleshooting
|
||||||
|
|
||||||
|
For read-only troubleshooting on production, use tmux-hosts:
|
||||||
|
|
||||||
|
```
|
||||||
|
tmux-hosts
|
||||||
|
```
|
||||||
|
|
||||||
|
**Important**: This is for read-only investigation only. Do not make changes to production systems.
|
||||||
|
|
|
||||||
|
|
@ -279,7 +279,7 @@ class User(RBase, Base):
|
||||||
self.name = unicode(generate_password(size=8))
|
self.name = unicode(generate_password(size=8))
|
||||||
self.created = now_timestamp()
|
self.created = now_timestamp()
|
||||||
self.id = uuid.uuid1()
|
self.id = uuid.uuid1()
|
||||||
self.email = unicode(email)
|
self.email = unicode(email.lower())
|
||||||
|
|
||||||
# TODO: this field was never used & should be sunset.
|
# TODO: this field was never used & should be sunset.
|
||||||
self.email_id = unicode(generate_password(size=8))
|
self.email_id = unicode(generate_password(size=8))
|
||||||
|
|
@ -417,7 +417,7 @@ def get_user_by_id(dbsession, user_id):
|
||||||
def get_user_by_email(dbsession, email):
|
def get_user_by_email(dbsession, email):
|
||||||
"""Try to get User object by email or return None"""
|
"""Try to get User object by email or return None"""
|
||||||
if email:
|
if email:
|
||||||
return dbsession.query(User).filter(User.email == unicode(email)).one_or_none()
|
return dbsession.query(User).filter(func.lower(User.email) == unicode(email.lower())).one_or_none()
|
||||||
|
|
||||||
|
|
||||||
def get_or_create_user_by_email(dbsession, email):
|
def get_or_create_user_by_email(dbsession, email):
|
||||||
|
|
|
||||||
231
remarkbox/scripts/merge_duplicate_email_users.py
Normal file
231
remarkbox/scripts/merge_duplicate_email_users.py
Normal file
|
|
@ -0,0 +1,231 @@
|
||||||
|
"""
|
||||||
|
Merge duplicate user accounts that have the same email (case-insensitive).
|
||||||
|
|
||||||
|
This script finds users with emails that differ only by case (e.g.,
|
||||||
|
'user@example.com' and 'User@Example.com') and merges them into a single
|
||||||
|
account, keeping the oldest one.
|
||||||
|
|
||||||
|
Usage:
|
||||||
|
remarkbox_merge_duplicate_email_users -c production.ini --dry-run
|
||||||
|
remarkbox_merge_duplicate_email_users -c production.ini
|
||||||
|
"""
|
||||||
|
from collections import defaultdict
|
||||||
|
|
||||||
|
from pyramid.paster import bootstrap, setup_logging
|
||||||
|
|
||||||
|
from sqlalchemy import func
|
||||||
|
|
||||||
|
from ..models import User
|
||||||
|
|
||||||
|
from . import base_parser
|
||||||
|
|
||||||
|
|
||||||
|
def get_arg_parser():
|
||||||
|
parser = base_parser("Find and merge user accounts with duplicate emails (case-insensitive).")
|
||||||
|
parser.add_argument(
|
||||||
|
"--dry-run",
|
||||||
|
action="store_true",
|
||||||
|
default=False,
|
||||||
|
help="Show what would be merged without making changes.",
|
||||||
|
)
|
||||||
|
return parser
|
||||||
|
|
||||||
|
|
||||||
|
def find_duplicate_email_users(dbsession):
|
||||||
|
"""
|
||||||
|
Find all users with duplicate emails (case-insensitive).
|
||||||
|
|
||||||
|
Returns a dict mapping lowercase email -> list of User objects.
|
||||||
|
Only includes emails with more than one user.
|
||||||
|
"""
|
||||||
|
duplicates = defaultdict(list)
|
||||||
|
|
||||||
|
# Get all users
|
||||||
|
users = dbsession.query(User).all()
|
||||||
|
|
||||||
|
for user in users:
|
||||||
|
email_lower = user.email.lower()
|
||||||
|
duplicates[email_lower].append(user)
|
||||||
|
|
||||||
|
# Filter to only duplicates
|
||||||
|
return {
|
||||||
|
email: users
|
||||||
|
for email, users in duplicates.items()
|
||||||
|
if len(users) > 1
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
def merge_users(dbsession, keep_user, delete_user, dry_run=False):
|
||||||
|
"""
|
||||||
|
Merge delete_user into keep_user by transferring all related records.
|
||||||
|
|
||||||
|
Transfers:
|
||||||
|
- nodes (comments)
|
||||||
|
- votes
|
||||||
|
- watchers
|
||||||
|
- node_event_notifications
|
||||||
|
- namespace_user associations
|
||||||
|
- namespace_requests
|
||||||
|
- oauth records
|
||||||
|
- payments
|
||||||
|
- pay_what_you_can
|
||||||
|
"""
|
||||||
|
keep_id = keep_user.id
|
||||||
|
delete_id = delete_user.id
|
||||||
|
|
||||||
|
print(f" Merging '{delete_user.name}' ({delete_user.email}) into '{keep_user.name}' ({keep_user.email})")
|
||||||
|
|
||||||
|
if dry_run:
|
||||||
|
# Count records that would be transferred
|
||||||
|
from ..models import (
|
||||||
|
Node, Vote, Watcher, NodeEventNotification,
|
||||||
|
NamespaceUser, NamespaceRequest, Oauth, Payment, PayWhatYouCan
|
||||||
|
)
|
||||||
|
|
||||||
|
counts = {
|
||||||
|
'nodes': dbsession.query(Node).filter(Node.user_id == delete_id).count(),
|
||||||
|
'votes': dbsession.query(Vote).filter(Vote.user_id == delete_id).count(),
|
||||||
|
'watchers': dbsession.query(Watcher).filter(Watcher.user_id == delete_id).count(),
|
||||||
|
'notifications': dbsession.query(NodeEventNotification).filter(NodeEventNotification.user_id == delete_id).count(),
|
||||||
|
'namespace_users': dbsession.query(NamespaceUser).filter(NamespaceUser.user_id == delete_id).count(),
|
||||||
|
'namespace_requests': dbsession.query(NamespaceRequest).filter(NamespaceRequest.user_id == delete_id).count(),
|
||||||
|
'oauth': dbsession.query(Oauth).filter(Oauth.user_id == delete_id).count(),
|
||||||
|
'payments': dbsession.query(Payment).filter(Payment.user_id == delete_id).count(),
|
||||||
|
'pay_what_you_can': dbsession.query(PayWhatYouCan).filter(PayWhatYouCan.user_id == delete_id).count(),
|
||||||
|
}
|
||||||
|
|
||||||
|
for table, count in counts.items():
|
||||||
|
if count > 0:
|
||||||
|
print(f" Would transfer {count} {table}")
|
||||||
|
|
||||||
|
return
|
||||||
|
|
||||||
|
# Transfer all related records
|
||||||
|
from ..models import (
|
||||||
|
Node, Vote, Watcher, NodeEventNotification,
|
||||||
|
NamespaceUser, NamespaceRequest, Oauth, Payment, PayWhatYouCan
|
||||||
|
)
|
||||||
|
|
||||||
|
# Update nodes
|
||||||
|
updated = dbsession.query(Node).filter(Node.user_id == delete_id).update(
|
||||||
|
{Node.user_id: keep_id}, synchronize_session=False
|
||||||
|
)
|
||||||
|
if updated:
|
||||||
|
print(f" Transferred {updated} nodes")
|
||||||
|
|
||||||
|
# Update votes
|
||||||
|
updated = dbsession.query(Vote).filter(Vote.user_id == delete_id).update(
|
||||||
|
{Vote.user_id: keep_id}, synchronize_session=False
|
||||||
|
)
|
||||||
|
if updated:
|
||||||
|
print(f" Transferred {updated} votes")
|
||||||
|
|
||||||
|
# Update watchers
|
||||||
|
updated = dbsession.query(Watcher).filter(Watcher.user_id == delete_id).update(
|
||||||
|
{Watcher.user_id: keep_id}, synchronize_session=False
|
||||||
|
)
|
||||||
|
if updated:
|
||||||
|
print(f" Transferred {updated} watchers")
|
||||||
|
|
||||||
|
# Update notifications
|
||||||
|
updated = dbsession.query(NodeEventNotification).filter(NodeEventNotification.user_id == delete_id).update(
|
||||||
|
{NodeEventNotification.user_id: keep_id}, synchronize_session=False
|
||||||
|
)
|
||||||
|
if updated:
|
||||||
|
print(f" Transferred {updated} notifications")
|
||||||
|
|
||||||
|
# Update namespace_user associations
|
||||||
|
updated = dbsession.query(NamespaceUser).filter(NamespaceUser.user_id == delete_id).update(
|
||||||
|
{NamespaceUser.user_id: keep_id}, synchronize_session=False
|
||||||
|
)
|
||||||
|
if updated:
|
||||||
|
print(f" Transferred {updated} namespace associations")
|
||||||
|
|
||||||
|
# Update namespace requests
|
||||||
|
updated = dbsession.query(NamespaceRequest).filter(NamespaceRequest.user_id == delete_id).update(
|
||||||
|
{NamespaceRequest.user_id: keep_id}, synchronize_session=False
|
||||||
|
)
|
||||||
|
if updated:
|
||||||
|
print(f" Transferred {updated} namespace requests")
|
||||||
|
|
||||||
|
# Update oauth records
|
||||||
|
updated = dbsession.query(Oauth).filter(Oauth.user_id == delete_id).update(
|
||||||
|
{Oauth.user_id: keep_id}, synchronize_session=False
|
||||||
|
)
|
||||||
|
if updated:
|
||||||
|
print(f" Transferred {updated} oauth records")
|
||||||
|
|
||||||
|
# Update payments
|
||||||
|
updated = dbsession.query(Payment).filter(Payment.user_id == delete_id).update(
|
||||||
|
{Payment.user_id: keep_id}, synchronize_session=False
|
||||||
|
)
|
||||||
|
if updated:
|
||||||
|
print(f" Transferred {updated} payments")
|
||||||
|
|
||||||
|
# Update pay_what_you_can (1-to-1, may need special handling)
|
||||||
|
existing_pwuc = dbsession.query(PayWhatYouCan).filter(PayWhatYouCan.user_id == keep_id).first()
|
||||||
|
delete_pwuc = dbsession.query(PayWhatYouCan).filter(PayWhatYouCan.user_id == delete_id).first()
|
||||||
|
if delete_pwuc:
|
||||||
|
if existing_pwuc:
|
||||||
|
# Keep user already has one, delete the duplicate's
|
||||||
|
dbsession.delete(delete_pwuc)
|
||||||
|
print(f" Deleted duplicate pay_what_you_can record")
|
||||||
|
else:
|
||||||
|
delete_pwuc.user_id = keep_id
|
||||||
|
print(f" Transferred pay_what_you_can record")
|
||||||
|
|
||||||
|
dbsession.flush()
|
||||||
|
|
||||||
|
# Delete the duplicate user
|
||||||
|
dbsession.delete(delete_user)
|
||||||
|
dbsession.flush()
|
||||||
|
print(f" Deleted user '{delete_user.name}'")
|
||||||
|
|
||||||
|
|
||||||
|
def main():
|
||||||
|
parser = get_arg_parser()
|
||||||
|
args = parser.parse_args()
|
||||||
|
setup_logging(args.config)
|
||||||
|
|
||||||
|
with bootstrap(args.config) as env, env["request"].tm as tm:
|
||||||
|
request = env["request"]
|
||||||
|
dbsession = request.dbsession
|
||||||
|
|
||||||
|
print("Searching for duplicate email users (case-insensitive)...")
|
||||||
|
duplicates = find_duplicate_email_users(dbsession)
|
||||||
|
|
||||||
|
if not duplicates:
|
||||||
|
print("No duplicate email users found.")
|
||||||
|
return
|
||||||
|
|
||||||
|
print(f"Found {len(duplicates)} email(s) with duplicate users:\n")
|
||||||
|
|
||||||
|
for email, users in duplicates.items():
|
||||||
|
# Sort by created timestamp (oldest first)
|
||||||
|
users_sorted = sorted(users, key=lambda u: u.created)
|
||||||
|
keep_user = users_sorted[0]
|
||||||
|
delete_users = users_sorted[1:]
|
||||||
|
|
||||||
|
print(f"Email: {email}")
|
||||||
|
print(f" Keeping: '{keep_user.name}' (id={keep_user.id}, created={keep_user.created})")
|
||||||
|
|
||||||
|
for delete_user in delete_users:
|
||||||
|
print(f" Deleting: '{delete_user.name}' (id={delete_user.id}, created={delete_user.created})")
|
||||||
|
merge_users(dbsession, keep_user, delete_user, dry_run=args.dry_run)
|
||||||
|
|
||||||
|
# Normalize the kept user's email to lowercase
|
||||||
|
if keep_user.email != email:
|
||||||
|
if args.dry_run:
|
||||||
|
print(f" Would normalize email from '{keep_user.email}' to '{email}'")
|
||||||
|
else:
|
||||||
|
keep_user.email = email
|
||||||
|
dbsession.add(keep_user)
|
||||||
|
dbsession.flush()
|
||||||
|
print(f" Normalized email to '{email}'")
|
||||||
|
|
||||||
|
print()
|
||||||
|
|
||||||
|
if args.dry_run:
|
||||||
|
print("DRY RUN - No changes were made. Run without --dry-run to apply changes.")
|
||||||
|
else:
|
||||||
|
print("Done. All duplicate users have been merged.")
|
||||||
|
|
@ -36,6 +36,18 @@ class TestUser(unittest.TestCase):
|
||||||
def test_email_set(self):
|
def test_email_set(self):
|
||||||
self.assertEqual(self.user.email, "russell@ballestrini.net")
|
self.assertEqual(self.user.email, "russell@ballestrini.net")
|
||||||
|
|
||||||
|
@mock.patch("remarkbox.models.user.is_user_name_available", mock_always_true)
|
||||||
|
def test_email_normalized_to_lowercase(self):
|
||||||
|
"""Emails should be stored lowercase to prevent case-sensitive duplicates."""
|
||||||
|
user = User("Russell@Ballestrini.NET")
|
||||||
|
self.assertEqual(user.email, "russell@ballestrini.net")
|
||||||
|
|
||||||
|
@mock.patch("remarkbox.models.user.is_user_name_available", mock_always_true)
|
||||||
|
def test_email_mixed_case_normalized(self):
|
||||||
|
"""Mixed case emails should be normalized to lowercase."""
|
||||||
|
user = User("Grop3r@Protonmail.com")
|
||||||
|
self.assertEqual(user.email, "grop3r@protonmail.com")
|
||||||
|
|
||||||
def test_new_password(self):
|
def test_new_password(self):
|
||||||
raw_password = self.user.new_password()
|
raw_password = self.user.new_password()
|
||||||
self.assertEqual(len(raw_password), 6)
|
self.assertEqual(len(raw_password), 6)
|
||||||
|
|
|
||||||
|
|
@ -746,3 +746,105 @@ class AnonymousCommentingFunctionalTests(FunctionalTests):
|
||||||
|
|
||||||
self.dbsession.expire(ns)
|
self.dbsession.expire(ns)
|
||||||
self.assertTrue(ns.allow_anonymous)
|
self.assertTrue(ns.allow_anonymous)
|
||||||
|
|
||||||
|
|
||||||
|
class EmailCaseInsensitivityTests(FunctionalTests):
|
||||||
|
"""
|
||||||
|
Tests to prevent duplicate accounts from case-insensitive email addresses.
|
||||||
|
|
||||||
|
Regression test for issue where users 'G' and 'Groupr' both had emails
|
||||||
|
'grop3r@protonmail.com' and 'Grop3r@protonmail.com' causing duplicate notifications.
|
||||||
|
"""
|
||||||
|
|
||||||
|
@classmethod
|
||||||
|
def setUpClass(cls):
|
||||||
|
try:
|
||||||
|
FunctionalTests.setUpClass.im_func(cls)
|
||||||
|
except AttributeError:
|
||||||
|
FunctionalTests.setUpClass.__func__(cls)
|
||||||
|
|
||||||
|
def tearDown(self):
|
||||||
|
super(EmailCaseInsensitivityTests, self).tearDown()
|
||||||
|
# Clean up any test users created
|
||||||
|
for email in ["casetest@example.com", "CASETEST@EXAMPLE.COM", "CaseTest@Example.com"]:
|
||||||
|
user = get_user_by_email(self.dbsession, email)
|
||||||
|
if user:
|
||||||
|
self.dbsession.delete(user)
|
||||||
|
self.dbsession.flush()
|
||||||
|
self.tm.commit()
|
||||||
|
|
||||||
|
def test_get_user_by_email_case_insensitive(self):
|
||||||
|
"""get_user_by_email should find users regardless of email case."""
|
||||||
|
# Create user with lowercase email
|
||||||
|
user = get_or_create_user_by_email(self.dbsession, "casetest@example.com")
|
||||||
|
self.dbsession.add(user)
|
||||||
|
self.dbsession.flush()
|
||||||
|
self.tm.commit()
|
||||||
|
|
||||||
|
# Should find with different cases
|
||||||
|
found_lower = get_user_by_email(self.dbsession, "casetest@example.com")
|
||||||
|
found_upper = get_user_by_email(self.dbsession, "CASETEST@EXAMPLE.COM")
|
||||||
|
found_mixed = get_user_by_email(self.dbsession, "CaseTest@Example.com")
|
||||||
|
|
||||||
|
self.assertIsNotNone(found_lower)
|
||||||
|
self.assertIsNotNone(found_upper)
|
||||||
|
self.assertIsNotNone(found_mixed)
|
||||||
|
self.assertEqual(found_lower.id, found_upper.id)
|
||||||
|
self.assertEqual(found_lower.id, found_mixed.id)
|
||||||
|
|
||||||
|
def test_get_or_create_returns_existing_regardless_of_case(self):
|
||||||
|
"""get_or_create_user_by_email should return existing user regardless of email case."""
|
||||||
|
# Create user with lowercase email
|
||||||
|
user1 = get_or_create_user_by_email(self.dbsession, "casetest@example.com")
|
||||||
|
self.dbsession.add(user1)
|
||||||
|
self.dbsession.flush()
|
||||||
|
user1_id = user1.id
|
||||||
|
self.tm.commit()
|
||||||
|
|
||||||
|
# Try to get/create with uppercase - should return same user
|
||||||
|
user2 = get_or_create_user_by_email(self.dbsession, "CASETEST@EXAMPLE.COM")
|
||||||
|
self.assertEqual(user1_id, user2.id)
|
||||||
|
|
||||||
|
# Try to get/create with mixed case - should return same user
|
||||||
|
user3 = get_or_create_user_by_email(self.dbsession, "CaseTest@Example.com")
|
||||||
|
self.assertEqual(user1_id, user3.id)
|
||||||
|
|
||||||
|
def test_new_user_email_stored_lowercase(self):
|
||||||
|
"""New user emails should be normalized to lowercase."""
|
||||||
|
user = get_or_create_user_by_email(self.dbsession, "CASETEST@EXAMPLE.COM")
|
||||||
|
self.dbsession.add(user)
|
||||||
|
self.dbsession.flush()
|
||||||
|
user_id = user.id
|
||||||
|
self.tm.commit()
|
||||||
|
|
||||||
|
# Requery to avoid detached instance error
|
||||||
|
user = get_user_by_email(self.dbsession, "casetest@example.com")
|
||||||
|
# Email should be stored lowercase
|
||||||
|
self.assertEqual(user.email, "casetest@example.com")
|
||||||
|
|
||||||
|
def test_no_duplicate_accounts_from_case_variations(self):
|
||||||
|
"""Ensure case variations don't create duplicate accounts."""
|
||||||
|
# Create first user
|
||||||
|
user1 = get_or_create_user_by_email(self.dbsession, "casetest@example.com")
|
||||||
|
self.dbsession.add(user1)
|
||||||
|
self.dbsession.flush()
|
||||||
|
user1_id = user1.id
|
||||||
|
self.tm.commit()
|
||||||
|
|
||||||
|
# Attempt to create with different cases - should all return same user
|
||||||
|
user2 = get_or_create_user_by_email(self.dbsession, "CASETEST@EXAMPLE.COM")
|
||||||
|
user3 = get_or_create_user_by_email(self.dbsession, "CaseTest@Example.com")
|
||||||
|
user4 = get_or_create_user_by_email(self.dbsession, "casetest@EXAMPLE.COM")
|
||||||
|
|
||||||
|
# All should be the same user
|
||||||
|
self.assertEqual(user1_id, user2.id)
|
||||||
|
self.assertEqual(user1_id, user3.id)
|
||||||
|
self.assertEqual(user1_id, user4.id)
|
||||||
|
|
||||||
|
# Verify only one user exists with this email (case-insensitive)
|
||||||
|
from remarkbox.models.user import User
|
||||||
|
from sqlalchemy import func
|
||||||
|
count = self.dbsession.query(User).filter(
|
||||||
|
func.lower(User.email) == "casetest@example.com"
|
||||||
|
).count()
|
||||||
|
self.assertEqual(count, 1)
|
||||||
|
|
|
||||||
1
setup.py
1
setup.py
|
|
@ -67,6 +67,7 @@ setup(
|
||||||
"remarkbox_json_import = remarkbox.scripts.json_import:main",
|
"remarkbox_json_import = remarkbox.scripts.json_import:main",
|
||||||
"remarkbox_json_import2 = remarkbox.scripts.json_import2:main",
|
"remarkbox_json_import2 = remarkbox.scripts.json_import2:main",
|
||||||
"remarkbox_merge_dupes = remarkbox.scripts.merge_dupes:main",
|
"remarkbox_merge_dupes = remarkbox.scripts.merge_dupes:main",
|
||||||
|
"remarkbox_merge_duplicate_email_users = remarkbox.scripts.merge_duplicate_email_users:main",
|
||||||
"remarkbox_invalidate_node_cache = remarkbox.scripts.invalidate_node_cache:main",
|
"remarkbox_invalidate_node_cache = remarkbox.scripts.invalidate_node_cache:main",
|
||||||
"remarkbox_recompute_node_depths = remarkbox.scripts.recompute_node_depths:main",
|
"remarkbox_recompute_node_depths = remarkbox.scripts.recompute_node_depths:main",
|
||||||
"remarkbox_safe_approve_all_nodes = remarkbox.scripts.safe_approve_all_nodes:main",
|
"remarkbox_safe_approve_all_nodes = remarkbox.scripts.safe_approve_all_nodes:main",
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue