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

feat : APP-363 another user purchased credits modal #2500

Open
wants to merge 15 commits into
base: dev
Choose a base branch
from

Conversation

r41ph
Copy link
Contributor

@r41ph r41ph commented Oct 10, 2024

Description

https://regennetwork.atlassian.net/browse/APP-363

This PR also addresses https://regennetwork.atlassian.net/browse/APP-409

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • provided a link to the relevant issue or specification
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

How to test

  1. https://deploy-preview-2500--regen-marketplace.netlify.app/projects/1

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

Copy link

netlify bot commented Oct 10, 2024

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit e4c032b
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/672b545a895959000867bd72
😎 Deploy Preview https://deploy-preview-2500--regen-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@r41ph
Copy link
Contributor Author

r41ph commented Oct 10, 2024

@blushi

Relevant files:
web-components/src/components/inputs/new/CustomSelect/CustomSelect.test.tsx
web-components/src/components/modal/index.tsx
web-marketplace/src/components/organisms/BuyFiatModal/BuyFiatModal.test.tsx
web-marketplace/src/components/organisms/BuyFiatModal/BuyFiatModal.tsx
web-marketplace/src/components/organisms/BuyFiatModal/BuyFiatModal.types.ts
web-marketplace/src/pages/BuyCredits/BuyCredits.Form.tsx
web-marketplace/src/pages/BuyCredits/BuyCredits.utils.ts

@r41ph r41ph requested a review from blushi October 10, 2024 10:27
@r41ph
Copy link
Contributor Author

r41ph commented Oct 10, 2024

@blushi I'm not sure how @erikalogie @clevinson can test this, can you help please?

@blushi
Copy link
Member

blushi commented Oct 10, 2024

@blushi I'm not sure how @erikalogie @clevinson can test this, can you help please?

They need to go through the buy flow with an initial user but then before clicking "purchase", they need to purchase the same credits (with crypto, since fiat microservice PR is not merged yet) with another account in another incognito window or another browser (or with CLI for @clevinson), testing the 2 cases:

  • purchase a credits amounts above the one entered by the initial user
  • purchase all credits

@r41ph
Copy link
Contributor Author

r41ph commented Oct 10, 2024

Is there any 'testing' user I can use to do this myself too?

@r41ph r41ph changed the base branch from dev to feat-APP-204-buy-credits October 10, 2024 14:55
@r41ph r41ph changed the base branch from feat-APP-204-buy-credits to dev October 10, 2024 16:02
@r41ph r41ph force-pushed the feat-APP-363-another-user-purchased-credits-modal branch 2 times, most recently from 6e7c705 to cc6f2a3 Compare October 16, 2024 11:37
@erikalogie
Copy link
Collaborator

Can I test this with credit card now?

@blushi
Copy link
Member

blushi commented Oct 17, 2024

Can I test this with credit card now?

yes, but both users should purchase with credit card so the same sell orders get selected

@r41ph r41ph requested a review from blushi October 17, 2024 10:06
@erikalogie
Copy link
Collaborator

I am trying to purchase with credit card but the next button is greyed out, not sure what I am doing wrong:

Screen.Recording.2024-10-17.at.7.23.49.AM.mov

@blushi
Copy link
Member

blushi commented Oct 17, 2024

I am trying to purchase with credit card but the next button is greyed out, not sure what I am doing wrong:

Screen.Recording.2024-10-17.at.7.23.49.AM.mov

are you logged in? with which type of account?

@erikalogie
Copy link
Collaborator

I am logged in with a web 3.0 account that has no email associated. So in this case, the email field should not be optional, as it shows here. Should this be a separate bug @blushi?

@blushi
Copy link
Member

blushi commented Oct 17, 2024

I am logged in with a web 3.0 account that has no email associated. So in this case, the email field should not be optional, as it shows here. Should this be a separate bug @blushi?

yes please file a separate bug

@erikalogie
Copy link
Collaborator

