Skip to content
Merged
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
138 changes: 138 additions & 0 deletions todo/migrations/0004_backfill_user_role_data.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
# Generated by Django 5.1.5 on 2025-09-23 07:25

from django.db import migrations
import logging
import sys
from todo.repositories.user_role_repository import UserRoleRepository
from todo.repositories.team_repository import TeamRepository, UserTeamDetailsRepository
from todo.constants.role import RoleScope, RoleName
from datetime import datetime, timezone
from todo.services.enhanced_dual_write_service import EnhancedDualWriteService

logger = logging.getLogger(__name__)


def fix_team_roles(apps, schema_editor):
Copy link

Choose a reason for hiding this comment

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

Unclear unused migration parameters category Readability

Tell me more
What is the issue?

The function parameters 'apps' and 'schema_editor' are unused but required by Django's migration system. This isn't immediately clear to readers.

Why this matters

Developers might wonder why these parameters exist or try to use them, leading to confusion about the migration's implementation.

Suggested change ∙ Feature Preview
def fix_team_roles(apps, schema_editor):  # Django migration parameters - unused
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

logger.info("\n--- Starting Team Roles Fix Script ---")
if "test" in sys.argv:
logger.info("\n--- Skipping Team Roles Fix Script in test environment ---")
return
teams_data = TeamRepository.get_collection().find({"is_deleted": False})
roles_scope = RoleScope.TEAM.value

roles_ensured = 0
roles_deactivated = 0
dual_write_service = EnhancedDualWriteService()

roles_to_assign = []
roles_to_remove = []
postgres_operations = []

for team in teams_data:
team_id = str(team["_id"])
team_owner = team["created_by"]
team_members = list(UserTeamDetailsRepository.get_by_team_id(team_id))
for member in team_members:
member_id = str(member.user_id)
if member_id == team_owner:
member_roles = UserRoleRepository.get_user_roles(member_id, roles_scope, team_id)
existing_roles = [role.role_name for role in member_roles]
for role in (RoleName.OWNER.value, RoleName.ADMIN.value, RoleName.MEMBER.value):
Copy link

Choose a reason for hiding this comment

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

Implicit role hierarchy category Readability

Tell me more
What is the issue?

The role hierarchy tuple is hardcoded within the loop, making it difficult to understand the relationship between these roles at a glance.

Why this matters

Future maintainers might miss the significance of this role ordering or struggle to modify the role hierarchy.

Suggested change ∙ Feature Preview
TEAM_OWNER_ROLES = (RoleName.OWNER.value, RoleName.ADMIN.value, RoleName.MEMBER.value)

# Later in the code
for role in TEAM_OWNER_ROLES:
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

if role not in existing_roles:
roles_to_assign.append(
{
"user_id": member_id,
"role_name": role,
"scope": roles_scope,
"team_id": team_id,
"is_active": True,
"created_at": datetime.now(timezone.utc),
"created_by": "system",
}
)
else:
member_roles = UserRoleRepository.get_user_roles(member_id, roles_scope, team_id)
has_member_role = False
for role in member_roles:
if role.role_name == RoleName.MEMBER.value:
has_member_role = True
else:
roles_to_remove.append(
Comment on lines +56 to +60
Copy link

Choose a reason for hiding this comment

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

Incorrect role removal logic for team owners category Functionality

Tell me more
What is the issue?

The logic incorrectly marks all non-member roles for removal, including active owner/admin roles that should be preserved for team owners.

Why this matters

Team owners will lose their owner and admin roles, breaking the team hierarchy and potentially causing authorization failures when owners try to perform administrative actions.

Suggested change ∙ Feature Preview

Only mark roles for removal for non-owners, and preserve owner/admin roles for team owners:

for role in member_roles:
    if role.role_name == RoleName.MEMBER.value:
        has_member_role = True
    elif member_id != team_owner:  # Only remove non-member roles for non-owners
        roles_to_remove.append(...)
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

{
"mongo_id": role.id,
"user_id": member_id,
"role_name": role.role_name,
"scope": roles_scope,
"team_id": role.team_id,
"is_active": role.is_active,
"created_by": role.created_by,
"created_at": role.created_at,
}
)
if not has_member_role:
roles_to_assign.append(
{
"user_id": member_id,
"role_name": RoleName.MEMBER.value,
"scope": roles_scope,
"team_id": team_id,
"is_active": True,
"created_at": datetime.now(timezone.utc),
"created_by": "system",
}
)
Comment on lines +20 to +83
Copy link

