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-204 buy credits #2477

Merged
merged 53 commits into from
Oct 15, 2024
Merged

feat: APP-204 buy credits #2477

merged 53 commits into from
Oct 15, 2024

Conversation

blushi
Copy link
Member

@blushi blushi commented Sep 18, 2024

Description

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

This large PR has been initially code reviewed as part of separate smaller PRs:
#2480
#2482
#2484

Not implemented yet:

Since this PR was already too big, I've decided to have follow-ups for the following items (mapped to tasks on Jira):

  • Order summary card
  • Buy flow from project card (only available from project page in this work)
  • Success page/modal (for fiat we need our fiat payment microservice to be implemented first, but for crypto you should see our existing tx successful modal, which will need to be reworked)
  • Buying from prefinance project
  • Redirect to buy flow after logging in with google from the buy flow
  • Show error message on Choose Credits form (crypto) if not enough coins to pay for the credits

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

Please have a look at the list above of what's not implemented yet before testing.

  1. Project with fiat credits available: https://deploy-preview-2477--regen-marketplace.netlify.app/project/C01-001
  2. Project without fiat credits available: https://deploy-preview-2477--regen-marketplace.netlify.app/project/KSH01-001

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 Sep 18, 2024

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit 35ed494
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/670e7dd85bc6ae00086fdd9e
😎 Deploy Preview https://deploy-preview-2477--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.

