Feature: Add missing dhis2 periods#385
Conversation
…eeks. Add tests for existing period types
BundleMonNo change in files bundle size Groups updated (1)
Final result: ✅ View report in BundleMon website ➡️ |
xurxodev
left a comment
There was a problem hiding this comment.
thanks @nshandra
This PR expands the supported DHIS2 dataset period types and adds a new TypeScript implementation of buildAllPossiblePeriods, along with Jest coverage for all newly added variants.
It's great.
I've found some issues to review.
1. Should fix
src/domain/entities/DataForm.tsdeclaresperiodType?: DataFormPeriod, butsrc/webapp/utils/periods.tscan throw for missing/unsupportedperiodType.
buildAllPossiblePeriodsshould not throw whenperiodTypeis missing; either acceptperiodType?: DataFormPeriodand return[], or add an early guard in the function.- Improve type safety/readability in
src/webapp/utils/periods.tsand push it towards a more functional/immutable style without introducing mutableletstate. It's safe this change because now we have unit test. Examples:- eliminate
let current;ingenerateSixMonthlyNovPeriodsby computingcurrentas aconstexpression (ternary) and, where feasible, useclone().add(...)to avoid mutating Moment instances; - type the accumulator/output arrays explicitly (e.g.
const dates: string[] = []) so the contract is clear at a glance; - make intermediate values explicit/typed in helpers (so the types of
start/end/currentand formatting pieces are obvious during review).
- eliminate
2. Recommendations non blocking
- Tests status:
src/test/utils/periods.spec.tscovers the newly added period variants and passes (yarn test --runInBand src/test/utils/periods.spec.ts). Consider adding a couple more edge cases (e.g.startDate > endDate) to validate robustness.
…rting undefined, use unknown instead.
|
Added:
Refactor:
|
xurxodev
left a comment
There was a problem hiding this comment.
thanks @nshandra
The following items emerged from the commits that addressed the first review round.
- Should fix
1.1. FinancialType and WeeklyPeriodType duplicate DataFormPeriod subsets
src/webapp/utils/periods.ts:233 defines type FinancialType = "FinancialApril" | "FinancialJuly" | "FinancialOct" | "FinancialNov" and WeeklyPeriodType at line 77. These are subsets of DataFormPeriod but defined
independently — if a new financial or weekly period is added to dataFormPeriods, these local types won't update. Extract them from DataFormPeriod using Extract<>:
type FinancialType = Extract<DataFormPeriod, Financial${string}>;
type WeeklyPeriodType = Extract<DataFormPeriod, "Weekly" | Weekly${string}>;
This ensures they stay in sync with the source of truth.
1.2. isDataFormPeriod type guard uses any
src/domain/entities/DataForm.ts:47 — the parameter type should be unknown instead of any:
export function isDataFormPeriod(value: unknown): value is DataFormPeriod {
return typeof value === "string" && (dataFormPeriods as readonly string[]).includes(value);
}
Using any defeats the purpose of a type guard — callers can pass anything without the compiler flagging it.
1.3. generateFinancialPeriods uses moment().month(monthName) — implicit locale dependency
src/webapp/utils/periods.ts:237 — moment().month(monthName).month() parses month names using the current locale. If the app locale is set to something other than English, "April" won't resolve to month index 3 and
periods will be silently wrong. Use an explicit map instead:
const financialMonthMap: Record<string, number> = { April: 3, July: 6, Oct: 9, Nov: 10 };
const startMonthIndex = financialMonthMap[monthName];
- Recommendations non blocking
2.1. onPeriodValidationError is optional — silent failure for other consumers
src/webapp/components/template-selector/TemplateSelector.tsx:54 — onPeriodValidationError? is optional. If a future consumer of TemplateSelector doesn't pass it, the unsupported period selection will silently reset
state without any user feedback. Consider either making it required or adding a fallback (e.g. console.warn) inside the component.
2.2. Test import uses @jest/globals but project uses Vitest
src/test/utils/periods.spec.ts:2 — import { describe, it, expect } from "@jest/globals". The project migrated to Vitest. Consider importing from vitest instead to avoid confusion:
import { describe, it, expect } from "vitest";
1. To Fix
2. Recommended
|
📌 References
📝 Implementation
Added the following dataSet periods:
WeeklyWednesdayWeeklyThursdayWeeklySaturdayWeeklySundayBiWeeklyBiMonthlyQuarterlyNovSixMonthlySixMonthlyAprilSixMonthlyNovAdded tests for all periods.
🔥 Notes for the reviewer
📹 Screenshots/Screen capture
📑 Others
#869b9v6pp