Choose a reason for hiding this comment

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

N+1 Query Problem in Role Migration category Performance

Tell me more
What is the issue?

The migration performs N+1 database queries by calling UserRoleRepository.get_user_roles() inside nested loops for each team member, resulting in excessive database round trips.

Why this matters

This creates a performance bottleneck that scales poorly with the number of teams and members, potentially causing migration timeouts or database connection exhaustion in production environments with large datasets.

Suggested change ∙ Feature Preview

Batch load all user roles upfront using a single query or implement bulk operations. For example:

# Load all relevant user roles at once
all_user_ids = set()
for team in teams_data:
    team_members = list(UserTeamDetailsRepository.get_by_team_id(str(team["_id"])))
    all_user_ids.update(str(member.user_id) for member in team_members)

# Single query to get all roles
all_roles = UserRoleRepository.get_roles_for_users(list(all_user_ids), roles_scope)
roles_by_user_team = defaultdict(list)
for role in all_roles:
    roles_by_user_team[(role.user_id, role.team_id)].append(role)
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

if roles_to_assign:
result = UserRoleRepository.get_collection().insert_many(roles_to_assign)
roles_ensured = len(result.inserted_ids)
logger.info(f"Successfully inserted {roles_ensured} new roles.")

for role_data, mongo_id in zip(roles_to_assign, result.inserted_ids):
postgres_operations.append(
{
"operation": "create",
"collection_name": "user_roles",
"mongo_id": str(mongo_id),
"data": role_data,
}
)

if roles_to_remove:
for role in roles_to_remove:
mongo_id = str(role["mongo_id"])
postgres_operations.append(
{
"operation": "update",
"collection_name": "user_roles",
"mongo_id": mongo_id,
"data": {
"user_id": role["user_id"],
"role_name": role["role_name"],
"scope": role["scope"],
"team_id": role["team_id"],
"is_active": False,
"created_at": role["created_at"],
"created_by": role["created_by"],
},
}
)
role_ids_to_remove = [roles["mongo_id"] for roles in roles_to_remove]
result = UserRoleRepository.get_collection().update_many(
{"_id": {"$in": role_ids_to_remove}}, {"$set": {"is_active": False}}
)
roles_deactivated = result.modified_count
logger.info(f"Successfully deactivated {roles_deactivated} roles.")
if postgres_operations:
logger.info(f"\nStarting sync of {len(postgres_operations)} operations to PostgreSQL...")
success = dual_write_service.batch_operations(postgres_operations)
if success:
logger.info("PostgreSQL sync completed successfully.")
else:
logger.warning("PostgreSQL sync encountered errors. Check logs for details.")
Copy link

Choose a reason for hiding this comment

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

Insufficient error details in log message category Logging

Tell me more
What is the issue?

The log message doesn't provide specific error information, making it difficult to diagnose issues without checking additional logs.

Why this matters

Lack of specific error information in the log message may lead to increased time spent on debugging and reduced ability to quickly identify and resolve issues.

Suggested change ∙ Feature Preview
logger.warning(f"[fix_team_roles] PostgreSQL sync encountered errors: {str(e)}. Check logs for details.")
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Comment on lines +84 to +130
Copy link
Contributor

Choose a reason for hiding this comment

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

Transaction consistency bug: The migration performs separate MongoDB operations (insert_many, update_many) and PostgreSQL sync without transactional guarantees. If any operation fails partially, the databases will be in an inconsistent state. For example, if PostgreSQL sync fails after MongoDB changes succeed, the dual-write system will be out of sync. Fix by implementing proper transaction handling or rollback mechanisms for failed operations.

