feat: implement module ordering with drag-and-drop support#4020
Open
HarshitVerma109 wants to merge 7 commits intoOWASP:mainfrom
Open
feat: implement module ordering with drag-and-drop support#4020HarshitVerma109 wants to merge 7 commits intoOWASP:mainfrom
HarshitVerma109 wants to merge 7 commits intoOWASP:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds persistent module ordering (model field + migration), exposes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 23 files
Confidence score: 4/5
- Minor risk:
orderedModulesinfrontend/src/components/ModuleCard.tsxcan get out of sync withmodules, so users may see stale or incorrectly ordered modules after refreshes or additions. - Overall impact appears limited to UI freshness/order and is unlikely to be merge-blocking.
- Pay close attention to
frontend/src/components/ModuleCard.tsx-orderedModulesisn’t synced whenmoduleschanges, which can leave the list stale.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/ModuleCard.tsx">
<violation number="1" location="frontend/src/components/ModuleCard.tsx:50">
P2: `orderedModules` is initialized from the `modules` prop but never synced when `modules` changes, so the UI can show a stale list/order after data refreshes or new modules are added.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
frontend/__tests__/a11y/components/ModuleCard.a11y.test.tsx (1)
6-8: Consider returning a more completeuseMutationtuple.Apollo's
useMutationreturns[mutateFunction, { data, loading, error, called, reset }]. The mock only returns[jest.fn()], so ifModuleCardever destructures the second element (e.g., forloadingstate), this test will break.🔧 Suggested improvement
jest.mock('@apollo/client/react', () => ({ - useMutation: jest.fn(() => [jest.fn()]), + useMutation: jest.fn(() => [jest.fn(), { data: undefined, loading: false, error: undefined }]), }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/a11y/components/ModuleCard.a11y.test.tsx` around lines 6 - 8, The test-level mock for Apollo's useMutation in frontend/__tests__/a11y/components/ModuleCard.a11y.test.tsx returns only [jest.fn()] which breaks if ModuleCard destructures the second tuple element; update the mock for useMutation to return a full tuple such as [jest.fn(), { data: null, loading: false, error: null, called: false, reset: jest.fn() }] so any access to data/loading/error/called/reset in ModuleCard works during tests.frontend/src/components/ModuleCard.tsx (1)
163-179:SortableModuleItemshould be extracted or memoized to avoid unnecessary re-renders.Every item re-renders on each drag frame because
SortableModuleItemis defined inline in the same file without memoization. SinceModuleItemcontains images and links, this can cause visible jank during drag, especially with many modules. Consider wrapping withReact.memo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleCard.tsx` around lines 163 - 179, SortableModuleItem is causing full-list re-renders during drag because it's declared inline and not memoized; extract it (or move it to module scope) and wrap it with React.memo to prevent unnecessary renders of ModuleItem on each drag frame. Specifically, keep the useSortable call and style computation inside the memoized SortableModuleItem (or pass stable props down) and ensure dragHandleProps (attributes + listeners) and module prop are stable/compared properly so React.memo skips updates; update the export/usage site to import/use the extracted/memoized SortableModuleItem.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/mentorship/api/internal/mutations/module.py`:
- Around line 413-424: Before applying the reorder, validate that
input_data.module_keys exactly matches the program's module keys: query
Module.objects.filter(program=program).values_list("key", flat=True) (use the
same select_for_update context if needed), compare the sets (and check for
duplicates in input_data.module_keys), and if they differ raise a validation
error (or return a failed response) instead of proceeding; only after the
exact-match check pass should you build key_to_order and call
Module.objects.bulk_update(modules, ["order"]). This ensures no missing or
unknown keys are silently ignored and prevents order collisions.
In `@frontend/src/components/ModuleCard.tsx`:
- Around line 128-134: displayedModules.map((m) => m.key) can yield undefined
IDs for dnd-kit; change the SortableContext items to use the same fallback used
for React keys (module.key || module.id) and likewise update useSortable calls
in SortableModuleItem to pass id: module.key || module.id so all draggable IDs
are always defined and consistent with the rendered keys (refer to
SortableContext, displayedModules.map, useSortable, module.key, module.id, and
SortableModuleItem).
- Line 50: The local state orderedModules (created via useState(modules)) will
not update when the modules prop changes; update ModuleCard to resync
orderedModules by adding a useEffect that listens to changes in the modules prop
and calls setOrderedModules(modules), or alternatively require the parent to
force remount by passing a key derived from modules (e.g., modules.map(m =>
m.key).join(',')) to the ModuleCard component; reference the orderedModules /
setOrderedModules variables and the modules prop when implementing the useEffect
or when changing the call site to add the key.
- Around line 64-98: The race condition comes from closing over the
updater-scope `prev` in handleDragEnd's mutation .catch, so when a later drag
completes the failed catch can overwrite newer state; fix by tracking an
"isSaving" flag and disabling new drags while a mutation is in flight or by
using a functional rollback that only reverts the specific moved item: add a
local state like `isSaving` and set it true before calling `reorderModules` and
false in both then/catch (and return early from handleDragEnd when isSaving is
true), OR instead of calling setOrderedModules(prev) in the .catch, call
setOrderedModules(current => { compute and return current with the failed
operation undone by moving the item from its current index back to the original
index using the original oldIndex/newIndex captured for that operation });
reference the functions/vars: handleDragEnd, setOrderedModules, reorderModules,
and addToast when implementing the chosen fix.
---
Nitpick comments:
In `@frontend/__tests__/a11y/components/ModuleCard.a11y.test.tsx`:
- Around line 6-8: The test-level mock for Apollo's useMutation in
frontend/__tests__/a11y/components/ModuleCard.a11y.test.tsx returns only
[jest.fn()] which breaks if ModuleCard destructures the second tuple element;
update the mock for useMutation to return a full tuple such as [jest.fn(), {
data: null, loading: false, error: null, called: false, reset: jest.fn() }] so
any access to data/loading/error/called/reset in ModuleCard works during tests.
In `@frontend/src/components/ModuleCard.tsx`:
- Around line 163-179: SortableModuleItem is causing full-list re-renders during
drag because it's declared inline and not memoized; extract it (or move it to
module scope) and wrap it with React.memo to prevent unnecessary renders of
ModuleItem on each drag frame. Specifically, keep the useSortable call and style
computation inside the memoized SortableModuleItem (or pass stable props down)
and ensure dragHandleProps (attributes + listeners) and module prop are
stable/compared properly so React.memo skips updates; update the export/usage
site to import/use the extracted/memoized SortableModuleItem.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/apps/mentorship/api/internal/mutations/module.py (1)
415-415: InconsistentValidationErrorcall style.Lines 415 and 425 pass the message as a positional argument (
ValidationError(msg)) while every otherValidationErrorin the file uses the keyword form (ValidationError(message=msg)). Functionally equivalent, but inconsistent.♻️ Proposed fix
- raise ValidationError(msg) + raise ValidationError(message=msg)Apply the same change to line 425.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/mentorship/api/internal/mutations/module.py` at line 415, Two ValidationError raises use a positional argument (ValidationError(msg)) which is inconsistent with the rest of the file; change those calls to the keyword form by replacing ValidationError(msg) with ValidationError(message=msg) for both occurrences (the ValidationError calls referencing the local variable msg) so the call style matches other uses in the module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/ModuleCard.tsx`:
- Around line 69-107: handleDragEnd currently performs side effects
(setIsSaving, calling reorderModules and addToast) inside the setOrderedModules
functional updater which must be pure; refactor so you compute oldIndex/newIndex
and newOrder synchronously by reading orderedModules (or capturing prev at call
time), call setOrderedModules(newOrder) outside the updater, then
setIsSaving(true) and invoke reorderModules with moduleKeys: newOrder.map(m =>
m.key); on mutation failure use the previously captured priorOrder to rollback
via setOrderedModules(priorOrder) and show the addToast, and always
setIsSaving(false) in finally; keep references to handleDragEnd,
setOrderedModules, setIsSaving, reorderModules, addToast and arrayMove when
applying the change.
---
Duplicate comments:
In `@backend/apps/mentorship/api/internal/mutations/module.py`:
- Around line 413-425: Replace the current existence/count checks with a strict
set equality check against the program's full module key set: fetch all module
keys for the given program (using Module.objects.filter(program=program) and
values_list('key', flat=True)), compare set(input_data.module_keys) to that full
set and raise ValidationError if they differ, and then load all Module rows for
the program with a single select_for_update() to lock them before reordering;
remove the redundant duplicate-key check since the set comparison covers
missing, unknown, and duplicate keys, and keep using ValidationError and the
modules variable for the locked queryset used to apply new orders.
---
Nitpick comments:
In `@backend/apps/mentorship/api/internal/mutations/module.py`:
- Line 415: Two ValidationError raises use a positional argument
(ValidationError(msg)) which is inconsistent with the rest of the file; change
those calls to the keyword form by replacing ValidationError(msg) with
ValidationError(message=msg) for both occurrences (the ValidationError calls
referencing the local variable msg) so the call style matches other uses in the
module.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/ModuleCard.tsx">
<violation number="1" location="frontend/src/components/ModuleCard.tsx:66">
P2: orderedModules will not refresh when module content changes without key changes, leaving stale module info in the UI.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
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/ModuleCard.tsx (1)
334-349:⚠️ Potential issue | 🟡 MinorNegative or zero duration produces confusing output.
If
endDateprecedesstartDate,daysis negative andMath.ceil(days / 7)produces0or a negative number (e.g.,"-1 weeks"). Similarly, identical dates yield"0 weeks". Consider adding a guard:Proposed guard
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)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleCard.tsx` around lines 334 - 349, getSimpleDuration currently computes weeks using Math.ceil(days/7) which yields 0 or negative values when endDate <= startDate; add a guard after computing ms to handle non-positive durations: if ms < 0 return 'Invalid duration' (or another explicit string per UX) and if ms === 0 return '0 weeks' (or '0 week' pluralized correctly), then continue computing weeks only for ms > 0; update the return logic in getSimpleDuration to use this guard and ensure pluralization uses the computed weeks.
🧹 Nitpick comments (2)
frontend/src/components/ModuleCard.tsx (2)
92-113: Mutation sendsm.keywithout fallback — inconsistent with identifiers elsewhere.Line 98 maps
m.keyfor the mutation payload, but everywhere else (lines 57, 85–86, 135, 148, 183, etc.) the code usesm.key || m.idas the identifier. Ifkeyis ever an empty string (e.g., a newly created module before slug generation), the mutation will send an empty string while drag IDs usedm.id.Since the
Moduletype defineskey: stringas required, this is low-risk, but the inconsistency could bite if upstream data changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleCard.tsx` around lines 92 - 113, The mutation is sending m.key directly which is inconsistent with other code paths that use m.key || m.id; update the reorder payload in the reorderModules call to map identifiers with a fallback (e.g., moduleKeys: newOrder.map(m => m.key || m.id)) so the mutation uses the same stable identifier as drag/drop and other functions (keep programKey and the existing error handling with addToast, setIsSaving, and setOrderedModules unchanged).
54-66: Sorted signature ignores external order changes.
moduleKeysSignaturesorts the keys before joining, so if another admin reorders the same set of modules, the signature stays identical andorderedModuleswon't resync. This means stale ordering until the component remounts or a module is added/removed.If this is acceptable for now, consider adding a brief comment explaining the trade-off. Otherwise, drop the
.sort()or include theorderfield in the signature so server-side reorder changes propagate.Option: include order in signature to detect reordering
const moduleKeysSignature = useMemo( () => - [...modules] - .map((m) => m.key || m.id) - .sort() - .join(','), + modules + .map((m) => `${m.key || m.id}:${m.order ?? ''}`) + .join(','), [modules] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleCard.tsx` around lines 54 - 66, The current moduleKeysSignature uses .sort(), which masks reorder changes; update the signature logic used in the useMemo that defines moduleKeysSignature so it reflects module order (either remove the .sort() and join in current array order, or include each module's order field in the key string like `${m.key||m.id}:${m.order ?? index}`) so that the useEffect watching moduleKeysSignature will resync orderedModules when the server-side order changes; modify the useMemo mapping over modules (function/moduleKeysSignature) and ensure the dependency comment on the useEffect remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/src/components/ModuleCard.tsx`:
- Around line 334-349: getSimpleDuration currently computes weeks using
Math.ceil(days/7) which yields 0 or negative values when endDate <= startDate;
add a guard after computing ms to handle non-positive durations: if ms < 0
return 'Invalid duration' (or another explicit string per UX) and if ms === 0
return '0 weeks' (or '0 week' pluralized correctly), then continue computing
weeks only for ms > 0; update the return logic in getSimpleDuration to use this
guard and ensure pluralization uses the computed weeks.
---
Nitpick comments:
In `@frontend/src/components/ModuleCard.tsx`:
- Around line 92-113: The mutation is sending m.key directly which is
inconsistent with other code paths that use m.key || m.id; update the reorder
payload in the reorderModules call to map identifiers with a fallback (e.g.,
moduleKeys: newOrder.map(m => m.key || m.id)) so the mutation uses the same
stable identifier as drag/drop and other functions (keep programKey and the
existing error handling with addToast, setIsSaving, and setOrderedModules
unchanged).
- Around line 54-66: The current moduleKeysSignature uses .sort(), which masks
reorder changes; update the signature logic used in the useMemo that defines
moduleKeysSignature so it reflects module order (either remove the .sort() and
join in current array order, or include each module's order field in the key
string like `${m.key||m.id}:${m.order ?? index}`) so that the useEffect watching
moduleKeysSignature will resync orderedModules when the server-side order
changes; modify the useMemo mapping over modules (function/moduleKeysSignature)
and ensure the dependency comment on the useEffect remains correct.
Contributor
There was a problem hiding this comment.
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/ModuleCard.tsx (1)
343-347:⚠️ Potential issue | 🟡 Minor
getSimpleDurationreturns negative weeks for invalid date ranges.When
endedAt < startedAt,msis negative:Math.floor(ms / 86_400_000)is negative, andMath.ceil(days / 7)yields0or a negative integer (e.g.,-1), producing"0 weeks"or"-1 weeks"instead of'N/A'.🛡️ Proposed fix
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)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleCard.tsx` around lines 343 - 347, getSimpleDuration currently computes ms/days/weeks and can produce negative or nonsensical values when endDate < startDate; modify getSimpleDuration to detect invalid ranges (e.g., if endDate.getTime() < startDate.getTime() or ms < 0) and immediately return 'N/A', otherwise proceed to compute days and weeks from ms and return the formatted string; reference the function name getSimpleDuration and variables endDate, startDate, ms, days, weeks when making the change.
🧹 Nitpick comments (1)
frontend/src/components/ModuleCard.tsx (1)
333-347: MovegetSimpleDurationtoutils/.Exporting a pure date-math utility from a UI component file couples concerns and makes the function harder to discover and test in isolation. The AI summary already references
frontend/src/utils/getSimpleDuration.tsas a dependency path; placing the function there and updating the import inModuleCard.tsxwould align with the existing project layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleCard.tsx` around lines 333 - 347, Extract the pure function getSimpleDuration out of ModuleCard.tsx into a new module frontend/src/utils/getSimpleDuration.ts as a named export (preserving its signature and return values), delete the implementation from ModuleCard.tsx, and update ModuleCard.tsx to import { getSimpleDuration } from 'frontend/src/utils/getSimpleDuration'; ensure the moved function keeps the same type annotations and behavior (handling string | number inputs, NaN checks, and pluralization) and run/tests to confirm no behavior change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/src/components/ModuleCard.tsx`:
- Around line 343-347: getSimpleDuration currently computes ms/days/weeks and
can produce negative or nonsensical values when endDate < startDate; modify
getSimpleDuration to detect invalid ranges (e.g., if endDate.getTime() <
startDate.getTime() or ms < 0) and immediately return 'N/A', otherwise proceed
to compute days and weeks from ms and return the formatted string; reference the
function name getSimpleDuration and variables endDate, startDate, ms, days,
weeks when making the change.
---
Nitpick comments:
In `@frontend/src/components/ModuleCard.tsx`:
- Around line 333-347: Extract the pure function getSimpleDuration out of
ModuleCard.tsx into a new module frontend/src/utils/getSimpleDuration.ts as a
named export (preserving its signature and return values), delete the
implementation from ModuleCard.tsx, and update ModuleCard.tsx to import {
getSimpleDuration } from 'frontend/src/utils/getSimpleDuration'; ensure the
moved function keeps the same type annotations and behavior (handling string |
number inputs, NaN checks, and pluralization) and run/tests to confirm no
behavior change.
Contributor
Author
|
@kasya , please review it and let me know if any changes are required. |
kasya
requested changes
Feb 21, 2026
Collaborator
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@HarshitVerma109 thanks for working on this so quickly!
Ordering works good 👍🏼 Noticed some issues with creating new modules when we already have some that are ordered ⬇️
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/tests/apps/mentorship/model/module_test.py (2)
187-191: Redundant MagicMock layer intest_order_field_default.The mock instance is used solely to carry the value read from
Module._meta.get_field("order").default; the assertion effectively reduces to:assert Module._meta.get_field("order").default == 0Dropping the mock makes the intent immediately obvious.
♻️ Proposed simplification
def test_order_field_default(self): """Test that order field defaults to 0.""" - mock_module = MagicMock(spec=Module) - mock_module.order = Module._meta.get_field("order").default - assert mock_module.order == 0 + assert Module._meta.get_field("order").default == 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/mentorship/model/module_test.py` around lines 187 - 191, The test_order_field_default uses a redundant MagicMock just to hold Module._meta.get_field("order").default; remove the MagicMock and assert the default directly by replacing the mock-based assertion with assert Module._meta.get_field("order").default == 0 in the test_order_field_default function so the test clearly checks the Module model's order field default.
193-249: Missingmax_order=0edge case across both new order tests.The production expression is
(max_order or 0) + 1. Whenmax_order=0(e.g., an existing module still carrying the default value of0), theorshort-circuits to0and the new module getsorder=1— identical to the "no existing modules" case. The two tests covermax_order=3andmax_order=Nonebut leave this third branch undocumented.Adding a third parametric case makes the
or 0coalescion explicit and guards against future regressions if the expression is ever refactored to(max_order if max_order is not None else 0) + 1.♻️ Suggested additional test
`@patch`("apps.common.models.TimestampedModel.save") `@patch`("apps.mentorship.models.Module.objects") def test_save_auto_assigns_order_1_when_max_is_zero(self, mock_objects, mock_super_save): """Test that max_order=0 is treated the same as no existing modules (order=1).""" mock_module = MagicMock(spec=Module) mock_module.pk = None mock_module.name = "Zero Order Module" mock_module.program = MagicMock( started_at=django.utils.timezone.datetime(2024, 1, 1, tzinfo=django.utils.timezone.UTC), ended_at=django.utils.timezone.datetime(2024, 12, 31, tzinfo=django.utils.timezone.UTC), ) mock_module.started_at = django.utils.timezone.datetime(2024, 1, 1, tzinfo=django.utils.timezone.UTC) mock_module.ended_at = django.utils.timezone.datetime(2024, 12, 31, tzinfo=django.utils.timezone.UTC) mock_objects.filter.return_value.aggregate.return_value = {"max_order": 0} Module.save(mock_module) assert mock_module.order == 1 mock_super_save.assert_called_once()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/mentorship/model/module_test.py` around lines 193 - 249, Add a third unit test covering the max_order == 0 edge case so Module.save's production expression "(max_order or 0) + 1" is exercised; create a test similar to test_save_auto_assigns_order_for_new_module that patches TimestampedModel.save and Module.objects, constructs a new Module-like MagicMock (pk = None, program/start/end dates), set mock_objects.filter.return_value.aggregate.return_value = {"max_order": 0}, call Module.save(mock_module), and assert mock_module.order == 1 and that the super save (TimestampedModel.save) was called once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/tests/apps/mentorship/model/module_test.py`:
- Around line 187-191: The test_order_field_default uses a redundant MagicMock
just to hold Module._meta.get_field("order").default; remove the MagicMock and
assert the default directly by replacing the mock-based assertion with assert
Module._meta.get_field("order").default == 0 in the test_order_field_default
function so the test clearly checks the Module model's order field default.
- Around line 193-249: Add a third unit test covering the max_order == 0 edge
case so Module.save's production expression "(max_order or 0) + 1" is exercised;
create a test similar to test_save_auto_assigns_order_for_new_module that
patches TimestampedModel.save and Module.objects, constructs a new Module-like
MagicMock (pk = None, program/start/end dates), set
mock_objects.filter.return_value.aggregate.return_value = {"max_order": 0}, call
Module.save(mock_module), and assert mock_module.order == 1 and that the super
save (TimestampedModel.save) was called once.
Contributor
Author
|
@kasya , please review it and let me know if any changes are required. |
kasya
previously approved these changes
Feb 21, 2026
Collaborator
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
Works great now!
Thanks @HarshitVerma109 👍🏼
arkid15r
requested changes
Feb 21, 2026
HarshitVerma109
dismissed stale reviews from kasya and coderabbitai[bot]
via
f9ba301
Contributor
Author
|
@arkid15r , please review it and let me know if any changes are required. |
HarshitVerma109
force-pushed
the
feat/module-ordering
branch
from
f9ba301 to
a447272
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
Resolves #3016
This PR adds a feature that lets Program Admins reorder modules by simply dragging and dropping them
Screen.Recording.2026-02-21.023207.mp4
Checklist
make check-testlocally: all warnings addressed, tests passed