Enable Module Deletion for Admins and Editing for Module Mentors#3054
Enable Module Deletion for Admins and Editing for Module Mentors#3054anurag2787 wants to merge 34 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds backend module deletion and tightens create/update authorization to allow program admins or assigned module mentors; frontend exposes Edit for module mentors and an admin-only Delete flow with mutation, cache updates, toasts, and test/jest adjustments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 0
🧹 Nitpick comments (5)
frontend/src/components/SingleModuleCard.tsx (2)
21-25: Code duplication: DELETE_MODULE_MUTATION is defined in both SingleModuleCard and CardDetailsPage.Consider extracting this mutation to a shared location (e.g., a mutations file or generated queries) to maintain a single source of truth.
45-46: Consider using route params instead of pathname parsing.Extracting
programKeyby splitting the pathname string is fragile and could break if the URL structure changes. Based on learnings, Next.js 13+ app router providesuseParamsto extract route parameters more reliably.🔎 Suggested approach
import { useParams } from 'next/navigation' // Inside component: const params = useParams() const programKey = params?.programKey as string || ''frontend/src/components/CardDetailsPage.tsx (3)
46-50: Duplicate mutation definition.This
DELETE_MODULE_MUTATIONis identical to the one inSingleModuleCard.tsx. Extract to a shared location to avoid maintenance burden.🔎 Suggested approach
Create a shared mutations file:
// e.g., frontend/src/graphql/mutations/moduleMutations.ts import { gql } from '@apollo/client' export const DELETE_MODULE_MUTATION = gql` mutation DeleteModule($programKey: String!, $moduleKey: String!) { deleteModule(programKey: $programKey, moduleKey: $moduleKey) } `Then import in both components.
127-174: Consider extracting the delete logic to reduce duplication and improve readability.This inline handler (~45 lines) duplicates the logic in
SingleModuleCard.tsx. Consider:
- Extracting to a custom hook (e.g.,
useModuleDeletion) that encapsulates the mutation, cache update, toasts, and navigation- Or at minimum, extracting to a named function within the component
This would improve maintainability and keep the JSX cleaner.
🔎 Example custom hook
// hooks/useModuleDeletion.ts export function useModuleDeletion(programKey: string) { const router = useRouter() const [deleteModule] = useMutation(DELETE_MODULE_MUTATION) const handleDelete = async (moduleKey: string, moduleName?: string) => { try { await deleteModule({ variables: { programKey, moduleKey }, update: (cache) => { // cache update logic }, }) addToast({ title: 'Success', description: `Module deleted successfully.`, color: 'success' }) router.push(`/my/mentorship/programs/${programKey}`) } catch (error) { addToast({ title: 'Error', description: 'Failed to delete module.', color: 'danger' }) console.error('Delete module error:', error) } } return { handleDelete } }
92-93: Minor optimization opportunity.
useMutation(DELETE_MODULE_MUTATION)is always initialized even whenonDeleteis provided externally and the internal mutation won't be used. This is a minor inefficiency but not a blocking issue.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/mentorship/api/internal/mutations/module.pyfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/EntityActions.tsxfrontend/src/components/SingleModuleCard.tsxfrontend/src/types/card.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/SingleModuleCard.tsxfrontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/components/SingleModuleCard.tsxfrontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.
Applied to files:
frontend/src/components/SingleModuleCard.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/components/SingleModuleCard.tsxfrontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
🧬 Code graph analysis (4)
frontend/src/components/EntityActions.tsx (1)
frontend/src/types/button.ts (1)
Button(4-9)
frontend/src/components/SingleModuleCard.tsx (1)
frontend/src/types/__generated__/programsQueries.generated.ts (1)
GetProgramAndModulesDocument(37-37)
backend/apps/mentorship/api/internal/mutations/module.py (4)
backend/apps/mentorship/models/module.py (2)
Module(17-99)save(92-99)backend/apps/mentorship/models/mentor.py (1)
Mentor(11-40)backend/apps/mentorship/api/internal/nodes/program.py (1)
admins(32-34)backend/apps/mentorship/models/program.py (1)
save(83-87)
frontend/src/components/CardDetailsPage.tsx (1)
frontend/src/types/__generated__/programsQueries.generated.ts (1)
GetProgramAndModulesDocument(37-37)
🔇 Additional comments (9)
frontend/src/types/card.ts (1)
63-63: LGTM!The optional
onDeleteprop withPromise<void>return type is appropriate for an async deletion handler.frontend/src/components/SingleModuleCard.tsx (2)
55-99: LGTM!The delete handler has proper error handling, cache updates with defensive try/catch, user feedback via toasts, and appropriate post-delete navigation.
124-131: LGTM!Admin permission check is correctly applied before rendering EntityActions with the delete callback.
frontend/src/components/EntityActions.tsx (3)
68-77: Error handling in handleDeleteConfirm relies on the onDelete callback.The
try/finallyblock here doesn't catch errors - it assumesonDeletehandles its own errors. This is acceptable since callers (SingleModuleCard, CardDetailsPage) do handle errors, but consider whether the modal should remain open on failure for retry.Currently,
setDeleteModalOpen(false)only runs on success (inside try, after await). IfonDeletethrows, the modal stays open which is actually the correct UX for allowing retry.
92-96: LGTM!Delete option is conditionally rendered only when
onDeleteis provided, with appropriate red styling to indicate a destructive action.
158-186: LGTM!The confirmation modal implementation is solid with clear messaging, loading states, and proper button disabling during the deletion process. The warning about irreversibility is appropriate for a destructive action.
backend/apps/mentorship/api/internal/mutations/module.py (3)
403-434: LGTM on permission checks.The mutation correctly enforces:
- User authentication via
IsAuthenticated- User must be a mentor (not just any authenticated user)
- Mentor must be an admin of the module's program
This aligns with the PR objective of restricting deletion to program admins.
439-450: LGTM!The experience level cleanup logic correctly checks for orphaned levels and removes them only when no other module in the program uses them. This mirrors the pattern in
update_module.
452-462: LGTM!Returning a success message string is appropriate for a deletion mutation since the module no longer exists and cannot be returned as a node. The logging provides a good audit trail.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/src/components/SingleModuleCard.tsx (1)
34-34: Consider adding a fallback or validation for programKey extraction.The pathname-based extraction could result in an empty string if the URL structure doesn't match the expected pattern. While
programKeydefaults to'', passing an empty string toEntityActionscould lead to a failed delete mutation.🔎 Suggested improvement
- const programKey = pathname?.split('/programs/')[1]?.split('/')[0] || '' + const programKey = pathname?.split('/programs/')[1]?.split('/')[0]Then conditionally render
EntityActionsonly whenprogramKeyis truthy:- {isAdmin && ( + {isAdmin && programKey && ( <EntityActions type="module" programKey={programKey} moduleKey={module.key} /> )}frontend/src/components/EntityActions.tsx (5)
3-13: Consider organizing imports and moving mutation to a queries file.The GraphQL mutation could be moved to a dedicated queries/mutations file (e.g.,
moduleMutations.ts) for consistency with the project's pattern whereGetProgramAndModulesDocumentis imported from a generated file. Additionally, the imports could be better organized with external libraries grouped together.🔎 Suggested import reorganization
'use client' +import { gql, useMutation } from '@apollo/client' -import { useMutation } from '@apollo/client/react' +import { Button } from '@heroui/button' +import { Modal, ModalBody, ModalContent, ModalFooter, ModalHeader } from '@heroui/modal' import { addToast } from '@heroui/toast' import { useRouter } from 'next/navigation' import type React from 'react' import { useState, useRef, useEffect } from 'react' import { FaEllipsisV } from 'react-icons/fa' -import { gql } from '@apollo/client' -import { Modal, ModalContent, ModalHeader, ModalBody, ModalFooter } from '@heroui/modal' -import { Button } from '@heroui/button' + import { ProgramStatusEnum } from 'types/__generated__/graphql' import { GetProgramAndModulesDocument } from 'types/__generated__/programsQueries.generated'
116-118: Unnecessary delay before navigation.The 500ms
setTimeoutbefore navigation is not needed and could cause unexpected behavior if the user interacts with the UI during the delay. The toast will persist across navigation, so immediate navigation is safe.🔎 Proposed fix
addToast({ title: 'Success', description: 'Module has been deleted successfully.', color: 'success', }) setDeleteModalOpen(false) - setTimeout(() => { - router.push(`/my/mentorship/programs/${programKey}`) - }, 500) + router.push(`/my/mentorship/programs/${programKey}`) } catch (error) {
105-107: Avoid prefixing unused variables with underscore when you're using them.The variable
_erris prefixed with underscore (convention for unused variables) but is actually being used inconsole.error. Useerrinstead.🔎 Proposed fix
- } catch (_err) { - console.error('Error updating cache:', _err) + } catch (err) { + console.error('Error updating cache:', err) }
186-190: Minor: Handler function recreated on each render.The
handleMenuItemClickfunction is defined inside the map callback, causing it to be recreated on every render for each option. This is a minor performance consideration.🔎 Proposed optimization using data attribute
- {options.map((option) => { - const handleMenuItemClick = (e: React.MouseEvent) => { - e.preventDefault() - e.stopPropagation() - handleAction(option.key) - } - - return ( - <button - key={option.key} - type="button" - role="menuitem" - onClick={handleMenuItemClick} + {options.map((option) => ( + <button + key={option.key} + type="button" + role="menuitem" + data-action={option.key} + onClick={(e) => { + e.preventDefault() + e.stopPropagation() + handleAction(option.key) + }}Or keep the current approach as it's functionally correct and the performance impact is negligible for this small list.
119-125: Consider extracting specific error messages.The error toast shows a generic message. For better user experience, consider extracting the specific error message from the GraphQL response when available.
🔎 Proposed improvement
} catch (error) { + const errorMessage = error instanceof Error ? error.message : 'Failed to delete module. Please try again.' addToast({ title: 'Error', - description: 'Failed to delete module. Please try again.', + description: errorMessage, color: 'danger', }) console.error('Delete module error:', error) } finally {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/CardDetailsPage.tsxfrontend/src/components/EntityActions.tsxfrontend/src/components/SingleModuleCard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/CardDetailsPage.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/EntityActions.tsx
🧬 Code graph analysis (1)
frontend/src/components/EntityActions.tsx (1)
frontend/src/types/__generated__/programsQueries.generated.ts (1)
GetProgramAndModulesDocument(37-37)
🔇 Additional comments (5)
frontend/src/components/SingleModuleCard.tsx (2)
53-53: LGTM!Good addition of
data-testidattribute for test automation.
66-72: LGTM!The EntityActions integration correctly passes the required props and is appropriately gated behind the
isAdmincheck.frontend/src/components/EntityActions.tsx (3)
15-19: LGTM!The mutation definition is correct and follows GraphQL conventions.
144-148: LGTM!The delete option is correctly added to module actions with appropriate styling. The admin-only access is enforced by the parent component (
SingleModuleCard) which only rendersEntityActionswhenisAdminis true.
210-238: LGTM!The delete confirmation modal is well-implemented with proper loading states, disabled buttons during deletion, and clear warning message about the irreversible action.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/EntityActions.tsx (1)
29-35: PR objectives incomplete: Module editing permissions for mentors not implemented.According to Issue #2858, this PR should implement two features:
- ✅ Module deletion for program admins
- ❌ Module editing for module mentors (assigned to the module)
The current implementation only addresses module deletion. The Edit option (line 155) is shown for all modules, but there's no logic to:
- Verify if the current user is a mentor assigned to the specific module
- Show/hide the Edit option based on mentor assignment
- Enforce that only assigned mentors (and admins) can edit a module
Do you want me to help design the implementation for module mentor editing permissions? This would require:
- Passing mentor assignment data to this component
- Conditionally rendering the Edit option based on mentor status
- Ensuring backend validation enforces the same rules
🧹 Nitpick comments (1)
frontend/src/components/EntityActions.tsx (1)
126-129: Fragile error message parsing for permission checks.Checking if the error message includes 'Permission' via string matching (line 127) is brittle and will break if the backend error format changes. Consider checking the GraphQL error code or type instead.
🔎 More robust error handling
const description = - error instanceof Error && error.message.includes('Permission') + (error as any)?.graphQLErrors?.[0]?.extensions?.code === 'FORBIDDEN' ? 'You do not have permission to delete this module.' : 'Failed to delete module. Please try again.'Adjust the error code check based on your backend's actual GraphQL error extensions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/EntityActions.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/EntityActions.tsx
🧬 Code graph analysis (1)
frontend/src/components/EntityActions.tsx (2)
frontend/src/types/__generated__/programsQueries.generated.ts (1)
GetProgramAndModulesDocument(37-37)frontend/src/types/button.ts (1)
Button(4-9)
🔇 Additional comments (4)
frontend/src/components/EntityActions.tsx (4)
38-42: LGTM!The state management for modal visibility and deletion loading state is properly initialized, and the mutation hook is correctly set up.
62-64: LGTM!The delete action correctly triggers the confirmation modal before proceeding with deletion, which aligns with the requirement for a confirmation step from Issue #2858.
220-248: LGTM!The delete confirmation modal correctly implements:
- Confirmation message warning about irreversible action
- Proper loading state management with
isLoadinganddisabledprops- Cancel and Delete actions with appropriate styling
- Conditional rendering only for module type
111-113: Clarify the return type expectation for thedeleteModulemutation.The backend mutation returns a success message string (
str), not a boolean. While the check!result?.data?.deleteModuleworks correctly because it verifies a non-empty string was returned, this behavior depends on an implicit contract with the backend. If the mutation ever returns an empty string or null, the check would fail even on success. Consider either:
- Making this contract explicit in a comment linking to the backend implementation
- Having the backend return a boolean for explicit success/failure semantics instead of relying on string presence
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
frontend/src/components/EntityActions.tsx (3)
15-19: Import the mutation from the dedicated mutations file instead of defining it inline.This concern was already raised in previous reviews. The mutation should be imported from
frontend/src/graphql/mutations/deleteModule.tsto maintain separation of concerns and improve reusability.
87-107: Handle cache read failure gracefully.This concern was already raised in previous reviews. The cache update should not throw when the query isn't populated, as the deletion may succeed on the backend while the cache update fails.
156-156: Delete option visibility should be controlled at the component level.This concern was already raised in previous reviews. The delete option should only be shown to program admins, and the component should enforce this via an
isAdminprop rather than relying on parent components to gate visibility.
🧹 Nitpick comments (2)
frontend/src/components/EntityActions.tsx (2)
219-247: Modal implementation is solid.The delete confirmation modal follows good UX patterns with clear messaging, proper loading states, and appropriate color coding for the destructive action.
Optional: Enhance accessibility with explicit ARIA attributes
Consider adding explicit ARIA attributes for improved screen reader support:
- <Modal isOpen={deleteModalOpen} onClose={() => setDeleteModalOpen(false)}> + <Modal + isOpen={deleteModalOpen} + onClose={() => setDeleteModalOpen(false)} + aria-labelledby="delete-modal-title" + aria-describedby="delete-modal-description" + > <ModalContent> - <ModalHeader className="flex flex-col gap-1">Delete Module</ModalHeader> + <ModalHeader className="flex flex-col gap-1" id="delete-modal-title"> + Delete Module + </ModalHeader> <ModalBody> - <p>Are you sure you want to delete this module? This action cannot be undone.</p> + <p id="delete-modal-description"> + Are you sure you want to delete this module? This action cannot be undone. + </p> </ModalBody>
125-128: Consider more robust error type discrimination.The permission error detection relies on substring matching in error messages, which is fragile. If the backend returns structured error types or codes, use those instead for more reliable error handling.
Example: Using GraphQL error extensions
If the backend includes error codes in extensions:
- const description = - error instanceof Error && error.message.includes('Permission') - ? 'You do not have permission to delete this module.' - : 'Failed to delete module. Please try again.' + let description = 'Failed to delete module. Please try again.' + + if (error instanceof ApolloError) { + const permissionError = error.graphQLErrors.find( + (e) => e.extensions?.code === 'FORBIDDEN' || e.extensions?.code === 'UNAUTHORIZED' + ) + if (permissionError) { + description = 'You do not have permission to delete this module.' + } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/EntityActions.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-08-30T12:52:32.214Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/graphql-codegen.ts:52-52
Timestamp: 2025-08-30T12:52:32.214Z
Learning: When using GraphQL Codegen with async configuration that needs to fetch data (like CSRF tokens), export the async function directly instead of using top-level await: `export default async function()` rather than `export default await function()`.
Applied to files:
frontend/src/components/EntityActions.tsx
🧬 Code graph analysis (1)
frontend/src/components/EntityActions.tsx (2)
frontend/src/types/__generated__/programsQueries.generated.ts (1)
GetProgramAndModulesDocument(37-37)frontend/src/types/button.ts (1)
Button(4-9)
🔇 Additional comments (3)
frontend/src/components/EntityActions.tsx (3)
38-42: LGTM! State and mutation setup is appropriate.The modal visibility and loading states are properly initialized, and the mutation hook follows standard Apollo Client patterns.
62-64: LGTM! Delete action correctly triggers confirmation modal.The handler appropriately opens the confirmation modal before proceeding with deletion.
78-138: Backend implementation properly enforces admin-only deletion with correct return type.The backend
delete_modulemutation (./backend/apps/mentorship/api/internal/mutations/module.py:405-462) correctly:
- Returns a non-empty success message string (
"Module '{name}' has been deleted successfully.")- Enforces admin-only deletion with two explicit permission checks: verifies user is a mentor, then verifies they are an admin of the program
- Raises
PermissionDeniedwith descriptive messages for authorization failures- Handles missing modules with
ObjectDoesNotExistThe frontend's check
if (!result?.data?.deleteModule)is appropriate for this implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/components/EntityActions.tsx (2)
114-116: Consider verifying the boolean return value.The validation checks that
deleteModuleexists in the response but doesn't verify it'strue. Since the mutation returns a boolean, explicitly checking the value would make success/failure clearer.🔎 Suggested improvement
-if (!result?.data || typeof result.data !== 'object' || !('deleteModule' in result.data)) { - throw new Error('Delete mutation failed on server') +if (!result?.data?.deleteModule) { + throw new Error('Delete mutation returned false') }
126-136: Consider adding console logging for debugging.While the user-facing error handling is appropriate, adding
console.error(error)would help with debugging in production environments.🔎 Suggested improvement
} catch (error) { + console.error('Module deletion failed:', error) + const description = error instanceof Error && error.message.includes('Permission') ? 'You do not have permission to delete this module.'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/EntityActions.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-08-30T12:52:32.214Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/graphql-codegen.ts:52-52
Timestamp: 2025-08-30T12:52:32.214Z
Learning: When using GraphQL Codegen with async configuration that needs to fetch data (like CSRF tokens), export the async function directly instead of using top-level await: `export default async function()` rather than `export default await function()`.
Applied to files:
frontend/src/components/EntityActions.tsx
🔇 Additional comments (4)
frontend/src/components/EntityActions.tsx (4)
42-46: LGTM! State management and mutation setup are appropriate.The state additions for modal visibility and deletion progress tracking are well-structured, and the
useMutationhook follows standard Apollo Client patterns with correct typing.
66-68: LGTM! Delete action case is properly integrated.The delete action case correctly opens the confirmation modal and follows the existing pattern of other action handlers.
118-125: LGTM! Success handling and navigation flow are well-designed.The sequence of showing a success toast, closing the modal, and navigating to the program page provides good UX after module deletion.
221-249: LGTM! Delete confirmation modal is well-implemented.The modal provides excellent UX with:
- Clear warning about irreversible action
- Proper disabled states during deletion
- Loading indicator on the delete button
- Correct HeroUI Button usage with
onPress- Appropriate color variants for visual hierarchy
anurag2787
force-pushed
the
module-removal-and-editing
branch
from
4c25e5a to
55beb10
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
frontend/src/components/EntityActions.tsx (3)
19-23: Import the mutation from the dedicated mutations file instead of defining it inline.This issue was already identified in a previous review. Defining mutations inline breaks separation of concerns and makes the mutation harder to reuse and test. The mutation should be imported from
frontend/src/graphql/mutations/deleteModule.ts.
97-99: Handle cache read failure gracefully.This issue was already identified in a previous review. The cache update throws an error if
existingorexisting.getProgramModulesis missing, but this error will be caught by the generic catch block and shown to users as "Failed to delete module" even if the backend deletion succeeded.
158-158: Delete option visibility should be controlled at the component level, not delegated to parents.This issue was already identified in a previous review. The delete option is unconditionally included in the module options array without any permission check. The component should have an
isAdminprop to conditionally include the delete option, rather than relying on parent components to enforce permissions.
🧹 Nitpick comments (3)
frontend/src/components/EntityActions.tsx (1)
127-130: Consider using error codes instead of string matching for error classification.The current approach checks if the error message contains "Permission" to differentiate permission errors from other failures. This pattern is fragile and depends on the backend error message format remaining stable.
💡 Alternative approach
If the backend provides structured error codes (e.g., GraphQL error extensions), you could match on those instead:
const description = error instanceof Error && (error as any).graphQLErrors?.[0]?.extensions?.code === 'PERMISSION_DENIED' ? 'You do not have permission to delete this module.' : 'Failed to delete module. Please try again.'This would make error handling more robust and maintainable.
backend/apps/mentorship/api/internal/mutations/module.py (2)
439-450: Verify the experience level cleanup logic and ordering.The experience level cleanup occurs before the module is deleted. While the logic appears correct (checking if other modules use the level before removing it from the program), ensure this doesn't cause issues if the deletion fails after the experience level is removed.
Consider moving the experience level cleanup after the successful module deletion to maintain consistency with the actual state:
🔎 Suggested refactoring for safer cleanup ordering
program = module.program module_name = module.name +experience_level_to_remove = module.experience_level +# Delete the module +module.delete() + +logger.info( + "User '%s' deleted module '%s' from program '%s'.", + user.username, + module_name, + program_key, +) # Clean up experience levels if this module is the only one using it -experience_level_to_remove = module.experience_level if ( experience_level_to_remove in program.experience_levels and not Module.objects.filter( program=program, experience_level=experience_level_to_remove - ) - .exclude(id=module.id) - .exists() + ).exists() ): program.experience_levels.remove(experience_level_to_remove) program.save(update_fields=["experience_levels"]) -# Delete the module -module.delete() - -logger.info( - "User '%s' deleted module '%s' from program '%s'.", - user.username, - module_name, - program_key, -) return f"Module '{module_name}' has been deleted successfully."Note: After deletion, you can remove the
.exclude(id=module.id)clause since the module no longer exists.
439-450: Consider extracting experience level cleanup into a shared helper function.The experience level cleanup logic is similar to the logic in
update_module(lines 387-397). Both mutations check if an experience level should be removed from the program when it's no longer used by any modules. Extracting this into a helper function would improve maintainability and reduce duplication.🔎 Proposed helper function extraction
Add this helper function near the top of the file, after the existing helper functions:
def _cleanup_unused_experience_level( program: Program, experience_level: str, exclude_module_id: int | None = None ) -> None: """Remove experience level from program if no modules use it.""" if experience_level not in program.experience_levels: return modules_query = Module.objects.filter( program=program, experience_level=experience_level ) if exclude_module_id is not None: modules_query = modules_query.exclude(id=exclude_module_id) if not modules_query.exists(): program.experience_levels.remove(experience_level) program.save(update_fields=["experience_levels"])Then update
delete_module:program = module.program module_name = module.name +experience_level_to_remove = module.experience_level -# Clean up experience levels if this module is the only one using it -experience_level_to_remove = module.experience_level -if ( - experience_level_to_remove in program.experience_levels - and not Module.objects.filter( - program=program, experience_level=experience_level_to_remove - ) - .exclude(id=module.id) - .exists() -): - program.experience_levels.remove(experience_level_to_remove) - program.save(update_fields=["experience_levels"]) +# Clean up experience levels if this module is the only one using it +_cleanup_unused_experience_level(program, experience_level_to_remove, exclude_module_id=module.id) # Delete the module module.delete()And similarly update
update_modulelines 387-397.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/types/__generated__/EntityActions.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (2)
backend/apps/mentorship/api/internal/mutations/module.pyfrontend/src/components/EntityActions.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-08-30T12:52:32.214Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/graphql-codegen.ts:52-52
Timestamp: 2025-08-30T12:52:32.214Z
Learning: When using GraphQL Codegen with async configuration that needs to fetch data (like CSRF tokens), export the async function directly instead of using top-level await: `export default async function()` rather than `export default await function()`.
Applied to files:
frontend/src/components/EntityActions.tsx
🧬 Code graph analysis (1)
frontend/src/components/EntityActions.tsx (1)
frontend/src/types/__generated__/programsQueries.generated.ts (1)
GetProgramAndModulesDocument(37-37)
🔇 Additional comments (8)
frontend/src/components/EntityActions.tsx (6)
3-13: LGTM!The imports are appropriate for the delete module functionality, including Apollo Client hooks, HeroUI components, and the query document for cache updates.
15-17: LGTM!The interface correctly types the mutation response.
42-46: LGTM!The state management for modal visibility and deletion progress is appropriate, and the Apollo Client useMutation hook is used correctly.
66-68: LGTM!The delete_module case correctly triggers the confirmation modal.
82-140: Overall deletion flow is well-structured.The function properly manages state, executes the mutation, updates the cache, and provides user feedback. The early return guard and error handling are appropriate.
221-249: LGTM!The delete confirmation modal is well-implemented with proper user experience patterns:
- Clear confirmation message about irreversible action
- Disabled interactions during deletion
- Loading state on the delete button
- Appropriate danger styling for destructive action
backend/apps/mentorship/api/internal/mutations/module.py (2)
422-434: Permission check logic is correct for program admin deletion.The permission validation properly restricts module deletion to program admins, which aligns with the requirement that only program admins should be able to delete modules.
453-453: Cascading deletions are properly configured; no action needed.All related models have
on_delete=models.CASCADEconfigured: Task, IssueUserInterest, MentorModule, and MenteeModule all reference Module with CASCADE deletion. Many-to-many relationships (issues, mentors) do not require on_delete settings and are handled through their junction tables. No custom delete methods or deletion signals exist that could interfere with the standard cascade behavior.Likely an incorrect or invalid review comment.
|
Hy @arkid15r Currently working on this PR will soon make it ready for review |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/apps/mentorship/api/internal/mutations/module.py (1)
406-465: Delete module mutation is well-implemented.The mutation correctly:
- Validates user authentication and mentor status
- Restricts deletion to program admins only
- Cleans up orphaned experience levels
- Uses atomic transactions for data consistency
- Logs the deletion action for audit purposes
Consider extracting the experience level cleanup logic (lines 444-453) into a reusable helper since it's duplicated in
update_module(lines 391-400), but this is a minor refactor opportunity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/mentorship/api/internal/mutations/module.pyfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/EntityActions.tsxfrontend/src/components/SingleModuleCard.tsxfrontend/src/graphql/mutations/deleteModule.ts
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/components/CardDetailsPage.tsxfrontend/src/components/SingleModuleCard.tsxfrontend/src/components/EntityActions.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-08-30T12:52:32.214Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/graphql-codegen.ts:52-52
Timestamp: 2025-08-30T12:52:32.214Z
Learning: When using GraphQL Codegen with async configuration that needs to fetch data (like CSRF tokens), export the async function directly instead of using top-level await: `export default async function()` rather than `export default await function()`.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
📚 Learning: 2025-07-14T16:18:07.287Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleForm.tsx:112-134
Timestamp: 2025-07-14T16:18:07.287Z
Learning: In the OWASP Nest mentorship system, date validation for modules (ensuring start date precedes end date) is handled on the backend in the module GraphQL mutations via the `_validate_module_dates` helper function in backend/apps/mentorship/graphql/mutations/module.py, which prevents invalid date ranges from being stored in the database.
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
📚 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
🧬 Code graph analysis (1)
frontend/src/components/EntityActions.tsx (2)
frontend/src/graphql/mutations/deleteModule.ts (1)
DELETE_MODULE_MUTATION(3-7)frontend/src/types/__generated__/programsQueries.generated.ts (1)
GetProgramAndModulesDocument(37-37)
🔇 Additional comments (7)
backend/apps/mentorship/api/internal/mutations/module.py (1)
345-349: Module mentor editing permission is now correctly implemented.The permission check now allows both program admins (
is_program_admin) and module mentors (is_module_mentor) to edit modules, addressing the PR objective. This resolves the previous review comment about missing mentor editing permissions.frontend/src/graphql/mutations/deleteModule.ts (1)
1-7: Clean GraphQL mutation definition.The mutation is well-structured, properly exported, and follows Apollo Client conventions. It correctly matches the backend
delete_modulemutation signature.frontend/src/components/CardDetailsPage.tsx (1)
102-106: Admin prop correctly passed to EntityActions.The
isAdmin={true}prop is appropriately passed only when the user is confirmed to be a program admin (viaaccessLevel === 'admin'and login check againstadminslist). This correctly enables the delete functionality for authorized users.frontend/src/components/SingleModuleCard.tsx (1)
65-65: Correct propagation of admin status.The
isAdminprop is correctly derived from the access level and admin login check, and properly passed toEntityActions. SinceEntityActionsis only rendered whenisAdminis true, this ensures the delete option is only available to program admins.frontend/src/components/EntityActions.tsx (3)
74-130: Delete handler implementation looks solid.The implementation correctly:
- Guards against missing
moduleKey- Updates cache gracefully when data exists (addresses previous review feedback)
- Validates the mutation result before showing success (addresses previous review feedback)
- Provides user-friendly error messages for permission issues
- Properly manages loading state in
finallyblock
211-239: Delete confirmation modal is well-implemented.The modal correctly:
- Only renders for module type
- Disables buttons during deletion to prevent double-submission
- Shows loading state on the delete button
- Provides clear warning about irreversibility
145-149: Delete option correctly gated byisAdminprop.The delete option is now conditionally included based on the
isAdminprop rather than relying solely on parent components for permission enforcement. This addresses the previous review feedback about making the component self-protective.
- Add delete_module mutation in backend with permission checks - Update update_module to allow both program admins and module mentors - Implement delete functionality in EntityActions component - Add DELETE_MODULE_MUTATION with inline gql definition - Pass isAdmin prop to EntityActions for delete button visibility - Fix mutation result validation to check actual value - Add Apollo Client mock in jest.setup.ts for tests - All tests passing except unrelated CreateModule timeout
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/components/EntityActions.tsx (1)
15-19: Import the mutation from the dedicated mutations file.As noted in the previous review, the DELETE_MODULE_MUTATION should be imported from
frontend/src/graphql/mutations/deleteModule.tsrather than defined inline. This maintains separation of concerns and enables reusability.
🧹 Nitpick comments (4)
frontend/__tests__/unit/pages/CreateModule.test.tsx (1)
52-128: Investigate the need for 10-second timeout.The test timeout of 10000ms is quite long and suggests potential flakiness or performance issues. Standard Jest tests should complete within 5 seconds (the default timeout).
Consider:
- Identifying which async operation is taking so long
- Using
waitForwith shorter, explicit timeouts for specific assertions- Reviewing whether component logic can be optimized
- Checking if test setup (mocks, renders) can be streamlined
If the timeout is necessary due to actual component behavior, document why with a comment explaining the expected delay.
frontend/src/components/EntityActions.tsx (3)
21-23: Consider using GraphQL Codegen types for consistency.While the inline type definition resolves the missing type issue, using the GraphQL-generated type would ensure consistency with other mutations and automatically stay in sync with schema changes.
🔎 Alternative approach
+import type { DeleteModuleMutation } from 'types/__generated__/graphql' -type DeleteModuleResponse = { - deleteModule: boolean -} -const [deleteModule] = useMutation<DeleteModuleResponse>(DELETE_MODULE_MUTATION) +const [deleteModule] = useMutation<DeleteModuleMutation>(DELETE_MODULE_MUTATION)
93-111: Cache update correctly handles missing data.The conditional check on line 99 properly addresses the previous concern about cache read failures. The update will now silently skip if the query isn't cached, allowing the backend state to remain the source of truth.
💡 Optional optimization
Consider using
cache.modifyfor better performance:update(cache) { cache.modify({ fields: { getProgramModules(existingModules = [], { readField }) { return existingModules.filter( (moduleRef) => readField('key', moduleRef) !== moduleKey ) }, }, }) }This avoids reading and writing the entire query.
127-136: Enhance error handling to distinguish error types.The current error handling relies on string matching (
error.message.includes('Permission')), which is fragile. GraphQL errors should be parsed from the error object's structure for more reliable error categorization.🔎 Improved error handling
} catch (error) { let description = 'Failed to delete module. Please try again.' if (error instanceof ApolloError) { if (error.graphQLErrors?.some(e => e.extensions?.code === 'FORBIDDEN')) { description = 'You do not have permission to delete this module.' } else if (error.networkError) { description = 'Network error. Please check your connection and try again.' } else if (error.graphQLErrors?.length > 0) { description = error.graphQLErrors[0].message } } addToast({ title: 'Error', description, color: 'danger', }) }Import ApolloError:
import { ApolloError } from '@apollo/client'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/types/__generated__/EntityActions.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (5)
frontend/__tests__/unit/pages/CreateModule.test.tsxfrontend/jest.setup.tsfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/EntityActions.tsxfrontend/src/components/SingleModuleCard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/SingleModuleCard.tsx
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/components/EntityActions.tsxfrontend/__tests__/unit/pages/CreateModule.test.tsxfrontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/EntityActions.tsxfrontend/__tests__/unit/pages/CreateModule.test.tsx
📚 Learning: 2025-08-30T12:52:32.214Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/graphql-codegen.ts:52-52
Timestamp: 2025-08-30T12:52:32.214Z
Learning: When using GraphQL Codegen with async configuration that needs to fetch data (like CSRF tokens), export the async function directly instead of using top-level await: `export default async function()` rather than `export default await function()`.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/pages/CreateModule.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/pages/CreateModule.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/pages/CreateModule.test.tsx
🧬 Code graph analysis (1)
frontend/src/components/EntityActions.tsx (2)
frontend/src/types/__generated__/programsQueries.generated.ts (1)
GetProgramAndModulesDocument(37-37)frontend/src/types/button.ts (1)
Button(4-9)
🔇 Additional comments (5)
frontend/src/components/CardDetailsPage.tsx (1)
106-113: LGTM!The addition of
isAdmin={true}correctly propagates admin status to the EntityActions component, enabling proper permission-based rendering of the delete option. The conditional checks appropriately verify module type, admin access level, and admin membership.frontend/src/components/EntityActions.tsx (4)
114-116: Result validation correctly checks boolean value.The validation now properly verifies that
deleteModuleis truthy, not just present in the response. This addresses the previous concern about handling{deleteModule: false}responses.
158-160: Delete option visibility properly controlled by isAdmin prop.The conditional inclusion of the delete option based on
isAdmincorrectly implements component-level permission enforcement, addressing the previous architectural concern. The component no longer relies solely on parent components for access control.
224-250: LGTM!The delete confirmation modal is well-implemented with:
- Clear confirmation message warning about irreversibility
- Proper loading states (
isLoadinganddisabledprops)- Appropriate color coding (danger for destructive action)
- Clean UX with Cancel/Delete options
31-31: LGTM!The
isAdminprop is appropriately typed as optional with a boolean value, ensuring backward compatibility while enabling the new delete permission control.
|
Hi @kasya I have implemented the functionality to delete a module however while implementing module editing iam unable to access my/mentorship even though i assigned myself as mentor for module |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/apps/mentorship/api/internal/mutations/module.py (3)
87-98: Extract duplicated mentor resolution logic into a helper function.This mentor resolution pattern (checking
nest_userthengithub_user) is duplicated in three places:create_module(lines 88-98),update_module(lines 356-366), anddelete_module(lines 484-498). Extract this into a reusable helper function to reduce duplication and improve maintainability.♻️ Proposed helper function
Add this helper function near the top of the file, after
resolve_mentors_from_logins:def resolve_user_as_mentor(user, error_message: str = "Only mentors can perform this action.") -> Mentor: """Resolve a user to their Mentor record, checking both nest_user and github_user.""" mentor = None with contextlib.suppress(Mentor.DoesNotExist): mentor = Mentor.objects.get(nest_user=user) if mentor is None and hasattr(user, "github_user"): with contextlib.suppress(Mentor.DoesNotExist): mentor = Mentor.objects.get(github_user=user.github_user) if mentor is None: raise PermissionDenied(error_message) return mentorThen replace lines 88-98 with:
- creator_as_mentor = None - with contextlib.suppress(Mentor.DoesNotExist): - creator_as_mentor = Mentor.objects.get(nest_user=user) - - if creator_as_mentor is None and hasattr(user, "github_user"): - with contextlib.suppress(Mentor.DoesNotExist): - creator_as_mentor = Mentor.objects.get(github_user=user.github_user) - - if creator_as_mentor is None: - msg = "Only mentors can create modules." - raise PermissionDenied(msg) + creator_as_mentor = resolve_user_as_mentor(user, "Only mentors can create modules.")Apply similar changes to lines 356-366 and 484-498.
414-436: Consider simplifying the mentor assignment update logic.The current logic is functionally correct but complex due to the nested checks and the mutation of
input_data.mentor_loginstoNone. Consider restructuring for clarity:♻️ Alternative approach for clarity
if input_data.mentor_logins is not None: - if not is_program_admin: - current_logins = { - login.lower() - for login in module.mentors.values_list("github_user__login", flat=True) - } - requested_logins = {login.lower() for login in input_data.mentor_logins} - - if requested_logins != current_logins: - msg = "Only program admins can modify mentor assignments." - logger.warning( - "Unauthorized mentor assignment attempt: Non-admin mentor '%s' " - "tried to modify mentors for module '%s'.", - user.username, - module.name, - ) - raise PermissionDenied(msg) - # Mentor list unchanged; skip the update - input_data.mentor_logins = None - - if input_data.mentor_logins is not None: - mentors_to_set = resolve_mentors_from_logins(input_data.mentor_logins) - module.mentors.set(mentors_to_set) + # Check if mentor assignments are being changed + current_logins = { + login.lower() + for login in module.mentors.values_list("github_user__login", flat=True) + } + requested_logins = {login.lower() for login in input_data.mentor_logins} + mentors_changed = requested_logins != current_logins + + if mentors_changed: + if not is_program_admin: + msg = "Only program admins can modify mentor assignments." + logger.warning( + "Unauthorized mentor assignment attempt: Non-admin mentor '%s' " + "tried to modify mentors for module '%s'.", + user.username, + module.name, + ) + raise PermissionDenied(msg) + + # Admin is changing mentors - proceed with update + mentors_to_set = resolve_mentors_from_logins(input_data.mentor_logins) + module.mentors.set(mentors_to_set)This eliminates the confusing double-check and makes the intent clearer: if mentors are changing, require admin; if unchanged, skip the update.
443-455: Extract duplicated experience level cleanup logic.The logic to remove unused experience levels from
program.experience_levelsis duplicated in bothupdate_module(lines 443-455) anddelete_module(lines 507-518). Extract this into a helper function to reduce duplication.♻️ Proposed helper function
Add this helper function:
def cleanup_unused_experience_level(program: Program, experience_level: str, exclude_module_id: int | None = None) -> None: """Remove experience level from program if no modules are using it. Args: program: The program to clean up experience_level: The experience level to check exclude_module_id: Optional module ID to exclude from the check (e.g., module being deleted) """ if experience_level not in program.experience_levels: return queryset = Module.objects.filter(program=program, experience_level=experience_level) if exclude_module_id is not None: queryset = queryset.exclude(id=exclude_module_id) if not queryset.exists(): program.experience_levels.remove(experience_level) program.save(update_fields=["experience_levels"])Then replace lines 443-455 in
update_modulewith:- # Remove old experience level if no other module is using it - if ( - old_experience_level != module.experience_level - and old_experience_level in module.program.experience_levels - and not Module.objects.filter( - program=module.program, experience_level=old_experience_level - ) - .exclude(id=module.id) - .exists() - ): - module.program.experience_levels.remove(old_experience_level) + # Remove old experience level if no other module is using it + if old_experience_level != module.experience_level: + cleanup_unused_experience_level(module.program, old_experience_level, module.id)And replace lines 507-518 in
delete_modulewith:- # Clean up experience levels if this module is the only one using it - experience_level_to_remove = module.experience_level - if ( - experience_level_to_remove in program.experience_levels - and not Module.objects.filter( - program=program, experience_level=experience_level_to_remove - ) - .exclude(id=module.id) - .exists() - ): - program.experience_levels.remove(experience_level_to_remove) - program.save(update_fields=["experience_levels"]) + # Clean up experience levels if this module is the only one using it + cleanup_unused_experience_level(program, module.experience_level, module.id)Also applies to: 507-518
📜 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 (8)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleForm.tsx:112-134
Timestamp: 2025-07-14T16:18:07.287Z
Learning: In the OWASP Nest mentorship system, date validation for modules (ensuring start date precedes end date) is handled on the backend in the module GraphQL mutations via the `_validate_module_dates` helper function in backend/apps/mentorship/graphql/mutations/module.py, which prevents invalid date ranges from being stored in the database.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
📚 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
📚 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-14T16:18:07.287Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleForm.tsx:112-134
Timestamp: 2025-07-14T16:18:07.287Z
Learning: In the OWASP Nest mentorship system, date validation for modules (ensuring start date precedes end date) is handled on the backend in the module GraphQL mutations via the `_validate_module_dates` helper function in backend/apps/mentorship/graphql/mutations/module.py, which prevents invalid date ranges from being stored in the database.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
📚 Learning: 2025-07-31T07:05:25.056Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 0
File: :0-0
Timestamp: 2025-07-31T07:05:25.056Z
Learning: In the OWASP Nest project, Django views may not properly access authenticated users from sessions created by Strawberry GraphQL mutations. The issue occurs because Django's AuthenticationMiddleware doesn't always populate request.user from session data that GraphQL context successfully uses via info.context.request.user. The solution is to manually resolve users from session data using request.session.get('_auth_user_id') and User.objects.select_related('github_user').get(pk=user_id) to match the same authentication mechanism used by GraphQL context.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
📚 Learning: 2025-07-08T17:24:36.501Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
🧬 Code graph analysis (1)
backend/apps/mentorship/api/internal/mutations/module.py (3)
backend/apps/mentorship/models/mentor.py (1)
Mentor(11-40)backend/apps/mentorship/api/internal/nodes/module.py (2)
issues(76-87)mentors(38-40)backend/apps/mentorship/models/module.py (2)
Module(17-99)save(92-99)
🔇 Additional comments (4)
backend/apps/mentorship/api/internal/mutations/module.py (4)
153-153: LGTM: Consistent error messaging.The standardized error messages using constants and consistent formatting improve code clarity and maintainability.
Also applies to: 163-164, 168-168, 196-196, 206-207, 211-212, 238-238, 253-253, 300-300, 315-315, 354-354
368-382: LGTM: Proper dual permission model implementation.The permission check correctly allows both program admins and module mentors to edit modules, with appropriate logging for unauthorized attempts. The detailed error message clearly communicates the permission requirements.
457-462: LGTM: Proper audit logging.The success logging provides a good audit trail for module updates without leaking sensitive information.
466-530: LGTM: Solid delete_module implementation.The new
delete_modulemutation correctly implements admin-only deletion with proper:
- Permission enforcement (program admins only)
- Cleanup of unused experience levels before deletion
- Transaction safety via
@transaction.atomic- Audit logging for security tracking
- Appropriate error handling and user feedback
The logic for experience level cleanup correctly uses
.exclude(id=module.id)since the cleanup happens before the module is deleted.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/CardDetailsPage.tsx`:
- Around line 133-148: The code incorrectly references `data` (which is
undefined) instead of the `session` variable returned by useSession; update the
calculation of `currentUserLogin` to use `session` (e.g., (session as
ExtendedSession)?.user?.login) and keep the rest of the logic (`isAdmin`,
`isMentor`, and the `EntityActions` render) the same so the permission checks
work and EntityActions can render when appropriate.
♻️ Duplicate comments (1)
frontend/src/components/EntityActions.tsx (1)
15-19: Import the mutation from a dedicated mutations file instead of defining it inline.The AI-generated summary indicates
frontend/src/graphql/mutations/deleteModule.tsexists in the codebase. Defining mutations inline breaks separation of concerns and makes the mutation harder to reuse and test.🔧 Proposed fix
-const DELETE_MODULE_MUTATION = gql` - mutation DeleteModule($programKey: String!, $moduleKey: String!) { - deleteModule(programKey: $programKey, moduleKey: $moduleKey) - } -` +import { DELETE_MODULE_MUTATION } from '../graphql/mutations/deleteModule'
🧹 Nitpick comments (2)
frontend/src/components/EntityActions.tsx (2)
126-142: Consider logging unexpected errors for debugging.The error handling provides good user-facing messages but silently discards error details for unexpected failures. Adding a
console.errorwould help with debugging in development without affecting user experience.🔧 Proposed fix
} catch (error) { let description = 'Failed to delete module. Please try again.' if (error instanceof Error) { if (error.message.includes('Permission') || error.message.includes('not have permission')) { description = 'You do not have permission to delete this module. Only program admins can delete modules.' } else if (error.message.includes('Unauthorized')) { description = 'Unauthorized: You must be a program admin to delete modules.' + } else { + console.error('Module deletion failed:', error) } } addToast({ title: 'Error', description, color: 'danger', }) } finally {
232-260: Prevent modal dismissal while deletion is in progress.The Cancel button is correctly disabled, but the modal can still be dismissed by clicking the backdrop or pressing Escape. This could leave the UI in an inconsistent state if the user dismisses the modal while deletion is processing.
🔧 Proposed fix
- <Modal isOpen={deleteModalOpen} onClose={() => setDeleteModalOpen(false)}> + <Modal + isOpen={deleteModalOpen} + onClose={() => setDeleteModalOpen(false)} + isDismissable={!isDeleting} + isKeyboardDismissDisabled={isDeleting} + hideCloseButton={isDeleting} + >
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/jest.setup.tsfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/EntityActions.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/jest.setup.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleForm.tsx:112-134
Timestamp: 2025-07-14T16:18:07.287Z
Learning: In the OWASP Nest mentorship system, date validation for modules (ensuring start date precedes end date) is handled on the backend in the module GraphQL mutations via the `_validate_module_dates` helper function in backend/apps/mentorship/graphql/mutations/module.py, which prevents invalid date ranges from being stored in the database.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/components/EntityActions.tsxfrontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-08-30T12:52:32.214Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/graphql-codegen.ts:52-52
Timestamp: 2025-08-30T12:52:32.214Z
Learning: When using GraphQL Codegen with async configuration that needs to fetch data (like CSRF tokens), export the async function directly instead of using top-level await: `export default async function()` rather than `export default await function()`.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
frontend/src/components/EntityActions.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
🧬 Code graph analysis (2)
frontend/src/components/EntityActions.tsx (1)
frontend/src/types/__generated__/programsQueries.generated.ts (1)
GetProgramAndModulesDocument(37-37)
frontend/src/components/CardDetailsPage.tsx (3)
frontend/src/types/auth.ts (1)
ExtendedSession(8-20)backend/apps/mentorship/api/internal/nodes/program.py (1)
admins(32-34)backend/apps/mentorship/api/internal/nodes/module.py (1)
mentors(38-40)
🔇 Additional comments (5)
frontend/src/components/EntityActions.tsx (4)
84-112: LGTM on the cache update handling.The cache update now gracefully handles missing data by checking
existing?.getProgramModulesbefore writing, which addresses the previous concern about cache read failures. The backend deletion succeeds independently of the cache state.
114-116: LGTM on the mutation result verification.The check now properly verifies
!result.data.deleteModule(the boolean value) rather than just checking for presence, ensuring the success path only executes when the server explicitly returns success.
21-23: LGTM on the type definition.The
DeleteModuleResponsetype is now properly defined locally with the correctdeleteModule: booleanshape, resolving the previous TypeScript compilation concern.Also applies to: 48-48
164-166: LGTM on admin-gated delete option.The delete option is now correctly conditionally included based on the
isAdminprop, ensuring the component is self-protective rather than relying solely on parent components to enforce permissions.frontend/src/components/CardDetailsPage.tsx (1)
136-139: Verify theisAdmincomputation logic.The current logic requires both
accessLevel === 'admin'AND the user being in the admins list. This seems redundant—ifaccessLevelis derived from the user being an admin, checkingadmins?.some()duplicates that check. If they're independent, the double-check may be intentional for defense-in-depth.Confirm whether both conditions are necessary, or if
isAdminshould simply be:const isAdmin = admins?.some((admin) => admin.login === currentUserLogin)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/apps/mentorship/api/internal/mutations/module.py`:
- Around line 533-543: After calling module.delete() in delete_module, ensure
you invalidate the module and program caches on commit to avoid stale UI: wrap
cache invalidation in django.db.transaction.on_commit so it runs after the DB
change, e.g. call your cache invalidation helpers (e.g.
cache.invalidate_module(module.id) and cache.invalidate_program(program_key) or
cache.delete(f"module:{module.id}") and cache.delete(f"program:{program_key}"))
inside on_commit right after module.delete(), then proceed to logger.info and
return.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/apps/mentorship/api/internal/mutations/module.py`:
- Around line 536-541: The current code calls
invalidate_program_cache(program_key) immediately and has inconsistent
indentation for the _invalidate() body; move the program invalidation into the
on_commit callback and fix indentation so both invalidations run only after
commit. Specifically, update the _invalidate function (the one that currently
calls invalidate_module_cache(module_key, program_key)) to also call
invalidate_program_cache(program_key) inside its body with correct 4-space
indentation, then register that function via transaction.on_commit(_invalidate)
(remove the standalone immediate call to invalidate_program_cache).
- Around line 102-103: The PermissionDenied raised after the
program.admins.check (the conditional using
program.admins.filter(id=creator_as_mentor.id).exists()) lacks a descriptive
message; update that raise PermissionDenied to include a clear message such as
"Only program admins can create modules." so it matches the style of the earlier
check (the one that uses "Only mentors can create modules.") and helps with
debugging and user feedback.
🧹 Nitpick comments (1)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx (1)
106-128: PassisAdmintoModuleFormand conditionally hide the mentor logins field for non-admins.The backend correctly omits
mentorLoginsfrom the mutation for non-admins, but non-admin mentors can still see and edit this field in the UI with no effect. This creates a confusing experience. SinceisAdminis already available in the edit page component, pass it toModuleFormand hide the mentor logins field when!isAdminto align the UI with actual permissions.
There was a problem hiding this comment.
Actionable comments posted: 0
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)
465-478: Also invalidate program cache on module updates to avoid stale program views.Create/delete already invalidate program caches; update should too for consistency and to prevent stale module listings.
🛠️ Proposed fix
def _invalidate(): invalidate_module_cache(old_module_key, program_key) if module.key != old_module_key: invalidate_module_cache(module.key, program_key) + invalidate_program_cache(program_key) transaction.on_commit(_invalidate)
♻️ Duplicate comments (1)
backend/apps/mentorship/api/internal/mutations/module.py (1)
539-544: Move program cache invalidation into on_commit.
invalidate_program_cache(program_key)currently runs before commit, so a rollback would still evict cache. Move it into the_invalidate()callback.🛠️ Proposed fix
def _invalidate(): invalidate_module_cache(module_key, program_key) - - invalidate_program_cache(program_key) + invalidate_program_cache(program_key) transaction.on_commit(_invalidate)
🧹 Nitpick comments (1)
backend/apps/mentorship/api/internal/mutations/module.py (1)
90-100: Drop redundanthasattr(user, "github_user")guards in authenticated mutations.In these authenticated flows,
user.github_usershould be guaranteed. The guard can silently skip mentor resolution and complicates typing. Use direct access (optionally assert) and only suppressMentor.DoesNotExist.♻️ Suggested simplification (apply similarly in update_module / delete_module)
- if creator_as_mentor is None and hasattr(user, "github_user"): - with contextlib.suppress(Mentor.DoesNotExist): - creator_as_mentor = Mentor.objects.get(github_user=user.github_user) + if creator_as_mentor is None: + with contextlib.suppress(Mentor.DoesNotExist): + creator_as_mentor = Mentor.objects.get(github_user=user.github_user)Based on learnings, ...
Also applies to: 366-373, 504-506
|
3 participants
Proposed change
This PR implement functionality to delete a module from a program
Resolves #2858
Screencast.from.2025-12-27.21-01-14.webm
Checklist
make check-testlocally and all tests passed