Suggested change
if roles_to_assign:
result = UserRoleRepository.get_collection().insert_many(roles_to_assign)
roles_ensured = len(result.inserted_ids)
logger.info(f"Successfully inserted {roles_ensured} new roles.")
for role_data, mongo_id in zip(roles_to_assign, result.inserted_ids):
postgres_operations.append(
{
"operation": "create",
"collection_name": "user_roles",
"mongo_id": str(mongo_id),
"data": role_data,
}
)
if roles_to_remove:
for role in roles_to_remove:
mongo_id = str(role["mongo_id"])
postgres_operations.append(
{
"operation": "update",
"collection_name": "user_roles",
"mongo_id": mongo_id,
"data": {
"user_id": role["user_id"],
"role_name": role["role_name"],
"scope": role["scope"],
"team_id": role["team_id"],
"is_active": False,
"created_at": role["created_at"],
"created_by": role["created_by"],
},
}
)
role_ids_to_remove = [roles["mongo_id"] for roles in roles_to_remove]
result = UserRoleRepository.get_collection().update_many(
{"_id": {"$in": role_ids_to_remove}}, {"$set": {"is_active": False}}
)
roles_deactivated = result.modified_count
logger.info(f"Successfully deactivated {roles_deactivated} roles.")
if postgres_operations:
logger.info(f"\nStarting sync of {len(postgres_operations)} operations to PostgreSQL...")
success = dual_write_service.batch_operations(postgres_operations)
if success:
logger.info("PostgreSQL sync completed successfully.")
else:
logger.warning("PostgreSQL sync encountered errors. Check logs for details.")
if roles_to_assign or roles_to_remove:
# Prepare operations but don't execute them yet
mongo_operations = []
postgres_operations = []
# Prepare MongoDB and PostgreSQL operations for roles to assign
if roles_to_assign:
mongo_operations.append(("insert", roles_to_assign))
logger.info(f"Preparing to insert {len(roles_to_assign)} new roles.")
# Prepare MongoDB and PostgreSQL operations for roles to remove
if roles_to_remove:
role_ids_to_remove = [role["mongo_id"] for role in roles_to_remove]
mongo_operations.append(("update", role_ids_to_remove))
logger.info(f"Preparing to deactivate {len(roles_to_remove)} roles.")
for role in roles_to_remove:
mongo_id = str(role["mongo_id"])
postgres_operations.append(
{
"operation": "update",
"collection_name": "user_roles",
"mongo_id": mongo_id,
"data": {
"user_id": role["user_id"],
"role_name": role["role_name"],
"scope": role["scope"],
"team_id": role["team_id"],
"is_active": False,
"created_at": role["created_at"],
"created_by": role["created_by"],
},
}
)
# Execute MongoDB operations with rollback capability
mongo_results = []
try:
# Execute MongoDB operations
for op_type, data in mongo_operations:
if op_type == "insert" and data:
result = UserRoleRepository.get_collection().insert_many(data)
roles_ensured = len(result.inserted_ids)
logger.info(f"Successfully inserted {roles_ensured} new roles.")
mongo_results.append(("insert", result.inserted_ids))
# Add PostgreSQL operations for inserted roles
for role_data, mongo_id in zip(data, result.inserted_ids):
postgres_operations.append(
{
"operation": "create",
"collection_name": "user_roles",
"mongo_id": str(mongo_id),
"data": role_data,
}
)
elif op_type == "update" and data:
result = UserRoleRepository.get_collection().update_many(
{"_id": {"$in": data}}, {"$set": {"is_active": False}}
)
roles_deactivated = result.modified_count
logger.info(f"Successfully deactivated {roles_deactivated} roles.")
mongo_results.append(("update", data))
# Execute PostgreSQL operations
if postgres_operations:
logger.info(f"Starting sync of {len(postgres_operations)} operations to PostgreSQL...")
success = dual_write_service.batch_operations(postgres_operations)
if not success:
raise Exception("PostgreSQL sync failed")
logger.info("PostgreSQL sync completed successfully.")
except Exception as e:
# Rollback MongoDB changes if PostgreSQL sync fails
logger.error(f"Error during migration: {str(e)}")
logger.warning("Rolling back MongoDB changes...")
for op_type, ids in mongo_results:
if op_type == "insert":
# Delete inserted documents
UserRoleRepository.get_collection().delete_many({"_id": {"$in": ids}})
logger.info(f"Rolled back {len(ids)} inserted roles.")
elif op_type == "update":
# Reactivate deactivated roles
UserRoleRepository.get_collection().update_many(
{"_id": {"$in": ids}}, {"$set": {"is_active": True}}
)
logger.info(f"Rolled back {len(ids)} deactivated roles.")
raise Exception(f"Migration failed and was rolled back: {str(e)}")

Spotted by Diamond

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.



class Migration(migrations.Migration):
dependencies = [
("todo", "0003_alter_postgresuserrole_unique_together_and_more"),
]

operations = [migrations.RunPython(fix_team_roles, migrations.RunPython.noop)]