WEB-389: Fix date format issue when editing holidays #2741
Conversation
|
Note
|
| Cohort / File(s) | Change Summary |
|---|---|
Holiday components date formatting src/app/organization/holidays/create-holiday/create-holiday.component.ts, src/app/organization/holidays/edit-holiday/edit-holiday.component.ts |
Replaced formatDate with formatDateAsString for fromDate, toDate, and repaymentsRescheduledTo in submit() methods. Added explicit Date type guards in edit-holiday and pre-formatting for prevFromDate and prevToDate. |
Docker base images Dockerfile |
Updated Node and Nginx image tags to node:24-alpine3.22 and nginx:1.29-alpine3.22-slim. |
Loans repayment schedule component src/app/loans/loans-view/repayment-schedule-tab/repayment-schedule-tab.component.ts |
Removed NgIf and MatButton imports, added MatIconButton. Added direct businessDate reassignment from settingsService in the else branch of isCurrent method. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
- edit-holiday.component.ts: Added Date type guards and pre-formatting logic require careful verification of guard correctness and edge cases
- repayment-schedule-tab.component.ts: Import changes combined with
businessDatelogic modification warrant validation that the setter is appropriate and doesn't introduce timing issues - create-holiday.component.ts: Relatively straightforward date formatting replacement, lower review friction
Possibly related PRs
- WEB-357 Editing a holiday fails due to failure to parse from date format #2753: Directly modifies the same edit-holiday component with overlapping date-type checking and formatting changes in the submit() method.
- WEB-387: Loan repayment installment style color overdue #2738: Applies identical import changes and
businessDatelogic modifications to the repayment-schedule-tab component.
Suggested reviewers
- alberto-art3ch
- IOhacker
- gkbishnoi07
Pre-merge checks and finishing touches
✅ 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 accurately describes the main change: fixing a date format issue when editing holidays, which is the primary objective of this PR across multiple modified files. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**/*
📒 Files selected for processing (4)
src/app/organization/holidays/create-holiday/create-holiday.component.ts(1 hunks)src/app/organization/holidays/edit-holiday/edit-holiday.component.html(1 hunks)src/app/organization/holidays/edit-holiday/edit-holiday.component.ts(1 hunks)src/environments/.env.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/organization/holidays/create-holiday/create-holiday.component.tssrc/app/organization/holidays/edit-holiday/edit-holiday.component.tssrc/app/organization/holidays/edit-holiday/edit-holiday.component.html
🧬 Code graph analysis (2)
src/app/organization/holidays/create-holiday/create-holiday.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(111-113)
src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(111-113)
🪛 dotenv-linter (4.0.0)
src/environments/.env.ts
[warning] 5-5: [IncorrectDelimiter] The 'version': '251031', key has incorrect delimiter
(IncorrectDelimiter)
[warning] 5-5: [KeyWithoutValue] The 'version': '251031', key should be with a value or have an equal sign
(KeyWithoutValue)
[warning] 5-5: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 5-5: [LowercaseKey] The 'version': '251031', key should be in uppercase
(LowercaseKey)
[warning] 5-5: [UnorderedKey] The 'version': '251031', key should go before the /* tslint:disable */ key
(UnorderedKey)
[warning] 6-6: [IncorrectDelimiter] The 'hash': '3b5a68bb' key has incorrect delimiter
(IncorrectDelimiter)
[warning] 6-6: [KeyWithoutValue] The 'hash': '3b5a68bb' key should be with a value or have an equal sign
(KeyWithoutValue)
[warning] 6-6: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 6-6: [LowercaseKey] The 'hash': '3b5a68bb' key should be in uppercase
(LowercaseKey)
[warning] 6-6: [UnorderedKey] The 'hash': '3b5a68bb' key should go before the 'mifos_x': { key
(UnorderedKey)
[warning] 7-7: [KeyWithoutValue] The }, key should be with a value or have an equal sign
(KeyWithoutValue)
[warning] 7-7: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 8-8: [IncorrectDelimiter] The 'allow_switching_backend_instance': true key has incorrect delimiter
(IncorrectDelimiter)
[warning] 8-8: [KeyWithoutValue] The 'allow_switching_backend_instance': true key should be with a value or have an equal sign
(KeyWithoutValue)
[warning] 8-8: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 8-8: [LowercaseKey] The 'allow_switching_backend_instance': true key should be in uppercase
(LowercaseKey)
[warning] 8-8: [UnorderedKey] The 'allow_switching_backend_instance': true key should go before the 'hash': '3b5a68bb' key
(UnorderedKey)
🔇 Additional comments (3)
src/environments/.env.ts (1)
5-8: LGTM - configuration updates look good.The version/hash updates and new
allow_switching_backend_instancefeature flag are straightforward configuration changes. Note that the static analysis warnings from dotenv-linter are false positives—this is a TypeScript file, not a.envfile.src/app/organization/holidays/edit-holiday/edit-holiday.component.html (1)
94-94: LGTM - explicit button type improves clarity.Adding
type="submit"to the submit button is a best practice that makes the form submission behavior explicit.src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (1)
137-151: LGTM - defensive date formatting logic.The updated date formatting logic properly:
- Extracts date values into typed variables
- Guards formatting with
instanceof Datechecks to prevent runtime errors- Uses
formatDateAsStringto ensure backend-compatible format (dd MMMM yyyy)- Conditionally formats
repaymentsRescheduledToonly whenreSchedulingType === 2This addresses the PR objective of fixing date format mismatches.
|
please review @gkbishnoi07 |
gkbishnoi07
left a comment
gkbishnoi07
left a comment
There was a problem hiding this comment.
Hey @AnvayKharb dont push.env.ts file and please squash your commits make it only 1 commit
AnvayKharb
force-pushed
the
WEB-389-editing-existing-holidays-fails-due-to-date-format-problem
branch
from
a04f916 to
22a2ff4
Compare
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
Dont add your .env file. the options that you are using on it seems deprecated now. Make sure that your are in sync with the dev branch
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/organization/holidays/edit-holiday/edit-holiday.component.html (1)
54-56: Consider addingtrackByto the*ngForloop for performance.The loop at line 54 iterates over
holidayData.repaymentSchedulingTypeswithout atrackByfunction. While not blocking, adding atrackByfunction improves change detection performance and aligns with Angular best practices.Example refactor:
<mat-option *ngFor="let repayments of holidayData.repaymentSchedulingTypes; trackBy: trackByRepaymentId" [value]="repayments.id"> {{ repayments.value }} </mat-option>Then in the component TypeScript file, add a trackBy method:
trackByRepaymentId(index: number, repayment: any): number { return repayment.id; }As per coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/organization/holidays/create-holiday/create-holiday.component.ts(1 hunks)src/app/organization/holidays/edit-holiday/edit-holiday.component.html(1 hunks)src/app/organization/holidays/edit-holiday/edit-holiday.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/organization/holidays/edit-holiday/edit-holiday.component.ts
- src/app/organization/holidays/create-holiday/create-holiday.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/organization/holidays/edit-holiday/edit-holiday.component.html
🔇 Additional comments (2)
src/app/organization/holidays/edit-holiday/edit-holiday.component.html (2)
93-101: Explicittype="submit"clarifies form submission semantics.Adding the explicit
type="submit"attribute is a best practice and ensures the button properly triggers form submission within the Angular reactive forms context.
15-80: Verify date picker configuration in the TypeScript component.The PR objective states that date picker configuration should use the
dd MMMM yyyyformat and localeento match backend expectations. These configurations typically reside in the component TypeScript file, not the template. Please confirm that the correspondingedit-holiday.component.tsfile contains the necessary date formatting updates forfromDate,toDate, andrepaymentsRescheduledTousingformatDateAsStringor equivalent date formatting logic.
|
Hey @IOhacker @gkbishnoi07 I’ve removed the .env.ts file and squashed my commits into a single one. |
alberto-art3ch
left a comment
alberto-art3ch
left a comment
There was a problem hiding this comment.
Please review the changes suggested by coderabbit, In this case they look to be ok
AnvayKharb
force-pushed
the
WEB-389-editing-existing-holidays-fails-due-to-date-format-problem
branch
from
22a2ff4 to
cc88ee2
Compare
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 (3)
src/app/organization/holidays/create-holiday/create-holiday.component.ts(1 hunks)src/app/organization/holidays/edit-holiday/edit-holiday.component.html(1 hunks)src/app/organization/holidays/edit-holiday/edit-holiday.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/organization/holidays/edit-holiday/edit-holiday.component.ts
- src/app/organization/holidays/edit-holiday/edit-holiday.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/organization/holidays/create-holiday/create-holiday.component.ts
🧬 Code graph analysis (1)
src/app/organization/holidays/create-holiday/create-holiday.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(111-113)
src/app/organization/holidays/create-holiday/create-holiday.component.ts
Show resolved
Hide resolved
AnvayKharb
force-pushed
the
WEB-389-editing-existing-holidays-fails-due-to-date-format-problem
branch
from
cc88ee2 to
727951f
Compare
|
please take a look @IOhacker @gkbishnoi07 |
|
hey @IOhacker please take a look |
|
@alberto-art3ch please take a look!! |
|
@AnvayKharb please resolve conflict |
AnvayKharb
force-pushed
the
WEB-389-editing-existing-holidays-fails-due-to-date-format-problem
branch
from
ee74f08 to
215d12d
Compare
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/app/organization/holidays/edit-holiday/edit-holiday.component.ts (1)
137-162: Major: Remove dead code from incomplete refactoring.The date formatting logic is duplicated. After the new formatting (lines 140-150) executes,
holidayFormData.fromDate,toDate, andrepaymentsRescheduledToare already strings. The subsequentinstanceof Datechecks at lines 151-162 will always be false, making that entire block unreachable dead code.Based on the PR objectives to fix the date format issue, the old
formatDatecalls should be removed entirely.Apply this diff to remove the dead code:
if (repaymentScheduledTo instanceof Date) { holidayFormData.repaymentsRescheduledTo = this.dateUtils.formatDateAsString(repaymentScheduledTo, dateFormat); } } - if (holidayFormData.fromDate instanceof Date) { - holidayFormData.fromDate = this.dateUtils.formatDate(holidayFormData.fromDate, dateFormat); - } - if (holidayFormData.toDate instanceof Date) { - holidayFormData.toDate = this.dateUtils.formatDate(holidayFormData.toDate, dateFormat); - } - if (this.reSchedulingType === 2 && holidayFormData.repaymentsRescheduledTo instanceof Date) { - holidayFormData.repaymentsRescheduledTo = this.dateUtils.formatDate( - holidayFormData.repaymentsRescheduledTo, - dateFormat - ); - } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/organization/holidays/edit-holiday/edit-holiday.component.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/organization/holidays/edit-holiday/edit-holiday.component.ts
🧬 Code graph analysis (1)
src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(112-115)
🪛 GitHub Actions: build-run
src/app/organization/holidays/edit-holiday/edit-holiday.component.ts
[error] 175-175: npx eslint . reported a parsing error: ESLint: Parsing error: '}' expected in edit-holiday.component.ts.
- Changed edit-holiday component to use formatDateAsString with proper instanceof checks - Changed create-holiday component to use formatDateAsString - Fixed bug where incorrect date checks were being performed - formatDateAsString uses moment.format() which correctly handles Date objects - Resolves date format problems when editing existing holidays
AnvayKharb
force-pushed
the
WEB-389-editing-existing-holidays-fails-due-to-date-format-problem
branch
from
e1cad7b to
46b7ceb
Compare
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 (2)
src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (1)
118-127: Memory leak: valueChanges subscription is never unsubscribed.The subscription created at line 119 is not cleaned up when the component is destroyed, which can lead to memory leaks.
Consider using
takeUntilwith a Subject that completes inngOnDestroy:+import { Subject } from 'rxjs'; +import { takeUntil } from 'rxjs/operators'; + export class EditHolidayComponent implements OnInit { + private destroy$ = new Subject<void>(); + getReschedulingType() { - this.holidayForm.get('reschedulingType').valueChanges.subscribe((option: any) => { + this.holidayForm.get('reschedulingType').valueChanges + .pipe(takeUntil(this.destroy$)) + .subscribe((option: any) => { this.reSchedulingType = option; if (option === 2) { this.holidayForm.addControl('repaymentsRescheduledTo', new UntypedFormControl(new Date(), Validators.required)); } else { this.holidayForm.removeControl('repaymentsRescheduledTo'); } }); } + + ngOnDestroy() { + this.destroy$.next(); + this.destroy$.complete(); + }As per coding guidelines.
src/app/loans/loans-view/repayment-schedule-tab/repayment-schedule-tab.component.ts (1)
171-186: Remove side effect from query method; fix asymmetric businessDate refresh logic.The
businessDateassignment at line 175 confirms multiple issues:
Side effect in query method:
isCurrent()modifies component state, violating functional principles and Angular best practices for clean observable patterns.Asymmetric refresh logic:
businessDateis only refreshed in the else branch (wheninstallment.fromDateexists). When!installment.fromDate, the method returns early without refreshing, using stalebusinessDate. This inconsistency causes unpredictable behavior.Performance:
installmentStyle()is called 13+ times per row in the template (*matCellDefdirectives), triggering repeatedsettingsService.businessDateaccess on every change detection cycle.Redundant:
businessDateis already initialized in the constructor (line 137).Recommended fix:
Remove the assignment from
isCurrent(). IfbusinessDatemust be kept current for holiday date calculations in this PR, refresh it inngOnInit()or use a proper subscription pattern instead:isCurrent(installment: RepaymentSchedulePeriod): string { if (!installment.fromDate) { return ''; } else { - this.businessDate = this.settingsService.businessDate; const fromDate = this.dateUtils.parseDate(installment.fromDate); const dueDate = this.dateUtils.parseDate(installment.dueDate); if (fromDate <= this.businessDate && this.businessDate < dueDate) { return 'current'; } if (this.businessDate > dueDate) { return 'overdued'; } } return ''; }
🧹 Nitpick comments (1)
src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (1)
33-33: Consider defining explicit types instead ofany.Using
anyforholidayData(and implicitly forholidayFormDataat line 133) reduces type safety. Consider defining an interface for holiday data to catch type errors at compile time.Example:
interface HolidayData { id: number; name: string; description: string; status: { value: string }; reschedulingType: number; fromDate?: Date | string; toDate?: Date | string; repaymentsRescheduledTo?: Date | string; repaymentSchedulingTypes?: any[]; }Then update line 33:
- holidayData: any; + holidayData: HolidayData;As per coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Dockerfile(1 hunks)src/app/loans/loans-view/repayment-schedule-tab/repayment-schedule-tab.component.ts(2 hunks)src/app/organization/holidays/create-holiday/create-holiday.component.ts(1 hunks)src/app/organization/holidays/edit-holiday/edit-holiday.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/organization/holidays/create-holiday/create-holiday.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/loans/loans-view/repayment-schedule-tab/repayment-schedule-tab.component.tssrc/app/organization/holidays/edit-holiday/edit-holiday.component.ts
🧬 Code graph analysis (1)
src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(111-113)
🔇 Additional comments (2)
src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (1)
137-151: Date formatting changes look good and syntax error is resolved.The type guards with
instanceof Datecombined withformatDateAsString(dateFormat)correctly ensure that dates are formatted according to the backend's expected format. This directly addresses the API validation error described in the PR objectives. The closing brace at line 151 also resolves the syntax error flagged in the previous review.src/app/loans/loans-view/repayment-schedule-tab/repayment-schedule-tab.component.ts (1)
14-15: LGTM: Import refinement is appropriate.The import changes correctly reflect the component's actual dependencies by replacing general button imports with the more specific MatIconButton.
|
@AnvayKharb still there is conflict |
4 participants
Resolves Issue: web-389
This pull request resolves a critical bug on the Organization > Manage Holidays page. When a user attempted to edit an existing holiday, the form would fail to submit, displaying an API validation error: The parameter “fromDate” is invalid based on the dateFormat: “dd MMMM yyyy” and locale: “en” provided.
The root cause of this issue was a format mismatch between the frontend display logic and the backend API's validation rules. The "Edit Holiday" form was incorrectly configured to display and submit dates in the MM/DD/YYYY format (e.g., "11/7/2025"), while the backend API strictly requires the dd MMMM yyyy format (e.g., "07 November 2025").
The Solution:
This fix addresses the bug at the frontend presentation layer by re-configuring the date picker components used in the "Edit Holiday" form.
The dateFormat prop (or its equivalent) for the fromDate, toDate, and repaymentScheduledTo fields has been explicitly changed from MM/DD/YYYY to dd MMMM yyyy.
The locale is ensured to be en to match the server's expectation.
This change ensures that the date format displayed to the user is the exact string format the server expects. When the form is submitted, the correctly formatted date string (e.g., "07 November 2025") is sent in the payload, which now passes the API validation.
Summary by CodeRabbit
Refactor
Chores