I am logged in with a web 3.0 account that has no email associated. So in this case, the email field should not be optional, as it shows here. Should this be a separate bug @blushi?

yes please file a separate bug

Ok https://regennetwork.atlassian.net/browse/APP-406

@erikalogie
Copy link
Collaborator

@blushi @r41ph I'm really having a lot of trouble testing this and getting it to work, there seem to be some bugs: https://www.loom.com/share/41b9dfb86ea14b1f8f4d4aac0198f35b

@blushi
Copy link
Member

blushi commented Oct 17, 2024

@blushi @r41ph I'm really having a lot of trouble testing this and getting it to work, there seem to be some bugs: https://www.loom.com/share/41b9dfb86ea14b1f8f4d4aac0198f35b

could you try clearing your browser cache to see if that helps?

@blushi
Copy link
Member

blushi commented Oct 17, 2024

@blushi @r41ph I'm really having a lot of trouble testing this and getting it to work, there seem to be some bugs: https://www.loom.com/share/41b9dfb86ea14b1f8f4d4aac0198f35b

could you try clearing your browser cache to see if that helps?

The reason for having tradable credits selected might be that you tested the crypto flow with tradable credits but didn't go through the end so it kept this last step in your local storage. Then you got logged out and you come back to this saved step. We keep retiring state as true or false in local storage and it can only be changed through the radio buttons on the crypto version, but you don't have access to that since you're just a visitor. This is quite an edge case but I guess we should just reset retiring back to true as soon as the card payment option is selected.

@erikalogie
Copy link
Collaborator

@blushi @r41ph I'm really having a lot of trouble testing this and getting it to work, there seem to be some bugs: https://www.loom.com/share/41b9dfb86ea14b1f8f4d4aac0198f35b

could you try clearing your browser cache to see if that helps?

The reason for having tradable credits selected might be that you tested the crypto flow with tradable credits but didn't go through the end so it kept this last step in your local storage. Then you got logged out and you come back to this saved step. We keep retiring state as true or false in local storage and it can only be changed through the radio buttons on the crypto version, but you don't have access to that since you're just a visitor. This is quite an edge case but I guess we should just reset retiring back to true as soon as the card payment option is selected.

Yes that would be great. Could you open a separate issue if you think it should be separate and put in this sprint?

@erikalogie
Copy link
Collaborator

Ok here is what is happening for me now: https://www.loom.com/share/ad7c0db6177e4f5fb2ba3db0e46c2798

@blushi
Copy link
Member

blushi commented Oct 17, 2024

@blushi @r41ph I'm really having a lot of trouble testing this and getting it to work, there seem to be some bugs: https://www.loom.com/share/41b9dfb86ea14b1f8f4d4aac0198f35b

The email login issue is unrelated to this, not sure what's happening, it looks like it doesn't work anymore on prod too. I'll check the server logs.

@erikalogie
Copy link
Collaborator

@blushi @r41ph I'm really having a lot of trouble testing this and getting it to work, there seem to be some bugs: https://www.loom.com/share/41b9dfb86ea14b1f8f4d4aac0198f35b

could you try clearing your browser cache to see if that helps?

The reason for having tradable credits selected might be that you tested the crypto flow with tradable credits but didn't go through the end so it kept this last step in your local storage. Then you got logged out and you come back to this saved step. We keep retiring state as true or false in local storage and it can only be changed through the radio buttons on the crypto version, but you don't have access to that since you're just a visitor. This is quite an edge case but I guess we should just reset retiring back to true as soon as the card payment option is selected.

Yes that would be great. Could you open a separate issue if you think it should be separate and put in this sprint?

Related to this, I've noticed that if I go back in the flow after selecting the crypto option, choose "credit card" and enter some number, then navigate away and click the "buy" button again on the same project page, I would expect to end up at the last screen I was on, which is actually first screen of the flow with credit card selected, not the last screen. Probably not the most important to address right away as most users won't be toggling between the two flows because very few people will use the keplr option.

