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

refactor(frontend): update person-case forms for multi-channel flow #437

Closed
wants to merge 1 commit into from

Conversation

Dario-Au
Copy link
Contributor

@Dario-Au Dario-Au commented Mar 19, 2025

Summary

AB#18736 Summary and Send for validation screen
AB#19354 Copy screens to the multichannel flow

Notable Changes

Added new folder routes/protected/sin-application

  • Moved form elements from protected/person-case/{screen}.tsx to their own components protected/sin-application/{screen}Form.tsx
  • Moved types relating to the in-person application from protected/person-case/types.d.ts to protected/sin-application/types.d.ts
  • Moved validation.server.ts from protected/person-case/validation.server.ts to protected/sin-application/validation.server.ts

Updated validation.server.ts

  • Moved formData.get('input') from person-case screens to separate functions to parseScreenName()
  • Moved In-person application formatting from person-case/review.tsx & multi-channel/send-validation.tsx to formatSinApplication()

Added multi-channel/edit-application.tsx

  • When editing from multi-channel/send-validation.tsx, you are redirected to edit-application.tsx with a parameter to determine which form to render

Types of changes

What types of changes does this PR introduce?
(check all that apply by placing an x in the relevant boxes)

  • 🛠️ refactoring -- non-breaking change that improves code readability or structure
  • 🐛 bugfix -- non-breaking change that fixes an issue
  • new feature -- non-breaking change that adds functionality
  • 💥 breaking change -- change that causes existing functionality to no longer work as expected
  • 📚 documentation -- changes to documentation only
  • ⚙️ build or tooling -- ex: CI/CD, dependency upgrades

Checklist

Before submitting this PR, ensure that you have completed the following. You can fill these out now, or after creating the PR.
(check all that apply by placing an x in the relevant boxes)

  • code has been linted and formatted locally
  • added or updated tests to verify the changes
  • added adequate logging for new or updated functionality
  • ensured metrics and/or tracing are in place for monitoring and debugging
  • documentation has been updated to reflect the changes (if applicable)
  • linked this PR to a related issue (if applicable)
Linting and formatting
npm run lint:check
npm run format:check
Unit and e2e tests
npm run test
npm run test:e2e

Additional Notes

Context

The Summary and Send for Validation screen needs to be able to edit the sin application, however the screen cannot use the existing person-case screens as they are tied to the in-person flow.

Objective

The goal of this PR is to refactor person-case screens to be able to use their forms to edit the case in the multi-channel flow.

Screenshots (if applicable)

Provide screenshots or screen-recordings to help reviewers understand the visual impact of your changes, if relevant.

Sorry, something went wrong.

@Dario-Au Dario-Au force-pushed the refactor-person-case-forms branch 4 times, most recently from e52aa40 to 0f68775 Compare March 19, 2025 19:09
@Dario-Au Dario-Au force-pushed the refactor-person-case-forms branch from 0f68775 to b091bd7 Compare March 19, 2025 19:29
@sebastien-comeau
Copy link
Contributor

This PR is too large and contains repetitive issues that could have been avoided with smaller, more focused PRs. Additionally, creating a reusable form component without a clear understanding of what can be amended in existing SIN applications might not be the best approach. From my understanding, only a very small subset of a SIN application can be modified once it has been created—possibly just basic details like first name, last name, email, or birthdate. I also don’t have a complete picture of this multi-channel feature. That said, the creation and management of applications will likely differ, and a reusable component could be a viable approach in the long run. Does Interop or Power Platform have the capability to update the entire SIN application after it has been created?

@gregory-j-baker
Copy link
Contributor

I'm a bit confused about what this PR is. Initially I thought it was a re-branding (of sorts) of the in-person route (which I support since I never really liked the person-case name).. however I see there are no updated or new routes in the i18n-routes file.

I did skim through the code and it all seems good enough to me. The issues that exist in the person-case routes all exist here, of course.. which is expected since this is mostly copy/paste.

If we are creating a new flow, or even re-writing the existing flow, I think we should consider making some changes to how the form data is handled (as I mentioned a bit in our standup).


(addendum after looking at this PR a bit more...)

It just occurred to me that this PR is probably an attempt to extract out some reusable code/components for a future flow. If that's the case, that's cool.. but if that's accurate, then I think the folder structure could be improved to better encapsulate the code.. maybe something like:

routes/sin-application//
├─ components/ ← also types
├─ in-person/
├─ other-route/

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.

3 participants