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

Feature: View Claim Button #187

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

barebind
Copy link
Contributor

@barebind barebind commented Mar 5, 2024

Resolves #182

@barebind barebind changed the title feat: add view claim button Feature: View Claim Button Mar 5, 2024
@ubiquity-os-deployer
Copy link

@barebind barebind marked this pull request as ready for review March 5, 2024 08:30
@barebind barebind requested a review from 0x4007 as a code owner March 5, 2024 08:30
@0x4007
Copy link
Member

0x4007 commented Mar 5, 2024

Currently testing with this...

This does not work.

image

@@ -134,6 +134,9 @@ table[data-claim-rendered] #controls {
table[data-claim-rendered] button {
opacity: 0.5;
}
table[data-claim-rendered] button.hide {
display:none !important;
Copy link
Member

Choose a reason for hiding this comment

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

!important seems like a bad idea. You should use more specific selectors instead, or put this at the bottom of the css file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't want to make it more complicated for a simple hide function for this kind of feature,
otherwise we need to double every selector for buttons.
if that's okay I'm going to move the !important to the bottom of the css file.

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Left review.

@barebind
Copy link
Contributor Author

barebind commented Mar 5, 2024

This does not work.

we are definitely not on the same page on this one. :)

I'm guessing it's the same issue with the one in #173

if you are using auto generated permits via yarn start, then you need to insert them to database manually after they are generated, so that they can be updated later.
this feature assumes that the permits are generated and inserted to the database by ubiquibot.

that's the same conversation where we left on #173

@rndquu @pavlovcik

@0x4007
Copy link
Member

0x4007 commented Mar 5, 2024

this feature assumes that the permits are generated and inserted to the database by ubiquibot.

To clarify, this is not currently implemented?

Doesn't it make more sense to write the data when we're claiming from this UI? It seems that this dependency on an external system makes it less maintainable than it needs to be.

Anyways given that I have less time for code reviews nowadays I'll need to ultimately delegate my review to @rndquu

It would be helpful if you can provide me the functional test URL so I can see the UI.

@0x4007 0x4007 requested a review from rndquu March 5, 2024 08:52
@barebind
Copy link
Contributor Author

barebind commented Mar 5, 2024

To clarify, this is not currently implemented?

Doesn't it make more sense to write the data when we're claiming from this UI? It seems that this dependency on an external system makes it less maintainable than it needs to be.

yes inserting the permit to the db is not implemented. I really think that the permits should be inserted to the db where and when they are generated at first place which is ubiquibot.
so that features like #172 would makes more sense and tracking permits would be more consistent. and actually I thought that it's already implemented on the ubiquibot. seems like it's not the case.

assume that a permit is generated but the beneficiary doesn't even click the link and open the UI. in that case, the permit wouldn't be listed on audit pages as unclaimed.

It would be helpful if you can provide me the functional test URL so I can see the UI.

let me check about the functional test URL. I have no idea the automated deployments works right now. it might take some time.

@0x4007
Copy link
Member

0x4007 commented Mar 5, 2024

To clarify, this is not currently implemented?
Doesn't it make more sense to write the data when we're claiming from this UI? It seems that this dependency on an external system makes it less maintainable than it needs to be.

yes inserting the permit to the db is not implemented. I really think that the permits should be inserted to the db where and when they are generated at first place which is ubiquibot. so that features like #172 would makes more sense and tracking permits would be more consistent. and actually I thought that it's already implemented on the ubiquibot. seems like it's not the case.

I'm not sure how we can confirm that your implementation works if you haven't tested it.

It would be helpful if you can provide me the functional test URL so I can see the UI.

let me check about the functional test URL. I have no idea the automated deployments works right now. it might take some time.

You just need to provide the query parameter that represents the permit. We can use the latest deploy on this pull and then append the query parameter to test.

@barebind
Copy link
Contributor Author

barebind commented Mar 5, 2024

I'm not sure how we can confirm that your implementation works if you haven't tested it.

I've tested it locally as the ubiquibot is an external dependency in this case. seems like we were assuming different requirements for this task and the one before.

You just need to provide the query parameter that represents the permit. We can use the latest deploy on this pull and then append the query parameter to test.

if it is a test on automated deployments that you want, nothing really works I guess as there is no code inserting the permit to the db. As I said before I was assuming ubiquity bot is already inserting the permits to the database. If that's not the case we first need to decide where we should be putting the db insert logic, to the ubiquibot or to the UI.

@0x4007
Copy link
Member

0x4007 commented Mar 5, 2024

I've tested it locally

Can you explain how I can test this?

decide where we should be putting the db insert logic

assume that a permit is generated but the beneficiary doesn't even click the link and open the UI. in that case, the permit wouldn't be listed on audit pages as unclaimed.

Makes sense for the bot to write to the permits database upon permit generation!

@barebind
Copy link
Contributor Author

barebind commented Mar 5, 2024

Can you explain how I can test this?

on my local

1- edit generate-erc20-permit-url.ts as it outputs txData to the console when it's run via yarn start
2- start ui via yarn start
3- insert relevant txData to the local supabase database (nonce, deadline, signature etc.)
4- navigate to the UI via auto-generated 'ERC20 Testing URL' printed to the console when yarn start
5- test the UI

@0x4007
Copy link
Member

0x4007 commented Mar 6, 2024

I gave you edit access to our Supabase why not just insert the test permit and then provide the full url here? I can only do all those steps from computer but most of the time I'm reviewing from mobile.

@barebind
Copy link
Contributor Author

barebind commented Mar 6, 2024

I gave you edit access to our Supabase why not just insert the test permit and then provide the full url here?

you can test with this link

Currently testing with this...

This does not work.

also updated this one with placeholder tx on the db just in case.

@0x4007
Copy link
Member

0x4007 commented Mar 6, 2024

I gave you edit access to our Supabase why not just insert the test permit and then provide the full url here?

you can test with this link

Currently testing with this...
This does not work.

also updated this one with placeholder tx on the db just in case.

Am I supposed to claim it? It has two rewards embedded, bot are claimable according to the UI.

@barebind
Copy link
Contributor Author

barebind commented Mar 6, 2024

Am I supposed to claim it? It has two rewards embedded, bot are claimable according to the UI.

in order to test whole process I supposed you would want to claim it. I extracted your address from previous link. so if you want to claim you can. if you only want to test ui i also updated the previous link that you said didn't work. that's the supposed version of ui when you claim the permit.

rewards are generated via script with the amount 0. by default it creates two permits so i didn't edit it.

@0x4007
Copy link
Member

0x4007 commented Mar 6, 2024

Love to see it.

image

rewards are generated via script with the amount 0. by default it creates two permits so i didn't edit it.

This didn't happen for me so I don't understand why that would be the case for you.

@0x4007 0x4007 merged commit c3174d6 into ubiquity:development Mar 6, 2024
3 checks passed
@ubiquibot ubiquibot bot mentioned this pull request Mar 6, 2024
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.

View Claim Button
2 participants