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

Fix: Conditionally show the KYC Card in the Crypto-to-Fiat tab #2956

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

rumzledz
Copy link
Contributor

@rumzledz rumzledz commented Aug 16, 2024

Description

If the KYC status is not approved or if there is no liquidation address.

non-loading state loading state
Screenshot 2024-08-16 at 15 18 35 c2f loader

Testing

Important

In the bridgeXYZMutation/src/index.ts file, make sure you update the apiKey with our sandbox api key.
Connect a Dev Wallet that hasn't done a KYC check yet.

  1. Visit a Colony
  2. Open the UserHub
  3. Go to the Crypto to fiat tab
  4. Verify that you can see the KYC card and the form is disabled
    image
  5. Click "Complete verification"
  6. Verify you are taken to the Crypto to fiat page
  7. Completed step 1 Verification and make sure you're approved
  8. Go back to a Colony
  9. Open the UserHub
  10. Go to the Crypto to fiat tab
  11. Verify that the KYC card is not present anymore and that the form is enabled

Resolves #2953

@rumzledz rumzledz self-assigned this Aug 16, 2024
@rumzledz rumzledz marked this pull request as ready for review August 16, 2024 14:23
@rumzledz rumzledz requested review from a team as code owners August 16, 2024 14:23
@rumzledz rumzledz force-pushed the fix/2953-conditionally-show-kyc-card-in-userhub branch from 4a88ebd to 8db6ecf Compare August 16, 2024 14:58
Nortsova
Nortsova previously approved these changes Aug 19, 2024
Copy link
Contributor

@Nortsova Nortsova left a comment

Choose a reason for hiding this comment

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

Hey @rumzledz , thanks for this nice fix! It works great! ✨

I can confirm that the loading state is present:
image

I can also confirm that I can see the KYC card, and the form is disabled:
image

I also need to create a liquidation address in order to make the KYC card disappear.

After I filled in the bank details, the KYC card disappeared from the UserHub, and the form became enabled:
image


This is probably not related to current PR, but it is something that I found quite important:

Note 1: There was a feature to open the verification modal by clicking "Complete verification," but it seems this functionality was removed. I'm not sure if it should be re-implemented now (from step 5 to 6).

Note 2: Sometimes, on submit, there is an error in the terminal, but nothing happens in the UI.
image
As a result, the user doesn't understand why the form is not being submitted. In the example provided, an API key is missing, but it could be any API error, and the user would never know what's going on:
image

Note 3: The "Update details" link should redirect to the Crypto-to-Fiat tab, but instead, it redirects to http://localhost:9091/account/profile
image

@rumzledz
Copy link
Contributor Author

rumzledz commented Aug 19, 2024

Thanks for reviewing this @Nortsova !

Note 1: There was a feature to open the verification modal by clicking "Complete verification," but it seems this functionality was removed. I'm not sure if it should be re-implemented now (from step 5 to 6).

I double checked the original implementation of this button after our call earlier and I can confirm that on master, it just simply visits the USER_HOME_ROUTE which is not even the Crypto-to-fiat tab, it's just the User account tab 🤔 The only change I made is to make it navigate to the Crypto-to-fiat tab since that's on the issue. I'll speak to product to determine the actual behaviour here regarding modals and will deal with it separately 👌

Note 2: Sometimes, on submit, there is an error in the terminal, but nothing happens in the UI.

This is worth investigating separately as this is also on the master branch, great find!

Note 3: The "Update details" link should redirect to the Crypto-to-Fiat tab, but instead, it redirects to http://localhost:9091/account/profile

This is current behaviour so I left it as it is since there's no mention of it in the issue. But I will speak to product to confirm and will amend it if necessary since it will be a very tiny change 😄

@rumzledz rumzledz force-pushed the fix/2953-conditionally-show-kyc-card-in-userhub branch from 8db6ecf to 898e601 Compare August 19, 2024 14:08
@rumzledz
Copy link
Contributor Author

rumzledz commented Aug 19, 2024

I've just pushed a fix for your point no. 3 regarding the "Update details" button @Nortsova and I had to rebase as well 👀 But yeah, as for your points 1 & 2, it would be better if we dealt with those separately 😄 Can I get a re-review when you're free? Cheers!

@rumzledz rumzledz requested review from Nortsova and a team August 19, 2024 14:11
@rumzledz
Copy link
Contributor Author

Another update for you @Nortsova 😂 We won't be required to show any modals when the user lands on the Crypto-to-fiat page after clicking the Complete verification button

Copy link
Contributor

@Nortsova Nortsova left a comment

Choose a reason for hiding this comment

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

Thank you @rumzledz for considering all my comments! I checked it again and everything works as expected! ✨ Great job!

Copy link
Collaborator

@jakubcolony jakubcolony left a comment

Choose a reason for hiding this comment

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

Smashing work @rumzledz 💪

Loading state is looking great:
image


The banner shows when either KYC has not been completed or liquidation address is missing:

Screen.Recording.2024-08-20.at.21.49.07.mov

Once there is a liquidation address, the banner disappears:

image

Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

Yup, this is working as expected. Nicely done.

Screenshot from 2024-08-23 17-14-11
Screenshot from 2024-08-23 17-14-35
Screenshot from 2024-08-23 17-17-12

I do want to note that one of your testing steps is missing, adding bank details, as without them (as it should I might add), the tab should not be active

Nice work once again

@rumzledz rumzledz merged commit d71856d into master Aug 23, 2024
4 of 6 checks passed
@rumzledz rumzledz deleted the fix/2953-conditionally-show-kyc-card-in-userhub branch August 23, 2024 16:17
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.

Crypto-to-fiat: Add KYC status checks to user hub tab
4 participants