Setting up initial iDenfy KYC verification #190
Merged
Conversation
This file contains 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
|
Task linked: CU-85zrjpj3n KYC |
|
Coverage after merging CU-85zrjpj3n_KYC_Jonatan-Escobar into master
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Visit the preview URL for this PR (updated for commit bfcee6d): https://newm-artist-portal--pr190-cu-85zrjpj3n-kyc-jon-u3dfpmbz.web.app (expires Mon, 03 Apr 2023 21:59:11 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: ec3483d4c62309afc398865bfd6a9fc9e03e1d46 |
escobarjonatan
commented
Feb 27, 2023
src/api/idenfy/types.ts
Outdated
| readonly externalRef?: string | null; | ||
| readonly firstName?: string; | ||
| readonly generateDigitString?: boolean; | ||
| readonly lastName?: string; |
There was a problem hiding this comment.
along with the required clientId that we send to them, we can also send the optional firstName lastName fields. This would ensure the person doing the verification session has the same name as the profile. Maybe we can do that post MVP?
| @@ -4,4 +4,7 @@ import { mockProfile } from "./mockProfile"; | |||
| export const mockSession: SessionState = { | |||
| isLoggedIn: true, | |||
| profile: mockProfile, | |||
| idenfy: { | |||
| authToken: "", | |||
| }, | |||
There was a problem hiding this comment.
I thought we should put the authToken in state so we dont trigger an api call to fetch a token every time they open/close the modal? We can set the expiry time of the session/token in the admin settings as well
There was a problem hiding this comment.
I'd recommend saving it in a cookie, similar to the NEWM session token. Should be slightly more secure.
| @@ -0,0 +1,28 @@ | |||
| import { styled } from "@mui/material/styles"; | |||
There was a problem hiding this comment.
Copied from the token sale
src/modules/session/thunks.ts
Outdated
|
|
||
| dispatch(receiveIdenfyToken(idenfyTokenResponse.data.authToken)); | ||
|
|
||
| dispatch(updateProfile({ verifiedStatus: "pending" })); |
There was a problem hiding this comment.
This is something that has been on my mind. We update status to "pending" after we initiate the verification session. if the user decides not to finish the process, it will remain in pending. They will still be able to create a new session if they login out/in or refresh. So technically this will still work but it would not be an accurate status. @scandycuz thoughts?
There was a problem hiding this comment.
If we only trigger the "pending" status as part of the redirect that navigates the user after they complete the process, that should be fine. We could either have a query param in the redirect url that tells the page to update the verification status in the back-end, or the redirect page could make that API call and then navigate to the page where the user should arrive.
The next best thing would be if iDenfy made a call to our back-end when the verification process started, but I imagine that isn't possible.
There was a problem hiding this comment.
@scandycuz After learning more about iDenfy and the process. The piece that i couldn't get around was what you last stated, currently there's no support for adding a callback on token request.
However, it looks like we(BE) will be getting an automatic callback as soon as the user finishes their session. At that point BE will update the status to pending and then approved as soon as they get the second callback.
We can configure the success/fail url and add a param like you suggested and then send a request to BE for the status, but at that point, why not go with the other solution to send BE a status request every 1 minute or so after triggering a session verification. I can see us having to send interval requests even with the query solution.
I updated the code to remove changing status for now though
There was a problem hiding this comment.
If I understand correctly, it sounds like there are two potential options, and I imagine both options should start when the user completes the front-end flow in the iFrame, that way we know they've completed the process:
Option 1:
Start pinging the back-end for "verified" or "unverified" status as soon as user completes flow
- We should probably limit the pinging to 30 minutes or so, just in case there is a back-end issue where the iDenfy call couldn't get through to the NEWM API.
- We'll probably want to maintain some sort of persisted "pending" status on the front-end, along with the timestamp of when it started pinging, so that the app will know whether to ping after the user closes the browser and re-opens it (this could be done post MVP if we wanted, as it's more of a nice-to-have).
- Also, if we go that route, we probably don't need the "pending" status on the NEWM back-end, since the front-end will only look for the "verified" or "unverified" status to stop pinging.
Pros:
- Don't have to worry about a manually setting a "pending" status on the back-end that may get out of sync with iDenfy's status
Cons
- The front-end will maintain it's own logic to determine if and how long it should ping for the status
Option 2:
When the user completes the flow, make a call to the NEWM server that manually updates the status to "pending".
- Front-end has logic to always ping the back-end when status from back-end is "pending"
Pros
- Front-end doesn't have to manage the "pending" status of the verification as this is stored on the back-end
Cons
- Artist portal would be updating status to "pending", but iDenfy is the source of truth, so there is a possibility the status could get out of sync
- Probably need two separate endpoints on the back-end for updating the status, one that iDenfy calls and one that the artist portal calls.
Let me know if that makes sense, I think either approach should be fine.
There was a problem hiding this comment.
I imagine the first option would be the best. Less complex on the back-end, and persisting the timestap for when the front-end started pinging should be straightforward enough. I wouldn't implement it for now though, we should implement redux-persist first (it's implemented in the token sale app but not the artist portal), and then we'll be able to just store verificationPingStartedAt in Redux so we don't have to use LocalStorage.
Assuming we go with that approach, we should proably reach out to Walter and let him know that an isVerified boolean actually works better on the back-end.
| import { FunctionComponent } from "react"; | ||
| import { useSelector } from "react-redux"; | ||
| import { selectSession } from "modules/session"; | ||
| import { Modal } from "components"; |
There was a problem hiding this comment.
I noticed that this modal that was brought in from the token sale is full screen. Are we going to keep our modals full screen or did we import the wrong one? @scandycuz
There was a problem hiding this comment.
I was concerned that having a partial screen modal would be too small for viewing the artist agreement PDF. It seems fine to allow for different sized modals, depending on the need. We could add an isFullscreen boolean if we wanted.
There was a problem hiding this comment.
I will have a conversation with product bc i feel like the full screen is a better option for iframes as well to allow flexibility vs trying to fit a full page in 600px lol
src/pages/home/profile/Profile.tsx
Outdated
|
|
||
| const handleVerificationSession = () => { | ||
| if (!authToken) { | ||
| dispatch(requestVerificationToken({ clientId: id })); |
There was a problem hiding this comment.
If you are testing this out in your local, I would recommend commenting this line out after you get the authToken and hardcode that in. We have a limited amount of testing tokens and would like to use them once the BE work is done so that is more E2E.
|
Coverage after merging CU-85zrjpj3n_KYC_Jonatan-Escobar into master
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #190 +/- ##
==========================================
- Coverage 30.98% 29.14% -1.85%
==========================================
Files 181 186 +5
Lines 1646 1702 +56
Branches 331 336 +5
==========================================
- Hits 510 496 -14
- Misses 1091 1165 +74
+ Partials 45 41 -4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Coverage after merging CU-85zrjpj3n_KYC_Jonatan-Escobar into master
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Coverage after merging CU-85zrjpj3n_KYC_Jonatan-Escobar into master
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Reference discussion: #190 (comment) It's not letting me comment on the actual thread but Trevor and i had a slack discussion where we discussed option #1 more in depth and this is the path forward: After the user finishes the process through iFrame or other device:
On fail:
Before we do this, we need to implement redux-persist. |
scandycuz
approved these changes
Mar 6, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Add the ability for a user to be able to verify themselves through iDenfy KYC services.
idenfy480.mov