fix/user-roles: add migration script to backfill user role details #279
Conversation
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
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughAdds a Django management command that iterates all teams to backfill and correct user roles per team. It assigns OWNER/ADMIN/MEMBER to the team owner (excluding MODERATOR), ensures only MEMBER for other members, deactivates incorrect roles, and outputs counters for ensured and deactivated roles. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Operator as CLI Operator
participant Django as Django mgmt command
participant TeamRepo as TeamRepository
participant UTDRepo as UserTeamDetailsRepository
participant RoleRepo as UserRoleRepository
Operator->>Django: python manage.py migrate_add_roles_to_teams
Django->>TeamRepo: list_all_teams()
TeamRepo-->>Django: teams
loop for each team
Django->>UTDRepo: get_team_owner(team)
UTDRepo-->>Django: owner
rect rgba(200,240,255,0.3)
note right of Django: Ensure owner roles (OWNER, ADMIN, MEMBER)<br/>Exclude MODERATOR
Django->>RoleRepo: ensure_role(owner, team, OWNER/ADMIN/MEMBER)
end
Django->>UTDRepo: list_team_members(team)
UTDRepo-->>Django: members
loop for each member
alt member is owner
note right of Django: Already handled
else other member
Django->>RoleRepo: get_roles(member, team)
RoleRepo-->>Django: roles
rect rgba(230,255,230,0.4)
note right of Django: Deactivate any non-MEMBER roles<br/>Ensure MEMBER exists
Django->>RoleRepo: deactivate_roles(!= MEMBER)
Django->>RoleRepo: ensure_role(member, team, MEMBER)
end
end
end
end
Django-->>Operator: Print roles_ensured and roles_deactivated totals
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
|---|---|---|
 |
Inefficient individual role processing ▹ view | ✅ Fix detected |
 |
Inaccurate role assignment counter ▹ view | ✅ Fix detected |
|
N+1 database query pattern ▹ view | ✅ Fix detected |
|
Unchecked MEMBER role assignment result ▹ view | ✅ Fix detected |
 |
Negative condition complexity ▹ view | ✅ Fix detected |
 |
Insufficient context in command help message ▹ view | ✅ Fix detected |
 |
Inflexible role assignment logic ▹ view | ✅ Fix detected |
 |
Excessive role assignment to team owners ▹ view | ✅ Fix detected |
 |
