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: migrate MyInfo functionality to TypeScript #560

Merged
merged 59 commits into from
Nov 17, 2020

Conversation

mantariksh
Copy link
Contributor

@mantariksh mantariksh commented Nov 2, 2020

This PR extracts MyInfo functionality from combined SPCP/MyInfo modules, and places it in its own service written in TypeScript.

Why a service and not a module?

I wrote this with the future PublicForm migration in mind. Currently, the two middlewares exported by the MyInfo controller (addMyInfo and verifyMyInfoVals) are middlewares in the form retrieval and submissions flow respectively. However, we want the logic in all these middlewares to eventually be combined into one function which calls several services. The intention here is for MyInfo to be one of those services, and for the logic in the current MyInfo controller functions to be moved into the PublicForm controllers in future.

What about tests?

I have removed the old tests so that this PR builds successfully. The new tests will come in a separate PR and will be merged into this branch.

@mantariksh mantariksh force-pushed the refactor/ts-myinfo branch 4 times, most recently from c78d5f8 to 79a5c38 Compare November 10, 2020 15:15
@mantariksh mantariksh marked this pull request as ready for review November 10, 2020 15:59
@mantariksh mantariksh requested review from karrui and tshuli November 10, 2020 15:59
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

lgtm, do test submitting for all MyInfo fields before merging

@mantariksh mantariksh force-pushed the refactor/ts-myinfo branch 2 times, most recently from d1bf584 to 1a6e9c6 Compare November 11, 2020 09:00
@mantariksh
Copy link
Contributor Author

waiting to finish writing tests + test on staging before merging

Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

very nice and clean 😍

src/app/services/myinfo/myinfo.errors.ts Outdated Show resolved Hide resolved
src/app/services/myinfo/myinfo.service.ts Show resolved Hide resolved
src/app/services/myinfo/myinfo.service.ts Show resolved Hide resolved
src/app/services/myinfo/myinfo.service.ts Outdated Show resolved Hide resolved
src/app/services/myinfo/myinfo.service.ts Show resolved Hide resolved
@mantariksh
Copy link
Contributor Author

unfortunately MyInfo staging is still down so we can't test this on staging yet. will give them a call later today

@mantariksh mantariksh force-pushed the refactor/ts-myinfo branch 2 times, most recently from 7295fb2 to 87599eb Compare November 16, 2020 03:27
@mantariksh
Copy link
Contributor Author

myinfo staging still not working

@mantariksh mantariksh merged commit d330cb6 into develop Nov 17, 2020
@karrui karrui mentioned this pull request Nov 17, 2020
@karrui karrui deleted the refactor/ts-myinfo branch November 18, 2020 07:24
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.

4 participants