WEB-892 feat: Extract close client payload construction into utility …#3439
Open
Agaba-Ed wants to merge 1 commit intoopenMF:devfrom
Open
WEB-892 feat: Extract close client payload construction into utility …#3439Agaba-Ed wants to merge 1 commit intoopenMF:devfrom
Agaba-Ed wants to merge 1 commit intoopenMF:devfrom
Conversation
|
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:
Note
|
| Cohort / File(s) | Summary |
|---|---|
Close Client Component src/app/clients/clients-view/client-actions/close-client/close-client.component.ts |
Removed inline payload building and date handling from submit(); changed injected fields to private readonly; now calls buildCloseClientPayload(...) and passes returned payload to executeClientCommand('close', ...). |
Close Client Utilities src/app/clients/clients-view/client-actions/close-client/close-client.utils.ts |
Added CloseClientFormValue / CloseClientPayload exports, isValidDate() type guard, and buildCloseClientPayload() to coerce/validate closureReasonId, normalize closureDate (format Date via injected dateUtils or accept validated trimmed strings), and attach dateFormat/locale. |
Close Client Tests src/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.ts |
New comprehensive Jest tests covering date formatting behaviors, invalid-date and invalid-reasonId error cases, immutability of input, boundary cases, numeric-string handling for closureReasonId, and isValidDate() semantics. |
Sequence Diagram(s)
sequenceDiagram
participant User as User
participant Component as CloseClientComponent
participant Util as buildCloseClientPayload
participant DateUtils as dateUtils.formatDate
participant API as Server API
User->>Component: submit(form)
Component->>Util: buildCloseClientPayload(form, dateUtils, locale, dateFormat)
alt closureDate is Date
Util->>DateUtils: formatDate(Date, dateFormat)
DateUtils-->>Util: formattedDate
else closureDate is string
Util-->>Util: trim and validate string
end
Util-->>Component: payload (closureDate:string, closureReasonId, dateFormat, locale)
Component->>API: executeClientCommand('close', payload)
API-->>Component: response
Component->>User: show result
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- WEB-882: Add unit tests for CloseClientComponent #3425 — Touches CloseClientComponent submit/payload and date-formatting behavior; likely related.
Suggested reviewers
- gkbishnoi07
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly summarizes the main change: extracting close client payload construction into a utility function for improved testability and maintainability. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/app/clients/clients-view/client-actions/close-client/close-client.component.ts`:
- Line 93: The form currently uses UntypedFormGroup and passes
this.closeClientForm.value (with closureReasonId possibly a string) directly to
buildCloseClientPayload, causing validateClosureReasonId to throw; fix by
normalizing closureReasonId to a number in submit() before calling
buildCloseClientPayload (e.g. read const fv = this.closeClientForm.value; set
fv.closureReasonId = Number(fv.closureReasonId) or parseInt(...) and handle
NaN/invalid cases) or alternatively replace UntypedFormGroup with a typed
FormGroup interface so closureReasonId is a number at compile time; ensure the
change touches the submit() method and references to buildCloseClientPayload,
closureReasonId, validateClosureReasonId.
In
`@src/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.ts`:
- Around line 1-2: This test file is missing the required MPL 2.0 license
header; add the standard MPL 2.0 header block at the very top of the file that
contains the imports and tests referencing buildCloseClientPayload,
CloseClientFormValue, and isValidDate, then run the repository header tooling
(npm run headers:add and npm run headers:check) to ensure the header is
formatted correctly and committed.
In
`@src/app/clients/clients-view/client-actions/close-client/close-client.utils.ts`:
- Around line 81-82: The code checks typeof formValue.closureDate === 'string'
and uses trim() for validation but returns the untrimmed value; update the
branch in close-client.utils.ts so that closureDate is set to the normalized
trimmed string (e.g., closureDate = formValue.closureDate.trim()) before
returning the payload to avoid sending whitespace-padded dates to the API; keep
the existing empty-string guard in place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0af15b21-4874-4ece-8ad3-67aebbc85d61
📒 Files selected for processing (3)
src/app/clients/clients-view/client-actions/close-client/close-client.component.tssrc/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.tssrc/app/clients/clients-view/client-actions/close-client/close-client.utils.ts
src/app/clients/clients-view/client-actions/close-client/close-client.component.ts
Show resolved
Hide resolved
src/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.ts
Show resolved
Hide resolved
src/app/clients/clients-view/client-actions/close-client/close-client.utils.ts
Outdated
Show resolved
Hide resolved
Agaba-Ed
force-pushed
the
close-client-utility
branch
from
500e69a to
e938738
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.ts (1)
58-65: Add regression cases for malformed-but-non-empty string dates (and stringclosureReasonIdbehavior).Current cases don’t explicitly lock behavior for whitespace-only and malformed non-empty date strings, and they also don’t assert the utility’s string
closureReasonIdconversion path.Suggested test additions
describe('closureDate validation', () => { + it.each([ + ' ', + 'not-a-date' + ])('rejects malformed string closureDate: %p', (value) => { + expect(() => + callBuildPayload( + makeValidForm({ + closureDate: value + }) + ) + ).toThrow(TypeError); + }); + it('rejects invalid Date object', () => { const invalidDate = new Date('invalid'); @@ describe('closureReasonId validation', () => { + it('accepts numeric string closureReasonId when coercion is intended', () => { + const result = callBuildPayload( + makeValidForm({ + closureReasonId: '42' as unknown as number + }) + ); + expect(result.closureReasonId).toBe(42); + }); +Also applies to: 114-128, 131-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.ts` around lines 58 - 65, Add regression tests in close-client.utils.spec.ts to cover malformed-but-non-empty date strings and closureReasonId string conversion: create specs that pass a whitespace-only closureDate (e.g., ' ') and a malformed date string (e.g., 'not-a-date') into callBuildPayload(makeValidForm({ closureDate: '...' })) and assert formatDateSpy is not called and result.closureDate equals the original string; also add tests that pass closureReasonId as a stringified number and as a string to ensure the utility converts/handles it consistently (asserting the expected type/value on result.closureReasonId). Use the existing helpers callBuildPayload, makeValidForm and formatDateSpy to locate and follow current test patterns.
🤖 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/app/clients/clients-view/client-actions/close-client/close-client.utils.ts`:
- Around line 84-86: The code currently accepts any non-empty string for
formValue.closureDate and assigns it to closureDate; replace this with strict
validation by checking the trimmed string against a date format (e.g., a
YYYY-MM-DD regex) or attempting to parse it and confirming it yields a valid
date that round-trips to the same formatted string before assigning to
closureDate; if validation fails, do not accept the value (return/omit it from
the payload or surface a validation error) so malformed strings like "abc" are
not used in payload generation. Ensure you update the code paths that build the
payload to expect a validated closureDate and add/update tests for
formValue.closureDate validation.
---
Nitpick comments:
In
`@src/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.ts`:
- Around line 58-65: Add regression tests in close-client.utils.spec.ts to cover
malformed-but-non-empty date strings and closureReasonId string conversion:
create specs that pass a whitespace-only closureDate (e.g., ' ') and a
malformed date string (e.g., 'not-a-date') into callBuildPayload(makeValidForm({
closureDate: '...' })) and assert formatDateSpy is not called and
result.closureDate equals the original string; also add tests that pass
closureReasonId as a stringified number and as a string to ensure the utility
converts/handles it consistently (asserting the expected type/value on
result.closureReasonId). Use the existing helpers callBuildPayload,
makeValidForm and formatDateSpy to locate and follow current test patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 49b1d972-224b-4d7e-993f-75a6b9ed006b
📒 Files selected for processing (3)
src/app/clients/clients-view/client-actions/close-client/close-client.component.tssrc/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.tssrc/app/clients/clients-view/client-actions/close-client/close-client.utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/clients/clients-view/client-actions/close-client/close-client.component.ts
src/app/clients/clients-view/client-actions/close-client/close-client.utils.ts
Outdated
Show resolved
Hide resolved
Agaba-Ed
force-pushed
the
close-client-utility
branch
from
e938738 to
777a6f8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.ts (2)
93-100: Immutability check is shallow and can miss mutableDatechanges.Line 95 snapshots by spread only; if a
Dateobject were mutated in-place, this test could still pass.♻️ Proposed tweak
it('does not mutate input object', () => { const formValue = makeValidForm(); - const snapshot = { ...formValue }; + const snapshot: CloseClientFormValue = { + ...formValue, + closureDate: + formValue.closureDate instanceof Date + ? new Date(formValue.closureDate.getTime()) + : formValue.closureDate + }; callBuildPayload(formValue); expect(formValue).toEqual(snapshot); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.ts` around lines 93 - 100, The immutability test for callBuildPayload is only doing a shallow copy via spread on the result of makeValidForm, so in-place mutations of nested objects like Date would be missed; update the test to take a deep snapshot of the input (for example using structuredClone, a deepClone helper, or JSON serialization where appropriate) before calling callBuildPayload(formValue) and then assert deep equality with the snapshot to ensure nested/mutable objects (e.g., Date instances) were not mutated.
109-109: Assert descriptive error messages, not onlyTypeError.Line 109 (and similar invalid-input assertions) checks error type only. Since this utility promises descriptive failures, assert message patterns too so regressions are caught.
🧪 Example assertion hardening
- expect(() => callBuildPayload(makeValidForm({ closureDate: invalidDate }))).toThrow(TypeError); + const act = () => callBuildPayload(makeValidForm({ closureDate: invalidDate })); + expect(act).toThrow(TypeError); + expect(act).toThrow(/Invalid closureDate/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.ts` at line 109, Update the tests that currently assert only the error type (e.g., expect(() => callBuildPayload(makeValidForm({ closureDate: invalidDate }))).toThrow(TypeError)) to assert the error message content as well: call the same invocation but use toThrowError or toThrow with a string/RegExp that matches the expected descriptive message from callBuildPayload (e.g., a pattern indicating an invalid closure date). Apply the same change for other invalid-input assertions in this spec so tests verify both error type and the descriptive message produced by callBuildPayload / makeValidForm input validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.ts`:
- Around line 93-100: The immutability test for callBuildPayload is only doing a
shallow copy via spread on the result of makeValidForm, so in-place mutations of
nested objects like Date would be missed; update the test to take a deep
snapshot of the input (for example using structuredClone, a deepClone helper, or
JSON serialization where appropriate) before calling callBuildPayload(formValue)
and then assert deep equality with the snapshot to ensure nested/mutable objects
(e.g., Date instances) were not mutated.
- Line 109: Update the tests that currently assert only the error type (e.g.,
expect(() => callBuildPayload(makeValidForm({ closureDate: invalidDate
}))).toThrow(TypeError)) to assert the error message content as well: call the
same invocation but use toThrowError or toThrow with a string/RegExp that
matches the expected descriptive message from callBuildPayload (e.g., a pattern
indicating an invalid closure date). Apply the same change for other
invalid-input assertions in this spec so tests verify both error type and the
descriptive message produced by callBuildPayload / makeValidForm input
validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31c96c87-d54b-45a6-a273-981e331ebb03
📒 Files selected for processing (3)
src/app/clients/clients-view/client-actions/close-client/close-client.component.tssrc/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.tssrc/app/clients/clients-view/client-actions/close-client/close-client.utils.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/clients/clients-view/client-actions/close-client/close-client.utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/clients/clients-view/client-actions/close-client/close-client.component.ts
Agaba-Ed
force-pushed
the
close-client-utility
branch
from
777a6f8 to
e2d878f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/app/clients/clients-view/client-actions/close-client/close-client.component.ts`:
- Around line 90-94: The submit() method currently calls
buildCloseClientPayload(...) which can throw a TypeError and will bubble up;
wrap the call to buildCloseClientPayload(this.closeClientForm.value,
this.dateUtils, locale, dateFormat) in a try/catch inside submit(), catch
TypeError (and other errors), set form-level validation feedback on
closeClientForm (e.g., mark invalid and attach a generic payload error message)
and avoid calling clientsService.executeClientCommand when payload building
fails; keep the existing success subscription flow when no error occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4fff882b-77c0-4a30-b80d-d3c61ddecf8a
📒 Files selected for processing (3)
src/app/clients/clients-view/client-actions/close-client/close-client.component.tssrc/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.tssrc/app/clients/clients-view/client-actions/close-client/close-client.utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/clients/clients-view/client-actions/close-client/close-client.utils.ts
src/app/clients/clients-view/client-actions/close-client/close-client.component.ts
Show resolved
Hide resolved
IOhacker
requested changes
Mar 30, 2026
Contributor
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
Why not use the Fineract Client that has been introduced by @gkbishnoi07
Contributor
Author
|
@IOhacker Thanks for the feedback! I wasn't aware of the Fineract client. Could you please point me to the PR it was introduced? |
Collaborator
https://github.com/[openMF/fineract-client-ts](https://github.com/openMF/fineract-client-ts) |
Agaba-Ed
force-pushed
the
close-client-utility
branch
from
e2d878f to
cf02a62
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/clients/clients-view/client-actions/close-client/close-client.utils.ts (1)
13-17: Consider aligning type with runtime reality.The
closureReasonIdis typed asnumber, but the runtime coercion at line 97-98 handlesstringinputs (fromUntypedFormGroup). Consider typing it asnumber | stringto accurately reflect the actual form input:export interface CloseClientFormValue { closureDate: Date | string; - closureReasonId: number; + closureReasonId: number | string; [key: string]: unknown; }This improves type accuracy and makes the string-to-number coercion in
buildCloseClientPayloadself-documenting rather than appearing as defensive code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/clients/clients-view/client-actions/close-client/close-client.utils.ts` around lines 13 - 17, Update the CloseClientFormValue interface to reflect actual runtime values by changing closureReasonId from number to number | string; then ensure buildCloseClientPayload remains the place where you coerce the possibly-string closureReasonId to a number (so the coercion in buildCloseClientPayload is clearly intentional and self-documenting when consuming values from an UntypedFormGroup).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/app/clients/clients-view/client-actions/close-client/close-client.utils.ts`:
- Around line 13-17: Update the CloseClientFormValue interface to reflect actual
runtime values by changing closureReasonId from number to number | string; then
ensure buildCloseClientPayload remains the place where you coerce the
possibly-string closureReasonId to a number (so the coercion in
buildCloseClientPayload is clearly intentional and self-documenting when
consuming values from an UntypedFormGroup).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59fa168e-5832-4323-b104-94e985adbbf9
📒 Files selected for processing (3)
src/app/clients/clients-view/client-actions/close-client/close-client.component.tssrc/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.tssrc/app/clients/clients-view/client-actions/close-client/close-client.utils.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/clients/clients-view/client-actions/close-client/close-client.utils.spec.ts
Contributor
Author
|
@IOhacker @gkbishnoi07 I have looked into the Fineract client and have a few questions. It currently depends on @angular/core v19, which suggests we may need to introduce some legacy dependencies into the codebase. I also noticed that there doesn’t appear to be explicit support for the closureDate and closureReasonId fields |
Agaba-Ed
force-pushed
the
close-client-utility
branch
from
cf02a62 to
afea074
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.
Description
Extracted close client API payload construction logic from the component into a dedicated
buildCloseClientPayload()utility function to improve testability, maintainability, and code reusability.Changes Made:
New
close-client.utils.ts: Centralized utility for payload formatting and validationbuildCloseClientPayload()- Main function that transforms and validates form dataisValidDate()- Exported type guard for Date validation, reusable across the appvalidateClosureReasonId()- Internal helper ensuring positive integer constraintNew
close-client.utils.spec.ts: Comprehensive test coverageit.each) for thorough validation coverageUpdated
close-client.component.ts:readonlymodifiers to 6 injected dependenciesValidation Details:
Dateobjects or pre-formatted stringsTypeErroron invalid inputDependencies:
Related issues and discussion
#892
Screenshots, if any
N/A (backend/utility extraction)
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
Refactor
Tests