Skip to content

WEB-918: Fix savings application edit flow#3535

Merged
IOhacker merged 1 commit intoopenMF:devfrom
YousufFFFF:WEB-918-fix-savings-edit
Apr 27, 2026
Merged

WEB-918: Fix savings application edit flow#3535
IOhacker merged 1 commit intoopenMF:devfrom
YousufFFFF:WEB-918-fix-savings-edit

Conversation

@YousufFFFF
Copy link
Copy Markdown
Member

@YousufFFFF YousufFFFF commented Apr 27, 2026

Description

This change fixes the savings account Modify Application flow so users are taken to the edit savings application page instead of getting an HTTP 400 error. The savings edit data is now loaded through supported account and template requests, which avoids the failing savings account template call that was breaking the page.

Dependencies: none beyond the existing savings account APIs.

Related issues and discussion

#WEB-918

After Video:

General.-.Google.Chrome.2026-04-27.20-05-26.mp4

Checklist

  • If you have multiple commits please combine them into one commit by squashing them.
  • Read and understood the contribution guidelines at web-app/.github/CONTRIBUTING.md.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed navigation when accessing savings account modifications to ensure proper account references.
    • Enhanced the efficiency of loading savings account template information through optimized data retrieval.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 27, 2026

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)
Validation error: Unrecognized key(s) in object: 'pre_merge_checks'
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Walkthrough

Navigation routing in the savings account view component was updated to construct a relative path including the savings account ID. Additionally, the savings service's data-fetching logic was restructured to conditionally fetch account data alone or with a template, using a two-step observable chain when template is needed.

Changes

Cohort / File(s) Summary
Savings Account Navigation
src/app/savings/savings-account-view/savings-account-view.component.ts
Modified 'Modify Application' action to navigate via a constructed relative path (../[id]/edit) instead of a simple relative route, deriving the ID from route parameters with a fallback to component data.
Savings Data Fetching
src/app/savings/savings.service.ts
Refactored getSavingsAccountAndTemplate to conditionally return account data alone when template is falsy, or execute a two-step observable flow (fetch account, then template) and merge results when template is truthy.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • IOhacker
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main objective of the PR: fixing the savings application edit flow by addressing the HTTP 400 error users encountered.
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.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/app/savings/savings-account-view/savings-account-view.component.ts (1)

222-231: Optional: simplify the relative path construction.

The paramMap.get('savingAccountId') || savingsAccountData.id fallback is defensive — the component is bound to the :savingAccountId route, so the param is always present and is the very source of savingsAccountData.id populated by the resolver. Either branch resolves to the same value. You can drop the fallback and use savingsAccountData.id directly for readability.

♻️ Proposed simplification
       case 'Modify Application':
-        this.router.navigate(
-          [
-            '../',
-            this.route.snapshot.paramMap.get('savingAccountId') || this.savingsAccountData.id,
-            'edit'
-          ],
-          { relativeTo: this.route }
-        );
+        this.router.navigate(['../', this.savingsAccountData.id, 'edit'], { relativeTo: this.route });
         break;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/app/savings/savings-account-view/savings-account-view.component.ts`
around lines 222 - 231, The navigation in the 'Modify Application' case uses a
redundant fallback: replace the expression using
route.snapshot.paramMap.get('savingAccountId') || this.savingsAccountData.id
with a single, clearer reference to this.savingsAccountData.id in the
router.navigate call (inside the case 'Modify Application' block) so the path is
constructed directly from the resolved savingsAccountData.id and remove the
unnecessary paramMap.get usage.
🤖 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/savings/savings-account-view/savings-account-view.component.ts`:
- Around line 222-231: The navigation in the 'Modify Application' case uses a
redundant fallback: replace the expression using
route.snapshot.paramMap.get('savingAccountId') || this.savingsAccountData.id
with a single, clearer reference to this.savingsAccountData.id in the
router.navigate call (inside the case 'Modify Application' block) so the path is
constructed directly from the resolved savingsAccountData.id and remove the
unnecessary paramMap.get usage.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1c04a2a0-ca57-4831-af37-ab1d1c80f4bd

📥 Commits

Reviewing files that changed from the base of the PR and between 2a6f1e3 and 53c483a.

📒 Files selected for processing (2)
  • src/app/savings/savings-account-view/savings-account-view.component.ts
  • src/app/savings/savings.service.ts

Copy link
Copy Markdown
Contributor

@IOhacker IOhacker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@IOhacker IOhacker merged commit 734a7b0 into openMF:dev Apr 27, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants