WEB-976: Fix generated report downloads#3727
Conversation
|
👋 Hi @SaaiAravindhRaja — thank you for your pull request. This PR is currently blocked because we do not have a Contributor License Agreement (CLA) on file for your GitHub account. To get unblocked:
|
mifos-cla-check
Bot
added
the
cla-required
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Blob download utility and coverage src/app/core/utils/file-download.utils.ts, src/app/core/utils/file-download.utils.spec.ts |
Adds downloadBlob to create, click, and asynchronously clean up a hidden download link, with Jest coverage for URL handling and DOM cleanup. |
Report export integration src/app/reports/run-report/run-report.component.ts, src/app/reports/run-report/table-and-sms/table-and-sms.component.ts |
Replaces duplicated XLSX download code with downloadBlob; the table-and-SMS export now awaits workbook generation and returns Promise<void>. |
| Estimated code review effort: 2 (Simple) | ~10 minutes |
Possibly related PRs
- openMF/web-app#3706: Both PRs modify the XLSX export paths in the report components.
Suggested reviewers: alberto-art3ch
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| 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. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly summarizes the main change: fixing generated report downloads. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/reports/run-report/run-report.component.ts (1)
526-533: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAwait the
exportToXLSPromise before clearing the processing state.
exportToXLSwas updated to be anasyncmethod, but its returnedPromiseis not awaited here. As a result,this.isProcessing = falseexecutes immediately before the Excel workbook generation completes. This prematurely removes the loading UI state while the file is still generating in the background.🐛 Proposed fix
- this.exportToXLS(reportName, res.data, displayedColumns); - } else { - this.alertService.alert({ - type: this.translateService.instant('errors.report.type'), - message: this.translateService.instant('errors.report.generatedNoData', { reportName }) - }); - } - this.isProcessing = false; + this.exportToXLS(reportName, res.data, displayedColumns) + .catch((err) => console.error('Export failed:', err)) + .finally(() => { + this.isProcessing = false; + this.cdr.markForCheck(); + }); + } else { + this.alertService.alert({ + type: this.translateService.instant('errors.report.type'), + message: this.translateService.instant('errors.report.generatedNoData', { reportName }) + }); + this.isProcessing = false; + this.cdr.markForCheck(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/reports/run-report/run-report.component.ts` around lines 526 - 533, Update the report-generation flow around exportToXLS so it awaits the async exportToXLS Promise before setting isProcessing to false. Preserve the existing no-data alert behavior and ensure the processing state remains active until workbook generation completes.
🧹 Nitpick comments (1)
src/app/reports/run-report/table-and-sms/table-and-sms.component.ts (1)
224-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRefactor CSV download to use the shared
downloadBlobutility.Instead of encoding the CSV as a data URI and injecting an anchor element directly, use the new
downloadBlobutility. This provides consistent download behavior, centralizes the logic, and avoids potential browser URL length limits associated with large data URIs.♻️ Proposed refactor
downloadCSV(fileName: string, delimiter: string) { const headers = this.displayedColumns; - let csv = this.csvData.map((object: any) => object.row.map((cell: any) => sanitizeCsvValue(cell)).join(delimiter)); - csv.unshift(`data:text/csv;charset=utf-8,${headers.map(sanitizeCsvValue).join(delimiter)}`); - csv = csv.join('\r\n'); - const link = document.createElement('a'); - link.setAttribute('href', encodeURI(csv)); - link.setAttribute('download', fileName); - document.body.appendChild(link); - link.click(); - document.body.removeChild(link); + const csvLines = this.csvData.map((object: any) => object.row.map((cell: any) => sanitizeCsvValue(cell)).join(delimiter)); + csvLines.unshift(headers.map(sanitizeCsvValue).join(delimiter)); + const csvString = csvLines.join('\r\n'); + + const blob = new Blob([csvString], { type: 'text/csv;charset=utf-8;' }); + downloadBlob(blob, fileName); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/reports/run-report/table-and-sms/table-and-sms.component.ts` around lines 224 - 235, Update the downloadCSV method to build the CSV text as it does currently, then pass it to the shared downloadBlob utility with the appropriate CSV content type and fileName. Remove the data-URI encoding and direct anchor creation, click, and cleanup logic while preserving the existing headers, delimiter, sanitization, and row formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/app/reports/run-report/run-report.component.ts`:
- Around line 526-533: Update the report-generation flow around exportToXLS so
it awaits the async exportToXLS Promise before setting isProcessing to false.
Preserve the existing no-data alert behavior and ensure the processing state
remains active until workbook generation completes.
---
Nitpick comments:
In `@src/app/reports/run-report/table-and-sms/table-and-sms.component.ts`:
- Around line 224-235: Update the downloadCSV method to build the CSV text as it
does currently, then pass it to the shared downloadBlob utility with the
appropriate CSV content type and fileName. Remove the data-URI encoding and
direct anchor creation, click, and cleanup logic while preserving the existing
headers, delimiter, sanitization, and row formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c73d9ee-8a1b-4fff-8e52-4621e4402d8a
📒 Files selected for processing (4)
src/app/core/utils/file-download.utils.spec.tssrc/app/core/utils/file-download.utils.tssrc/app/reports/run-report/run-report.component.tssrc/app/reports/run-report/table-and-sms/table-and-sms.component.ts
|
@SaaiAravindhRaja could you please sign the CLA? |
|
hi! I have signed the CLA, not sure why it isn't updated here |
|
can I check if its https://mifos.org/about-us/financial-legal/mifos-contributor-agreement/ or if there's another link too? thanks! |
|
@DavidH-1 could you please help us? |
2 participants
Description
filename.xlsxRelated issues and discussion
Screenshots, if any
No layout changes.
Checklist
web-app/.github/CONTRIBUTING.md.Testing
./node_modules/.bin/jest --config jest.config.ts src/app/core/utils/file-download.utils.spec.ts --runInBand --coverage=false./node_modules/.bin/eslint . && ./node_modules/.bin/stylelint "src/**/*.scss" && ./node_modules/.bin/prettier . --check && ./node_modules/.bin/htmlhint "src" --config .htmlhintrcnode --max-old-space-size=16384 ./node_modules/@angular/cli/bin/ng build --configuration production --output-hashing=none --output-path /tmp/mifos-web-app-build-check-*semgrep scan --config p/default --error --quiet src/app/core/utils/file-download.utils.ts src/app/core/utils/file-download.utils.spec.ts src/app/reports/run-report/table-and-sms/table-and-sms.component.ts src/app/reports/run-report/run-report.component.tsgitleaks git --log-opts origin/dev..HEAD --no-banner --redactgit diff --check origin/dev...HEADSummary by CodeRabbit
New Features
Bug Fixes
Tests