@blushi blushi requested a review from r41ph September 18, 2024 13:16
variant="determinate"
value={props.percentComplete}
/>
{!hideProgress && typeof percentComplete !== 'undefined' && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't this be percentComplete !== undefined instead of using typeof?

Copy link
Member Author

@blushi blushi Sep 19, 2024

Choose a reason for hiding this comment

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

I usually prefer using typeof because this is safer if the variable hasn't already been declared, it won't throw an error, but I agree in this case, we could also use percentComplete !== undefined I don't have a strong preference


type Props = {
onPrev?: () => void;
onSave?: () => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the "Save" button is always rendered but onSave is optional. Since the button is visible, maybe onSave should be required to avoid cases where the button has no functionality? Without onSave, the button could still be enabled and not function properly when clicked. An option could be to disable it if onSave is not present disabled={saveDisabled || !onSave}

Copy link
Member Author

Choose a reason for hiding this comment

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

if you look at the code for the SaveFooter you'll see the button has type="submit" which means that if it's within a Form it will just use onSubmit from this form on click so no need to provide onSave in this case

web-marketplace/src/pages/BuyCredits/BuyCredits.Form.tsx Outdated Show resolved Hide resolved
web-marketplace/src/pages/BuyCredits/BuyCredits.tsx Outdated Show resolved Hide resolved
@blushi blushi changed the title feat: APP-204 buy credits feat: APP-204 buy credits (step 1 refactor) Sep 23, 2024
@blushi blushi changed the title feat: APP-204 buy credits (step 1 refactor) feat: APP-204 buy credits (step 1 refactor+integration) Sep 25, 2024
@blushi blushi changed the title feat: APP-204 buy credits (step 1 refactor+integration) feat: APP-204 buy credits Sep 26, 2024
@blushi blushi requested a review from r41ph September 26, 2024 11:30
@blushi
Copy link
Member Author

blushi commented Sep 26, 2024

@erikalogie @clevinson see testing instructions

@blushi blushi marked this pull request as ready for review September 26, 2024 11:31
@erikalogie
Copy link
Collaborator

This PR is looking absolutely awesome @blushi. Amazing work!

One thing I'm wondering is, when I'm buying with crypto, if I have "retire credits now" selected and have an amount in the fields above, if I then select "buy tradable ecocredits" then the amount resets. Does it need to be this way? If not, could we change this in a follow-up issue?

@blushi
Copy link
Member Author

blushi commented Sep 30, 2024

This PR is looking absolutely awesome @blushi. Amazing work!

One thing I'm wondering is, when I'm buying with crypto, if I have "retire credits now" selected and have an amount in the fields above, if I then select "buy tradable ecocredits" then the amount resets. Does it need to be this way? If not, could we change this in a follow-up issue?

You're right, it doesn't always need to be reseted.
As an improvement we could:

  1. keep the amount if all credits are tradable
  2. set amount to min(current amount, amount credits tradable) if not all credits are tradable, like here: https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=174-102754&node-type=frame&m=dev?

projectId: string;
};
export const getFormModel = ({
_,
Copy link
Contributor

Choose a reason for hiding this comment

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

Another way that I think would be more efficient would be that instead of passing around _ we can use i18n from import { i18n } from '@lingui/core';

That way we could directly do: i18n._(msg`Choose credits`)

Copy link
Member Author

Choose a reason for hiding this comment

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

Reading https://lingui.dev/ref/react#uselingui, I believe this might cause some issues when the user switches languages

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right that there could be issues when switching languages. However, what I understand is that it can cause issues when i18n._() is used in memoized functions or components. As long as we avoid using it in those contexts it seems that it should work fine.

@erikalogie
Copy link
Collaborator

This PR is looking absolutely awesome @blushi. Amazing work!
One thing I'm wondering is, when I'm buying with crypto, if I have "retire credits now" selected and have an amount in the fields above, if I then select "buy tradable ecocredits" then the amount resets. Does it need to be this way? If not, could we change this in a follow-up issue?

You're right, it doesn't always need to be reseted. As an improvement we could:

  1. keep the amount if all credits are tradable
  2. set amount to min(current amount, amount credits tradable) if not all credits are tradable, like here: https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=174-102754&node-type=frame&m=dev?

Yes, that would be great. Do you want to open a follow-up task for this? Otherwise everything LGTM

@blushi
Copy link
Member Author

blushi commented Oct 1, 2024

This PR is looking absolutely awesome @blushi. Amazing work!
One thing I'm wondering is, when I'm buying with crypto, if I have "retire credits now" selected and have an amount in the fields above, if I then select "buy tradable ecocredits" then the amount resets. Does it need to be this way? If not, could we change this in a follow-up issue?

You're right, it doesn't always need to be reseted. As an improvement we could:

  1. keep the amount if all credits are tradable
  2. set amount to min(current amount, amount credits tradable) if not all credits are tradable, like here: https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=174-102754&node-type=frame&m=dev?

Yes, that would be great. Do you want to open a follow-up task for this? Otherwise everything LGTM

Just added it here

@blushi
Copy link
Member Author

blushi commented Oct 2, 2024

This PR is looking absolutely awesome @blushi. Amazing work!
One thing I'm wondering is, when I'm buying with crypto, if I have "retire credits now" selected and have an amount in the fields above, if I then select "buy tradable ecocredits" then the amount resets. Does it need to be this way? If not, could we change this in a follow-up issue?

You're right, it doesn't always need to be reseted. As an improvement we could:

  1. keep the amount if all credits are tradable
  2. set amount to min(current amount, amount credits tradable) if not all credits are tradable, like here: https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=174-102754&node-type=frame&m=dev?

Yes, that would be great. Do you want to open a follow-up task for this? Otherwise everything LGTM

Just added it here

@erikalogie

@erikalogie
Copy link
Collaborator

@blushi
I'm getting this nonsense error when clicking the "tradable credits" tab after entering a number greater than the amount of tradable credits that are available. I assume this is meant to be this error? https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=174-102754&t=KznaoTIxXgotIAZ2-1
Screen Shot 2024-10-02 at 9 33 30 AM

I'm also realizing that the same issue that I identified with switching between retired and tradable also applies to switching between fiat and crypto or switching between the different crypto currencies. If the user enters a certain number of credits, and then change switches between fiat and crypto, then number of credits should stick rather than reset to 0, and same with the different crypto currencies. If there isn't enough credits in that particular denom, then we should show this error: https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=174-102754&t=KznaoTIxXgotIAZ2-1.

Also, can we move the error messages to be left-aligned with the field?
Monosnap Regen Marketplace 2024-10-02 09-18-24

We also want to make sure that this doesn't happen by having the italicized text move down to make room for the error:
Monosnap Regen Marketplace 2024-10-02 09-31-27

Can we make the default number of credits when the flow first loads be 1 rather than 0?

I'm noticing that if I go through the flow and enter some number of credits and then exit out, and re-enter into it I come to the email part of the form. If I click the back button, then the number of credits is again 0, which shouldn't happen. It should store whatever the number of credits was that the user previously entered if we are taking them to where they left off in the form, and a number of 0 should not be possible.

Screen.Recording.2024-10-02.at.9.40.27.AM.mov

@blushi
Copy link
Member Author

blushi commented Oct 2, 2024

Can we make the default number of credits when the flow first loads be 1 rather than 0?

it could be that there are less than 1 credit available, ie 0.5

@erikalogie
Copy link
Collaborator

Can we make the default number of credits when the flow first loads be 1 rather than 0?

it could be that there are less than 1 credit available, ie 0.5

Could we make it 1 or the highest number available under 1?

@clevinson
Copy link
Member

Nice work!

Few bugs I want to highlight here as I'm going through QA-

Input forms

On the dollar / credit input forms we allow for several 0s to be typed such that a number is 0 padded. I think this shouldn't be allowed, and the moment a user enters a non-zero number, all 0 digits should be removed from the input field. This likely will get addressed by @erikalogie's comment above to have a non-zero default, but I wanted to highlight this issue explicitly incase it doesn't get resolved already by changing to non-zero defaults.

Screenshot 2024-10-02 at 4 39 33 PM

Fiat Pathway

No bugs!

Crypto Pathway

When I:

  • select a quantity of credits to purchase (with "retired credits now" selected)
  • change the purchase option to "Buy tradable ecocredits"

Then:

  • the quantity does not update
  • the purchase fails with an error on "auto retire" (see screenshot)
Screenshot 2024-10-02 at 4 04 12 PM

Separate from the above issue, when successfully purchasing tradable credits, the success modal at the end says "View retirement certificate", instead of something like "View ecocredits". I know that we haven't implemented the entire buy flow yet, and there are still follow-ups remaining, but just wanted to make sure this particular issue was being tracked.

@blushi
Copy link
Member Author

blushi commented Oct 3, 2024

Separate from the above issue, when successfully purchasing tradable credits, the success modal at the end says "View retirement certificate", instead of something like "View ecocredits". I know that we haven't implemented the entire buy flow yet, and there are still follow-ups remaining, but just wanted to make sure this particular issue was being tracked.

There's indeed a follow up issue for the confirmation step: https://regennetwork.atlassian.net/browse/APP-361
But when looking at the figma designs, I don't see how the confirmation screen is supposed to look like when buying tradable credits @erikalogie looks like there's always the retirement certificate on the background of the confirmation modal.

orderedSellOrders: UISellOrderInfo[];
creditTypePrecision?: number | null;
};
export const getCreditsAmount = ({
Copy link
Member Author

Choose a reason for hiding this comment

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

@r41ph just wanted to mention that those functions getCreditsAmount and getCurrencyAmount are one of the most important stuff to review in this PR, since it infers the sell orders (by the cheapest) based on the credits or currency amount provided

Copy link
Contributor

Choose a reason for hiding this comment

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

@blushi this looks great! I've tested it and it seems to work as expected. Just a few minor suggestions.

@blushi
Copy link
Member Author

blushi commented Oct 3, 2024

@erikalogie @clevinson could you have another look?

@erikalogie
Copy link
Collaborator

LGTM

@erikalogie
Copy link
Collaborator

Separate from the above issue, when successfully purchasing tradable credits, the success modal at the end says "View retirement certificate", instead of something like "View ecocredits". I know that we haven't implemented the entire buy flow yet, and there are still follow-ups remaining, but just wanted to make sure this particular issue was being tracked.

There's indeed a follow up issue for the confirmation step: https://regennetwork.atlassian.net/browse/APP-361 But when looking at the figma designs, I don't see how the confirmation screen is supposed to look like when buying tradable credits @erikalogie looks like there's always the retirement certificate on the background of the confirmation modal.

You are 100% correct. I just made a version here, wdyt? https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=5075-76841&t=lKYcqtNVA7T8TxHW-1

@blushi
Copy link
Member Author

blushi commented Oct 3, 2024

Separate from the above issue, when successfully purchasing tradable credits, the success modal at the end says "View retirement certificate", instead of something like "View ecocredits". I know that we haven't implemented the entire buy flow yet, and there are still follow-ups remaining, but just wanted to make sure this particular issue was being tracked.

There's indeed a follow up issue for the confirmation step: https://regennetwork.atlassian.net/browse/APP-361 But when looking at the figma designs, I don't see how the confirmation screen is supposed to look like when buying tradable credits @erikalogie looks like there's always the retirement certificate on the background of the confirmation modal.

You are 100% correct. I just made a version here, wdyt? https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=5075-76841&t=lKYcqtNVA7T8TxHW-1

Looks good, just left a comment.

@blushi blushi requested a review from r41ph October 7, 2024 07:28
@blushi blushi enabled auto-merge (squash) October 15, 2024 14:36
@blushi blushi merged commit 9a5fb65 into dev Oct 15, 2024
12 of 14 checks passed
@blushi blushi deleted the feat-APP-204-buy-credits branch October 15, 2024 14:38
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