WEB-678: Restructure Reporting Plugin to Support Branch Model Operations#3541
WEB-678: Restructure Reporting Plugin to Support Branch Model Operations#3541sakshar2303 wants to merge 3 commits intoopenMF:devfrom
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Reporting service implementation & re-export src/app/reporting-plugin/services/reports.service.ts, src/app/reports/reports.service.ts |
Adds a full ReportsService implementation (HTTP methods for reports, params, charts, Pentaho/BIRT runs) and converts the legacy src/app/reports/reports.service.ts into a re-export of the new service. |
Model additions & typing src/app/reporting-plugin/models/chart-data.model.ts, src/app/reporting-plugin/models/report-parameter.model.ts, src/app/reporting-plugin/models/select-option.model.ts |
Adds ChartDataResponse and strongly-typed ReportParameter and SelectOption (constructor signatures tightened); model file headers added. |
Resolver relocation src/app/reporting-plugin/resolvers/reports.resolver.ts, src/app/reporting-plugin/resolvers/run-report.resolver.ts, src/app/reports/reports-routing.module.ts |
Resolver imports updated to use app/reporting-plugin/resolvers; routing module adjusted to reference relocated resolvers. |
Component import updates (ReportsService) src/app/clients/.../general-tab.component.ts, src/app/clients/.../personal-data-tab.component.ts, src/app/loans/.../loans-transaction-reciept.resolver.ts, src/app/loans/.../export-transactions.component.ts, src/app/reports/run-report/birt/birt.component.ts, src/app/reports/run-report/pentaho/pentaho.component.ts, src/app/reports/run-report/table-and-sms/table-and-sms.component.ts, src/app/savings/.../savings-transaction-reciept.resolver.ts, src/app/savings/.../export-transactions.component.ts |
Updated imports to app/reporting-plugin/services/reports.service; no runtime logic changes. |
Component import updates (models) src/app/organization/sms-campaigns/.../*.component.ts, src/app/reports/run-report/chart/chart.component.ts, src/app/reports/run-report/run-report.component.ts |
Updated imports for ReportParameter / SelectOption / ChartData to app/reporting-plugin/models/*; no logic changes. |
Template change src/app/products/manage-tax-components/view-tax-component/view-tax-component.component.html |
Account-type label construction changed to use *.AccountType.code with translateKey under inputs.accounting instead of parsing .value. |
Sequence Diagram(s)
sequenceDiagram
participant Component as Client Component
participant Service as ReportsService
participant Http as HttpClient
participant API as Reporting API
Component->>Service: request report / params / run (formData)
Service->>Http: GET /runreports or /reports with HttpParams
Http->>API: network request
API-->>Http: response (JSON / arraybuffer)
Http-->>Service: response
Service-->>Component: mapped data (ChartData / ReportParameter[] / blob response)
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- WEB-797 Implement Eclipse BIRT reporting support #3279 — Moves/extends ReportsService to add BIRT run/params and reporting-plugin migration (touches same service surfaces).
- WEB-239: Fix Tax Components view page translation and layout #3103 — Adjusts the same tax component account-type translation rendering logic in the template.
- WEB-702 Enable functionality in mifos web-app to submit reports and tickets mifos web-app #3244 — Modifies Pentaho report handling and touches the same Pentaho/BIRT reporting components.
Suggested reviewers
- IOhacker
- alberto-art3ch
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title directly and accurately summarizes the main objective of the PR: restructuring the reporting plugin to support branch model operations, which is consistent with all the changes across the codebase. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ 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.
Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/app/reports/run-report/run-report.component.ts (2)
452-484:⚠️ Potential issue | 🟠 MajorAdd error handling + ensure
isProcessingalways resets inrunReportAndExport().
this.isProcessing = trueis set (Line 454) butthis.isProcessing = falseonly happens inside the success callback (Line 483). If the request errors, the UI can remain stuck in a processing state.Add an error callback to the subscription (or finalize) to reset
isProcessingand optionally surface a translated alert (without introducing new hardcoded strings).
As per coding guidelines, “clean observable patterns.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reports/run-report/run-report.component.ts` around lines 452 - 484, The subscription in runReportAndExport sets this.isProcessing = true but only resets it in the success handler; update reportsService.getRunReportData(reportName, payload) to use an RxJS finalize to always set this.isProcessing = false and add an error callback on the subscribe to surface an alert (use alertService.alert with translateService.instant for type/message using your existing translation keys) so errors don't leave the UI stuck; reference runReportAndExport, reportsService.getRunReportData, isProcessing, alertService.alert, translateService.instant and import/use finalize from 'rxjs/operators' (or rxjs) to perform the guaranteed reset.
325-341:⚠️ Potential issue | 🟠 MajorFix unsubscribed
valueChangessubscriptions and usesetControlto update existing child controls.The
valueChangessubscriptions insetChildControls()are created but never cleaned up. On every parent parameter value change, new subscriptions accumulate, causing a memory leak. Additionally, callingaddControl()on existing controls silently ignores the update—usecontains()andsetControl()instead to properly update child controls on subsequent parent value changes. ImplementOnDestroywithtakeUntilto clean up subscriptions.Proposed fix
export class RunReportComponent implements OnInit, OnDestroy { private destroy$ = new Subject<void>(); ngOnDestroy() { this.destroy$.next(); this.destroy$.complete(); } setChildControls() { this.parentParameters.forEach((param: ReportParameter) => { this.reportForm.get(param.name).valueChanges .pipe(takeUntil(this.destroy$)) .subscribe((option: any) => { param.childParameters.forEach((child: ReportParameter) => { const control = child.displayType === 'none' ? new UntypedFormControl(child.defaultVal) : new UntypedFormControl('', Validators.required); if (this.reportForm.contains(child.name)) { this.reportForm.setControl(child.name, control); } else { this.reportForm.addControl(child.name, control); } if (child.displayType === 'select') { const inputstring = `${child.name}?${param.inputName}=${option?.id}`; this.fetchSelectOptions(child, inputstring); } }); }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reports/run-report/run-report.component.ts` around lines 325 - 341, The valueChanges subscriptions created in setChildControls() leak and addControl() silently no-ops for existing controls; fix by introducing a component-level Subject (e.g., ngDestroy$) and pipe each valueChanges with takeUntil(this.ngDestroy$) (import Subject and takeUntil) and implement OnDestroy to call this.ngDestroy$.next() and complete(); inside the subscription, before adding child controls check this.reportForm.contains(child.name) and use this.reportForm.setControl(child.name, new UntypedFormControl(...)) to update existing controls (otherwise addControl for new ones), and keep fetchSelectOptions logic unchanged.src/app/reports/run-report/table-and-sms/table-and-sms.component.ts (1)
173-203:⚠️ Potential issue | 🟡 MinorFix XLS download filename (currently hardcoded).
In
exportToXLS(),fileNameis computed using the report name (Line 174), but the downloaded file usesa.download = 'filename.xlsx'(Line 199). This ignores the computed filename and is user-visible.Proposed fix
- a.download = 'filename.xlsx'; + a.download = fileName;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reports/run-report/table-and-sms/table-and-sms.component.ts` around lines 173 - 203, The download currently uses a hardcoded filename ('filename.xlsx') instead of the computed fileName; inside exportToXLS() replace the hardcoded a.download assignment with the computed fileName (ensure fileName includes the .xlsx extension or append it if missing) so that the anchor element uses fileName when triggering the download; update references around the fileName constant and the a.download assignment in exportToXLS() to use the same variable.src/app/reports/run-report/chart/chart.component.ts (1)
145-162:⚠️ Potential issue | 🟠 MajorFix invalid color format in
randomColor()— usergba()instead ofrgb().The current code returns
rgb(${r},${g},${b},0.6)which is invalid CSS syntax. Thergb()function does not accept an alpha channel as a fourth parameter; alpha transparency requires eitherrgba(r, g, b, a)or the modernrgb(r g b / a)form. This will cause Chart.js to fail parsing the color or apply unexpected defaults, breaking chart visualization.Proposed fix
randomColor() { const r = Math.floor(Math.random() * 255); const g = Math.floor(Math.random() * 255); const b = Math.floor(Math.random() * 255); - return `rgb(${r},${g},${b},0.6)`; + return `rgba(${r},${g},${b},0.6)`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reports/run-report/chart/chart.component.ts` around lines 145 - 162, The color string produced by randomColor() is invalid because it uses rgb(...) with four arguments; update randomColor() to return a valid CSS color using rgba(r,g,b,a) (or the modern rgb(r g b / a)) including the 0.6 alpha, so Chart.js can parse it; then ensure randomColorArray(length) continues to call randomColor() to build the array (functions: randomColor and randomColorArray).src/app/reporting-plugin/resolvers/run-report.resolver.ts (1)
1-16:⚠️ Potential issue | 🟠 MajorMajor: duplicate MPL license header blocks present.
This file contains the MPL header twice back-to-back (once at the top, and again in the added block around Line 9-15). This is likely to fail/complicate
npm run headers:check.Proposed fix
Remove the duplicate second header block so only one MPL header remains.
/** * Copyright since 2025 Mifos Initiative * * This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -/** - * Copyright since 2025 Mifos Initiative - * - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. - */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reporting-plugin/resolvers/run-report.resolver.ts` around lines 1 - 16, The file contains two identical MPL license header blocks; remove the duplicate second header block (the consecutive /** ... */ that appears after the first header) so only a single MPL header remains at the top of the file, ensuring the header:check step will pass; search for the repeated /** Copyright since 2025 Mifos Initiative ... */ block and delete the latter occurrence.src/app/clients/clients-view/general-tab/general-tab.component.ts (1)
111-114:⚠️ Potential issue | 🟠 MajorMajor: hardcoded user-facing error string must be i18n'd.
message: 'Failed to load Loan Application PDF report.'is hardcoded. Per repo guidelines, convert to an i18n key usingTranslateService.instant(...)and runnpm run translations:extractwhen adding the new key. This follows the established pattern inpersonal-data-tab.component.ts(same directory), which i18n's all alert messages.Suggested direction
+ import { TranslateService } from '@ngx-translate/core'; export class GeneralTabComponent implements OnDestroy { private destroy$ = new Subject<void>(); private alertService = inject(AlertService); + private translateService = inject(TranslateService);- message: 'Failed to load Loan Application PDF report.' + message: this.translateService.instant('errors.loanApplicationReportLoadError')Then add the translation key to your i18n files and run
npm run translations:extract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/clients/clients-view/general-tab/general-tab.component.ts` around lines 111 - 114, Replace the hardcoded alert message in general-tab.component.ts (the object with type: 'error' and message: 'Failed to load Loan Application PDF report.') with an i18n lookup using TranslateService.instant(...) (follow the pattern used in personal-data-tab.component.ts for alert messages), then add the new translation key to your i18n files and run npm run translations:extract to populate translations; ensure the alert uses the translated string instead of the literal.
🧹 Nitpick comments (10)
src/app/reports/run-report/chart/chart.component.ts (1)
57-69: Guard against undefineddataObjectinngOnChanges()/getRunReportData().
ngOnChanges()unconditionally callsgetRunReportData(), andgetRunReportData()dereferencesthis.dataObject.report.nameandthis.dataObject.formData. If the input is everundefined/nullduring initialization or if the parent fails to provide it, this will throw.Add a small defensive check before calling the service.
As per coding guidelines, “strict type safety”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reports/run-report/chart/chart.component.ts` around lines 57 - 69, ngOnChanges currently calls getRunReportData unconditionally which dereferences this.dataObject; add a defensive null/undefined check in ngOnChanges (or at the start of getRunReportData) to return early if this.dataObject is falsy or missing required properties (e.g., this.dataObject.report or this.dataObject.formData) before calling reportsService.getChartRunReportData(this.dataObject.report.name, this.dataObject.formData); keep using setPieChart(this.inputData) and hideOutput=false only after a successful response. Ensure checks reference the same symbols: ngOnChanges, getRunReportData, dataObject, getChartRunReportData, and setPieChart.src/app/reports/run-report/table-and-sms/table-and-sms.component.ts (1)
99-117: Add error handling ingetRunReportData()so progress + UI recover.The subscription in
getRunReportData()(Lines 101-116) only handles success; if the request errors,progressBarService.decrease()andhideOutputstate won’t be updated, potentially leaving the UI inconsistent.Add an error callback to the subscribe and ensure UI state/progress are updated in both success and failure paths.
As per coding guidelines, “clean observable patterns.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reports/run-report/table-and-sms/table-and-sms.component.ts` around lines 99 - 117, In getRunReportData(), add an error handler to the Observable subscription so UI and progress state are always updated on failure; specifically, extend the subscribe(...) call on reportsService.getRunReportData(...) with an error callback that calls progressBarService.decrease(), sets hideOutput = false (or an appropriate safe state), and sets notExistsReportData/csvData/displayedColumns appropriately (e.g., clear csvData and displayedColumns and set notExistsReportData = true) to ensure the UI recovers; keep existing success handling (setOutputTable, toBeExportedToRepo, columnTypes population) intact and only move shared finalization into both paths or a finalize-like cleanup to avoid duplication.src/app/loans/loans-view/transactions/export-transactions/export-transactions.component.ts (2)
64-68: Avoid deeproute.parent.parentwithout null-safety.The constructor uses
this.route.parent.parent.data.subscribe(...)which will throw ifparentorparent.parentis undefined for some routing configurations.Defensively guard (or restructure to use the correct injected ActivatedRoute at the level that owns
loanDetailsData).
As per coding guidelines, “strict type safety”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/transactions/export-transactions/export-transactions.component.ts` around lines 64 - 68, The constructor currently accesses this.route.parent.parent.data.subscribe(...) which can throw if parent or parent.parent is undefined; fix by adding null-safety or using optional chaining (e.g., check this.route.parent && this.route.parent.parent before subscribing) or, better, inject the correct ActivatedRoute that owns loanDetailsData and subscribe on that; ensure the subscription assigns loansAccountId = data.loanDetailsData.accountNo only when data and loanDetailsData exist and handle the undefined case (or bail out) to satisfy strict type safety.
94-111: Add error handling for the Pentaho run report request.
generate()subscribes togetPentahoRunReportData(...)and only setspentahoUrl/hideOutputon success. If the request fails, there’s no error path (and no UI recovery).Add an
errorcallback (orfinalize) to restorehideOutput/show an alert/log and prevent the component from getting stuck.
As per coding guidelines, “clean observable patterns” for src/app/**component.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/transactions/export-transactions/export-transactions.component.ts` around lines 94 - 111, The generate() method lacks error handling for the reportsService.getPentahoRunReportData(...) Observable; add an error path (or use pipe(finalize())) so the component doesn't get stuck with UI state. Specifically, update generate() to handle errors from getPentahoRunReportData (or finalize the stream) to ensure hideOutput is reset, log the error and/or show an alert to the user, and avoid setting pentahoUrl when the request fails; reference generate(), reportsService.getPentahoRunReportData, pentahoUrl, hideOutput and sanitizer when making the change.src/app/reporting-plugin/models/select-option.model.ts (1)
18-26: Avoidany[]in SelectOption constructor.
constructor(options: any[])makes it easy to pass malformed data (wrong order/types) without TypeScript catching it.Introduce a typed init shape (e.g.,
{ id: number|string; name: string }) or a fixed tuple type for[id, name].
As per coding guidelines, “src/app/**/*.ts: Use TypeScript … with strict typing conventions.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reporting-plugin/models/select-option.model.ts` around lines 18 - 26, The SelectOption constructor currently accepts an untyped options: any[] which allows malformed data; change the constructor to accept a strict typed shape (e.g., constructor(input: { id: number | string; name: string }) or a fixed tuple type [number|string, string]) and update the assignments in the SelectOption class to use input.id / input.name (or input[0] / input[1] with the tuple), adding any necessary parsing (e.g., Number(...) if you need numeric ids) and type annotations for id and name to satisfy strict TypeScript rules.src/app/reporting-plugin/models/chart-data.model.ts (1)
17-30: Type the ChartData input instead ofresponse: any.
constructor(response: any)andobject: anyparsing hides the expected API shape. This increases the chance of runtime failures if response structure changes (e.g., columnHeaders order).Define a response interface for
columnHeadersanddatarows, and narrow types when mapping.
As per coding guidelines, “Use TypeScript … with strict typing conventions.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reporting-plugin/models/chart-data.model.ts` around lines 17 - 30, The constructor currently uses untyped any for response and object which hides the expected API shape; define a typed interface (e.g., ChartApiResponse { columnHeaders: { columnName: string }[]; data: { row: (string|number)[] }[] }) and change constructor(response: ChartApiResponse) to use that type, then narrow the mappings in ChartData (use response.columnHeaders[x].columnName and response.data.map(r => String(r.row[0])) / Number(r.row[1]) or properly typed tuple access) so the keysLabel, valuesLabel, keys and values are strongly typed and not based on any/unknown; update references to columnHeaders, data and object.row accordingly and add minimal safe guards (optional chaining or defaults) where appropriate.src/app/reporting-plugin/models/report-parameter.model.ts (1)
18-45: Strengthen ReportParameter typing instead of pervasiveany/any[].
ReportParameterdefines multiple fields asany(defaultVal,selectOne,selectAll,parentParameterName, etc.) and the constructor acceptsoptions: any[]. That undermines the “strict typing conventions” expectation and makes downstream form logic fragile (runtime shape mismatches won’t be caught).Consider introducing a
ReportParameterInitinterface (or tuple type) matching the API response order, and type each field explicitly (or at leastunknownwith safe narrowing).
As per coding guidelines, “src/app/**/*.ts: Use TypeScript for all application code with strict typing conventions.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reporting-plugin/models/report-parameter.model.ts` around lines 18 - 45, ReportParameter currently uses many any types and options: any[] in the constructor; define a strongly-typed input shape (e.g. ReportParameterInit interface or tuple matching the API order) and replace options: any[] with that type in the ReportParameter constructor, then explicitly type fields like name:string, variable:string, label:string, displayType:string, formatType:string, defaultVal: unknown (or specific union), selectOne: boolean | string | null, selectAll: boolean | null, parentParameterName: string | null, inputName: string, selectOptions: Array<{ value: string; label: string }> (or appropriate item type), childParameters: ReportParameter[] (or typed array), pentahoName: string | null; update the constructor to safely assign values from the typed input (use defaults and narrowing/casts where needed) and remove any / any[] usages across the class (refer to ReportParameter, constructor, and the fields defaultVal, selectOne, selectAll, parentParameterName, selectOptions, childParameters, pentahoName).src/app/savings/common-resolvers/savings-transaction-reciept.resolver.ts (1)
33-46: Recommend typing resolver output instead ofObservable<any>.
resolve()returnsObservable<any>. Consider updating this to a concrete return type matchingReportsService.getPentahoRunReportData()(e.g.,Observable<HttpResponse<Blob>>or whatever the service actually returns), and avoidanyin resolver APIs.
Please confirm the exact return type ofReportsService.getPentahoRunReportData(...)insrc/app/reporting-plugin/services/reports.service.tsand then align this resolver’s signature accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/savings/common-resolvers/savings-transaction-reciept.resolver.ts` around lines 33 - 46, The resolver signature currently returns Observable<any>; change it to the concrete return type returned by ReportsService.getPentahoRunReportData (e.g., Observable<HttpResponse<Blob>> or Observable<Blob> depending on the service) by checking the exact return type in reports.service.ts, then replace Observable<any> in the resolve(route: ActivatedRouteSnapshot) method signature with that concrete type and import any necessary types (HttpResponse/Blob or the service's custom type); update SavingsTransactionRecieptResolver (resolve) and any callers to match the new type.src/app/loans/common-resolvers/loans-transaction-reciept.resolver.ts (1)
31-44: Recommend tightening resolver typing (avoidObservable<any>).
resolve(...): Observable<any>should be updated to a concrete type aligned withReportsService.getPentahoRunReportData(...)return value to avoidanyin resolver contracts.Please confirm the return type in
src/app/reporting-plugin/services/reports.service.tsand update the resolver signature accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/common-resolvers/loans-transaction-reciept.resolver.ts` around lines 31 - 44, The resolver currently uses a loose type Observable<any>; inspect ReportsService.getPentahoRunReportData in src/app/reporting-plugin/services/reports.service.ts to determine its concrete return type (e.g., Observable<ReportData>, Observable<Blob>, or another interface), then replace Observable<any> in loans-transaction-reciept.resolver.ts resolve signature with that concrete type, update the import for the type if needed, and ensure the returned Observable from resolve and the data object (R_transactionId) remain compatible with the chosen type.src/app/reporting-plugin/resolvers/run-report.resolver.ts (1)
38-40: Recommend typingresolve()output (avoidObservable<any>).
resolve(...): Observable<any>should be replaced with a concrete type that matchesReportsService.getReportParams(...)to avoidanyin resolver outputs.Please confirm the return type of
getReportParams(route.params['name'])insrc/app/reporting-plugin/services/reports.service.ts, then update this resolver signature accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reporting-plugin/resolvers/run-report.resolver.ts` around lines 38 - 40, The resolver return type is currently Observable<any>; inspect ReportsService.getReportParams in src/app/reporting-plugin/services/reports.service.ts to determine its concrete Observable<T> return type (e.g., Observable<ReportParams> or Observable<YourType>), then change resolve(route: ActivatedRouteSnapshot): Observable<any> to resolve(route: ActivatedRouteSnapshot): Observable<ThatConcreteType> (keeping the import/type reference consistent) so resolve and ReportsService.getReportParams use the same concrete type.
🤖 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/reporting-plugin/models/chart-data.model.ts`:
- Around line 9-16: Remove the duplicated MPL license header block that appears
a second time in the file (the repeated Mozilla Public License v.2.0 header);
keep only the first canonical header at the top of the file, delete the
redundant block below it, and ensure a single license header remains with normal
spacing before the rest of the module (chart-data.model.ts).
In `@src/app/reporting-plugin/models/report-parameter.model.ts`:
- Around line 9-16: Remove the duplicated MPL header by deleting the second
header block (the one starting at line 9) and keep only the initial header at
the top of the file; then replace loose any types used in this model by
adding/using appropriate TypeScript interfaces for the constructor parameter and
properties like defaultVal, selectOne, selectAll, selectOptions,
childParameters, and pentahoName in the ReportParameter class (or similarly
named class) so those fields and the constructor are strictly typed instead of
using any.
In `@src/app/reporting-plugin/models/select-option.model.ts`:
- Around line 9-16: Remove the duplicated MPL header block (keep the first
header and delete the second identical block) and update the SelectOption
constructor signature to use a stricter type instead of options: any[]; change
it to options: [number, string] (or define and use an explicit interface like
SelectOptionEntry { id: number; label: string } and accept options:
SelectOptionEntry[]) in the SelectOption class/constructor, and then update any
internal usage of options within the SelectOption constructor and any callers of
new SelectOption(...) (or methods that pass the options array) to match the new
typed shape.
In `@src/app/reporting-plugin/services/reports.service.ts`:
- Around line 1-15: Remove the duplicated MPL license header block so only a
single header remains at the top of the file; delete the second identical
comment block that begins at line 9 in the file and keep the first header, then
run the repository header tools (npm run headers:check and npm run headers:add)
to validate/update the file header; ensure no other duplicate header comments
remain in src/app/reporting-plugin/services/reports.service.ts.
---
Outside diff comments:
In `@src/app/clients/clients-view/general-tab/general-tab.component.ts`:
- Around line 111-114: Replace the hardcoded alert message in
general-tab.component.ts (the object with type: 'error' and message: 'Failed to
load Loan Application PDF report.') with an i18n lookup using
TranslateService.instant(...) (follow the pattern used in
personal-data-tab.component.ts for alert messages), then add the new translation
key to your i18n files and run npm run translations:extract to populate
translations; ensure the alert uses the translated string instead of the
literal.
In `@src/app/reporting-plugin/resolvers/run-report.resolver.ts`:
- Around line 1-16: The file contains two identical MPL license header blocks;
remove the duplicate second header block (the consecutive /** ... */ that
appears after the first header) so only a single MPL header remains at the top
of the file, ensuring the header:check step will pass; search for the repeated
/** Copyright since 2025 Mifos Initiative ... */ block and delete the latter
occurrence.
In `@src/app/reports/run-report/chart/chart.component.ts`:
- Around line 145-162: The color string produced by randomColor() is invalid
because it uses rgb(...) with four arguments; update randomColor() to return a
valid CSS color using rgba(r,g,b,a) (or the modern rgb(r g b / a)) including the
0.6 alpha, so Chart.js can parse it; then ensure randomColorArray(length)
continues to call randomColor() to build the array (functions: randomColor and
randomColorArray).
In `@src/app/reports/run-report/run-report.component.ts`:
- Around line 452-484: The subscription in runReportAndExport sets
this.isProcessing = true but only resets it in the success handler; update
reportsService.getRunReportData(reportName, payload) to use an RxJS finalize to
always set this.isProcessing = false and add an error callback on the subscribe
to surface an alert (use alertService.alert with translateService.instant for
type/message using your existing translation keys) so errors don't leave the UI
stuck; reference runReportAndExport, reportsService.getRunReportData,
isProcessing, alertService.alert, translateService.instant and import/use
finalize from 'rxjs/operators' (or rxjs) to perform the guaranteed reset.
- Around line 325-341: The valueChanges subscriptions created in
setChildControls() leak and addControl() silently no-ops for existing controls;
fix by introducing a component-level Subject (e.g., ngDestroy$) and pipe each
valueChanges with takeUntil(this.ngDestroy$) (import Subject and takeUntil) and
implement OnDestroy to call this.ngDestroy$.next() and complete(); inside the
subscription, before adding child controls check
this.reportForm.contains(child.name) and use
this.reportForm.setControl(child.name, new UntypedFormControl(...)) to update
existing controls (otherwise addControl for new ones), and keep
fetchSelectOptions logic unchanged.
In `@src/app/reports/run-report/table-and-sms/table-and-sms.component.ts`:
- Around line 173-203: The download currently uses a hardcoded filename
('filename.xlsx') instead of the computed fileName; inside exportToXLS() replace
the hardcoded a.download assignment with the computed fileName (ensure fileName
includes the .xlsx extension or append it if missing) so that the anchor element
uses fileName when triggering the download; update references around the
fileName constant and the a.download assignment in exportToXLS() to use the same
variable.
---
Nitpick comments:
In `@src/app/loans/common-resolvers/loans-transaction-reciept.resolver.ts`:
- Around line 31-44: The resolver currently uses a loose type Observable<any>;
inspect ReportsService.getPentahoRunReportData in
src/app/reporting-plugin/services/reports.service.ts to determine its concrete
return type (e.g., Observable<ReportData>, Observable<Blob>, or another
interface), then replace Observable<any> in
loans-transaction-reciept.resolver.ts resolve signature with that concrete type,
update the import for the type if needed, and ensure the returned Observable
from resolve and the data object (R_transactionId) remain compatible with the
chosen type.
In
`@src/app/loans/loans-view/transactions/export-transactions/export-transactions.component.ts`:
- Around line 64-68: The constructor currently accesses
this.route.parent.parent.data.subscribe(...) which can throw if parent or
parent.parent is undefined; fix by adding null-safety or using optional chaining
(e.g., check this.route.parent && this.route.parent.parent before subscribing)
or, better, inject the correct ActivatedRoute that owns loanDetailsData and
subscribe on that; ensure the subscription assigns loansAccountId =
data.loanDetailsData.accountNo only when data and loanDetailsData exist and
handle the undefined case (or bail out) to satisfy strict type safety.
- Around line 94-111: The generate() method lacks error handling for the
reportsService.getPentahoRunReportData(...) Observable; add an error path (or
use pipe(finalize())) so the component doesn't get stuck with UI state.
Specifically, update generate() to handle errors from getPentahoRunReportData
(or finalize the stream) to ensure hideOutput is reset, log the error and/or
show an alert to the user, and avoid setting pentahoUrl when the request fails;
reference generate(), reportsService.getPentahoRunReportData, pentahoUrl,
hideOutput and sanitizer when making the change.
In `@src/app/reporting-plugin/models/chart-data.model.ts`:
- Around line 17-30: The constructor currently uses untyped any for response and
object which hides the expected API shape; define a typed interface (e.g.,
ChartApiResponse { columnHeaders: { columnName: string }[]; data: { row:
(string|number)[] }[] }) and change constructor(response: ChartApiResponse) to
use that type, then narrow the mappings in ChartData (use
response.columnHeaders[x].columnName and response.data.map(r =>
String(r.row[0])) / Number(r.row[1]) or properly typed tuple access) so the
keysLabel, valuesLabel, keys and values are strongly typed and not based on
any/unknown; update references to columnHeaders, data and object.row accordingly
and add minimal safe guards (optional chaining or defaults) where appropriate.
In `@src/app/reporting-plugin/models/report-parameter.model.ts`:
- Around line 18-45: ReportParameter currently uses many any types and options:
any[] in the constructor; define a strongly-typed input shape (e.g.
ReportParameterInit interface or tuple matching the API order) and replace
options: any[] with that type in the ReportParameter constructor, then
explicitly type fields like name:string, variable:string, label:string,
displayType:string, formatType:string, defaultVal: unknown (or specific union),
selectOne: boolean | string | null, selectAll: boolean | null,
parentParameterName: string | null, inputName: string, selectOptions: Array<{
value: string; label: string }> (or appropriate item type), childParameters:
ReportParameter[] (or typed array), pentahoName: string | null; update the
constructor to safely assign values from the typed input (use defaults and
narrowing/casts where needed) and remove any / any[] usages across the class
(refer to ReportParameter, constructor, and the fields defaultVal, selectOne,
selectAll, parentParameterName, selectOptions, childParameters, pentahoName).
In `@src/app/reporting-plugin/models/select-option.model.ts`:
- Around line 18-26: The SelectOption constructor currently accepts an untyped
options: any[] which allows malformed data; change the constructor to accept a
strict typed shape (e.g., constructor(input: { id: number | string; name: string
}) or a fixed tuple type [number|string, string]) and update the assignments in
the SelectOption class to use input.id / input.name (or input[0] / input[1] with
the tuple), adding any necessary parsing (e.g., Number(...) if you need numeric
ids) and type annotations for id and name to satisfy strict TypeScript rules.
In `@src/app/reporting-plugin/resolvers/run-report.resolver.ts`:
- Around line 38-40: The resolver return type is currently Observable<any>;
inspect ReportsService.getReportParams in
src/app/reporting-plugin/services/reports.service.ts to determine its concrete
Observable<T> return type (e.g., Observable<ReportParams> or
Observable<YourType>), then change resolve(route: ActivatedRouteSnapshot):
Observable<any> to resolve(route: ActivatedRouteSnapshot):
Observable<ThatConcreteType> (keeping the import/type reference consistent) so
resolve and ReportsService.getReportParams use the same concrete type.
In `@src/app/reports/run-report/chart/chart.component.ts`:
- Around line 57-69: ngOnChanges currently calls getRunReportData
unconditionally which dereferences this.dataObject; add a defensive
null/undefined check in ngOnChanges (or at the start of getRunReportData) to
return early if this.dataObject is falsy or missing required properties (e.g.,
this.dataObject.report or this.dataObject.formData) before calling
reportsService.getChartRunReportData(this.dataObject.report.name,
this.dataObject.formData); keep using setPieChart(this.inputData) and
hideOutput=false only after a successful response. Ensure checks reference the
same symbols: ngOnChanges, getRunReportData, dataObject, getChartRunReportData,
and setPieChart.
In `@src/app/reports/run-report/table-and-sms/table-and-sms.component.ts`:
- Around line 99-117: In getRunReportData(), add an error handler to the
Observable subscription so UI and progress state are always updated on failure;
specifically, extend the subscribe(...) call on
reportsService.getRunReportData(...) with an error callback that calls
progressBarService.decrease(), sets hideOutput = false (or an appropriate safe
state), and sets notExistsReportData/csvData/displayedColumns appropriately
(e.g., clear csvData and displayedColumns and set notExistsReportData = true) to
ensure the UI recovers; keep existing success handling (setOutputTable,
toBeExportedToRepo, columnTypes population) intact and only move shared
finalization into both paths or a finalize-like cleanup to avoid duplication.
In `@src/app/savings/common-resolvers/savings-transaction-reciept.resolver.ts`:
- Around line 33-46: The resolver signature currently returns Observable<any>;
change it to the concrete return type returned by
ReportsService.getPentahoRunReportData (e.g., Observable<HttpResponse<Blob>> or
Observable<Blob> depending on the service) by checking the exact return type in
reports.service.ts, then replace Observable<any> in the resolve(route:
ActivatedRouteSnapshot) method signature with that concrete type and import any
necessary types (HttpResponse/Blob or the service's custom type); update
SavingsTransactionRecieptResolver (resolve) and any callers to match the new
type.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48d045af-d06e-4b86-82b2-11a90915a827
📒 Files selected for processing (24)
src/app/clients/clients-view/general-tab/general-tab.component.tssrc/app/clients/clients-view/personal-data-tab/personal-data-tab.component.tssrc/app/loans/common-resolvers/loans-transaction-reciept.resolver.tssrc/app/loans/loans-view/transactions/export-transactions/export-transactions.component.tssrc/app/organization/sms-campaigns/sms-campaign-stepper/edit-sms-campaign-step/edit-business-rule-parameters/edit-business-rule-parameters.component.tssrc/app/organization/sms-campaigns/sms-campaign-stepper/edit-sms-campaign-step/edit-sms-campaign-step.component.tssrc/app/organization/sms-campaigns/sms-campaign-stepper/sms-campaign-step/business-rule-parameters/business-rule-parameters.component.tssrc/app/organization/sms-campaigns/sms-campaign-stepper/sms-campaign-step/sms-campaign-step.component.tssrc/app/products/manage-tax-components/view-tax-component/view-tax-component.component.htmlsrc/app/reporting-plugin/models/chart-data.model.tssrc/app/reporting-plugin/models/report-parameter.model.tssrc/app/reporting-plugin/models/select-option.model.tssrc/app/reporting-plugin/resolvers/reports.resolver.tssrc/app/reporting-plugin/resolvers/run-report.resolver.tssrc/app/reporting-plugin/services/reports.service.tssrc/app/reports/reports-routing.module.tssrc/app/reports/reports.service.tssrc/app/reports/run-report/birt/birt.component.tssrc/app/reports/run-report/chart/chart.component.tssrc/app/reports/run-report/pentaho/pentaho.component.tssrc/app/reports/run-report/run-report.component.tssrc/app/reports/run-report/table-and-sms/table-and-sms.component.tssrc/app/savings/common-resolvers/savings-transaction-reciept.resolver.tssrc/app/savings/savings-account-view/transactions-tab/export-transactions/export-transactions.component.ts
| /** | ||
| * Copyright since 2025 Mifos Initiative | ||
| * | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
| */ | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
FILE="src/app/reporting-plugin/models/chart-data.model.ts"
rg -n "Copyright since 2025 Mifos Initiative" "$FILE" | wc -lRepository: openMF/web-app
Length of output: 60
🏁 Script executed:
cat -n src/app/reporting-plugin/models/chart-data.model.tsRepository: openMF/web-app
Length of output: 1203
Remove the duplicated MPL license header block at lines 9–15.
The file contains two identical copyright headers (lines 1–7 and lines 9–15). Keep only the first header to comply with the header check requirement and maintain a single canonical license block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/reporting-plugin/models/chart-data.model.ts` around lines 9 - 16,
Remove the duplicated MPL license header block that appears a second time in the
file (the repeated Mozilla Public License v.2.0 header); keep only the first
canonical header at the top of the file, delete the redundant block below it,
and ensure a single license header remains with normal spacing before the rest
of the module (chart-data.model.ts).
There was a problem hiding this comment.
@IOhacker — The review comment is valid. Looking at the full file content, the file indeed has two identical MPL 2.0 license headers:
- First header: Lines 1–7 (original, pre-existing)
- Second header: Lines 9–15 (added by this PR — visible in the diff)
The fix is to remove the newly added duplicate block. Here's the corrected file header:
/**
* Copyright since 2025 Mifos Initiative
*
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/
/** Chart Data Model */
export class ChartData {
// ...
}The same duplicate-header issue may also be present in the other newly modified model files (report-parameter.model.ts, select-option.model.ts) and resolvers, since the AI summary shows identical license header additions were made to all of them. It's worth auditing those files as well before merging.
🧠 Learnings used
Learnt from: DeathGun44
Repo: openMF/web-app PR: 3179
File: src/app/savings/common-resolvers/savings-account-actions.resolver.ts:44-57
Timestamp: 2026-02-17T12:26:35.042Z
Learning: In TypeScript files (e.g., src/app/.../*.ts), avoid using Observable<any> as a project-wide pattern for API responses. Treat typing the full API response layer as a cross-cutting refactor and track it as a separate enhancement backlog item rather than addressing it in individual bug fixes. When you encounter API responses, introduce specific interfaces/types for the response shapes and use proper typing instead of any, updating services and resolvers accordingly.
| /** | ||
| * Copyright since 2025 Mifos Initiative | ||
| * | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
| */ | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
FILE="src/app/reporting-plugin/models/select-option.model.ts"
rg -n "Copyright since 2025 Mifos Initiative" "$FILE" | wc -lRepository: openMF/web-app
Length of output: 60
🏁 Script executed:
cat -n src/app/reporting-plugin/models/select-option.model.tsRepository: openMF/web-app
Length of output: 928
Remove the duplicated MPL license header block (lines 9-15).
The file contains two identical copyright headers. Keep only the first header (lines 1-7) and remove the duplicate. This will ensure the file passes npm run headers:check validation.
Additionally, improve the SelectOption constructor typing by replacing options: any[] with a more specific type (e.g., options: [number, string] or an explicit interface) to comply with strict TypeScript typing conventions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/reporting-plugin/models/select-option.model.ts` around lines 9 - 16,
Remove the duplicated MPL header block (keep the first header and delete the
second identical block) and update the SelectOption constructor signature to use
a stricter type instead of options: any[]; change it to options: [number,
string] (or define and use an explicit interface like SelectOptionEntry { id:
number; label: string } and accept options: SelectOptionEntry[]) in the
SelectOption class/constructor, and then update any internal usage of options
within the SelectOption constructor and any callers of new SelectOption(...) (or
methods that pass the options array) to match the new typed shape.
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
Review the comments regarding duplicated license
| /** | ||
| * Copyright since 2025 Mifos Initiative | ||
| * | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
| */ | ||
|
|
|
@IOhacker — I have updated the PR to address the feedback provided in the review. Key changes include: Fixed License Headers: Removed duplicate MPL 2.0 license header blocks from all newly migrated files in src/app/reporting-plugin/. Verified with npm run headers:check. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/app/reporting-plugin/services/reports.service.ts (3)
34-78: Consider strict typing for lookup response DTOs in a dedicated refactoring.The service still uses
Observable<any>and untypedresponse: anyingetReports*,getReportParams,getSelectOptions,getPentahoParams, andgetBirtParamsmethods. Concrete DTOs (ReportParameter,SelectOption,ChartDataResponse) already exist in the models layer and are instantiated within these methods, so introducing proper generics likeObservable<ReportParameter[]>and typed responses would improve type safety downstream.However, introducing strict typing across all API response layers is a cross-cutting refactoring tracked separately from individual bug fix PRs. If this PR is not dedicated to typing improvements, consider deferring this enhancement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reporting-plugin/services/reports.service.ts` around lines 34 - 78, Update the untyped Observables and response parameters to concrete DTO types: change getReportParams(reportName: string): Observable<ReportParameter[]> and getSelectOptions(inputString: string): Observable<SelectOption[]> (and replace the map callback param from response: any to a typed response matching the API shape); change getPentahoParams(reportId: number) and the getReports()/getReportsByOffice(officeId: string) signatures to use the appropriate model DTO (e.g., Observable<ChartDataResponse> or Observable<Report> as defined in your models) and type their map callbacks accordingly so new ReportParameter(entry.row) and new SelectOption(entry.row) map over typed response.data instead of any.
122-185: Consider this as part of a project-wide API response typing initiative.The type improvement to
Observable<HttpResponse<ArrayBuffer>>is technically valid, butObservable<any>is a project-wide pattern across services and resolvers. Per project guidelines, comprehensive API response typing is tracked as a separate enhancement rather than addressed in individual PRs. If this improvement is desired, consider opening a dedicated issue to systematically update all services (e.g.,organization.service.ts,centers.service.ts) as part of a coordinated refactoring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reporting-plugin/services/reports.service.ts` around lines 122 - 185, The PR introduced more specific return types for report fetchers but the codebase standard is to keep service return types as Observable<any>; revert any changes that tighten typing in getPentahoRunReportData and getBirtRunReportData (and leave getBirtParams as Observable<any>) so these methods continue returning Observable<any> and match the project-wide pattern—if you want stronger types, open a separate issue/PR to refactor all services consistently.
86-114: Import ChartDataResponse and tighten type signatures for the run-report methods.The
getChartRunReportDatamethod constructs aChartDataobject from a response typed asany, butChartData's constructor explicitly expectsChartDataResponse. Additionally, both methods acceptformData: objectwhich provides no validation for parameter serialization. Add the missing import and type the response and formData parameters explicitly:Suggested changes
-import { ChartData } from '../models/chart-data.model'; +import { ChartData, ChartDataResponse } from '../models/chart-data.model';- getRunReportData(reportName: string, formData: object): Observable<any> { + getRunReportData(reportName: string, formData: Record<string, string | number | boolean>): Observable<any> {- getChartRunReportData(reportName: string, formData: object): Observable<ChartData> { + getChartRunReportData(reportName: string, formData: Record<string, string | number | boolean>): Observable<ChartData> {- .pipe(map((response: any) => new ChartData(response))); + .pipe(map((response: ChartDataResponse) => new ChartData(response)));Aligns with strict typing conventions per
src/app/**/*.tscoding guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reporting-plugin/services/reports.service.ts` around lines 86 - 114, Import ChartDataResponse and tighten the method signatures: update getRunReportData(reportName: string, formData: Record<string, string | number | boolean>): Observable<ChartDataResponse> and getChartRunReportData(reportName: string, formData: Record<string, string | number | boolean>): Observable<ChartData>; in both methods iterate Object.entries(formData) but convert each value to string when calling HttpParams.set; call this.http.get<ChartDataResponse>(`/runreports/${reportName}`, { params: httpParams }) in both places and in getChartRunReportData keep the .pipe(map((response: ChartDataResponse) => new ChartData(response))); add the missing import for ChartDataResponse used by ChartData.
🤖 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/reporting-plugin/models/report-parameter.model.ts`:
- Around line 13-40: Change the ReportParameter constructor to accept a fixed
tuple instead of a loose array (e.g. [string, string, string, string, string,
string|number|boolean|null, string|null, string|null, string|null]) so the
compiler enforces the nine expected indices used to set name, variable, label,
displayType, formatType, defaultVal, selectOne, selectAll, parentParameterName
in the ReportParameter class; update the constructor parameter type and remove
the explicit casts (as string | null) when assigning these fields. Also address
the unused pentahoName property on ReportParameter—either initialize pentahoName
in the constructor from an appropriate tuple slot or remove the pentahoName
declaration if it’s not needed. Ensure inputName is still set as
`R_${this.variable}` after assignment.
---
Nitpick comments:
In `@src/app/reporting-plugin/services/reports.service.ts`:
- Around line 34-78: Update the untyped Observables and response parameters to
concrete DTO types: change getReportParams(reportName: string):
Observable<ReportParameter[]> and getSelectOptions(inputString: string):
Observable<SelectOption[]> (and replace the map callback param from response:
any to a typed response matching the API shape); change
getPentahoParams(reportId: number) and the
getReports()/getReportsByOffice(officeId: string) signatures to use the
appropriate model DTO (e.g., Observable<ChartDataResponse> or Observable<Report>
as defined in your models) and type their map callbacks accordingly so new
ReportParameter(entry.row) and new SelectOption(entry.row) map over typed
response.data instead of any.
- Around line 122-185: The PR introduced more specific return types for report
fetchers but the codebase standard is to keep service return types as
Observable<any>; revert any changes that tighten typing in
getPentahoRunReportData and getBirtRunReportData (and leave getBirtParams as
Observable<any>) so these methods continue returning Observable<any> and match
the project-wide pattern—if you want stronger types, open a separate issue/PR to
refactor all services consistently.
- Around line 86-114: Import ChartDataResponse and tighten the method
signatures: update getRunReportData(reportName: string, formData: Record<string,
string | number | boolean>): Observable<ChartDataResponse> and
getChartRunReportData(reportName: string, formData: Record<string, string |
number | boolean>): Observable<ChartData>; in both methods iterate
Object.entries(formData) but convert each value to string when calling
HttpParams.set; call
this.http.get<ChartDataResponse>(`/runreports/${reportName}`, { params:
httpParams }) in both places and in getChartRunReportData keep the
.pipe(map((response: ChartDataResponse) => new ChartData(response))); add the
missing import for ChartDataResponse used by ChartData.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88c6113a-0a1a-45d2-906f-59915463203a
📒 Files selected for processing (6)
src/app/reporting-plugin/models/chart-data.model.tssrc/app/reporting-plugin/models/report-parameter.model.tssrc/app/reporting-plugin/models/select-option.model.tssrc/app/reporting-plugin/resolvers/reports.resolver.tssrc/app/reporting-plugin/resolvers/run-report.resolver.tssrc/app/reporting-plugin/services/reports.service.ts
✅ Files skipped from review due to trivial changes (2)
- src/app/reporting-plugin/resolvers/reports.resolver.ts
- src/app/reporting-plugin/resolvers/run-report.resolver.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/reporting-plugin/models/select-option.model.ts
- src/app/reporting-plugin/models/chart-data.model.ts
| export class ReportParameter { | ||
| name: string; | ||
| variable: string; | ||
| label: string; | ||
| displayType: string; | ||
| formatType: string; | ||
| defaultVal: string | number | boolean | null; | ||
| selectOne: string | null; | ||
| selectAll: string | null; | ||
| parentParameterName: string | null; | ||
| inputName: string; | ||
| selectOptions: SelectOption[] = []; | ||
| childParameters: ReportParameter[] = []; | ||
| pentahoName: string | null; | ||
|
|
||
| constructor(options: (string | number | boolean | null)[]) { | ||
| this.name = options[0] as string; | ||
| this.variable = options[1] as string; | ||
| this.label = options[2] as string; | ||
| this.displayType = options[3] as string; | ||
| this.formatType = options[4] as string; | ||
| this.defaultVal = options[5]; | ||
| this.selectOne = options[6]; | ||
| this.selectAll = options[7]; | ||
| this.parentParameterName = options[8] as string | null; | ||
| this.inputName = `R_${this.variable}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '"strictPropertyInitialization"|"strict": true' -g 'tsconfig*.json'
rg -n 'pentahoName|constructor\(options' src/app/reporting-plugin/models/report-parameter.model.tsRepository: openMF/web-app
Length of output: 204
🏁 Script executed:
# Find where ReportParameter is instantiated
rg -n 'new ReportParameter' -A 3 src/app/reporting-plugin/services/reports.service.ts
# Check for any usage of pentahoName
rg -n 'pentahoName' src/app/reporting-plugin/
# Check the structure of entry.row to understand the payload shape
rg -n 'entry\.row|\.row\[' src/app/reporting-plugin/services/reports.service.ts -B 2 -A 2Repository: openMF/web-app
Length of output: 699
🏁 Script executed:
# Check all usages of pentahoName across the entire codebase
rg -n 'pentahoName' --type ts --type html --type tsx
# Get the full ReportParameter model to see the complete context
cat -n src/app/reporting-plugin/models/report-parameter.model.ts
# Check if there are any tests or other files that might show the expected payload shape
rg -n 'entry\.row' -B 3 -A 3 src/app/reporting-plugin/Repository: openMF/web-app
Length of output: 3000
🏁 Script executed:
# Check the main tsconfig.json for strict mode setting
cat -n tsconfig.json | grep -A 5 -B 5 '"strict"'
# Also verify if there are any compilation errors reported
git statusRepository: openMF/web-app
Length of output: 258
🏁 Script executed:
# Check main tsconfig.json directly
head -50 tsconfig.jsonRepository: openMF/web-app
Length of output: 741
Improve type safety of the constructor by using a tuple type instead of a loose array.
The constructor accepts (string | number | boolean | null)[] but always accesses exactly 9 indices matching the API payload from entry.row. This should be a tuple type for better type safety and to allow the compiler to guarantee the correct properties are assigned without casts.
Additionally, pentahoName is declared on line 26 but never assigned in the constructor and has no usages in the codebase—either initialize it or remove it.
🔧 Proposed fix
export class ReportParameter {
name: string;
variable: string;
label: string;
displayType: string;
formatType: string;
defaultVal: string | number | boolean | null;
selectOne: string | null;
selectAll: string | null;
parentParameterName: string | null;
inputName: string;
selectOptions: SelectOption[] = [];
childParameters: ReportParameter[] = [];
- pentahoName: string | null;
+ pentahoName: string | null = null;
- constructor(options: (string | number | boolean | null)[]) {
- this.name = options[0] as string;
- this.variable = options[1] as string;
- this.label = options[2] as string;
- this.displayType = options[3] as string;
- this.formatType = options[4] as string;
+ constructor(
+ options: [string, string, string, string, string, string | number | boolean | null, string | null, string | null, string | null]
+ ) {
+ this.name = options[0];
+ this.variable = options[1];
+ this.label = options[2];
+ this.displayType = options[3];
+ this.formatType = options[4];
this.defaultVal = options[5];
this.selectOne = options[6];
this.selectAll = options[7];
- this.parentParameterName = options[8] as string | null;
+ this.parentParameterName = options[8];
this.inputName = `R_${this.variable}`;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/reporting-plugin/models/report-parameter.model.ts` around lines 13 -
40, Change the ReportParameter constructor to accept a fixed tuple instead of a
loose array (e.g. [string, string, string, string, string,
string|number|boolean|null, string|null, string|null, string|null]) so the
compiler enforces the nine expected indices used to set name, variable, label,
displayType, formatType, defaultVal, selectOne, selectAll, parentParameterName
in the ReportParameter class; update the constructor parameter type and remove
the explicit casts (as string | null) when assigning these fields. Also address
the unused pentahoName property on ReportParameter—either initialize pentahoName
in the constructor from an appropriate tuple slot or remove the pentahoName
declaration if it’s not needed. Ensure inputName is still set as
`R_${this.variable}` after assignment.
2 participants
Description
Restructure the existing Reporting plugin to better support branch model operations while maintaining backward compatibility with current functionality.
Key Changes:
ReportsService, core models (ChartData,ReportParameter,SelectOption), and resolvers (ReportsResolver,RunReportResolver) to a new dedicated directory:src/app/reporting-plugin/.ReportsServicewith a newgetReportsByOffice(officeId: string)method to support explicit branch-based filtering.ReportsServiceat the old location (src/app/reports/reports.service.ts) to ensure zero breakage for existing dependencies.Screenshots (Mandatory for UI Changes)
Impact
This change reduces coupling between the Reporting feature and other core modules, making the reporting logic easier to maintain and extend for multi-branch operations.
Summary by CodeRabbit
Refactor
Refactor
Chores