Merge pull request #279 from Hariom01010/fix/backfill-user-roles#280
Merge pull request #279 from Hariom01010/fix/backfill-user-roles#280iamitprakash merged 1 commit intomainfrom
Conversation
fix/user-roles: add migration script to backfill user role details
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests
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 |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Unclear unused migration parameters ▹ view | |
 |
N+1 Query Problem in Role Migration ▹ view | |
 |
Insufficient error details in log message ▹ view | |
|
Implicit role hierarchy ▹ view | |
 |
Incorrect role removal logic for team owners ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| todo/migrations/0004_backfill_user_role_data.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| 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): | ||
| 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( | ||
| { | ||
| "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", | ||
| } | ||
| ) |
There was a problem hiding this comment.
N+1 Query Problem in Role Migration 
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
| if success: | ||
| logger.info("PostgreSQL sync completed successfully.") | ||
| else: | ||
| logger.warning("PostgreSQL sync encountered errors. Check logs for details.") |
There was a problem hiding this comment.
Insufficient error details in log message 
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def fix_team_roles(apps, schema_editor): |
There was a problem hiding this comment.
Unclear unused migration parameters 
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 - unusedProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| 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): |
There was a problem hiding this comment.
Implicit role hierarchy
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
| for role in member_roles: | ||
| if role.role_name == RoleName.MEMBER.value: | ||
| has_member_role = True | ||
| else: | ||
| roles_to_remove.append( |
There was a problem hiding this comment.
Incorrect role removal logic for team owners 
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| 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.") |
There was a problem hiding this comment.
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.
| 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
Is this helpful? React 👍 or 👎 to let us know.
2 participants
fix/user-roles: add migration script to backfill user role details
Date: 23 Sep 2025
Developer Name: @Hariom01010
Issue Ticket Number
PR going in main: #279
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes
Description by Korbit AI
What change is being made?
Backfill and sync user roles across teams by inserting missing roles (OWNER, ADMIN, MEMBER) for team owners and ensuring members have at least MEMBER role, while deactivating non-member roles; add dual-write synchronization to PostgreSQL.
Why are these changes being made?
Ensure consistent role data across MongoDB and PostgreSQL, and correct missing/incorrect roles after migrations; skip in test environment to avoid side effects.