Insufficient intermediate logging ▹ view | ✅ Fix detected |
Files scanned
| File Path | Reviewed |
|---|---|
| todo/management/commands/migrate_add_roles_to_teams.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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
todo/management/commands/migrate_add_roles_to_teams.py (1)
1-44: Repo/methods present — fix enum/string usage, duplicate class, and owner-type compare before prod
- Confirmed: UserRoleRepository and methods exist (todo/repositories/user_role_repository.py — get_user_roles @67, assign_role @113, remove_role_by_id @121) and TeamRepository + UserTeamDetailsRepository are defined in todo/repositories/team_repository.py (class @126, get_by_team_id @262).
- Duplicate class: UserTeamDetailsRepository is also defined in todo/repositories/user_team_details_repository.py (@6) — ensure the import from team_repository is intentional.
- Type mismatch: migrate_add_roles_to_teams.py passes RoleName.value and RoleScope.TEAM.value (strings) to repo methods that are type-hinted for RoleName/RoleScope (see user_role_repository.py @113). Pass enums (RoleName/RoleScope) or make the repo accept strings. (migrate_add_roles_to_teams.py calls at ~lines 24–26 and 37–40.)
- Owner comparison: the script uses team["created_by"] compared to str(member.user_id); created_by is a PyObjectId in todo/models/team.py (@31). Normalize types (e.g., str(team["created_by"])) before comparing.
Fix the above before running in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
todo/management/commands/migrate_add_roles_to_teams.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
todo/management/commands/migrate_add_roles_to_teams.py (3)
todo/repositories/user_role_repository.py (1)
UserRoleRepository(14-165)todo/repositories/team_repository.py (1)
TeamRepository(11-123)todo/constants/role.py (2)
RoleScope(4-6)RoleName(9-13)
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
72
to
115
| if roles_to_assign: | ||
| result = UserRoleRepository.get_collection().insert_many(roles_to_assign) | ||
| roles_ensured = len(result.inserted_ids) | ||
| self.stdout.write(self.style.SUCCESS(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 | ||
| self.stdout.write(self.style.SUCCESS(f"Successfully deactivated {roles_deactivated} roles.")) | ||
| if postgres_operations: | ||
| self.stdout.write(self.style.WARNING(f"\nStarting sync of {len(postgres_operations)} operations to PostgreSQL...")) | ||
| success = dual_write_service.batch_operations(postgres_operations) | ||
| if success: | ||
| self.stdout.write(self.style.SUCCESS("PostgreSQL sync completed successfully.")) | ||
| else: | ||
| self.stdout.write(self.style.ERROR("PostgreSQL sync encountered errors. Check logs for details.")) |
Contributor
There was a problem hiding this comment.
Data consistency risk: MongoDB operations complete first (lines 73-108), then PostgreSQL sync happens (line 111). If the PostgreSQL sync fails (success = False), the databases will be in an inconsistent state with no rollback mechanism. This violates data integrity in a dual-write system. Consider implementing transaction-like behavior with rollback capability, or at minimum, fail the entire operation if PostgreSQL sync fails instead of just logging an error.
Suggested change
| if roles_to_assign: | |
| result = UserRoleRepository.get_collection().insert_many(roles_to_assign) | |
| roles_ensured = len(result.inserted_ids) | |
| self.stdout.write(self.style.SUCCESS(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 | |
| self.stdout.write(self.style.SUCCESS(f"Successfully deactivated {roles_deactivated} roles.")) | |
| if postgres_operations: | |
| self.stdout.write(self.style.WARNING(f"\nStarting sync of {len(postgres_operations)} operations to PostgreSQL...")) | |
| success = dual_write_service.batch_operations(postgres_operations) | |
| if success: | |
| self.stdout.write(self.style.SUCCESS("PostgreSQL sync completed successfully.")) | |
| else: | |
| self.stdout.write(self.style.ERROR("PostgreSQL sync encountered errors. Check logs for details.")) | |
| if roles_to_assign: | |
| # Store MongoDB operations to allow rollback if PostgreSQL sync fails | |
| mongo_operations_performed = False | |
| inserted_ids = [] | |
| try: | |
| result = UserRoleRepository.get_collection().insert_many(roles_to_assign) | |
| inserted_ids = result.inserted_ids | |
| roles_ensured = len(inserted_ids) | |
| mongo_operations_performed = True | |
| self.stdout.write(self.style.SUCCESS(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 | |
| }) | |
| except Exception as e: | |
| self.stdout.write(self.style.ERROR(f"Failed to insert roles in MongoDB: {str(e)}")) | |
| raise | |
| if roles_to_remove: | |
| role_ids_to_remove = [] | |
| try: | |
| 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 = [role["mongo_id"] for role 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 | |
| self.stdout.write(self.style.SUCCESS(f"Successfully deactivated {roles_deactivated} roles.")) | |
| except Exception as e: | |
| self.stdout.write(self.style.ERROR(f"Failed to deactivate roles in MongoDB: {str(e)}")) | |
| raise | |
| if postgres_operations: | |
| self.stdout.write(self.style.WARNING(f"\nStarting sync of {len(postgres_operations)} operations to PostgreSQL...")) | |
| success = dual_write_service.batch_operations(postgres_operations) | |
| if success: | |
| self.stdout.write(self.style.SUCCESS("PostgreSQL sync completed successfully.")) | |
| else: | |
| # Attempt to rollback MongoDB changes if PostgreSQL sync fails | |
| if mongo_operations_performed and inserted_ids: | |
| try: | |
| self.stdout.write(self.style.WARNING("PostgreSQL sync failed. Attempting to rollback MongoDB changes...")) | |
| UserRoleRepository.get_collection().delete_many({"_id": {"$in": inserted_ids}}) | |
| self.stdout.write(self.style.SUCCESS("MongoDB rollback completed.")) | |
| except Exception as rollback_error: | |
| self.stdout.write(self.style.ERROR(f"MongoDB rollback failed: {str(rollback_error)}")) | |
| if role_ids_to_remove: | |
| try: | |
| UserRoleRepository.get_collection().update_many( | |
| {"_id": {"$in": role_ids_to_remove}}, | |
| {"$set": {"is_active": True}} | |
| ) | |
| self.stdout.write(self.style.SUCCESS("MongoDB role deactivation rollback completed.")) | |
| except Exception as rollback_error: | |
| self.stdout.write(self.style.ERROR(f"MongoDB role deactivation rollback failed: {str(rollback_error)}")) | |
| self.stdout.write(self.style.ERROR("PostgreSQL sync failed. MongoDB changes have been rolled back.")) | |
| raise CommandError("Failed to sync changes to PostgreSQL. Operation aborted.") |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| }, | ||
| } | ||
| ) | ||
| role_ids_to_remove = [roles["mongo_id"] for roles in roles_to_remove] |
Contributor
There was a problem hiding this comment.
The variable name in the list comprehension doesn't match the iteration variable. The line should be:
role_ids_to_remove = [role["mongo_id"] for role in roles_to_remove]Currently it uses roles as the iteration variable but then tries to access roles["mongo_id"], which would cause an error since roles is a single dictionary from the list, not the entire list itself.
Suggested change
| role_ids_to_remove = [roles["mongo_id"] for roles in roles_to_remove] | |
| role_ids_to_remove = [role["mongo_id"] for role in roles_to_remove] |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
- ignore migration script in test env
…010/todo-backend into fix/backfill-user-roles
10 tasks
iamitprakash
added a commit
that referenced
this pull request
Sep 23, 2025
Merge pull request #279 from Hariom01010/fix/backfill-user-roles
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Date: 22 Sep 2025
Developer Name: @Hariom01010
Issue Ticket Number
Closes #278
Description
A migration script is written to backfill user role data for teams.
To run the script, connect to the backend server and enter the following command:
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Before running script for a team whose member don't have roles:
After running script:
Test Coverage
Screenshot 1
Additional Notes
Description by Korbit AI
What change is being made?
Add a new Django migration (0004_backfill_user_role_data.py) that backfills and synchronizes user role data across team members by ensuring proper role assignments (OWNER/ADMIN/MEMBER) for team owners, ensuring MEMBER role for members, deactivating non-MEMBER roles, and syncing changes to PostgreSQL via a dual-write service.
Why are these changes being made?
Restore and enforce consistent user role data across teams, and keep MongoDB and PostgreSQL representations in sync during backfill. The script is skipped in test environments to avoid interference.