Aman-Mittal commented on code in PR #308: URL: https://github.com/apache/fineract-backoffice-ui/pull/308#discussion_r3743326926
########## src/app/features/reporting/report-execution.service.ts: ########## @@ -0,0 +1,182 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { HttpClient, HttpParams } from '@angular/common/http'; +import { Injectable, inject } from '@angular/core'; +import { Observable, forkJoin, map, of, switchMap } from 'rxjs'; + +import { ConfigService } from '../../core/services/config.service'; + +export interface ReportSelectOption { + readonly id: string | number; + readonly name: string; + readonly isAll?: boolean; +} + +export interface ReportParameter { + readonly name: string; + readonly variable: string; + readonly label: string; + readonly displayType: string; + readonly formatType: string; + readonly defaultValue: unknown; + readonly selectOne: unknown; + readonly selectAll: unknown; + readonly parentParameterName: string | null; + readonly queryParameter: string; + readonly options: readonly ReportSelectOption[]; +} + +export interface ReportResult { + readonly columnHeaders?: readonly Record<string, unknown>[]; + readonly data?: readonly Record<string, unknown>[]; + readonly [key: string]: unknown; +} + +interface GenericResultset { + readonly data?: readonly { readonly row?: unknown }[]; +} + +export type ReportParameterValues = Readonly<Record<string, string | number>>; + +/** + * The generated run-reports client exposes only a fixed positional subset of report parameters. + * Report definitions are dynamic, so this service deliberately uses a named query map instead. + */ +@Injectable({ providedIn: 'root' }) +export class ReportExecutionService { + private readonly http = inject(HttpClient); + private readonly config = inject(ConfigService); + + getReportParameters(reportName: string): Observable<ReportParameter[]> { + const params = new HttpParams().set('R_reportListing', reportName).set('parameterType', 'true'); + + return this.http.get<GenericResultset>(this.reportUrl('FullParameterList'), { params }).pipe( + map((response) => (response.data ?? []).map((entry) => this.toReportParameter(entry.row))), + switchMap((parameters) => this.loadSelectOptions(parameters)), + ); + } + + runReport(reportName: string, values: ReportParameterValues): Observable<ReportResult> { + const params = this.buildRunParams(values, false, 'HTML'); + return this.http.get<ReportResult>(this.reportUrl(reportName), { params }); + } + + downloadCsv(reportName: string, values: ReportParameterValues): Observable<string> { + const params = this.buildRunParams(values, true, 'CSV'); + return this.http.get(this.reportUrl(reportName), { params, responseType: 'text' }); + } + + private loadSelectOptions(parameters: ReportParameter[]): Observable<ReportParameter[]> { + const requests = parameters.map((parameter) => { + if (parameter.displayType !== 'select') { + return of(parameter); + } + + const params = new HttpParams().set('parameterType', 'true'); + return this.http.get<GenericResultset>(this.reportUrl(parameter.name), { params }).pipe( + map((response) => { + const options = (response.data ?? []).map((entry) => this.toSelectOption(entry.row)); + const offersAll = String(parameter.selectAll).toUpperCase() === 'Y'; + const alreadyOffersAll = options.some((option) => String(option.id) === '-1'); + + return { + ...parameter, + options: + offersAll && !alreadyOffersAll + ? [...options, { id: '-1', name: '', isAll: true }] + : options, + }; + }), + ); Review Comment: This fetch is eager and the `forkJoin` below is all-or-nothing, so one lookup that cannot succeed takes the whole parameter form down with it. Two of the platform's stock lookups are **cascading** — they declare a parent and their SQL cannot run until the parent has a value: ``` GET /runreports/loanOfficerIdSelectAll?parameterType=true → 403 (data integrity / SQL error) GET /runreports/loanProductIdSelectAll?parameterType=true → 403 ``` `loanOfficerIdSelectAll` declares parent `OfficeIdSelectOne`, `loanProductIdSelectAll` declares parent `currencyIdSelectAll`. Because `forkJoin` errors as soon as any inner observable errors, `getReportParameters` fails and the screen renders `report-parameters-error` with no fields at all. I enumerated every Table report against a running Fineract: **20 of 59 stop rendering a parameter form entirely**, among them Active Loans – Summary, Active Loans – Details, Portfolio at Risk, Portfolio at Risk by Branch, Loans Pending Approval, Loans Awaiting Disbursal, Written-Off Loans, Rescheduled Loans, Client Loans Listing and Expected Payments By Date. The suite stays green because `report-parameter-backend.spec.ts` exercises `Client Listing`, whose only parameter is `OfficeIdSelectOne` — the case that works. This does not need cascading implemented (that is #301). It needs the fetch bounded: skip a parameter that declares a parent, and let a failed lookup degrade to an empty option list rather than failing its siblings. ```suggestion // A parameter that declares a parent cannot be fetched yet: its query filters on the // parent's value, and without one the platform answers 403 with a SQL error. Populating // these as the parent is chosen is #301; until then they render empty rather than taking // the rest of the form down. if (parameter.displayType !== 'select' || parameter.parentParameterName) { return of(parameter); } const params = new HttpParams().set('parameterType', 'true'); return this.http.get<GenericResultset>(this.reportUrl(parameter.name), { params }).pipe( map((response) => { const options = (response.data ?? []).map((entry) => this.toSelectOption(entry.row)); const offersAll = String(parameter.selectAll).toUpperCase() === 'Y'; const alreadyOffersAll = options.some((option) => String(option.id) === '-1'); return { ...parameter, options: offersAll && !alreadyOffersAll ? [...options, { id: '-1', name: '', isAll: true }] : options, }; }), // One lookup the tenant's data cannot satisfy should cost that one field, not the report. catchError(() => of(parameter)), ); ``` `catchError` needs adding to the `rxjs` import on line 21. Worth adding a backend test over a report with a cascading parameter — `Active Loans - Summary` is the obvious one — so this failure mode is covered rather than sitting just outside the suite. ########## src/app/features/reporting/report-execution.service.ts: ########## @@ -0,0 +1,182 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { HttpClient, HttpParams } from '@angular/common/http'; +import { Injectable, inject } from '@angular/core'; +import { Observable, forkJoin, map, of, switchMap } from 'rxjs'; + +import { ConfigService } from '../../core/services/config.service'; + +export interface ReportSelectOption { + readonly id: string | number; + readonly name: string; + readonly isAll?: boolean; +} + +export interface ReportParameter { + readonly name: string; + readonly variable: string; + readonly label: string; + readonly displayType: string; + readonly formatType: string; + readonly defaultValue: unknown; + readonly selectOne: unknown; + readonly selectAll: unknown; + readonly parentParameterName: string | null; + readonly queryParameter: string; + readonly options: readonly ReportSelectOption[]; +} + +export interface ReportResult { + readonly columnHeaders?: readonly Record<string, unknown>[]; + readonly data?: readonly Record<string, unknown>[]; + readonly [key: string]: unknown; +} + +interface GenericResultset { + readonly data?: readonly { readonly row?: unknown }[]; +} + +export type ReportParameterValues = Readonly<Record<string, string | number>>; + +/** + * The generated run-reports client exposes only a fixed positional subset of report parameters. + * Report definitions are dynamic, so this service deliberately uses a named query map instead. + */ +@Injectable({ providedIn: 'root' }) +export class ReportExecutionService { + private readonly http = inject(HttpClient); + private readonly config = inject(ConfigService); + + getReportParameters(reportName: string): Observable<ReportParameter[]> { + const params = new HttpParams().set('R_reportListing', reportName).set('parameterType', 'true'); + + return this.http.get<GenericResultset>(this.reportUrl('FullParameterList'), { params }).pipe( + map((response) => (response.data ?? []).map((entry) => this.toReportParameter(entry.row))), + switchMap((parameters) => this.loadSelectOptions(parameters)), + ); + } + + runReport(reportName: string, values: ReportParameterValues): Observable<ReportResult> { + const params = this.buildRunParams(values, false, 'HTML'); + return this.http.get<ReportResult>(this.reportUrl(reportName), { params }); + } + + downloadCsv(reportName: string, values: ReportParameterValues): Observable<string> { + const params = this.buildRunParams(values, true, 'CSV'); + return this.http.get(this.reportUrl(reportName), { params, responseType: 'text' }); + } + + private loadSelectOptions(parameters: ReportParameter[]): Observable<ReportParameter[]> { + const requests = parameters.map((parameter) => { + if (parameter.displayType !== 'select') { + return of(parameter); + } + + const params = new HttpParams().set('parameterType', 'true'); + return this.http.get<GenericResultset>(this.reportUrl(parameter.name), { params }).pipe( + map((response) => { + const options = (response.data ?? []).map((entry) => this.toSelectOption(entry.row)); + const offersAll = String(parameter.selectAll).toUpperCase() === 'Y'; + const alreadyOffersAll = options.some((option) => String(option.id) === '-1'); + + return { + ...parameter, + options: + offersAll && !alreadyOffersAll + ? [...options, { id: '-1', name: '', isAll: true }] + : options, + }; + }), + ); + }); + + return requests.length > 0 ? forkJoin(requests) : of([]); + } + + private toReportParameter(row: unknown): ReportParameter { + if (!Array.isArray(row) || row.length < 9) { + throw new Error('The report parameter template returned an invalid row.'); + } Review Comment: Minor, and only a hazard if a tenant ever returns a shorter row: this `throw` is inside a `map`, so a single malformed row fails the entire parameter list rather than the one field it describes. Every row on a stock instance is 9 columns, so nothing is broken today — but skipping the row would keep the rest of the report usable if that ever stops being true. ########## src/app/features/reporting/report-execution.service.ts: ########## @@ -0,0 +1,182 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { HttpClient, HttpParams } from '@angular/common/http'; +import { Injectable, inject } from '@angular/core'; Review Comment: ```suggestion import { Observable, catchError, forkJoin, map, of, switchMap } from 'rxjs'; ``` Goes with the suggestion on `loadSelectOptions` below. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
