Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Review third) Update FI profile - mail api integration #283

Merged
merged 48 commits into from
Mar 20, 2024

Conversation

meissadia
Copy link
Collaborator

@meissadia meissadia commented Feb 29, 2024

Part 3 of #222

Screenshot 2024-02-29 at 4 05 55 PM

Changes

  • Formats form data for email submission
  • Triggers Mail API call upon submission
  • Replace "Simulated" submission with actual Mail API call

Planned improvements

  • Data submission (formatFinancialProfileObject): Do further comparison against defaultValues to determine which data was changed, so that we only send SBL Help the info they actually need to update. This may also be resolved if we can address the forwardRef errors we've been seeing, which may fix the issue of changes to these fields not being registered in react-hook-form

How to test this PR

  1. In console ensure routing is enabled: setIsRoutingEnabled(true)
  2. Login as a user with an associated institution
  3. Click on the Institution to View institution profile
  4. Click on Update institution profile
  5. Change some Institution data
  6. Hit submit
  7. Hopefully an email gets generated 🤞🏾
  8. In console, you should also see the data that will be sent.
    Screenshot 2024-02-28 at 5 41 29 PM

Notes

  • Submission data format (decoded)
{
     "tax_id": "tax_id",
     "rssd_id": "rss_id",
     "parent_legal_name": "parent-name",
     "parent_lei": "parent_lei",
     "parent_rssd_id": "parent-rssd",
     "top_holder_legal_name": "top-name",
     "top_holder_lei": "top-lei",
     "top_holder_rssd_id": "top-rssd",
     "sbl_institution_types": "bankSavings,minorityDepository,other",
     "sbl_institution_types_other": "Test SBL Type",
     "additional_details": "details"
}
  • Submission data format (encoded)
tax_id=tax_id&rssd_id=rss_id&parent_legal_name=parent-name&parent_lei=parent_lei&parent_rssd_id=parent-rssd&top_holder_legal_name=top-name&top_holder_lei=top-lei&top_holder_rssd_id=top-rssd&sbl_institution_types=bankSavings%2CminorityDepository%2Cother&sbl_institution_types_other=Test+SBL+Type&additional_details=details

meissadia and others added 25 commits February 22, 2024 12:03
- [Add] Fetch Institution data
- [Add] Breadcrumb to return to View page
- [Add] Review FI details
Co-authored-by: S T <shindigira@gmail.com>
-  Improved population of existing form data
- Temp fix for TextArea resizing issue
- Implements `Clear form` functionality
@meissadia meissadia changed the title Update FI profile phase2 (part II - mail api integration) (Review third) Update FI profile phase2 (part II - mail api integration) Feb 29, 2024
@meissadia meissadia changed the title (Review third) Update FI profile phase2 (part II - mail api integration) (Review third) Update FI profile - mail api integration Mar 5, 2024
src/api/requests/submitUpdateFinancialProfile.ts Outdated Show resolved Hide resolved
src/pages/ProfileForm/types.ts Outdated Show resolved Hide resolved
src/pages/Filing/UpdateFinancialProfile/UfpForm.tsx Outdated Show resolved Hide resolved
src/pages/Filing/UpdateFinancialProfile/UfpForm.tsx Outdated Show resolved Hide resolved
src/utils/formatting.tsx Outdated Show resolved Hide resolved
@meissadia
Copy link
Collaborator Author

Self-notes:

  • Institution types are always Objects, so we can remove any String checks

Base automatically changed from 222-update-fi-profile-phase3-beta to main March 13, 2024 15:24
@meissadia meissadia linked an issue Mar 14, 2024 that may be closed by this pull request
25 tasks
Copy link
Contributor

@shindigira shindigira left a comment

Choose a reason for hiding this comment

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

Coming along really nicely as this is a more complicated form than others! I found a few issues.

Did not focus on Zod/TypeScript, input validation and error handling in this review.

Copy link
Contributor

@billhimmelsbach billhimmelsbach left a comment

Choose a reason for hiding this comment

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

I went ahead and addressed the outstanding review comments, and I'm approving this for now and looking forward to the next PR! 👍

@billhimmelsbach billhimmelsbach dismissed shindigira’s stale review March 20, 2024 21:37

Dismissing this review while you're out of town. I addressed your comments above Sigmund, and we can chat about the load on error bug you found when you return. 👍

@meissadia meissadia merged commit d5f3f60 into main Mar 20, 2024
2 checks passed
@meissadia meissadia deleted the 222-update-fi-profile-phase2-mail-api-integration branch March 20, 2024 21:37
@shindigira
Copy link
Contributor

@billhimmelsbach Thanks for this. Will try and reproduce the problem and write about it.

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.

[Page] Update your financial institution profile
3 participants