Assign and un-assign issue via GitHub API #3056
Conversation
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds GitHub client integration and error-message constants to module mutations; synchronizes assignees with GitHub when assigning/unassigning issues, creates/updates per-assignee Tasks (including new CLOSED status), manages per-assignee deadlines, and tightens module/issue/mentor validations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/apps/mentorship/api/internal/mutations/module.py (2)
168-179: Avoid catching bareException.The generic
Exceptionhandler (lines 176-179) is overly broad and could mask unexpected errors likeAttributeErrororKeyErrorthat indicate bugs. Consider removing it or limiting to specific expected exceptions.🔎 Proposed fix
try: gh_client = get_github_client() gh_repository = gh_client.get_repo(issue.repository.path) gh_issue = gh_repository.get_issue(number=issue.number) gh_issue.add_to_assignees(user_login) except GithubException as e: raise ValidationError(message=f"Failed to assign issue on GitHub: {e}") from e - - except Exception as e: - raise ValidationError( - message=f"Unexpected error while assigning issue on GitHub: {e}" - ) from e
256-267: Same broadExceptioncatch as noted above.Consider removing the generic
Exceptionhandler for consistency and to avoid masking bugs.🔎 Proposed fix
try: gh_client = get_github_client() gh_repository = gh_client.get_repo(issue.repository.path) gh_issue = gh_repository.get_issue(number=issue.number) gh_issue.remove_from_assignees(user_login) except GithubException as e: raise ValidationError(message=f"Failed to unassign issue on GitHub: {e}") from e - - except Exception as e: - raise ValidationError( - message=f"Unexpected error while unassigning issue on GitHub: {e}" - ) from e
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/mentorship/api/internal/mutations/module.pybackend/apps/mentorship/models/task.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
📚 Learning: 2025-07-16T13:49:58.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/module.py:28-29
Timestamp: 2025-07-16T13:49:58.648Z
Learning: In the OWASP Nest mentorship system, mentors can be created with only github_user initially (without nest_user) when assigned to modules. This allows mentors to be assigned before they've signed into the system. When these users eventually sign in, the nest_user is automatically linked to the existing github_user, creating a complete mentor profile. This design provides flexibility in mentor assignment workflows.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
🧬 Code graph analysis (1)
backend/apps/mentorship/api/internal/mutations/module.py (9)
backend/apps/mentorship/api/internal/nodes/module.py (3)
CreateModuleInput(163-177)UpdateModuleInput(181-196)issues(76-87)backend/apps/mentorship/models/mentee.py (1)
Mentee(11-49)backend/apps/mentorship/models/mentee_module.py (1)
MenteeModule(11-45)backend/apps/mentorship/models/module.py (2)
Module(17-99)save(92-99)backend/apps/mentorship/models/program.py (2)
Program(18-87)save(83-87)backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-36)backend/apps/mentorship/models/task.py (1)
Task(10-76)backend/apps/github/models/repository.py (1)
path(157-159)backend/apps/github/api/internal/nodes/issue.py (1)
assignees(45-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (7)
backend/apps/mentorship/models/task.py (1)
26-26: LGTM!The new
CLOSEDstatus is a valid addition that supports the task lifecycle when issues are unassigned. The value fits within themax_length=20constraint.backend/apps/mentorship/api/internal/mutations/module.py (6)
10-29: LGTM!Good use of centralized error message constants and appropriate imports for GitHub API integration.
234-238: Verify permission asymmetry between assign and unassign.
assign_issue_to_userrequires only mentor status (lines 149-151), whileunassign_issue_from_useradditionally requires program admin status. If this is intentional, consider adding a comment explaining the rationale.
289-311: LGTM!Good use of centralized error message constants.
349-371: LGTM!Consistent use of centralized error message constants.
401-402: LGTM!Consistent use of centralized error message constant.
183-192: LGTM!The
MenteeandMenteeModulecreation with appropriate defaults handles the case wheremodule.started_atis None. Based on learnings, mentors can be created with onlygithub_userinitially, and this pattern correctly applies to mentees as well.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/mentorship/api/internal/mutations/module.py (1)
310-319: Align task creation behavior across the mentorship system.The code creates a Task for each assignee on an issue (lines 310-317), but this contradicts the documented system design. The
mentorship_sync_module_issuescommand intentionally usesissue.assignees.first()to create only one task per issue for the first assignee. Either updateset_task_deadlineto also use the first assignee only (for consistency with the sync command and documented design), or update the sync command to handle all assignees—not just the first one.
♻️ Duplicate comments (2)
backend/apps/mentorship/api/internal/mutations/module.py (2)
362-371: Verify multiple-task-per-issue behavior against system design.Similar to
set_task_deadline, this function processes tasks for all assignees (lines 362-366), which contradicts the documented single-assignee-per-issue design principle.Note: The conditional bulk_update logic at lines 368-371 is a valid micro-optimization, though the single
.save()call for one task andbulk_update()for multiple tasks could be simplified to always usebulk_update()for consistency.
247-258: GitHub API call inside transaction may cause state inconsistency.Similar to
assign_issue_to_user, the GitHub unassignment at line 249 executes inside a@transaction.atomicblock. If the GitHub operation succeeds but the database transaction rolls back, the states will be inconsistent. The same architectural solution mentioned for assignment should apply here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/mentorship/api/internal/mutations/module.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
📚 Learning: 2025-07-16T13:49:58.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/module.py:28-29
Timestamp: 2025-07-16T13:49:58.648Z
Learning: In the OWASP Nest mentorship system, mentors can be created with only github_user initially (without nest_user) when assigned to modules. This allows mentors to be assigned before they've signed into the system. When these users eventually sign in, the nest_user is automatically linked to the existing github_user, creating a complete mentor profile. This design provides flexibility in mentor assignment workflows.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
🧬 Code graph analysis (1)
backend/apps/mentorship/api/internal/mutations/module.py (8)
backend/apps/mentorship/models/mentee.py (1)
Mentee(11-49)backend/apps/mentorship/models/mentee_module.py (1)
MenteeModule(11-45)backend/apps/mentorship/models/mentor.py (1)
Mentor(11-40)backend/apps/mentorship/models/module.py (2)
Module(17-99)save(92-99)backend/apps/mentorship/models/program.py (2)
Program(18-87)save(83-87)backend/apps/mentorship/models/task.py (2)
Task(10-76)Status(19-26)backend/apps/github/models/repository.py (1)
path(157-159)backend/apps/owasp/api/internal/nodes/member_snapshot.py (1)
github_user(32-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (3)
backend/apps/mentorship/api/internal/mutations/module.py (3)
10-12: LGTM!The GitHub client imports and centralized error message constants are well-structured and support the new GitHub integration features.
Also applies to: 27-29
185-209: LGTM!The MenteeModule and Task creation logic is correct. The re-assignment handling at lines 206-209 properly resets
assigned_atandstatustoIN_PROGRESSwhen re-assigning a previously closed task, addressing the concern from past reviews.
45-47: GitHub client authentication is properly implemented.The
get_github_client()function authenticates using server-level credentials configured via GitHub App (App ID, Installation ID, private key) or falls back to a PAT token from theGITHUB_TOKENenvironment variable. Both approaches are validated during initialization and raiseBadCredentialsExceptionif credentials are missing. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
backend/apps/mentorship/api/internal/mutations/module.py (3)
178-182: GitHub API call inside transaction can cause state inconsistency.This concern was already raised in previous reviews. The GitHub API call executes inside the
@transaction.atomicblock, which can cause inconsistency if the external call succeeds but the database transaction subsequently fails and rolls back. The PR description indicates this architectural concern is being tracked for a separate PR.
248-252: GitHub API call inside transaction can cause state inconsistency.Same architectural concern as in
assign_issue_to_user(lines 178-182): the GitHub API call executes inside the@transaction.atomicblock, which can cause inconsistency if the external call succeeds but the database transaction fails and rolls back.
197-210: Critical: Re-assignment doesn't update CLOSED tasks to IN_PROGRESS.The current logic only updates the task when
not created and task.assigned_at is None, but this doesn't handle the case where a task was previously assigned (assigned_at set), then unassigned (status set to CLOSED at lines 256-259 without clearing assigned_at), and is now being re-assigned. In this scenario:
createdis False (task exists)task.assigned_atis NOT None (was set during first assignment)- The condition evaluates to False
- The task status remains CLOSED instead of being updated to IN_PROGRESS
🔎 Proposed fix
task, created = Task.objects.get_or_create( module=module, issue=issue, assignee=gh_user, defaults={ "assigned_at": now, "status": Task.Status.IN_PROGRESS, }, ) -if not created and task.assigned_at is None: +if not created: + if task.assigned_at is None: + task.assigned_at = now + if task.status == Task.Status.CLOSED: + task.status = Task.Status.IN_PROGRESS - task.assigned_at = now - task.status = Task.Status.IN_PROGRESS - task.save(update_fields=["assigned_at", "status"]) + task.save(update_fields=["assigned_at", "status"])Or more concisely:
task, created = Task.objects.get_or_create( module=module, issue=issue, assignee=gh_user, defaults={ "assigned_at": now, "status": Task.Status.IN_PROGRESS, }, ) -if not created and task.assigned_at is None: - task.assigned_at = now +if not created: + needs_update = False + if task.assigned_at is None: + task.assigned_at = now + needs_update = True + if task.status != Task.Status.IN_PROGRESS: + task.status = Task.Status.IN_PROGRESS + needs_update = True + if needs_update: - task.status = Task.Status.IN_PROGRESS - task.save(update_fields=["assigned_at", "status"]) + task.save(update_fields=["assigned_at", "status"])
🧹 Nitpick comments (1)
backend/apps/mentorship/api/internal/mutations/module.py (1)
33-50: Consider adding explicit return type hint.The helper function is well-structured and centralizes GitHub issue retrieval logic. For improved clarity and IDE support, consider adding an explicit return type annotation.
🔎 Proposed enhancement
+from apps.github.models import Issue +from github import GithubObject + -def get_github_issue(module: Module, issue_number: int) -> tuple: +def get_github_issue(module: Module, issue_number: int) -> tuple[Issue, GithubObject.Issue]: """Get local issue and GitHub issue objects for a module issue."""Note: Adjust the GitHub issue type import based on your PyGithub usage patterns.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/mentorship/api/internal/mutations/module.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
📚 Learning: 2025-07-16T13:49:58.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/module.py:28-29
Timestamp: 2025-07-16T13:49:58.648Z
Learning: In the OWASP Nest mentorship system, mentors can be created with only github_user initially (without nest_user) when assigned to modules. This allows mentors to be assigned before they've signed into the system. When these users eventually sign in, the nest_user is automatically linked to the existing github_user, creating a complete mentor profile. This design provides flexibility in mentor assignment workflows.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
🔇 Additional comments (4)
backend/apps/mentorship/api/internal/mutations/module.py (4)
10-10: LGTM! GitHub integration imports added.The imports of
GithubExceptionandget_github_clientproperly support the new GitHub API integration for issue assignment and unassignment workflows.Also applies to: 12-12
27-31: LGTM! Error message constants improve consistency.Centralizing error messages as module-level constants is a good practice that improves maintainability and ensures consistent messaging across the codebase.
256-259: Note: Unassignment preserves assigned_at, contributing to re-assignment bug.When unassigning, the task status is set to CLOSED but
assigned_atis not cleared. This preserves historical assignment data but requires the re-assignment logic (lines 197-210) to check for CLOSED status explicitly, which it currently doesn't do correctly. This is addressed in the critical issue comment on lines 197-210.If the intent is to preserve assignment history, the current approach is reasonable, but ensure the re-assignment logic properly handles CLOSED tasks as flagged above.
184-195: LGTM! Mentee and MenteeModule creation logic is correct.The code properly creates or retrieves the Mentee and MenteeModule records when assigning an issue, with appropriate defaults for started_at and ended_at based on module dates.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/apps/mentorship/api/internal/mutations/module.py (1)
197-210: Fix incomplete re-assignment logic for CLOSED tasks.The current logic only handles tasks where
assigned_at is None(line 207), but when a task is unassigned (lines 258-261), only thestatusis set toCLOSED—assigned_atis not cleared. Therefore, when re-assigning a previously unassigned task:
get_or_createreturns the existing task withcreated=Falsetask.assigned_atis NOTNone(it still has the old timestamp)- The condition on line 207 evaluates to
False- The task status remains
CLOSEDinstead of being updated toIN_PROGRESSSolution: Check if the task status is
CLOSEDrather than checking ifassigned_at is None, or update both fields unconditionally when re-assigning.🔎 Proposed fix to handle re-assignment of CLOSED tasks
- if not created and task.assigned_at is None: + if not created and task.status == Task.Status.CLOSED: task.assigned_at = now task.status = Task.Status.IN_PROGRESS task.save(update_fields=["assigned_at", "status"])Alternatively, always update when re-assigning:
- if not created and task.assigned_at is None: + if not created: task.assigned_at = now task.status = Task.Status.IN_PROGRESS task.save(update_fields=["assigned_at", "status"])
🧹 Nitpick comments (3)
backend/apps/mentorship/api/internal/mutations/module.py (3)
33-50: Consider adding error handling for GitHub API calls.The helper function
get_github_issuecalls GitHub APIs (lines 46-48) but doesn't wrap them in error handling. WhileGithubExceptionis imported and caught at call sites, GitHub API calls can fail due to network issues, rate limiting, authentication failures, or repository access issues. Consider whether centralized error handling in this helper would improve code maintainability.🔎 Optional refactor to centralize GitHub API error handling
def get_github_issue(module: Module, issue_number: int) -> tuple: """Get local issue and GitHub issue objects for a module issue.""" issue = ( module.issues.select_related("repository", "repository__owner") .filter(number=issue_number) .first() ) if issue is None: raise ObjectDoesNotExist(ISSUE_NOT_FOUND_IN_MODULE_MSG) if not issue.repository: raise ValidationError(message="Issue has no repository.") - gh_client = get_github_client() - gh_repository = gh_client.get_repo(issue.repository.path) - gh_issue = gh_repository.get_issue(number=issue.number) + try: + gh_client = get_github_client() + gh_repository = gh_client.get_repo(issue.repository.path) + gh_issue = gh_repository.get_issue(number=issue.number) + except GithubException as e: + raise ValidationError(message=f"Failed to fetch issue from GitHub: {e}") from e return issue, gh_issue
177-181: Architectural note: GitHub-first approach can cause state inconsistency.The GitHub API call (line 179) executes before the database transaction (line 183). If the GitHub operation succeeds but the database transaction subsequently fails, GitHub will have the assignment while the database won't, creating an inconsistent state. The PR description notes that error-handling improvements are deferred to a separate PR, but this architectural concern should be tracked.
Consider implementing one of these approaches in the follow-up PR:
- Compensating transaction: If the DB transaction fails, roll back the GitHub assignment
- DB-first with retry: Persist to DB first, then update GitHub; if GitHub fails, roll back DB
- Idempotency: Ensure operations can be safely retried to achieve eventual consistency
249-253: Same architectural concern applies to unassign operation.Similar to the assign operation, the GitHub API call (line 251) executes before the database transaction (line 255). If the GitHub unassignment succeeds but the database transaction fails, state inconsistency will occur. This should be addressed in the same follow-up PR that handles the assign operation's transaction boundaries.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/mentorship/api/internal/mutations/module.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/module.py:28-29
Timestamp: 2025-07-16T13:49:58.648Z
Learning: In the OWASP Nest mentorship system, mentors can be created with only github_user initially (without nest_user) when assigned to modules. This allows mentors to be assigned before they've signed into the system. When these users eventually sign in, the nest_user is automatically linked to the existing github_user, creating a complete mentor profile. This design provides flexibility in mentor assignment workflows.
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
📚 Learning: 2025-07-16T13:49:58.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/module.py:28-29
Timestamp: 2025-07-16T13:49:58.648Z
Learning: In the OWASP Nest mentorship system, mentors can be created with only github_user initially (without nest_user) when assigned to modules. This allows mentors to be assigned before they've signed into the system. When these users eventually sign in, the nest_user is automatically linked to the existing github_user, creating a complete mentor profile. This design provides flexibility in mentor assignment workflows.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
🧬 Code graph analysis (1)
backend/apps/mentorship/api/internal/mutations/module.py (6)
backend/apps/mentorship/api/internal/nodes/module.py (4)
CreateModuleInput(163-177)ModuleNode(19-159)UpdateModuleInput(181-196)issues(76-87)backend/apps/mentorship/models/mentee.py (1)
Mentee(11-49)backend/apps/mentorship/models/mentee_module.py (1)
MenteeModule(11-45)backend/apps/mentorship/models/module.py (2)
Module(17-99)save(92-99)backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-36)backend/apps/mentorship/models/task.py (2)
Task(10-76)Status(19-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (6)
backend/apps/mentorship/api/internal/mutations/module.py (6)
10-10: LGTM! GitHub integration imports are appropriate.The new imports support GitHub API operations and task/mentee management functionality.
Also applies to: 12-12, 19-19
27-30: LGTM! Error message constants improve maintainability.Centralizing error messages promotes consistency and makes updates easier.
314-322: Clarify expected behavior when setting deadline on CLOSED tasks.When setting a deadline, the code creates or retrieves tasks for each assignee (lines 314-316) and updates
assigned_atif it'sNone(lines 317-319), but it doesn't check or update the taskstatus. If a task is inCLOSEDstatus (e.g., after unassignment), setting a deadline won't reopen it.Please verify: should setting a deadline on a
CLOSEDtask automatically change its status toIN_PROGRESSor another active status?
364-376: LGTM! Efficient deadline clearing with smart bulk update optimization.The logic correctly handles clearing deadlines for all assignees, and the optimization at lines 371-374 (using
save()for a single task andbulk_update()for multiple tasks) is a nice touch for performance.
389-389: LGTM! Consistent error messaging.Using the centralized
MODULE_NOT_FOUND_MSGconstant improves maintainability.
169-171: Add admin permission check toassign_issue_to_userfor consistency withunassign_issue_from_user.The
assign_issue_to_usermutation (lines 169-171) only checks if the user is a mentor, whileunassign_issue_from_user(lines 242-243) requires the mentor to also be a program admin. Both operations modify module and GitHub state, making this asymmetry inconsistent. Either both mutations should require admin privileges, or the admin check should be removed from unassign. No documentation explains the design intent for this difference.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/apps/mentorship/api/internal/mutations/module.py (3)
116-116: Consider adding an error message for clarity.The
PermissionDeniedexception is raised without a message. Adding a descriptive message would improve debugging and user experience.🔎 Suggested improvement
- raise PermissionDenied + raise PermissionDenied("Only program admins can create modules.")
241-243: Consider adding error messages for clarity.Both
PermissionDeniedexceptions are raised without messages. Adding descriptive messages would improve debugging and user experience.🔎 Suggested improvements
mentor = Mentor.objects.filter(nest_user=user).first() if mentor is None: - raise PermissionDenied + raise PermissionDenied("Only mentors can unassign issues.") if not module.program.admins.filter(id=mentor.id).exists(): - raise PermissionDenied + raise PermissionDenied("Only program admins can unassign issues.")
404-404: Consider adding an error message for clarity.The
PermissionDeniedexception is raised without a message. Adding a descriptive message would improve debugging and user experience.🔎 Suggested improvement
- raise PermissionDenied + raise PermissionDenied("Only program admins can update modules.")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/mentorship/api/internal/mutations/module.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/module.py:28-29
Timestamp: 2025-07-16T13:49:58.648Z
Learning: In the OWASP Nest mentorship system, mentors can be created with only github_user initially (without nest_user) when assigned to modules. This allows mentors to be assigned before they've signed into the system. When these users eventually sign in, the nest_user is automatically linked to the existing github_user, creating a complete mentor profile. This design provides flexibility in mentor assignment workflows.
📚 Learning: 2025-08-17T11:55:55.990Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
📚 Learning: 2025-07-16T13:49:58.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/module.py:28-29
Timestamp: 2025-07-16T13:49:58.648Z
Learning: In the OWASP Nest mentorship system, mentors can be created with only github_user initially (without nest_user) when assigned to modules. This allows mentors to be assigned before they've signed into the system. When these users eventually sign in, the nest_user is automatically linked to the existing github_user, creating a complete mentor profile. This design provides flexibility in mentor assignment workflows.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (2)
backend/apps/mentorship/api/internal/mutations/module.py (2)
177-183: GitHub-then-DB flow partially addresses past architectural concern.The GitHub API call now executes outside the transaction block (improvement from past review), ensuring GitHub assignment happens first. However, the reverse inconsistency risk remains: if the GitHub assignment succeeds but the database transaction fails, GitHub will have the assignment while the application state won't reflect it. Since the PR description indicates error-handling improvements are deferred to a separate PR, this architectural concern should continue to be tracked for future work.
442-457: LGTM: Experience level management is well-implemented.The logic correctly maintains the program's experience levels as the union of all module experience levels. When a module's experience level changes, the new level is added and the old level is removed only if no other modules use it. The exclusion query (lines 449-453) properly prevents premature removal.
|
|
||
| mentor = Mentor.objects.filter(nest_user=user).first() | ||
| if mentor is None: | ||
| raise PermissionDenied(msg="Only mentors can assign issues.") |
There was a problem hiding this comment.
Fix incorrect PermissionDenied syntax.
PermissionDenied doesn't accept a msg= keyword argument. The message should be passed as a positional argument, otherwise it won't be displayed when the exception is raised.
🔎 Proposed fix
- raise PermissionDenied(msg="Only mentors can assign issues.")
+ raise PermissionDenied("Only mentors can assign issues.")This same issue appears at lines 290 and 350. All instances should be fixed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raise PermissionDenied(msg="Only mentors can assign issues.") | |
| raise PermissionDenied("Only mentors can assign issues.") |
🤖 Prompt for AI Agents
In backend/apps/mentorship/api/internal/mutations/module.py around line 171
(also apply same fix at lines ~290 and ~350), the PermissionDenied exception is
being constructed with a keyword argument msg= which is invalid; replace
PermissionDenied(msg="...") with PermissionDenied("...") so the message is
passed as the positional argument and will be displayed when raised.
Updated backend logic for issue assignment. The flow now also assigns the issue on GitHub via the GB API. If the GitHub assignment fails, the app-level assignment will not proceed.
We also create a Task on issue assignment now.
For unassignment, the issue is first unassigned on GitHub, followed by updating the app state upon a successful response.
Validation and error handling improvements will be addressed in a separate PR.
Resolves #2861
Checklist
make check-testlocally and all tests passed