@blushi
Copy link
Member

blushi commented Oct 17, 2024

@blushi @r41ph I'm really having a lot of trouble testing this and getting it to work, there seem to be some bugs: https://www.loom.com/share/41b9dfb86ea14b1f8f4d4aac0198f35b

The email login issue is unrelated to this, not sure what's happening, it looks like it doesn't work anymore on prod too. I'll check the server logs.

Nevermind, got the email on prod, I guess the server took a bit more time to process it or my email client was laggy.
This is an issue on staging only related to some recent changes.

@blushi
Copy link
Member

blushi commented Oct 17, 2024

@blushi @r41ph I'm really having a lot of trouble testing this and getting it to work, there seem to be some bugs: https://www.loom.com/share/41b9dfb86ea14b1f8f4d4aac0198f35b

could you try clearing your browser cache to see if that helps?

The reason for having tradable credits selected might be that you tested the crypto flow with tradable credits but didn't go through the end so it kept this last step in your local storage. Then you got logged out and you come back to this saved step. We keep retiring state as true or false in local storage and it can only be changed through the radio buttons on the crypto version, but you don't have access to that since you're just a visitor. This is quite an edge case but I guess we should just reset retiring back to true as soon as the card payment option is selected.

Yes that would be great. Could you open a separate issue if you think it should be separate and put in this sprint?

Related to this, I've noticed that if I go back in the flow after selecting the crypto option, choose "credit card" and enter some number, then navigate away and click the "buy" button again on the same project page, I would expect to end up at the last screen I was on, which is actually first screen of the flow with credit card selected, not the last screen. Probably not the most important to address right away as most users won't be toggling between the two flows because very few people will use the keplr option.

I see, right now, we only save the step as soon as the user hits the "next" button so I guess we should upgrade that to change as soon as we visit a given step. Could you create an issue for that?

@blushi
Copy link
Member

blushi commented Oct 17, 2024

@blushi @r41ph I'm really having a lot of trouble testing this and getting it to work, there seem to be some bugs: https://www.loom.com/share/41b9dfb86ea14b1f8f4d4aac0198f35b

could you try clearing your browser cache to see if that helps?

The reason for having tradable credits selected might be that you tested the crypto flow with tradable credits but didn't go through the end so it kept this last step in your local storage. Then you got logged out and you come back to this saved step. We keep retiring state as true or false in local storage and it can only be changed through the radio buttons on the crypto version, but you don't have access to that since you're just a visitor. This is quite an edge case but I guess we should just reset retiring back to true as soon as the card payment option is selected.

Yes that would be great. Could you open a separate issue if you think it should be separate and put in this sprint?

Yeah this isn't related to this work so should be in a separate issue: https://regennetwork.atlassian.net/browse/APP-409

@erikalogie
Copy link
Collaborator

@blushi @r41ph I'm really having a lot of trouble testing this and getting it to work, there seem to be some bugs: https://www.loom.com/share/41b9dfb86ea14b1f8f4d4aac0198f35b

could you try clearing your browser cache to see if that helps?

The reason for having tradable credits selected might be that you tested the crypto flow with tradable credits but didn't go through the end so it kept this last step in your local storage. Then you got logged out and you come back to this saved step. We keep retiring state as true or false in local storage and it can only be changed through the radio buttons on the crypto version, but you don't have access to that since you're just a visitor. This is quite an edge case but I guess we should just reset retiring back to true as soon as the card payment option is selected.

Yes that would be great. Could you open a separate issue if you think it should be separate and put in this sprint?

Related to this, I've noticed that if I go back in the flow after selecting the crypto option, choose "credit card" and enter some number, then navigate away and click the "buy" button again on the same project page, I would expect to end up at the last screen I was on, which is actually first screen of the flow with credit card selected, not the last screen. Probably not the most important to address right away as most users won't be toggling between the two flows because very few people will use the keplr option.

