Add module ordering #3101
Add module ordering #3101
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds persistent module ordering (Module.position + migration), a backend GraphQL mutation and query ordering to set module order, cache versioning with a mutation-triggered invalidation extension, and frontend drag‑and‑drop reordering UI, wiring, types, tests, and DnD dependencies. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
backend/apps/mentorship/models/module.py (1)
48-51: Consider adding a database index for query performance.The
positionfield is now used in the primary ordering forget_program_modules. Addingdb_index=Truewould optimize queries that order by position.🔎 Proposed enhancement
position = models.IntegerField( verbose_name="Position", default=0, + db_index=True, )Note: This would require a new migration to add the index.
backend/apps/mentorship/migrations/0007_module_position.py (1)
11-17: Consider adding a data migration for existing modules.All existing modules will have
position=0after this migration, which means their relative order will be undefined when querying by position. Consider adding a data migration to set initial positions based onstarted_atorcreated_atto preserve a sensible default order.Example data migration approach
# In a subsequent migration or combined with this one using RunPython from django.db import migrations def set_initial_positions(apps, schema_editor): Module = apps.get_model('mentorship', 'Module') for program_id in Module.objects.values_list('program_id', flat=True).distinct(): modules = Module.objects.filter(program_id=program_id).order_by('started_at', 'id') for idx, module in enumerate(modules): module.position = idx module.save(update_fields=['position']) class Migration(migrations.Migration): # ... existing code ... operations = [ # ... existing AddField ... migrations.RunPython(set_initial_positions, migrations.RunPython.noop), ]backend/apps/mentorship/api/internal/mutations/module.py (1)
439-448: Performance: Use bulk update instead of individual queries in the loop.The current implementation executes N separate
UPDATEqueries (one per module). Consider usingbulk_updatefor better performance.Additionally, verify if partial reordering is intentional—modules not included in
module_keyswill retain their old positions, which could lead to duplicate positions or unexpected ordering.Proposed optimization using bulk_update
- existing_modules = Module.objects.filter(program=program) - existing_module_keys = {m.key for m in existing_modules} - - if not all(mk in existing_module_keys for mk in input_data.module_keys): - raise ValidationError( - message="One or more module keys are invalid or do not belong to this program." - ) - - for index, module_key in enumerate(input_data.module_keys): - existing_modules.filter(key=module_key).update(position=index) + existing_modules = list(Module.objects.filter(program=program)) + module_by_key = {m.key: m for m in existing_modules} + + if not all(mk in module_by_key for mk in input_data.module_keys): + raise ValidationError( + message="One or more module keys are invalid or do not belong to this program." + ) + + # Optionally validate all modules are included + if set(input_data.module_keys) != set(module_by_key.keys()): + raise ValidationError( + message="All program modules must be included in the reorder request." + ) + + modules_to_update = [] + for index, module_key in enumerate(input_data.module_keys): + module = module_by_key[module_key] + module.position = index + modules_to_update.append(module) + + Module.objects.bulk_update(modules_to_update, ["position"])frontend/src/app/my/mentorship/programs/[programKey]/page.tsx (1)
87-111: Well-implemented optimistic update pattern with rollback.The implementation correctly:
- Validates admin permissions before proceeding
- Saves previous state for rollback on error
- Applies optimistic UI update before the mutation
- Reverts state on failure
Consider adding a success toast for consistency with
updateStatus:Optional: Add success toast
try { const input = { programKey: programKey, moduleKeys: moduleKeys, } setModules(moduleOrder) await updateOrder({ variables: { input } }) + addToast({ + title: 'Module order updated', + description: 'The module order has been saved.', + variant: 'solid', + color: 'success', + timeout: 3000, + }) } catch (err) {backend/apps/common/extensions.py (1)
94-97: Prefer direct enum comparison over string conversion.Converting the enum to string for comparison is fragile and could break if the enum's string representation changes.
Proposed fix
+from strawberry.types import OperationType + class CacheInvalidationExtension(SchemaExtension): """CacheInvalidationExtension class.""" def on_execute(self): """Invalidate cache on mutation.""" - if str(self.execution_context.operation_type) == "OperationType.MUTATION": + if self.execution_context.operation_type == OperationType.MUTATION: bump_cache_version()frontend/src/components/ModuleCard.tsx (1)
51-52: Consider defensive check fordraftModules.The non-null assertion
draftModules!assumesdraftModulesis nevernullwhenisReorderingistrue. While the current logic maintains this invariant, a defensive check would be safer:Proposed fix
- const currentModules = isReordering ? draftModules! : modules + const currentModules = isReordering && draftModules ? draftModules : modules
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlfrontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/moduleMutations.generated.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
backend/apps/common/extensions.pybackend/apps/mentorship/api/internal/mutations/module.pybackend/apps/mentorship/api/internal/nodes/module.pybackend/apps/mentorship/api/internal/queries/module.pybackend/apps/mentorship/migrations/0007_module_position.pybackend/apps/mentorship/models/module.pybackend/settings/graphql.pybackend/tests/apps/common/extensions_test.pyfrontend/__tests__/unit/components/ModuleCard.test.tsxfrontend/package.jsonfrontend/src/app/my/mentorship/programs/[programKey]/page.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/ModuleCard.tsxfrontend/src/server/mutations/moduleMutations.tsfrontend/src/types/card.tspackage.json
🧰 Additional context used
🧠 Learnings (5)
📓 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.
📚 Learning: 2025-07-13T05:55:46.436Z
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.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
📚 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 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/app/my/mentorship/programs/[programKey]/page.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/app/my/mentorship/programs/[programKey]/page.tsx
🧬 Code graph analysis (7)
frontend/src/types/card.ts (2)
backend/apps/mentorship/models/module.py (1)
Module(17-103)frontend/src/types/mentorship.ts (1)
Module(37-51)
backend/apps/mentorship/api/internal/nodes/module.py (1)
frontend/src/types/__generated__/graphql.ts (1)
SetModuleOrderInput(993-996)
backend/apps/mentorship/api/internal/mutations/module.py (4)
backend/apps/mentorship/api/internal/nodes/module.py (1)
SetModuleOrderInput(200-204)backend/apps/mentorship/api/internal/nodes/program.py (2)
ProgramNode(15-34)admins(32-34)backend/apps/mentorship/models/module.py (1)
Module(17-103)backend/apps/mentorship/models/program.py (1)
Program(18-87)
frontend/src/components/ModuleCard.tsx (1)
frontend/src/types/mentorship.ts (1)
Module(37-51)
frontend/__tests__/unit/components/ModuleCard.test.tsx (1)
frontend/src/components/ModuleCard.tsx (1)
getSimpleDuration(229-244)
backend/settings/graphql.py (1)
backend/apps/common/extensions.py (2)
CacheExtension(53-88)CacheInvalidationExtension(91-97)
frontend/src/app/my/mentorship/programs/[programKey]/page.tsx (2)
backend/apps/mentorship/models/module.py (1)
Module(17-103)frontend/src/types/mentorship.ts (1)
Module(37-51)
🔇 Additional comments (18)
backend/apps/mentorship/api/internal/queries/module.py (2)
25-25: LGTM! Position-based ordering correctly implemented.The ordering now correctly prioritizes the
positionfield first, then falls back tostarted_atas a tie-breaker, which aligns with the module ordering feature.
29-36: Project modules correctly usestarted_atordering—position-based ordering is intentionally program-scoped only.The
set_module_ordermutation only acceptsprogram_keyinput and filters modules byprogram=program, with no project-level equivalent. This is intentional architectural design: position management is reserved for program-level module ordering, while project modules usestarted_atas the default ordering criteria.frontend/src/types/card.ts (1)
71-71: LGTM! Clean addition of module ordering callback.The optional
setModuleOrdercallback is well-typed and follows existing patterns in the interface.frontend/src/server/mutations/moduleMutations.ts (1)
63-69: LGTM! Mutation definition is correct.The
SET_MODULE_ORDERmutation is properly structured and matches the backend GraphQL schema. The minimal return of onlyidis acceptable for this operation.backend/apps/mentorship/api/internal/nodes/module.py (1)
199-204: LGTM! Input type correctly defined.The
SetModuleOrderInputclass is properly structured with appropriate fields and follows the existing patterns in the codebase.backend/settings/graphql.py (2)
7-7: LGTM! Correct import of cache invalidation extension.The import of
CacheInvalidationExtensionenables version-based cache invalidation for mutations.
44-46: LGTM! Extension order is correct.The ordering of extensions is crucial:
CacheInvalidationExtensionmust precedeCacheExtensionto ensure that mutations bump the cache version before any cache reads occur. This prevents stale cached data after mutations.frontend/package.json (1)
21-23: @dnd-kit package versions are valid and secure.All three specified versions exist on npm and are free from known vulnerabilities:
- @dnd-kit/[email protected] (published Dec 2024)
- @dnd-kit/[email protected] (published Dec 4, 2024)
- @dnd-kit/[email protected] (latest published)
No security advisories or CVEs are reported for these packages.
backend/tests/apps/common/extensions_test.py (2)
143-154: LGTM!The test correctly mocks the cache version lookup (
mock_cache.get.return_value = 1) to simulate the version-aware caching introduced inCacheExtension.generate_key(). This ensures the cache hit scenario works with the new versioning mechanism.
156-165: LGTM!The cache miss test appropriately mocks the version lookup while using
side_effectonget_or_setto simulate the miss behavior. This properly tests the new caching flow.frontend/src/components/CardDetailsPage.tsx (2)
51-51: LGTM!The
setModuleOrderprop is correctly added to the component's destructured props, enabling parent components to pass the ordering callback.
316-321: LGTM!The
setModuleOrdercallback is properly passed toModuleCardalong with the necessaryaccessLevelandadminsprops, enabling the reordering functionality for admin users.backend/apps/mentorship/api/internal/mutations/module.py (1)
83-83: LGTM!Calculating the new position as the count of existing modules ensures newly created modules are appended at the end of the list.
frontend/src/app/my/mentorship/programs/[programKey]/page.tsx (1)
33-34: LGTM!The mutation hook is correctly set up without an
onErrorhandler since error handling is done in thesetModuleOrderfunction itself with rollback logic.frontend/__tests__/unit/components/ModuleCard.test.tsx (2)
103-125: LGTM!Comprehensive tests for
getSimpleDurationcovering edge cases: missing dates, invalid dates, and correct duration calculations with both ISO strings and Unix timestamps.
169-206: LGTM!Good coverage of admin-specific reordering functionality:
- Visibility of "Customize order" button based on access level
- State transitions between normal and reordering modes
- Callback invocation on save
Consider adding a test that verifies the order changes after a simulated drag operation in a future iteration.
frontend/src/components/ModuleCard.tsx (2)
229-244: LGTM!The
getSimpleDurationutility function correctly handles:
- Missing start/end dates
- Invalid date parsing
- Both ISO string and Unix timestamp inputs
- Proper pluralization of "week/weeks"
163-224: LGTM!The
ModuleItemcomponent correctly integrates with DnD Kit:
- Uses
useSortablehook with proper disabled state- Applies transform and transition styles
- Handles drag overlay opacity
- Conditionally renders the drag handle for admin users in reordering mode
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/apps/common/extensions.py (1)
92-98: Prefer enum comparison over string comparison for operation type.The string comparison
"OperationType.MUTATION"is fragile—if the enum's string representation changes in future Strawberry versions, this would break silently. Use theOperationTypeenum directly for type-safe comparison.Proposed fix
from strawberry.extensions import SchemaExtension from strawberry.permission import PermissionExtension from strawberry.schema import Schema +from strawberry.types import OperationType from strawberry.utils.str_converters import to_camel_caseclass CacheInvalidationExtension(SchemaExtension): """CacheInvalidationExtension class.""" def on_execute(self): """Invalidate cache on mutation.""" - if str(self.execution_context.operation_type) == "OperationType.MUTATION": + if self.execution_context.operation_type == OperationType.MUTATION: bump_cache_version()frontend/src/components/ModuleCard.tsx (2)
51-52: Consider adding a fallback for defensive coding.The non-null assertion on
draftModules!relies on the state machine being correct (i.e.,isReorderingis only true whendraftModulesis set). While this holds with current code, a fallback would be safer against future refactoring.Proposed fix
- const currentModules = isReordering ? draftModules! : modules + const currentModules = (isReordering && draftModules) ? draftModules : modules
198-206: Add accessible label to drag handle button.The drag handle button lacks accessible text for screen reader users. Consider adding
aria-labelandaria-roledescriptionfor better accessibility.Proposed fix
{isAdmin && isReordering && ( <button {...attributes} {...listeners} className="absolute top-2 right-2 cursor-grab text-gray-400" + aria-label={`Reorder ${module.name}`} + aria-roledescription="sortable" > <FaGripVertical /> </button> )}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
backend/apps/common/extensions.pyfrontend/package.jsonfrontend/src/components/ModuleCard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/package.json
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
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.
📚 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/ModuleCard.tsx
🧬 Code graph analysis (1)
frontend/src/components/ModuleCard.tsx (3)
backend/apps/mentorship/models/module.py (1)
Module(17-103)frontend/src/types/mentorship.ts (1)
Module(37-51)frontend/src/types/button.ts (1)
Button(4-9)
🔇 Additional comments (7)
backend/apps/common/extensions.py (3)
17-23: LGTM!The previous bug has been correctly addressed. The function now properly handles the cache miss case by checking for
Noneand initializing the version to1.
26-31: LGTM!The fallback logic correctly handles the case where the key doesn't exist by setting it to
2(simulating a bump from the initial value of1). There's a minor theoretical race betweenincrfailing andset(2), but this is benign—worst case, the version jumps, which still achieves the goal of cache invalidation.
68-71: LGTM!Version embedding in the cache key is a clean approach for invalidation. When the version bumps, new requests generate different keys, effectively bypassing stale cache entries. Using
sort_keys=Trueensures deterministic key generation.frontend/src/components/ModuleCard.tsx (4)
1-29: LGTM!The imports are well-organized and all dnd-kit dependencies are correctly imported for the drag-and-drop functionality.
31-42: LGTM!The props interface extension with optional
setModuleOrderis clean, and the state variables for managing reordering flow are appropriately typed.
77-83: LGTM! Previous concern addressed.The null check for
setModuleOrderanddraftModuleshas been properly added, addressing the previous review feedback.
231-246: LGTM!The
getSimpleDurationutility is well-implemented with proper input validation, handles both timestamp formats, and correctly pluralizes the output.
kart-u
force-pushed
the
add_module_ordering
branch
from
aceb5e2 to
7938a65
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
frontend/src/components/ModuleCard.tsx (1)
138-147: Non-null assertion in DragOverlay could cause runtime error.The
find()on line 141 could returnundefinedduring rapid state changes, but the non-null assertion would crash. This issue was flagged in the previous review but remains unaddressed.🔎 Recommended fix (from previous review)
+ const activeModule = activeId ? currentModules.find((m) => m.id === activeId) : null // ... <DragOverlay> - {activeId ? ( + {activeModule ? ( <ModuleItem - module={currentModules.find((m) => m.id === activeId)!} + module={activeModule} isAdmin={isAdmin} isReordering={isReordering} isOverlay /> ) : null} </DragOverlay>This guards against the edge case where
activeIdis set but the module isn't found.
🧹 Nitpick comments (3)
frontend/src/components/ModuleCard.tsx (1)
52-52: Consider a guard fordraftModulesto improve type safety.The non-null assertion assumes
draftModulesis always set whenisReorderingis true. While this holds given the currentstartReorderlogic, adding a guard makes the code more robust to future changes.🔎 Optional refactor
- const currentModules = isReordering ? draftModules! : modules + const currentModules = draftModules ?? modulesThis safely falls back to
modulesifdraftModulesis unexpectedly null.backend/apps/common/extensions.py (2)
17-23: Add type casting for defensive programming.While the cache should always store an integer (initialized to 1, incremented by
cache.incr()), different cache backends may handle serialization differently. Consider casting the returned value tointfor robustness.🔎 Suggested refactor
def get_cache_version() -> int: """Get the current cache version.""" version = cache.get(CACHE_VERSION_KEY) if version is None: cache.set(CACHE_VERSION_KEY, 1, timeout=None) return 1 - return version + return int(version)
26-31: Addtimeout=Nonefor consistency.The
cache.set()call should includetimeout=Noneto matchget_cache_version()and ensure the cache version persists indefinitely.🔎 Suggested refactor
def bump_cache_version() -> None: """Bump the cache version.""" try: cache.incr(CACHE_VERSION_KEY) except ValueError: - cache.set(CACHE_VERSION_KEY, 2) + cache.set(CACHE_VERSION_KEY, 2, timeout=None)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlfrontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/moduleMutations.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (15)
backend/apps/common/extensions.pybackend/apps/mentorship/api/internal/mutations/module.pybackend/apps/mentorship/api/internal/nodes/module.pybackend/apps/mentorship/api/internal/queries/module.pybackend/apps/mentorship/migrations/0007_module_position.pybackend/apps/mentorship/models/module.pybackend/settings/graphql.pybackend/tests/apps/common/extensions_test.pyfrontend/__tests__/unit/components/ModuleCard.test.tsxfrontend/package.jsonfrontend/src/app/my/mentorship/programs/[programKey]/page.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/ModuleCard.tsxfrontend/src/server/mutations/moduleMutations.tsfrontend/src/types/card.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- frontend/src/app/my/mentorship/programs/[programKey]/page.tsx
- backend/apps/mentorship/api/internal/nodes/module.py
- frontend/src/components/CardDetailsPage.tsx
- frontend/src/server/mutations/moduleMutations.ts
- frontend/tests/unit/components/ModuleCard.test.tsx
- frontend/package.json
- frontend/src/types/card.ts
- backend/settings/graphql.py
- backend/apps/mentorship/models/module.py
- backend/apps/mentorship/migrations/0007_module_position.py
- backend/apps/mentorship/api/internal/queries/module.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-13T05:55:46.436Z
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.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.py
📚 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-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/ModuleCard.tsx
📚 Learning: 2025-12-29T22:01:43.685Z
Learnt from: ShashaankS
Repo: OWASP/Nest PR: 3086
File: frontend/src/components/SortBy.tsx:45-45
Timestamp: 2025-12-29T22:01:43.685Z
Learning: In frontend/src/components/SortBy.tsx, the SelectItem uses hover:bg-transparent intentionally for better visual appeal and theme consistency. This design choice prioritizes a cleaner aesthetic over hover background feedback.
Applied to files:
frontend/src/components/ModuleCard.tsx
🧬 Code graph analysis (1)
backend/apps/mentorship/api/internal/mutations/module.py (4)
backend/apps/mentorship/api/internal/nodes/module.py (1)
SetModuleOrderInput(200-204)backend/apps/mentorship/api/internal/nodes/program.py (2)
ProgramNode(15-34)admins(32-34)backend/apps/mentorship/models/module.py (1)
Module(17-103)backend/apps/mentorship/models/program.py (1)
Program(18-87)
🔇 Additional comments (8)
backend/apps/mentorship/api/internal/mutations/module.py (1)
15-18: LGTM - Imports are appropriate.The new imports support the
set_module_ordermutation:SetModuleOrderInputprovides the input type andProgramNodeis the return type.frontend/src/components/ModuleCard.tsx (4)
1-37: LGTM - Props and imports are well-structured.The optional
setModuleOrderprop correctly allows external persistence while keeping the component reusable in contexts where reordering isn't needed.
62-90: LGTM - Reordering handlers are correctly implemented.The state management flow is sound:
startReorder→ drag operations →handleDragEndupdates draft →saveReorderpersists with proper guards (addressing the previous review comment).
94-116: LGTM - Admin controls are properly scoped.The UI correctly restricts reordering controls to admins and provides clear save/cancel actions during reordering mode.
173-235: LGTM - ModuleItem drag integration is well-implemented.The
useSortablehook is correctly configured with conditional disabling, and the visibility logic properly hides the original item during drag while showing the overlay. The grip handle is appropriately scoped to admins in reordering mode.backend/tests/apps/common/extensions_test.py (1)
146-146: LGTM! Mock updates align with versioned cache keys.The addition of
mock_cache.get.return_value = 1in both tests correctly simulates the cache version retrieval that now occurs insidegenerate_key(). Without these mocks,get_cache_version()would receive a MagicMock default instead of an integer, potentially breaking the key generation logic.Also applies to: 159-159
backend/apps/common/extensions.py (2)
14-14: LGTM! Clear constant definition.The cache version key uses an appropriate namespace prefix and descriptive name.
68-74: LGTM! Version-aware cache key generation.The updated
generate_key()correctly incorporates the cache version into the payload, ensuring that all cached queries are invalidated when the version is bumped. The deterministic JSON serialization withsort_keys=Truemaintains consistency.
kart-u
force-pushed
the
add_module_ordering
branch
from
15be8d7 to
73ab070
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
backend/apps/common/extensions.py (1)
92-98: Use direct enum comparison instead of string comparison.As discussed in the previous review, comparing the string representation of
operation_typeis fragile. Based on learnings, importOperationTypefromgraphql(graphql-core) and compare directly against the enum member.Proposed fix
Add the import at the top of the file:
+from graphql import OperationType from strawberry.extensions import SchemaExtensionThen update the comparison:
class CacheInvalidationExtension(SchemaExtension): """CacheInvalidationExtension class.""" def on_execute(self): """Invalidate cache on mutation.""" - if str(self.execution_context.operation_type) == "OperationType.MUTATION": + if self.execution_context.operation_type == OperationType.MUTATION: bump_cache_version()frontend/src/components/ModuleCard.tsx (1)
138-147: Avoid duplicate find() calls and non-null assertion.The code checks if the module exists but then calls
find()again with a non-null assertion, which could fail during rapid state changes. Store the result in a variable to avoid callingfind()twice and eliminate the unsafe assertion.Proposed fix
+ const activeModule = activeId ? currentModules.find((m) => m.id === activeId) : null + <DragOverlay> - {activeId && currentModules.find((m) => m.id === activeId) ? ( + {activeModule ? ( <ModuleItem - module={currentModules.find((m) => m.id === activeId)!} + module={activeModule} isAdmin={isAdmin} isReordering={isReordering} isOverlay /> ) : null} </DragOverlay>
🧹 Nitpick comments (2)
frontend/src/app/my/mentorship/programs/[programKey]/page.tsx (2)
87-111: Add success feedback after module reordering completes.The optimistic update and error rollback pattern is well-implemented, but there's no success toast to confirm the operation completed successfully. Users should receive feedback when their reordering is persisted.
🔎 Proposed enhancement
setModules(moduleOrder) await updateOrder({ variables: { input } }) + addToast({ + title: 'Module order updated', + description: 'The module order has been successfully saved.', + variant: 'solid', + color: 'success', + timeout: 3000, + }) } catch (err) { setModules(prevModuleOrder) handleAppError(err)
101-104: Use property shorthand for cleaner code.Line 102 uses
programKey: programKey, which can be simplified using ES6 shorthand property syntax.🔎 Proposed refactor
const input = { - programKey: programKey, - moduleKeys: moduleKeys, + programKey, + moduleKeys, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlfrontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/moduleMutations.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (15)
backend/apps/common/extensions.pybackend/apps/mentorship/api/internal/mutations/module.pybackend/apps/mentorship/api/internal/nodes/module.pybackend/apps/mentorship/api/internal/queries/module.pybackend/apps/mentorship/migrations/0007_module_position.pybackend/apps/mentorship/models/module.pybackend/settings/graphql.pybackend/tests/apps/common/extensions_test.pyfrontend/__tests__/unit/components/ModuleCard.test.tsxfrontend/package.jsonfrontend/src/app/my/mentorship/programs/[programKey]/page.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/ModuleCard.tsxfrontend/src/server/mutations/moduleMutations.tsfrontend/src/types/card.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/src/components/CardDetailsPage.tsx
- backend/apps/mentorship/migrations/0007_module_position.py
- backend/tests/apps/common/extensions_test.py
- frontend/src/server/mutations/moduleMutations.ts
- backend/apps/mentorship/api/internal/nodes/module.py
- backend/apps/mentorship/models/module.py
- frontend/tests/unit/components/ModuleCard.test.tsx
- backend/apps/mentorship/api/internal/queries/module.py
🧰 Additional context used
🧠 Learnings (11)
📚 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/settings/graphql.pybackend/apps/mentorship/api/internal/mutations/module.pybackend/apps/common/extensions.py
📚 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/ModuleCard.tsx
📚 Learning: 2025-12-29T22:01:53.874Z
Learnt from: ShashaankS
Repo: OWASP/Nest PR: 3086
File: frontend/src/components/SortBy.tsx:45-45
Timestamp: 2025-12-29T22:01:53.874Z
Learning: In frontend/src/components/SortBy.tsx, the SelectItem uses hover:bg-transparent intentionally for better visual appeal and theme consistency. This design choice prioritizes a cleaner aesthetic over hover background feedback.
Applied to files:
frontend/src/components/ModuleCard.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/app/my/mentorship/programs/[programKey]/page.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/app/my/mentorship/programs/[programKey]/page.tsxbackend/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:
frontend/src/app/my/mentorship/programs/[programKey]/page.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. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/page.tsx
📚 Learning: 2025-07-13T05:55:46.436Z
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.
Applied to files:
backend/apps/mentorship/api/internal/mutations/module.pybackend/apps/common/extensions.py
📚 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.pybackend/apps/common/extensions.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-06-29T00:41:32.198Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1676
File: backend/apps/owasp/graphql/filters/project_health_metrics.py:17-22
Timestamp: 2025-06-29T00:41:32.198Z
Learning: In the OWASP Nest codebase, when implementing GraphQL filters that convert string values to enums (like ProjectLevel), do not catch ValueError exceptions for invalid values. Let the errors propagate to provide proper error responses to GraphQL clients rather than silently ignoring invalid input.
Applied to files:
backend/apps/common/extensions.py
🧬 Code graph analysis (4)
frontend/src/types/card.ts (2)
backend/apps/mentorship/models/module.py (1)
Module(17-103)frontend/src/types/mentorship.ts (1)
Module(37-51)
backend/settings/graphql.py (1)
backend/apps/common/extensions.py (2)
CacheExtension(54-89)CacheInvalidationExtension(92-98)
frontend/src/components/ModuleCard.tsx (2)
frontend/src/types/mentorship.ts (1)
Module(37-51)frontend/src/types/button.ts (1)
Button(5-11)
backend/apps/mentorship/api/internal/mutations/module.py (4)
backend/apps/mentorship/api/internal/nodes/module.py (1)
SetModuleOrderInput(200-204)backend/apps/mentorship/api/internal/nodes/program.py (2)
ProgramNode(15-34)admins(32-34)backend/apps/mentorship/models/program.py (1)
Program(18-87)backend/apps/mentorship/models/module.py (1)
Module(17-103)
🔇 Additional comments (18)
frontend/package.json (1)
21-23: LGTM! DnD Kit dependencies added correctly.The three @dnd-kit packages are standard, well-maintained libraries for implementing drag-and-drop functionality. The version ranges follow best practices.
backend/apps/mentorship/api/internal/mutations/module.py (2)
15-18: LGTM! Imports support the new mutation.The imports for
SetModuleOrderInputandProgramNodeare correctly added to support the newset_module_ordermutation.
113-113: LGTM! Position correctly assigned on module creation.The newly created module is assigned
position=new_position, ensuring it's appended at the end of the module sequence.frontend/src/types/card.ts (1)
71-71: LGTM! Type definition correctly extends DetailsCardProps.The optional
setModuleOrderprop is appropriately typed and aligns with the new module ordering functionality.backend/settings/graphql.py (1)
44-46: LGTM! Cache invalidation extension correctly positioned.The
CacheInvalidationExtensionis placed beforeCacheExtensionin the extensions list, ensuring cache versions are bumped on mutations before subsequent queries check the cache. This ordering is critical for the version-based invalidation strategy to work correctly.frontend/src/app/my/mentorship/programs/[programKey]/page.tsx (3)
10-10: LGTM! Mutation document correctly imported.The
SetModuleOrderDocumentimport supports the new module ordering functionality.
33-33: LGTM! Mutation hook properly initialized.The
useMutationhook forSetModuleOrderDocumentis correctly set up.
156-156: LGTM! Handler correctly wired to DetailsCard.The
setModuleOrderhandler is properly passed to theDetailsCardcomponent, enabling module reordering functionality.backend/apps/common/extensions.py (3)
17-23: LGTM!The cache miss handling is now correct. The function properly checks for
Noneand initializes the version to 1 when needed.
26-31: LGTM!The
cache.incr()call correctly raisesValueErrorwhen the key doesn't exist, and the fallback initialization to 2 is appropriate.
68-74: LGTM!Including the cache version in the key payload effectively implements version-based cache invalidation. This ensures stale cached results are automatically invalidated when mutations occur.
frontend/src/components/ModuleCard.tsx (7)
1-43: LGTM!The imports, props, and state management are well-structured for implementing drag-and-drop reordering. The addition of
setModuleOrderprop enables external persistence of the reordered modules, and the state variables (isReordering,draftModules,activeId) properly support the draft-commit pattern for reordering.
45-83: LGTM!The drag event handlers and reordering workflow are correctly implemented. The
handleDragEndproperly usesarrayMoveto reorder items, and the state management follows best practices.
84-90: LGTM!The null check for
setModuleOrderbefore calling it correctly prevents runtime errors when the optional prop is not provided.
92-116: LGTM!The admin controls provide clear affordances for the reordering workflow, with distinct states for viewing ("Customize order") versus editing ("Save order" / "Cancel"). The UI properly restricts reordering to admin users as required by the PR objectives.
117-137: LGTM!The
DndContextandSortableContextare properly configured with appropriate collision detection and sorting strategy. The event handler wiring correctly manages the drag lifecycle.
173-235: LGTM!The
ModuleItemcomponent correctly integrates with dnd-kit'suseSortablehook and handles the drag state appropriately. The logic for hiding the original item during drag (shouldHideOriginal) prevents visual duplication, and the conditional rendering of the drag handle properly restricts interaction to admin users during reordering mode.
239-254: LGTM!The
getSimpleDurationhelper correctly handles various input formats (string/number timestamps), validates the dates, and formats the duration in a user-friendly way. Exporting it for unit tests is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
backend/apps/mentorship/api/internal/mutations/module.py (1)
439-461: Consider evaluating the queryset once for efficiency.The
existing_modulesqueryset is evaluated twice—once in the set comprehension (Line 440) and again in the dict comprehension (Line 451). While the impact is minimal for typical module counts, converting to a list upfront would be more efficient.🔎 Proposed optimization
- existing_modules = Module.objects.filter(program=program) - existing_module_keys = {m.key for m in existing_modules} + existing_modules = list(Module.objects.filter(program=program)) + existing_module_keys = {m.key for m in existing_modules}This forces a single database query and ensures the list is reused in the subsequent dict comprehension at Line 451.
frontend/src/components/ModuleCard.tsx (4)
52-53: Consider removing non-null assertion for safer code.The
draftModules!non-null assertion on line 52 is technically safe given the current control flow (startReorder always sets draftModules before isReordering), but it's a code smell that could cause issues if the logic changes.Proposed fix
- const currentModules = isReordering ? draftModules! : modules + const currentModules = (isReordering && draftModules) ? draftModules : modules
138-147: Optimize to avoid doublefind()call.The
find()is called twice - once for the guard check and once for the prop. Extract to a variable to avoid the redundant lookup.Proposed fix
<DragOverlay> - {activeId && currentModules.find((m) => m.id === activeId) ? ( + {(() => { + const activeModule = activeId ? currentModules.find((m) => m.id === activeId) : null + return activeModule ? ( <ModuleItem - module={currentModules.find((m) => m.id === activeId)!} + module={activeModule} isAdmin={isAdmin} isReordering={isReordering} isOverlay /> - ) : null} + ) : null + })()} </DragOverlay>Or define
activeModuleearlier in the component:const activeModule = activeId ? currentModules.find((m) => m.id === activeId) : null // ... in JSX: <DragOverlay> {activeModule ? ( <ModuleItem module={activeModule} isAdmin={isAdmin} isReordering={isReordering} isOverlay /> ) : null} </DragOverlay>
96-99: Consider using the same Button component for consistency.The "Customize order" button uses a native
<button>withonClick, while Save/Cancel use@heroui/buttonwithonPress. Consider unifying for consistent styling and behavior.
240-255: Consider handling edge cases for duration calculation.The function works correctly for typical cases, but consider these edge cases:
- If
end < start(dates reversed), the result is negative weeks.- If
start === end, the result is "0 weeks" which may look odd.Proposed defensive handling
export const getSimpleDuration = (start: string | number, end: string | number): string => { if (!start || !end) return 'N/A' const startDate = typeof start === 'number' ? new Date(start * 1000) : new Date(start) const endDate = typeof end === 'number' ? new Date(end * 1000) : new Date(end) if (Number.isNaN(startDate.getTime()) || Number.isNaN(endDate.getTime())) { return 'Invalid duration' } const ms = endDate.getTime() - startDate.getTime() + if (ms <= 0) return 'N/A' const days = Math.floor(ms / (1000 * 60 * 60 * 24)) - const weeks = Math.ceil(days / 7) + const weeks = Math.max(1, Math.ceil(days / 7)) return `${weeks} week${weeks !== 1 ? 's' : ''}` }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/mentorship/api/internal/mutations/module.pyfrontend/src/components/ModuleCard.tsx
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-07-13T05:55:46.436Z
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.
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 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:
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-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-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: 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/ModuleCard.tsx
📚 Learning: 2025-12-29T22:01:53.874Z
Learnt from: ShashaankS
Repo: OWASP/Nest PR: 3086
File: frontend/src/components/SortBy.tsx:45-45
Timestamp: 2025-12-29T22:01:53.874Z
Learning: In frontend/src/components/SortBy.tsx, the SelectItem uses hover:bg-transparent intentionally for better visual appeal and theme consistency. This design choice prioritizes a cleaner aesthetic over hover background feedback.
Applied to files:
frontend/src/components/ModuleCard.tsx
🧬 Code graph analysis (1)
frontend/src/components/ModuleCard.tsx (2)
frontend/src/types/mentorship.ts (1)
Module(37-51)frontend/src/types/button.ts (1)
Button(5-11)
🔇 Additional comments (7)
backend/apps/mentorship/api/internal/mutations/module.py (3)
15-15: LGTM! Clean imports for the new mutation.The new imports are properly organized and necessary for the
set_module_ordermutation functionality.Also applies to: 18-18
80-80: LGTM! Race condition properly addressed.The use of
select_for_update()andprogram.modules.count()correctly prevents duplicate position assignments during concurrent module creation. This addresses the critical race condition flagged in previous reviews.Also applies to: 83-83, 112-112
406-438: LGTM! Solid permission checks and error handling.The mutation properly validates that the user is both a mentor and a program admin before allowing reordering. Error messages are clear and logging is appropriate for debugging permission issues.
frontend/src/components/ModuleCard.tsx (4)
1-30: LGTM!The imports are well-organized and appropriate for the drag-and-drop functionality using dnd-kit.
72-83: LGTM!The drag handlers are well-implemented. The arrayMove call correctly uses findIndex to locate items, and the state updates are properly sequenced.
84-90: LGTM!The null check for
setModuleOrderwas properly addressed from the previous review.
173-236: LGTM!The ModuleItem component correctly implements the sortable behavior with proper accessibility support (aria-label on drag handle). The visibility logic during drag operations is well-handled.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/components/ModuleCard.tsx (1)
138-147: Add non-null assertion tofind()result in DragOverlay.Line 141 passes
find()result to themoduleprop, butModuleItemexpectsmodule: Modulewhilefind()returnsModule | undefined. This creates a type mismatch.Although the runtime guard on line 139 ensures
find()will succeed, TypeScript cannot infer this correlation. You need to either add a non-null assertion or extract a variable.🔎 Proposed fix
Option 1: Non-null assertion (simpler)
<DragOverlay> {activeId && currentModules.some((m) => m.id === activeId) ? ( <ModuleItem - module={currentModules.find((m) => m.id === activeId)} + module={currentModules.find((m) => m.id === activeId)!} isAdmin={isAdmin} isReordering={isReordering} isOverlay /> ) : null} </DragOverlay>Option 2: Extract variable (more explicit)
+ const activeModule = activeId ? currentModules.find((m) => m.id === activeId) : null <DragOverlay> - {activeId && currentModules.some((m) => m.id === activeId) ? ( + {activeModule ? ( <ModuleItem - module={currentModules.find((m) => m.id === activeId)} + module={activeModule} isAdmin={isAdmin} isReordering={isReordering} isOverlay /> ) : null} </DragOverlay>
🧹 Nitpick comments (2)
frontend/src/components/ModuleCard.tsx (2)
32-54: Consider a more defensive check fordraftModules.Line 52 uses a non-null assertion (
draftModules!) which is logically safe sinceisReorderingis onlytruewhendraftModulesis set. However, for defensive programming and better type safety, consider an explicit guard:🔎 Suggested refinement
- const currentModules = isReordering ? draftModules! : modules + const currentModules = isReordering && draftModules ? draftModules : modulesThis makes the null check explicit and aligns with TypeScript's type narrowing.
94-116: Add spacing around ternary operators.Missing spaces around ternary operators on lines 96 and 110 reduce readability.
🔎 Formatting fix
- {isReordering ?( + {isReordering ? ( <> <Button type="button" onPress={saveReorder} color="primary" className="font-medium"> Save order </Button> <Button type="button" variant="bordered" onPress={cancelReorder} className="font-medium" > Cancel </Button> </> - ):( + ) : ( <button onClick={startReorder} className="text-blue-400 hover:underline"> Customize order </button> )}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ModuleCard.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 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/ModuleCard.tsx
📚 Learning: 2025-12-29T22:01:53.874Z
Learnt from: ShashaankS
Repo: OWASP/Nest PR: 3086
File: frontend/src/components/SortBy.tsx:45-45
Timestamp: 2025-12-29T22:01:53.874Z
Learning: In frontend/src/components/SortBy.tsx, the SelectItem uses hover:bg-transparent intentionally for better visual appeal and theme consistency. This design choice prioritizes a cleaner aesthetic over hover background feedback.
Applied to files:
frontend/src/components/ModuleCard.tsx
🧬 Code graph analysis (1)
frontend/src/components/ModuleCard.tsx (2)
frontend/src/types/mentorship.ts (1)
Module(37-51)frontend/src/types/button.ts (1)
Button(5-11)
🔇 Additional comments (3)
frontend/src/components/ModuleCard.tsx (3)
1-30: LGTM: Imports are well-organized.All necessary dependencies for drag-and-drop functionality are properly imported, including dnd-kit components, the grip icon, and existing utilities.
62-90: LGTM: Reordering handlers are correctly implemented.All event handlers properly manage state transitions:
startReorderinitializes draft statecancelReordercleans up all reordering statehandleDragEndusesarrayMovecorrectly with early returns for no-op casessaveReorderincludes proper null guards (previous review comment addressed)
173-255: LGTM: ModuleItem and helper function are well-implemented.The drag-and-drop integration is solid:
useSortablehook properly configured with conditional disable- Transform and opacity provide good visual feedback during drag
shouldHideOriginallogic correctly hides the original item during drag- Drag handle includes accessible
aria-label(previous review comment addressed)getSimpleDurationhelper properly handles edge cases and is exported for testing
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/apps/mentorship/api/internal/mutations/module.py (1)
450-460: Consider adding a position-change check to skip unnecessary updates.The current implementation updates all modules' positions regardless of whether they changed. For programs with many modules where only a few positions changed, this could be optimized.
🔎 Proposed optimization
modules_by_key = {m.key: m for m in existing_modules} modules_to_update = [] for index, module_key in enumerate(input_data.module_keys): module = modules_by_key[module_key] - module.position = index - modules_to_update.append(module) + if module.position != index: + module.position = index + modules_to_update.append(module) - Module.objects.bulk_update(modules_to_update, ["position"]) + if modules_to_update: + Module.objects.bulk_update(modules_to_update, ["position"]) return programBased on learnings, Strawberry GraphQL automatically converts the returned
Programinstance toProgramNode, so the return type is correct.frontend/src/components/ModuleCard.tsx (1)
52-52: Consider defensive check instead of non-null assertion.The non-null assertion on
draftModules!assumesisReorderinganddraftModulesare always in sync. While the current control flow ensures this, it's fragile to future changes.🔎 More defensive alternative
- const currentModules = isReordering ? draftModules! : modules + const currentModules = isReordering && draftModules ? draftModules : modulesThis gracefully falls back to
modulesif the invariant is ever broken, preventing runtime errors.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/mentorship/api/internal/mutations/module.pyfrontend/src/components/ModuleCard.tsx
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-07-13T05:55:46.436Z
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.
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 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:
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-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-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: 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/ModuleCard.tsx
📚 Learning: 2025-12-29T22:01:53.874Z
Learnt from: ShashaankS
Repo: OWASP/Nest PR: 3086
File: frontend/src/components/SortBy.tsx:45-45
Timestamp: 2025-12-29T22:01:53.874Z
Learning: In frontend/src/components/SortBy.tsx, the SelectItem uses hover:bg-transparent intentionally for better visual appeal and theme consistency. This design choice prioritizes a cleaner aesthetic over hover background feedback.
Applied to files:
frontend/src/components/ModuleCard.tsx
🧬 Code graph analysis (2)
backend/apps/mentorship/api/internal/mutations/module.py (4)
backend/apps/mentorship/api/internal/nodes/module.py (1)
SetModuleOrderInput(200-204)backend/apps/mentorship/api/internal/nodes/program.py (2)
ProgramNode(15-34)admins(32-34)backend/apps/mentorship/models/program.py (1)
Program(18-87)backend/apps/mentorship/models/module.py (1)
Module(17-103)
frontend/src/components/ModuleCard.tsx (2)
frontend/src/types/mentorship.ts (1)
Module(37-51)frontend/src/types/button.ts (1)
Button(5-11)
🔇 Additional comments (12)
backend/apps/mentorship/api/internal/mutations/module.py (6)
12-18: LGTM!The new imports for
SetModuleOrderInputandProgramNodeare correctly added and necessary for the newset_module_ordermutation.
79-83: LGTM!The use of
select_for_update()on the Program correctly prevents race conditions when computingnew_positionfor concurrent module creations. The position is correctly assigned as the current module count, placing new modules at the end.
101-113: LGTM!The
position=new_positionassignment correctly initializes the module's position based on the count computed under the row lock.
406-417: LGTM!The
select_for_update()on the Program row correctly serializes concurrentset_module_orderrequests for the same program, preventing race conditions. The transaction.atomic decorator ensures the lock is held until commit.
419-437: LGTM!The permission checks are thorough: the mutation correctly validates that the user is both a Mentor and an admin of the program, with appropriate logging for access denials.
439-448: LGTM!The validation logic is correct:
- Checks for duplicate keys in the input to prevent position conflicts.
- Ensures the input contains exactly all existing modules in the program, preventing partial reordering or inclusion of invalid keys.
frontend/src/components/ModuleCard.tsx (6)
1-30: LGTM: Imports are clean and appropriate.All imports are properly organized and used. DnD Kit dependencies (@dnd-kit/core, @dnd-kit/sortable, @dnd-kit/utilities) are the standard packages for implementing drag-and-drop functionality, and FaGripVertical provides the visual drag handle affordance.
32-39: LGTM: Props interface design is sound.Making
setModuleOrderoptional is the correct design choice, allowing the component to function in read-only contexts while supporting persistence when the callback is provided.
62-90: LGTM: Reordering logic is well-structured and safe.The handlers correctly manage the draft state pattern:
startReordercreates a draft copy before enabling reorder modehandleDragEndsafely updates the draft usingarrayMovewith proper guardssaveReorderincludes proper null checks before invoking the callbackcancelReordercomprehensively resets all reordering stateThe past review's concern about null-checking
setModuleOrderhas been properly addressed.
94-116: LGTM: Admin controls are properly structured.The conditional UI correctly shows Save/Cancel during reordering and a "Customize order" trigger otherwise. Admin-only access is properly gated, and all buttons have clear labels and proper event handlers.
173-236: LGTM: ModuleItem integration with DnD Kit is well-implemented.The component properly integrates sortable functionality:
useSortableis correctly configured withdisabledstate when not reordering- Transform, transition, and opacity provide smooth visual feedback
- The
shouldHideOriginallogic correctly hides the original item during drag while showing the overlay- Drag handle button includes proper
aria-labelfor accessibility (as addressed in past reviews)- The spread of
attributesandlistenerson the drag handle enables both mouse and keyboard interactionsBased on learnings: The direct access to
module.experienceLevelon line 222 is safe since this field is required in the Module type.
240-255: LGTM: Duration calculation utility is robust.The function handles both string and Unix timestamp inputs, includes proper validation for invalid dates, and uses
Math.ceilto round up partial weeks, which is appropriate for duration display. The export is intentional for test usage.
kart-u
force-pushed
the
add_module_ordering
branch
from
99bf137 to
a18f2e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/apps/common/extensions.py (2)
92-98: ImportOperationTypefromgraphqland use direct enum comparison.As confirmed in the previous review and per the learnings, the string comparison should be replaced with a direct enum comparison using
OperationTypeimported from thegraphqlpackage.Add this import at the top of the file:
from graphql import OperationTypeThen update the comparison:
class CacheInvalidationExtension(SchemaExtension): """CacheInvalidationExtension class.""" def on_execute(self): """Invalidate cache on mutation.""" - if str(self.execution_context.operation_type) == "OperationType.MUTATION": + if self.execution_context.operation_type == OperationType.MUTATION: bump_cache_version()Based on learnings: Import
OperationTypefrom thegraphql(graphql-core) package as Strawberry uses it under the hood.
95-98: Addyieldstatement toon_execute()to pass control to the next extension hook.Strawberry's
SchemaExtension.on_execute()must be a generator that yields to continue the execution pipeline. Withoutyield, the extension chain halts prematurely and subsequent extensions or execution phases do not run.Additionally, simplify the operation type comparison by using the enum directly instead of string comparison:
Suggested fix
def on_execute(self): """Invalidate cache on mutation.""" - if str(self.execution_context.operation_type) == "OperationType.MUTATION": + if self.execution_context.operation_type == OperationType.MUTATION: bump_cache_version() + yield
🧹 Nitpick comments (3)
backend/apps/mentorship/api/internal/mutations/module.py (1)
439-440: Materialize the queryset to avoid double database hits.The
existing_modulesqueryset is iterated once to buildexisting_module_keys(line 440), and then again to buildmodules_by_key(line 450). Consider materializing it to a list to avoid executing two queries.🔎 Proposed fix
- existing_modules = Module.objects.filter(program=program) + existing_modules = list(Module.objects.filter(program=program)) existing_module_keys = {m.key for m in existing_modules}frontend/src/app/my/mentorship/programs/[programKey]/page.tsx (1)
33-33: AddonErrorhandler to the mutation for consistency.The
updateProgrammutation (line 29-31) includes anonError: handleAppErrorhandler, butupdateOrderdoesn't. While errors are caught in the try-catch block, GraphQL network errors or mutation-level errors might not propagate to the catch block depending on Apollo Client configuration.🔎 Proposed fix
- const [updateOrder] = useMutation(SetModuleOrderDocument) + const [updateOrder] = useMutation(SetModuleOrderDocument, { + onError: handleAppError, + })frontend/src/components/ModuleCard.tsx (1)
52-52: The non-null assertion ondraftModules!is safe but implicit.The assertion works because
isReorderingis only set totrueviastartReorder(), which first setsdraftModules. However, this coupling is not immediately obvious to readers.Consider making the invariant explicit with a guard:
const currentModules = isReordering && draftModules ? draftModules : modulesThis removes the assertion while maintaining the same behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlfrontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/moduleMutations.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (15)
backend/apps/common/extensions.pybackend/apps/mentorship/api/internal/mutations/module.pybackend/apps/mentorship/api/internal/nodes/module.pybackend/apps/mentorship/api/internal/queries/module.pybackend/apps/mentorship/migrations/0007_module_position.pybackend/apps/mentorship/models/module.pybackend/settings/graphql.pybackend/tests/apps/common/extensions_test.pyfrontend/__tests__/unit/components/ModuleCard.test.tsxfrontend/package.jsonfrontend/src/app/my/mentorship/programs/[programKey]/page.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/ModuleCard.tsxfrontend/src/server/mutations/moduleMutations.tsfrontend/src/types/card.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/apps/mentorship/api/internal/nodes/module.py
- frontend/src/components/CardDetailsPage.tsx
- backend/apps/mentorship/migrations/0007_module_position.py
- frontend/tests/unit/components/ModuleCard.test.tsx
- backend/settings/graphql.py
- backend/apps/mentorship/models/module.py
- backend/tests/apps/common/extensions_test.py
- frontend/package.json
🧰 Additional context used
🧠 Learnings (16)
📚 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/app/my/mentorship/programs/[programKey]/page.tsxbackend/apps/mentorship/api/internal/mutations/module.py
📚 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/app/my/mentorship/programs/[programKey]/page.tsxfrontend/src/components/ModuleCard.tsx
📚 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:
frontend/src/app/my/mentorship/programs/[programKey]/page.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. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/page.tsx
📚 Learning: 2025-07-13T05:55:46.436Z
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.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/page.tsxbackend/apps/mentorship/api/internal/mutations/module.pybackend/apps/common/extensions.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-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.pybackend/apps/common/extensions.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.pybackend/apps/common/extensions.pybackend/apps/mentorship/api/internal/queries/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.pybackend/apps/common/extensions.pybackend/apps/mentorship/api/internal/queries/module.py
📚 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/ModuleCard.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Applied to files:
frontend/src/components/ModuleCard.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/ModuleCard.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/ModuleCard.tsx
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
frontend/src/components/ModuleCard.tsx
📚 Learning: 2025-12-29T22:01:53.874Z
Learnt from: ShashaankS
Repo: OWASP/Nest PR: 3086
File: frontend/src/components/SortBy.tsx:45-45
Timestamp: 2025-12-29T22:01:53.874Z
Learning: In frontend/src/components/SortBy.tsx, the SelectItem uses hover:bg-transparent intentionally for better visual appeal and theme consistency. This design choice prioritizes a cleaner aesthetic over hover background feedback.
Applied to files:
frontend/src/components/ModuleCard.tsx
📚 Learning: 2025-06-29T00:41:32.198Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1676
File: backend/apps/owasp/graphql/filters/project_health_metrics.py:17-22
Timestamp: 2025-06-29T00:41:32.198Z
Learning: In the OWASP Nest codebase, when implementing GraphQL filters that convert string values to enums (like ProjectLevel), do not catch ValueError exceptions for invalid values. Let the errors propagate to provide proper error responses to GraphQL clients rather than silently ignoring invalid input.
Applied to files:
backend/apps/common/extensions.py
🔇 Additional comments (17)
backend/apps/mentorship/api/internal/queries/module.py (2)
25-25: LGTM! Ordering by position first is correct.The secondary sort by
started_atensures deterministic ordering when modules have equal positions (e.g., during migration or if positions aren't set).
29-36: Verify whetherget_project_modulesshould also order by position.
get_program_modulesnow orders by("position", "started_at"), butget_project_modulesstill uses only"started_at". If projects display modules that should respect admin-defined ordering, consider adding position ordering here as well.backend/apps/mentorship/api/internal/mutations/module.py (4)
12-18: LGTM! Imports are correctly added.The new
SetModuleOrderInputandProgramNodeimports support the new mutation.
79-83: LGTM! Race condition properly addressed.Using
select_for_update()on the program and calculatingnew_positionfromprogram.modules.count()prevents concurrent module creations from producing duplicate positions.
101-113: LGTM! Module creation correctly assigns position.New modules are appended at the end with
position=new_position.
406-460: LGTM! Theset_module_ordermutation is well-structured.The implementation correctly:
- Uses
select_for_update()to serialize concurrent reordering operations- Validates admin permissions
- Checks for duplicate keys and ensures all modules are included
- Uses
bulk_updatefor efficient database writesThe validation logic is sound and follows existing patterns in the codebase.
frontend/src/types/card.ts (1)
71-71: LGTM! Type definition is correct.The optional
setModuleOrderprop with(order: Module[]) => voidsignature correctly represents the callback for persisting module order changes.frontend/src/server/mutations/moduleMutations.ts (1)
63-69: LGTM! Mutation definition is correct.The minimal response (
idonly) is appropriate since the frontend already performs optimistic updates and doesn't need the full program data returned.frontend/src/app/my/mentorship/programs/[programKey]/page.tsx (2)
87-111: LGTM! Optimistic update pattern is correctly implemented.The implementation:
- Validates admin permissions before proceeding
- Stores previous state for rollback
- Updates local state optimistically before the mutation
- Rolls back on error with proper error handling
One minor note: consider adding a success toast similar to
updateStatusfor better UX feedback.
142-158: LGTM! Props are correctly wired.The
setModuleOrderprop is properly passed toDetailsCard, enabling the module reordering flow.frontend/src/components/ModuleCard.tsx (5)
1-30: LGTM! Imports are well-organized.The dnd-kit imports are appropriately selected for sortable list functionality.
72-90: LGTM! Drag handlers are correctly implemented.
handleDragEndproperly usesarrayMovefrom dnd-kit, andsaveReorderincludes the null check forsetModuleOrder(addressed from past review).
117-148: LGTM! DnD context is well-structured.The implementation correctly:
- Uses
closestCornerscollision detection- Provides a
DragOverlayfor smooth visual feedback- Hides the original item during drag via
shouldHideOriginal
173-236: LGTM! ModuleItem is well-implemented.The component correctly:
- Uses
useSortablewith conditionaldisabledstate- Applies transform/transition styles from dnd-kit
- Includes accessible
aria-labelon the drag handle (addressed from past review)- Conditionally hides the original during drag to prevent duplication
240-255: LGTM! Duration utility is well-implemented.The function handles:
- Missing start/end values
- Both timestamp (number) and ISO string inputs
- Invalid date validation
- Proper pluralization
Using
Math.ceilfor weeks ensures partial weeks round up, which is user-friendly.backend/apps/common/extensions.py (2)
17-23: LGTM! Cache version initialization is correct.The fix from the previous review properly handles the cache miss scenario by checking for
Noneand initializing the version to 1.
68-71: LGTM! Version-based cache key generation is correct.Including the cache version in the serialized key data ensures all cache keys change when
bump_cache_version()is called, effectively invalidating all cached entries. This aligns with the PR's goal of mutation-triggered cache invalidation.
| try: | ||
| admin = Mentor.objects.get(nest_user=user) | ||
| except Mentor.DoesNotExist as err: | ||
| msg = "You must be a mentor to update a program." |
There was a problem hiding this comment.
@kart-u this check is incorrect. The basic structure is like this:
- Admin -> can create and manage programs AND modules, add mentors to modules;
- Mentor -> can only edit modules they are assigned to.
So admin does not have to be a mentor to update a program. But they have to be an admin of the program. Like the check you have below on L430-437.
There was a problem hiding this comment.
hello @kasya
In create_module it was specified that only a mentor can create modules
except Mentor.DoesNotExist as e: and and then it was checked whether the user is an admin
I took inspiration from this to decide the permissions. If the permissions should be as you specified above, should I change it accordingly?
also are not program admins a subset of mentor (as in specified program model)??
There was a problem hiding this comment.
@kart-u the permissions should be as I described above, yes. They were not set correctly initially when this project was setup and was something we missed.
As for your second point - kinda the same thing :) I'm working on updating that right now. Admins should not be a subset of mentors.
I understand this blocks you from continuing work on this task. I'll try to push updates asap.
There was a problem hiding this comment.
Hello @kasya,thanks for the clarification
How about I update admin to be a subset of nest users (if you fine with it?) and then proceed with the permission structure you described above?
There was a problem hiding this comment.
I have added a custom db volume in commit 0882712
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docker-compose/local/compose.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-26T06:08:58.549Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 3041
File: .github/workflows/run-ci-cd.yaml:233-243
Timestamp: 2025-12-26T06:08:58.549Z
Learning: Ensure Redis image versions stay in sync across all environments by updating every relevant YAML file together (docker-compose files and CI/CD workflow configurations). When upgrading Redis, bump the image version in all docker-compose files and in all CI workflow YAMLs in one coordinated change to maintain parity across production, staging, local, E2E, and CI tests.
Applied to files:
docker-compose/local/compose.yaml
2 participants
Proposed change
Resolves #3016
PR Description
A brief overview of what has been done in this PR:-
Caching Fix
Why Version-Based Caching
Demo:-
https://github.com/user-attachments/assets/ed78f960-f522-4dfa-9dc3-76e95bce084d
Checklist
make check-testlocally and all tests passed