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

Allow admin edit grants #94

Merged
merged 10 commits into from
Mar 25, 2024
Merged

Allow admin edit grants #94

merged 10 commits into from
Mar 25, 2024

Conversation

technophile-04
Copy link
Member

Description

Screen.Recording.2024-03-24.at.2.10.14.AM.mov

Umm, not sure what's the best option to put edit Icon, please feel free to suggest other options 🙌

@technophile-04 technophile-04 requested a review from carletex March 23, 2024 20:47
Copy link

vercel bot commented Mar 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
grants-bg ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 25, 2024 2:32pm

@technophile-04 technophile-04 requested a review from Pabl0cks March 23, 2024 20:47
Copy link
Member

@Pabl0cks Pabl0cks left a comment

Choose a reason for hiding this comment

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

GJ @technophile-04 building this 🙌!

Been testing it, leaving a few comments:

  • I think we should limit the edition to "proposed" status, because editing in "submitted" status can get very messy if we change askAmount after first 50% payment.

  • When I edit the askAmount (either using "." or "," as decimal separator, in firebase it changes internally from number to string.
    It does not affect the grants workflow, but after completion we get error on homepage because it can't calculate totalEthGranted. I get this error:

    Error: sum.toFixed is not a function
    const totalEthGranted = Number.isInteger(sum) ? sum : sum.toFixed(2);
    
  • Not sure if this makes sense, but should we add some kind of max limit to askAmount in FE or BE to prevent strange behaviours or typos ending in wrong ETH amounts being paid to grantees?
    I guess is very very remote case, but maybe is good to have this safety check now that it can be an input and not dropdown values.

Umm, not sure what's the best option to put edit Icon, please feel free to suggest other options 🙌

Since is only for admins I think it may be fine like that, the pencil icon at title was my first thought as well. Other alternative would be an Edit button close to "Reject", but probably there are too many buttons there already.

@carletex
Copy link
Contributor

Thanks Shiv!

I think we should limit the edition to "proposed" status, because editing in "submitted" status can get very messy if we change askAmount after first 50% payment.

Agree! Another option is to allow edits on on submitted too, but disabling the askAmount (like you can edit typos, change desc, etc... but don't change the amount)

@technophile-04
Copy link
Member Author

When I edit the askAmount (either using "." or "," as decimal separator, in firebase it changes internally from number to string. It does not affect the grants workflow, but after completion we get error on homepage because it can't calculate totalEthGranted. I get this error:

Ohhh shit sorry my bad, I thought <input type="number" /> would return value as number but it instead return string value, fixed at b77e0a9, tysm for the catch 🙌

I think we should limit the edition to "proposed" status, because editing in "submitted" status can get very messy if we change askAmount after first 50% payment.

Agree! Another option is to allow edits on on submitted too, but disabling the askAmount (like you can edit typos, change desc, etc... but don't change the amount)

Yup disabled the askAmount for now when the grant status is submitted at frontend on 093df8f

Not sure if this makes sense, but should we add some kind of max limit to askAmount in FE or BE to prevent strange behaviours or typos ending in wrong ETH amounts being paid to grantees?

Yup makes sense ! I initially thought 25 ETH would be good max limit but I think we are also planning to allow ERC 20 like OP tokens in the future so not sure what the best max limit would be :(

Thanks all for the reviews 🙌

@technophile-04 technophile-04 requested a review from Pabl0cks March 25, 2024 11:21
@carletex
Copy link
Contributor

Great stuff @technophile-04 !! And thanks @Pabl0cks for the review!

Pushed a tiny naming thing!

Merging!!

@carletex carletex merged commit 2b28aaf into main Mar 25, 2024
2 of 3 checks passed
@carletex carletex deleted the admin-edit-grants branch March 25, 2024 14:32
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