I see, right now, we only save the step as soon as the user hits the "next" button so I guess we should upgrade that to change as soon as we visit a given step. Could you create an issue for that?

https://regennetwork.atlassian.net/browse/APP-410?atlOrigin=eyJpIjoiZTBkY2IwZjE2NzZlNDIxYmI0Y2M3NzM0MjRmNjkzN2MiLCJwIjoiaiJ9

@erikalogie
Copy link
Collaborator

@r41ph is this working and can be tested again? I've tried testing it so many times and have been unable to get it working. If you could provide the exact steps then maybe I can test it too.

Comment on lines 442 to 446
paymentOption === PAYMENT_OPTIONS.CARD
? cardSellOrders.sort((a, b) => a.usdPrice - b.usdPrice)
: filteredCryptoSellOrders?.sort(
(a, b) => Number(a.askAmount) - Number(b.askAmount),
) || [],
Copy link
Member

Choose a reason for hiding this comment

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

maybe we could extract this into a reusable function, the same logic is also used in ChooseCreditsForm

Comment on lines 235 to 286
setUserCanPurchaseCredits({
openModal: true,
amountAvailable: 0,
});
Copy link
Member

Choose a reason for hiding this comment

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

reading the dev note on https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=365-62773&node-type=instance&m=dev, I believe we should also check for other sell orders for the project, right now it's only checking the amount still available for the already selected sell orders

@blushi
Copy link
Member

blushi commented Oct 22, 2024

@erikalogie please have a look at my question in https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ?node-id=365-62773&m=dev#995229028, not all cases are fully covered

@r41ph r41ph force-pushed the feat-APP-363-another-user-purchased-credits-modal branch from d11948f to 638b2fe Compare October 23, 2024 20:26
@r41ph r41ph requested a review from blushi October 23, 2024 20:35
@r41ph r41ph force-pushed the feat-APP-363-another-user-purchased-credits-modal branch from 1edd6a2 to 26a76b9 Compare October 24, 2024 10:05
@r41ph
Copy link
Contributor Author

r41ph commented Oct 24, 2024

@erikalogie have a look at this please

@erikalogie
Copy link
Collaborator

Ok this worked successfully for me, but I did get this error, should I file a separate bug?

Screen Shot 2024-10-24 at 10 31 54 AM

Also, has this one been implemented, or does it need a separate task? https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=5596-91315&t=fdRLHdpN53VWcyQu-1

@blushi
Copy link
Member

blushi commented Oct 27, 2024

Ok this worked successfully for me, but I did get this error, should I file a separate bug?

Screen Shot 2024-10-24 at 10 31 54 AM

How did this happen? The payment intent / session had already been used. Had you already purchased some credits from this same tab?

Also, has this one been implemented, or does it need a separate task? https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=5596-91315&t=fdRLHdpN53VWcyQu-1

We had discussed implementing it here with @r41ph

handleClick: (action: string | null) => void;
}

export const BuyFiatModal = ({
Copy link
Member

Choose a reason for hiding this comment

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

it's a bit weird to call it BuyFiatModal when it's not only about fiat credits
maybe something like BuyWarningModal?

web-marketplace/src/pages/BuyCredits/BuyCredits.Form.tsx Outdated Show resolved Hide resolved
@r41ph r41ph force-pushed the feat-APP-363-another-user-purchased-credits-modal branch from 06c7cba to e4c032b Compare November 6, 2024 11:34
@blushi
Copy link
Member

blushi commented Nov 7, 2024

If i'm logged in with a web3 account and try to purchase fiat credits that have already all been purchased, then I get this popup (which is alright)

image

but then clicking on "choose new credits", I see the following. It should instead set payment option to crypto:

image

If I do the same with a visiting user, then I see the same popup for a second before being redirected to the project page, while I should see this: https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=5596-91315&node-type=instance&m=dev

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