(WEB-370): Handle Fineract 500 errors gracefully for provisioning entries#3284
Open
YousufFFFF wants to merge 1 commit intoopenMF:devfrom
Open
(WEB-370): Handle Fineract 500 errors gracefully for provisioning entries#3284YousufFFFF wants to merge 1 commit intoopenMF:devfrom
YousufFFFF wants to merge 1 commit intoopenMF:devfrom
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Create Component src/app/accounting/provisioning-entries/create-provisioning-entry/create-provisioning-entry.component.ts |
Replaced subscribe callback with object-based observer (subscribe({ next, error })). On HTTP 500 error after create, navigate to provisioning-entry list; success navigation remains via response.resourceId. Removed a TODO and adjusted whitespace. |
Resolvers src/app/accounting/provisioning-entries/view-provisioning-entry/provisioning-entry-entries.resolver.ts, src/app/accounting/provisioning-entries/view-provisioning-entry/provisioning-entry.resolver.ts |
Wrapped service calls with pipe(catchError(...)). For 500 errors, emit fallback objects ({ pageItems: [], error: true } and { id: provisioningEntryId, error: true } respectively); rethrow other errors. Added of, catchError, throwError imports. |
HTTP Interceptor src/app/core/http/error-handler.interceptor.ts |
Suppresses generic 500 error alert for provisioning-entry GET/POST requests. Added TranslateService import/use for translated messages. |
Sequence Diagram(s)
sequenceDiagram
participant User
participant CreateComp as CreateComponent
participant API as BackendAPI
participant Router
participant Interceptor as ErrorHandlerInterceptor
participant ResolverEntries as EntriesResolver
participant ResolverEntry as EntryResolver
User->>CreateComp: submit(createPayload)
CreateComp->>API: POST /provisioning-entries (via Interceptor)
Note over Interceptor,API: Interceptor may suppress 500 alerts for provisioning-entry requests
alt API returns 201 with resourceId
API-->>CreateComp: 201 { resourceId }
CreateComp->>Router: navigate(/provisioning-entry/:resourceId)
else API returns 500 (no resourceId)
API-->>CreateComp: 500 error
CreateComp->>Router: navigate(/provisioning-entries) // fallback on 500
end
User->>Router: open /provisioning-entry/:id
Router->>ResolverEntry: resolve(id)
ResolverEntry->>API: GET /provisioning-entries/:id (via Interceptor)
alt API success
API-->>ResolverEntry: 200 { entry }
ResolverEntry-->>Router: resolved entry
else API 500
API-->>ResolverEntry: 500
ResolverEntry-->>Router: { id, error: true } // fallback
end
Router->>ResolverEntries: resolve entries page
ResolverEntries->>API: GET /provisioning-entries (via Interceptor)
alt API success
API-->>ResolverEntries: 200 { pageItems }
ResolverEntries-->>Router: resolved pageItems
else API 500
API-->>ResolverEntries: 500
ResolverEntries-->>Router: { pageItems: [], error: true } // fallback
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
- alberto-art3ch
🚥 Pre-merge checks | ✅ 3
✅ 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 clearly and specifically identifies the main change: handling Fineract 500 errors gracefully for provisioning entries. It directly corresponds to the primary objective and changes across all modified files. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 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: 3
🧹 Nitpick comments (1)
src/app/accounting/provisioning-entries/create-provisioning-entry/create-provisioning-entry.component.ts (1)
82-92: Please replaceanyin the subscription handlers with concrete types.The new handler signatures (
response: any,error: any) reduce safety in this critical flow. Use a small response interface andHttpErrorResponsefor the error branch.Based on learnings: In TypeScript files (e.g., src/app/.../*.ts), avoid using Observable as a project-wide pattern for API responses; introduce specific interfaces/types for response shapes and use proper typing instead of any.Suggested typing update
+import { HttpErrorResponse } from '@angular/common/http'; + +interface CreateProvisioningEntryResponse { + resourceId: number | string; +} ... - next: (response: any) => { + next: (response: CreateProvisioningEntryResponse) => { this.router.navigate( ... - error: (error: any) => { + error: (error: HttpErrorResponse) => { if (error.status === 500) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/accounting/provisioning-entries/create-provisioning-entry/create-provisioning-entry.component.ts` around lines 82 - 92, The subscription handlers in create-provisioning-entry.component (the subscribe block with next: (response: any) and error: (error: any)) use any — replace these with concrete types: create a small interface (e.g., CreateProvisioningResponse { resourceId: string | number }) and use it for the next handler signature (next: (response: CreateProvisioningResponse) => ...); import and use Angular's HttpErrorResponse for the error handler (error: (error: HttpErrorResponse) => ...); also update the service method return type (e.g., createProvisioningEntry()) to Observable<CreateProvisioningResponse> so the subscribe call is fully typed and IDE/typechecker-safe.
🤖 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/accounting/provisioning-entries/view-provisioning-entry/provisioning-entry-entries.resolver.ts`:
- Around line 22-24: The resolver currently swallows all errors in the
getProvisioningEntryEntries observable by returning an empty payload in
catchError; change the catchError handler to only return of({ pageItems: [],
error: true }) when the HTTP error status is 500, and rethrow (propagate) other
errors so they can be handled upstream. Locate the pipe on
this.accountingService.getProvisioningEntryEntries(provisioningEntryId) and
replace the unconditional catchError with a conditional one that checks
err.status === 500 and returns the fallback, otherwise rethrows using
throwError(() => err) (or rethrowError equivalent) so non-500 failures are not
masked.
In
`@src/app/accounting/provisioning-entries/view-provisioning-entry/provisioning-entry.resolver.ts`:
- Around line 22-24: The catchError in the resolver's call to
accountingService.getProvisioningEntry is currently silencing all errors; change
it so only HTTP 500 responses are converted to the fallback object ({ id:
provisioningEntryId, error: true }) and all other errors are rethrown. Update
the catchError callback in the method that calls getProvisioningEntry (the
resolver function in provisioning-entry.resolver.ts) to inspect the error (e.g.,
err.status), return of({ id: provisioningEntryId, error: true }) when status ===
500, and rethrow other errors as an observable error (use RxJS throwError(() =>
err) or rethrow the error) so authorization/404/network errors propagate.
In `@src/app/core/http/error-handler.interceptor.ts`:
- Around line 88-95: The interceptor currently only suppresses alerts for GET
requests to provisioning entries; extend that logic to also suppress POST create
calls so the global alertService.alert is not shown for POST
/provisioningentries. Modify the check around request.url/request.method
(currently in the isProvisioningEntryGet variable) to detect either a GET to
'/provisioningentries/' or a POST to the collection endpoint (e.g.,
url.endsWith('/provisioningentries') && request.method === 'POST'), and use that
combined boolean to skip calling this.alertService.alert in
error-handler.interceptor.ts.
---
Nitpick comments:
In
`@src/app/accounting/provisioning-entries/create-provisioning-entry/create-provisioning-entry.component.ts`:
- Around line 82-92: The subscription handlers in
create-provisioning-entry.component (the subscribe block with next: (response:
any) and error: (error: any)) use any — replace these with concrete types:
create a small interface (e.g., CreateProvisioningResponse { resourceId: string
| number }) and use it for the next handler signature (next: (response:
CreateProvisioningResponse) => ...); import and use Angular's HttpErrorResponse
for the error handler (error: (error: HttpErrorResponse) => ...); also update
the service method return type (e.g., createProvisioningEntry()) to
Observable<CreateProvisioningResponse> so the subscribe call is fully typed and
IDE/typechecker-safe.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/accounting/provisioning-entries/create-provisioning-entry/create-provisioning-entry.component.tssrc/app/accounting/provisioning-entries/view-provisioning-entry/provisioning-entry-entries.resolver.tssrc/app/accounting/provisioning-entries/view-provisioning-entry/provisioning-entry.resolver.tssrc/app/core/http/error-handler.interceptor.ts
...counting/provisioning-entries/view-provisioning-entry/provisioning-entry-entries.resolver.ts
Outdated
Show resolved
Hide resolved
src/app/accounting/provisioning-entries/view-provisioning-entry/provisioning-entry.resolver.ts
Outdated
Show resolved
Hide resolved
YousufFFFF
force-pushed
the
fix-provisioning-entry-error-handling
branch
from
6052d17 to
2766e75
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/accounting/provisioning-entries/view-provisioning-entry/provisioning-entry-entries.resolver.ts (2)
20-29: Add proper typing to the error handler and return type.The HTTP 500 scoping is now correctly in place. However, to improve type safety per project conventions:
- Type the
errorparameter asHttpErrorResponseto catch type mismatches at compile time.- Replace
Observable<any>with a specific interface for the response shape.Based on learnings: "avoid using Observable as a project-wide pattern for API responses... introduce specific interfaces/types for the response shapes."
♻️ Proposed typing improvements
+import { HttpErrorResponse } from '@angular/common/http'; + +interface ProvisioningEntryEntriesResponse { + pageItems: any[]; + error?: boolean; +} + `@Injectable`() export class ProvisioningEntryEntriesResolver { private accountingService = inject(AccountingService); - resolve(route: ActivatedRouteSnapshot): Observable<any> { + resolve(route: ActivatedRouteSnapshot): Observable<ProvisioningEntryEntriesResponse> { const provisioningEntryId = route.paramMap.get('id'); return this.accountingService.getProvisioningEntryEntries(provisioningEntryId).pipe( - catchError((error) => { + catchError((error: HttpErrorResponse) => { if (error.status === 500) { return of({ pageItems: [], error: true }); } return throwError(() => error); }) ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/accounting/provisioning-entries/view-provisioning-entry/provisioning-entry-entries.resolver.ts` around lines 20 - 29, The resolver uses Observable<any> and an untyped error handler; update the resolve method signature to return Observable<ProvisioningEntryEntriesResponse> (define interface ProvisioningEntryEntriesResponse { pageItems: YourItemType[]; error?: boolean; ... }), update any related service method getProvisioningEntryEntries to return Observable<ProvisioningEntryEntriesResponse>, and type the catchError parameter as (error: HttpErrorResponse) => Observable<ProvisioningEntryEntriesResponse>; keep the same 500 branch returning of({ pageItems: [], error: true }) but ensure the returned object conforms to the new interface and import HttpErrorResponse from `@angular/common/http`.
12-13: Consider consolidating rxjs imports.Both import lines pull from
'rxjs'and can be merged into a single statement for cleaner code.♻️ Proposed consolidation
-import { Observable, of } from 'rxjs'; -import { catchError, throwError } from 'rxjs'; +import { Observable, of, catchError, throwError } from 'rxjs';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/accounting/provisioning-entries/view-provisioning-entry/provisioning-entry-entries.resolver.ts` around lines 12 - 13, Consolidate the duplicate rxjs imports in provisioning-entry-entries.resolver.ts by replacing the two separate import statements with a single import that includes Observable, of, catchError, and throwError (i.e., import { Observable, of, catchError, throwError } from 'rxjs';) so the symbols Observable, of, catchError, and throwError are all imported from the same statement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/app/accounting/provisioning-entries/view-provisioning-entry/provisioning-entry-entries.resolver.ts`:
- Around line 20-29: The resolver uses Observable<any> and an untyped error
handler; update the resolve method signature to return
Observable<ProvisioningEntryEntriesResponse> (define interface
ProvisioningEntryEntriesResponse { pageItems: YourItemType[]; error?: boolean;
... }), update any related service method getProvisioningEntryEntries to return
Observable<ProvisioningEntryEntriesResponse>, and type the catchError parameter
as (error: HttpErrorResponse) => Observable<ProvisioningEntryEntriesResponse>;
keep the same 500 branch returning of({ pageItems: [], error: true }) but ensure
the returned object conforms to the new interface and import HttpErrorResponse
from `@angular/common/http`.
- Around line 12-13: Consolidate the duplicate rxjs imports in
provisioning-entry-entries.resolver.ts by replacing the two separate import
statements with a single import that includes Observable, of, catchError, and
throwError (i.e., import { Observable, of, catchError, throwError } from
'rxjs';) so the symbols Observable, of, catchError, and throwError are all
imported from the same statement.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/accounting/provisioning-entries/create-provisioning-entry/create-provisioning-entry.component.tssrc/app/accounting/provisioning-entries/view-provisioning-entry/provisioning-entry-entries.resolver.tssrc/app/accounting/provisioning-entries/view-provisioning-entry/provisioning-entry.resolver.tssrc/app/core/http/error-handler.interceptor.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/accounting/provisioning-entries/create-provisioning-entry/create-provisioning-entry.component.ts
- src/app/accounting/provisioning-entries/view-provisioning-entry/provisioning-entry.resolver.ts
- src/app/core/http/error-handler.interceptor.ts
Member
Author
|
Hi @IOhacker! |
IOhacker
requested changes
Mar 3, 2026
| message: 'Internal Server Error. Please try again later.' | ||
| }); | ||
| const isProvisioningEntryGet = | ||
| request.url.includes('/provisioningentries') && (request.method === 'GET' || request.method === 'POST'); |
Contributor
There was a problem hiding this comment.
I think this piece of code reduce the possibility to catch the error in another endpoints or methods
Member
Author
There was a problem hiding this comment.
You're right to flag this!
Member
Author
There was a problem hiding this comment.
The check is intentionally scoped very narrowly, it only suppresses the toast for requests to /provisioningentries/ with GET or POST methods.
Member
Author
There was a problem hiding this comment.
All other endpoints and methods will still trigger the global error toast as normal.
Member
Author
There was a problem hiding this comment.
Also I think isClientImage404 uses this same pattern which is already in the codebase
Member
Author
|
Hi @IOhacker, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
The Create Provisioning Entry flow was displaying a false "Internal Server Error"
toast even when the entry was created successfully. This is caused by a known
Apache Fineract backend bug where provisioning entry endpoints return HTTP 500
despite completing the operation successfully.
Changes made:
create-provisioning-entry.component.tsto catchthe false 500 on POST and navigate to the list instead of showing an error toast.
catchErrorin both resolvers (provisioning-entry.resolver.tsandprovisioning-entry-entries.resolver.ts) to prevent the view page fromcrashing when the GET returns 500.
error-handler.interceptor.tsforGET requests to
/provisioningentries/{id}, following the same patternused for client image 404s.
Note: The blank fields (Created By, Created On, Amount to be Reserved) on the
view page are a Fineract backend issue and have to be reported to the Fineract team.
Related issue
WEB-370
After Video:
Home.-.Google.Chrome.2026-03-04.01-18-17.mp4
Checklist
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
Summary by CodeRabbit