MXWAR-82: fix(groups): staffId, externalId and dates silently dropped in create and edit group API calls#110
Conversation
📝 WalkthroughWalkthroughThe pull request extends group create/update payloads by adding two extended request interfaces with optional fields (staffId, externalId, submittedOnDate, activationDate, dateFormat, locale). Create and edit handlers now build and send these extended-typed payloads, normalizing and conditionally including fields before API calls. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/groups/groups-view/group-actions/EditGroups.tsx (1)
40-40:⚠️ Potential issue | 🟡 Minor
externalIdis never prefilled, and there is no way to clear an existing one.The effect at lines 44–71 deliberately skips populating
externalIdfromgroupData(per the comment on line 66), so a user editing a group cannot see the current value. Combined with the conditional at line 95 (if (externalId.trim())only adds it to the payload when non‑empty), this means:
- A user who only wants to change
namebut happens to type anexternalIdto "see what's there" will overwrite the existing one.- There is no way for a user to clear an existing
externalIdthrough this form (empty input ⇒ field omitted ⇒ server keeps the old value).Recommend prefilling from
groupData.externalId(extendingExtendedGroupResponseif it isn't there yet) so the form reflects current state, and treating an explicit empty string as "clear" rather than "skip" if that operation is supported by the API.Also applies to: 66-66, 95-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/groups-view/group-actions/EditGroups.tsx` at line 40, The form never pre-fills or lets users clear externalId: update the component to initialize and sync externalId from groupData by setting useState to groupData.externalId (and in the useEffect that currently skips externalId) and ensure ExtendedGroupResponse includes externalId so TypeScript knows the property exists; when building the update payload (where externalId is currently only added under if (externalId.trim())), change the logic to always include externalId (allowing an explicit empty string to be sent to clear the value) or explicitly send null/empty per the API contract instead of omitting the field.
🧹 Nitpick comments (2)
src/pages/groups/groups-view/group-actions/EditGroups.tsx (1)
84-97: Drop theRecord<string, unknown>indirection and type the payload directly.Typing the payload as
Record<string, unknown>and then casting toExtendedPutGroupsRequestat the call site bypasses the type safety the new interface was introduced to provide. Since every field is optional onExtendedPutGroupsRequest, you can declare the payload with that type and drop the cast.♻️ Suggested refactor
- const payload: Record<string, unknown> = { + const payload: ExtendedPutGroupsRequest = { name: name.trim(), locale: 'en', dateFormat: 'dd MMMM yyyy', } if (staffId) payload.staffId = Number(staffId) const sub = inputToFineractDate(submittedOn) if (sub) payload.submittedOnDate = sub const act = inputToFineractDate(activationOn) if (act) payload.activationDate = act if (externalId.trim()) payload.externalId = externalId.trim() - await groupsApi.update13(Number(id), payload as ExtendedPutGroupsRequest) + await groupsApi.update13(Number(id), payload)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/groups-view/group-actions/EditGroups.tsx` around lines 84 - 97, The payload is currently declared as Record<string, unknown> then cast to ExtendedPutGroupsRequest which defeats type safety; change the declaration of payload to be typed as ExtendedPutGroupsRequest directly (e.g., const payload: ExtendedPutGroupsRequest = { ... }) and keep the existing conditional assignments for staffId, submittedOn/activationOn (using inputToFineractDate), and externalId, then call groupsApi.update13(Number(id), payload) without a cast so the compiler ensures the shape matches ExtendedPutGroupsRequest.src/pages/groups/types.ts (1)
59-75: Optional: deduplicate the shared optional fields.
ExtendedPostGroupsRequestandExtendedPutGroupsRequestdeclare an identical set of six optional fields. Extracting a shared mixin keeps them in sync if/when the workaround is updated as the upstream OpenAPI spec evolves.♻️ Suggested refactor
+interface GroupRequestExtras { + staffId?: number + externalId?: string + submittedOnDate?: string + activationDate?: string + dateFormat?: string + locale?: string +} + -export interface ExtendedPostGroupsRequest extends PostGroupsRequest { - staffId?: number - externalId?: string - submittedOnDate?: string - activationDate?: string - dateFormat?: string - locale?: string -} - -export interface ExtendedPutGroupsRequest extends PutGroupsGroupIdRequest { - staffId?: number - externalId?: string - submittedOnDate?: string - activationDate?: string - dateFormat?: string - locale?: string -} +export interface ExtendedPostGroupsRequest + extends PostGroupsRequest, + GroupRequestExtras {} + +export interface ExtendedPutGroupsRequest + extends PutGroupsGroupIdRequest, + GroupRequestExtras {}It would also be worth leaving a short comment near these interfaces (mirroring the one above
ExtendedGroupResponse) explaining that the generatedPostGroupsRequest/PutGroupsGroupIdRequesttypes are missing fields the Fineract API actually accepts — so future maintainers know why the local extension exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/types.ts` around lines 59 - 75, Both ExtendedPostGroupsRequest and ExtendedPutGroupsRequest duplicate the same six optional fields; create a shared mixin/interface (e.g., GroupsRequestExtras) declaring staffId, externalId, submittedOnDate, activationDate, dateFormat, locale and have ExtendedPostGroupsRequest and ExtendedPutGroupsRequest extend that mixin instead of repeating the fields; also add a short comment above these interfaces (mirroring the note above ExtendedGroupResponse) explaining this local extension exists because the generated PostGroupsRequest / PutGroupsGroupIdRequest types omit fields accepted by the Fineract API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/groups/CreateGroups.tsx`:
- Around line 80-93: The create handler currently sends ISO dates while
declaring dateFormat 'dd MMMM yyyy', so convert form date inputs using
inputToFineractDate before calling groupsApi.create8: pass submittedOnDate:
inputToFineractDate(formData.submittedOnDate) (it's required) and
activationDate: formData.active && formData.activationDate ?
inputToFineractDate(formData.activationDate) : undefined; keep
staffId/externalId coercions as before and cast to ExtendedPostGroupsRequest;
then add an end-to-end verification (manual step or test) that the create
payload contains staffId/externalId/submittedOnDate/activationDate and that
retrieveOne15 returns those persisted values.
---
Outside diff comments:
In `@src/pages/groups/groups-view/group-actions/EditGroups.tsx`:
- Line 40: The form never pre-fills or lets users clear externalId: update the
component to initialize and sync externalId from groupData by setting useState
to groupData.externalId (and in the useEffect that currently skips externalId)
and ensure ExtendedGroupResponse includes externalId so TypeScript knows the
property exists; when building the update payload (where externalId is currently
only added under if (externalId.trim())), change the logic to always include
externalId (allowing an explicit empty string to be sent to clear the value) or
explicitly send null/empty per the API contract instead of omitting the field.
---
Nitpick comments:
In `@src/pages/groups/groups-view/group-actions/EditGroups.tsx`:
- Around line 84-97: The payload is currently declared as Record<string,
unknown> then cast to ExtendedPutGroupsRequest which defeats type safety; change
the declaration of payload to be typed as ExtendedPutGroupsRequest directly
(e.g., const payload: ExtendedPutGroupsRequest = { ... }) and keep the existing
conditional assignments for staffId, submittedOn/activationOn (using
inputToFineractDate), and externalId, then call groupsApi.update13(Number(id),
payload) without a cast so the compiler ensures the shape matches
ExtendedPutGroupsRequest.
In `@src/pages/groups/types.ts`:
- Around line 59-75: Both ExtendedPostGroupsRequest and ExtendedPutGroupsRequest
duplicate the same six optional fields; create a shared mixin/interface (e.g.,
GroupsRequestExtras) declaring staffId, externalId, submittedOnDate,
activationDate, dateFormat, locale and have ExtendedPostGroupsRequest and
ExtendedPutGroupsRequest extend that mixin instead of repeating the fields; also
add a short comment above these interfaces (mirroring the note above
ExtendedGroupResponse) explaining this local extension exists because the
generated PostGroupsRequest / PutGroupsGroupIdRequest types omit fields accepted
by the Fineract API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b37bccc8-83ec-4671-984b-fd7f5e67f731
📒 Files selected for processing (3)
src/pages/groups/CreateGroups.tsxsrc/pages/groups/groups-view/group-actions/EditGroups.tsxsrc/pages/groups/types.ts
adity-a34
force-pushed
the
MXWAR-82-fix-groups-silent-field-drop
branch
from
e2f1d78 to
49eaf0e
Compare
|
recheck @coderabbitai |
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
… calls Signed-off-by: Aditya Tiwari <adityatiwari342005@gmail.com>
adity-a34
force-pushed
the
MXWAR-82-fix-groups-silent-field-drop
branch
from
49eaf0e to
d0c838b
Compare
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)
src/pages/groups/groups-view/group-actions/EditGroups.tsx (1)
66-66:⚠️ Potential issue | 🟡 MinorEdge case: users cannot clear an existing
externalIdthrough this form.Per the comment,
externalIdis intentionally left blank on initial load, and line 95 only addsexternalIdto the payload when the trimmed value is non-empty. Net effect: a user who wants to remove an existing external ID has no way to do so through this form (submitting an empty field is a no-op rather than a clear). If clearing isn't a supported workflow, consider initializing the field with the current value (group.externalId) so the user at least sees what's stored and edits it explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/groups-view/group-actions/EditGroups.tsx` at line 66, The form currently leaves externalId blank and only adds it to the payload when trimmed value is non-empty, preventing users from clearing an existing externalId; update the EditGroups form so externalId is initialized with group.externalId (so users see and can edit the current value) and/or change the submit logic in the onSubmit handler to include externalId in the payload even when it's an empty string (or explicitly send null) so submitting an empty field clears the stored value; locate the externalId input state and the payload-building code in EditGroups (where group.externalId and the trimmed check are used) and implement one of these fixes to allow clearing.
🧹 Nitpick comments (3)
src/pages/groups/CreateGroups.tsx (1)
85-94: Date format issue resolved — payload now matchesdateFormat.
inputToFineractDatecorrectly converts the HTML<Input type="date">ISO output to thedd MMMM yyyyformat declared in the payload, matching the edit handler. One minor follow-up:externalId: formData.externalId || undefinedwill still send a whitespace-only string (e.g." "), whereasEditGroups.tsxusesexternalId.trim()(line 95). Consider mirroring the trim here for consistency.♻️ Proposed minor tweak
- externalId: formData.externalId || undefined, + externalId: formData.externalId.trim() || undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/CreateGroups.tsx` around lines 85 - 94, The payload may include whitespace-only externalId because CreateGroups uses "externalId: formData.externalId || undefined"; update this to mirror EditGroups by trimming the value before deciding to send it (use formData.externalId?.trim() and fall back to undefined) so the externalId field in the ExtendedPostGroupsRequest only contains a meaningful non-empty string or undefined.src/pages/groups/types.ts (1)
64-85: Optional: deduplicate the shared extension fields.Both
ExtendedPostGroupsRequestandExtendedPutGroupsRequestdeclare the exact same six optional fields. Consider extracting them to a shared type to keep the two in sync if more fields are discovered later.♻️ Proposed refactor
+interface ExtendedGroupsRequestFields { + staffId?: number + externalId?: string + submittedOnDate?: string + activationDate?: string + dateFormat?: string + locale?: string +} + export interface ExtendedPostGroupsRequest extends PostGroupsRequest { - staffId?: number - externalId?: string - submittedOnDate?: string - activationDate?: string - dateFormat?: string - locale?: string -} +} +export interface ExtendedPostGroupsRequest + extends PostGroupsRequest, + ExtendedGroupsRequestFields {} -export interface ExtendedPutGroupsRequest extends PutGroupsGroupIdRequest { - staffId?: number - externalId?: string - submittedOnDate?: string - activationDate?: string - dateFormat?: string - locale?: string -} +export interface ExtendedPutGroupsRequest + extends PutGroupsGroupIdRequest, + ExtendedGroupsRequestFields {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/types.ts` around lines 64 - 85, Both ExtendedPostGroupsRequest and ExtendedPutGroupsRequest declare the same six optional fields; extract those shared fields into a single reusable type (e.g., GroupRequestExtras or GroupRequestCommon) and have ExtendedPostGroupsRequest and ExtendedPutGroupsRequest extend their original bases plus that new shared type so the fields are declared only once and stay in sync (update the declarations for ExtendedPostGroupsRequest and ExtendedPutGroupsRequest to use the new shared type).src/pages/groups/groups-view/group-actions/EditGroups.tsx (1)
84-97: Fix looks good; consider typingpayloaddirectly to drop the cast.The full-payload submit correctly resolves the silent drop on update. Minor: declaring
payloadasRecord<string, unknown>and then casting at the call site bypasses compile-time checks for field name/type mistakes. You can type it asExtendedPutGroupsRequestfrom the start and still conditionally assign optional fields.♻️ Proposed refactor
- const payload: Record<string, unknown> = { + const payload: ExtendedPutGroupsRequest = { name: name.trim(), locale: 'en', dateFormat: 'dd MMMM yyyy', } if (staffId) payload.staffId = Number(staffId) const sub = inputToFineractDate(submittedOn) if (sub) payload.submittedOnDate = sub const act = inputToFineractDate(activationOn) if (act) payload.activationDate = act if (externalId.trim()) payload.externalId = externalId.trim() - await groupsApi.update13(Number(id), payload as ExtendedPutGroupsRequest) + await groupsApi.update13(Number(id), payload)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/groups-view/group-actions/EditGroups.tsx` around lines 84 - 97, The payload variable is currently typed as Record<string, unknown> and then cast at groupsApi.update13, which hides type errors; change the declaration of payload to use ExtendedPutGroupsRequest directly, keep the same conditional assignments (staffId, submittedOn -> inputToFineractDate, activationOn -> inputToFineractDate, externalId) and ensure optional fields use the correct property names (e.g., submittedOnDate, activationDate) so you can call groupsApi.update13(Number(id), payload) without a cast.
🤖 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 `@src/pages/groups/groups-view/group-actions/EditGroups.tsx`:
- Line 66: The form currently leaves externalId blank and only adds it to the
payload when trimmed value is non-empty, preventing users from clearing an
existing externalId; update the EditGroups form so externalId is initialized
with group.externalId (so users see and can edit the current value) and/or
change the submit logic in the onSubmit handler to include externalId in the
payload even when it's an empty string (or explicitly send null) so submitting
an empty field clears the stored value; locate the externalId input state and
the payload-building code in EditGroups (where group.externalId and the trimmed
check are used) and implement one of these fixes to allow clearing.
---
Nitpick comments:
In `@src/pages/groups/CreateGroups.tsx`:
- Around line 85-94: The payload may include whitespace-only externalId because
CreateGroups uses "externalId: formData.externalId || undefined"; update this to
mirror EditGroups by trimming the value before deciding to send it (use
formData.externalId?.trim() and fall back to undefined) so the externalId field
in the ExtendedPostGroupsRequest only contains a meaningful non-empty string or
undefined.
In `@src/pages/groups/groups-view/group-actions/EditGroups.tsx`:
- Around line 84-97: The payload variable is currently typed as Record<string,
unknown> and then cast at groupsApi.update13, which hides type errors; change
the declaration of payload to use ExtendedPutGroupsRequest directly, keep the
same conditional assignments (staffId, submittedOn -> inputToFineractDate,
activationOn -> inputToFineractDate, externalId) and ensure optional fields use
the correct property names (e.g., submittedOnDate, activationDate) so you can
call groupsApi.update13(Number(id), payload) without a cast.
In `@src/pages/groups/types.ts`:
- Around line 64-85: Both ExtendedPostGroupsRequest and ExtendedPutGroupsRequest
declare the same six optional fields; extract those shared fields into a single
reusable type (e.g., GroupRequestExtras or GroupRequestCommon) and have
ExtendedPostGroupsRequest and ExtendedPutGroupsRequest extend their original
bases plus that new shared type so the fields are declared only once and stay in
sync (update the declarations for ExtendedPostGroupsRequest and
ExtendedPutGroupsRequest to use the new shared type).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 627f3c38-4e37-4000-9494-7b6b161fcf20
📒 Files selected for processing (3)
src/pages/groups/CreateGroups.tsxsrc/pages/groups/groups-view/group-actions/EditGroups.tsxsrc/pages/groups/types.ts
…alId, init and clear externalId in edit Signed-off-by: Aditya Tiwari <adityatiwari342005@gmail.com>
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)
src/pages/groups/CreateGroups.tsx (1)
162-173:⚠️ Potential issue | 🟡 MinorActivation date input should be
requiredwhenactiveis checked.The submitted-on input (Line 193) is
required, but the activation date input has norequiredattribute. Withactive=trueand an emptyactivationDate, the payload becomes{ active: true, activationDate: undefined }, which Fineract will reject server-side — surfacing as a generic alert to the user instead of inline form validation. Since the field is conditionally rendered only whenactiveis checked, marking it required is safe and matches the form-level handling ofsubmittedOnDate.🛡️ Proposed fix
<Input type="date" value={formData.activationDate} onChange={e => setFormData(p => ({ ...p, activationDate: e.target.value })) } + required />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/CreateGroups.tsx` around lines 162 - 173, The activation date input rendered when formData.active is true is missing the required attribute, causing server-side rejections; update the conditional block in CreateGroups (the Input with value={formData.activationDate} and onChange that calls setFormData) to include required so the browser enforces entry when active is checked, matching the submittedOnDate behavior and preventing undefined activationDate in the payload.
🧹 Nitpick comments (1)
src/pages/groups/CreateGroups.tsx (1)
81-94: Optional: declare the payload as a typed const instead of casting.The edit handler in
EditGroups.tsxusesconst payload: ExtendedPutGroupsRequest = { ... }(per the relevant snippet), which lets TypeScript catch property typos and shape drift. Theas ExtendedPostGroupsRequestassertion here disables that check — for example, a typo likestaffIDwould compile silently. Aligning with the edit-side style also makes the two flows easier to compare.♻️ Proposed refactor
- await groupsApi.create8({ + const payload: ExtendedPostGroupsRequest = { name: formData.name, officeId: Number(formData.officeId), active: formData.active, staffId: formData.staffId ? Number(formData.staffId) : undefined, externalId: formData.externalId.trim() || undefined, submittedOnDate: inputToFineractDate(formData.submittedOnDate), activationDate: formData.active && formData.activationDate ? inputToFineractDate(formData.activationDate) : undefined, dateFormat: 'dd MMMM yyyy', locale: 'en', - } as ExtendedPostGroupsRequest) + } + await groupsApi.create8(payload)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/CreateGroups.tsx` around lines 81 - 94, Create a properly typed payload const instead of using a type assertion on the API call: build a const payload: ExtendedPostGroupsRequest = { name: formData.name, officeId: Number(formData.officeId), active: formData.active, staffId: formData.staffId ? Number(formData.staffId) : undefined, externalId: formData.externalId.trim() || undefined, submittedOnDate: inputToFineractDate(formData.submittedOnDate), activationDate: formData.active && formData.activationDate ? inputToFineractDate(formData.activationDate) : undefined, dateFormat: 'dd MMMM yyyy', locale: 'en' } and then call groupsApi.create8(payload); this preserves the existing conversions/logic (staffId, externalId, activationDate) while letting TypeScript validate the shape instead of using the as ExtendedPostGroupsRequest cast.
🤖 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 `@src/pages/groups/CreateGroups.tsx`:
- Around line 162-173: The activation date input rendered when formData.active
is true is missing the required attribute, causing server-side rejections;
update the conditional block in CreateGroups (the Input with
value={formData.activationDate} and onChange that calls setFormData) to include
required so the browser enforces entry when active is checked, matching the
submittedOnDate behavior and preventing undefined activationDate in the payload.
---
Nitpick comments:
In `@src/pages/groups/CreateGroups.tsx`:
- Around line 81-94: Create a properly typed payload const instead of using a
type assertion on the API call: build a const payload: ExtendedPostGroupsRequest
= { name: formData.name, officeId: Number(formData.officeId), active:
formData.active, staffId: formData.staffId ? Number(formData.staffId) :
undefined, externalId: formData.externalId.trim() || undefined, submittedOnDate:
inputToFineractDate(formData.submittedOnDate), activationDate: formData.active
&& formData.activationDate ? inputToFineractDate(formData.activationDate) :
undefined, dateFormat: 'dd MMMM yyyy', locale: 'en' } and then call
groupsApi.create8(payload); this preserves the existing conversions/logic
(staffId, externalId, activationDate) while letting TypeScript validate the
shape instead of using the as ExtendedPostGroupsRequest cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 004d5adc-6132-463f-86dc-8cbc5ef0bc40
📒 Files selected for processing (3)
src/pages/groups/CreateGroups.tsxsrc/pages/groups/groups-view/group-actions/EditGroups.tsxsrc/pages/groups/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/groups/groups-view/group-actions/EditGroups.tsx
1 participant
Description
The generated
PostGroupsRequestandPutGroupsGroupIdRequesttypes fromfineract.yamlare missing several fields the Fineract API actually accepts. This caused two silent data-loss bugs:staffId,externalId,submittedOnDate, andactivationDatewere commented out to satisfy TypeScript and never sent to the API.{ name }was passed togroupsApi.update13(), discarding everything else.Fix: extended the generated types locally in
types.tswithExtendedPostGroupsRequestandExtendedPutGroupsRequest, and updated both components to pass the complete payload.Related issues and discussion
[MXWAR-82](https://mifosforge.jira.com/browse/MXWAR-82)
Screenshots, if any
Before: Edit Group PUT payload:
{ "name": "Bug Test grp" }After: Edit Group PUT payload:
{ "name": "Bug Test grp", "staffId": 1, "externalId": "EXT-111", "submittedOnDate": "08 April 2026", "activationDate": "24 April 2026", "dateFormat": "dd MMMM yyyy", "locale": "en" }Checklist
CONTRIBUTING.md.Summary by CodeRabbit