gitcoinco / web

Grow Open Source
https://gitcoin.co
Other
1.79k stars 775 forks source link

round 8 bug / improve list #7947

Closed thelostone-mc closed 3 years ago

thelostone-mc commented 3 years ago

MAJOR

MINOR

owocki commented 3 years ago

just tested, feels pretty rough. here are my QA notes (all in latest chrome + metamask):

showstoppers

major

attention to detail stuff

could not test

octavioamu commented 3 years ago

on grant creation, why are we forcing people to add an @ symbol? seems like needless user work, we can just add it on the backend if needed. don't introduce unnecessary friction pls

I added better validation for this but extract the username of any format seems overkill to me. I was looking on some of the possible patterns https://regex101.com/r/s6JoZ5/3

owocki commented 3 years ago

testing latest build

thelostone-mc commented 3 years ago

@chibie could you take a look at the twitter handle ! let's make it such that it auto adds an @ if it's not present when frontend sends it to the backend ?

@owocki looks like there edit grants had to be fixed up a bit cause it was setting ETH payout address to undefined on edit. 7547b5f fixes it So when you do pull it in and test -> do ensure the ETH address for the grant is set in admin

zoek1 commented 3 years ago

@owocki I managed the twitter handle in the PR #7962

zoek1 commented 3 years ago

Also I'm still getting the error: zcash_payout_address must be a transparent address when i try to edit any grant, the fix isn't merged yet?

thelostone-mc commented 3 years ago

@zoek1 could you make sure your grant has

and you have the latest changes

owocki commented 3 years ago

I added better validation for this but extract the username of any format seems overkill to me.

i just dont want the grant creation form to fail validation bc of the lack of an @ symbol. seems this could be accomplished by just doing an alphanumeric regex. if we want to ensure consistence on the backend, the @ symbol can be added on hte backend.

more testing notes:

Untitled

octavioamu commented 3 years ago

after i add a grant to cart on grant detila page, the side cart no longer shows. where do i go to checkout?

I just pushed a commit adding the sidecart back

apbendi commented 3 years ago

Twitter verification on the Trust Bonus tab is broken in the grants branch, as far as I can tell. Seems like the regression came from this PR, which I commented on with a bit more context: https://github.com/gitcoinco/web/pull/7962

@zoek1, can you expand on why you were modifying the twitter verification endpoint? That endpoint is used for the Trust Bonus. Was it your intention to change the behavior of the Trust Bonus?

apbendi commented 3 years ago

Attempting to test SMS verification from the Trust Bonus tab gives the following error from the backend:

, uri, response, 'Unable to fetch record')
web_1            | twilio.base.exceptions.TwilioRestException:
web_1            | HTTP Error Your request was:
web_1            |
web_1            | GET /PhoneNumbers/REDACTED
web_1            |
web_1            | Twilio returned the following information:
web_1            |
web_1            | Unable to fetch record: Authenticate
web_1            |
web_1            | More information may be available here:
web_1            |
web_1            | https://www.twilio.com/docs/errors/20003

Has our API key or auth token changed? (Reminder not to post these here)

apbendi commented 3 years ago

Need a bit more direction on how to regression test Google Verification: https://github.com/gitcoinco/web/pull/7631#issuecomment-736207735

Should still be working but would like to test locally

zoek1 commented 3 years ago

@apbendi my intention wasn't to modify the trust bonus, just the twitter grant verification. The error is due to wrong conditional statement. The fix for that is here https://github.com/gitcoinco/web/pull/7974

danlipert commented 3 years ago

edit: fixed via https://github.com/gitcoinco/web/pull/7977

danlipert commented 3 years ago

edit: fixed via https://github.com/gitcoinco/web/pull/7976 allows starting @ symbol or not in frontend while it is stripped in backend per @owocki 's comment above

danlipert commented 3 years ago

While checking out with multiple ETH grants in my cart via zksync, I received an error alert in the UI while I was redirected to the zksync checkout. Zksync checkout worked perfectly as expected, and when finished I was returned to the grants page and received the success modal, but during this time if I return to the cart page the alert is shown which reads "Insufficient balance to complete checkout". I don't know if this refers to the zksync balance being insufficient as I a) did have enough funds in my wallet to complete checkout and b) did not have any L2 funds at this time, but either way if that is "expected behavior" its definitely confusing/scary for users and the copy and alert style need to be changed.

Screen Shot 2020-12-01 at 7 56 54 PM

thelostone-mc commented 3 years ago

^ @mds1

mds1 commented 3 years ago

Hey @danlipert. The insufficient balance modal should only show if you have insufficient funds in both L1 and L2. Can you clarify the steps to reproduce? I'm not sure I follow.

but during this time if I return to the cart page the alert is shown which reads "Insufficient balance to complete checkout"

This is the main part I'm confused on. The success modal redirects you off the cart page, so did you then go back the cart, and the cart was not cleared? Or did you add new grants to your cart after? Or something else?

Also, what was in your cart at the time of that second checkout? It looks like you may have had an empty "amount" field based on the console error messages showing vaule=""

danlipert commented 3 years ago

@mds1 I had sufficient funds in L1 but not L2. Not sure exactly how to reproduce. What happened was the zksync flow opened in a new tab, and i went back to the cart tab just to check and the error was there while I was going through the zksync flow. Once I finished the zksync flow I was redirected back to the success modal screen. I took the screenshot while I was going through the zksync flow. I'm pretty sure all the amount fields had something in them as I got the appropriate number of txns in the zksync checkout explorer link list, but they were all very small amounts.

mds1 commented 3 years ago

@danlipert Ahhh I see, got it, thanks for the additional info! I'll look into this today

willsputra commented 3 years ago

Browser: Brave Mac 1.17.73

Major

Minor

danlipert commented 3 years ago

Thanks @willsputra ! I also just got the SAI / DAI issue but only for one of the grants (gitcoin dev fund optional contribution) so probably has something to do with when the grant was created back when you could specify a specific type of token you accepted. I also received 4 emails for the 4 rolled up zksync contributions :/ But was able to successfully check out. @willsputra the error you got I believe has to do with the calculation of the zksync fee being larger than what you would have saved by using it or something like that.

aditya: reverting the 1 mail -> multi contribution PR would fix this

octavioamu commented 3 years ago
owocki commented 3 years ago
owocki commented 3 years ago

i just got 8 emails from making 8 contributions in 1 cart. is the bulk checkout email going to make it in?

aditya: Doubtful ! Given that subscription creation happens in celery. I'm not sure how we'd be able to wait for all celery tasks to be processed and only then send an email ! scope lift is working on it -> but I'm not sure how we'd actually do this

owocki commented 3 years ago

the banner should be < 300 px high on mobile, 400px on tablet, etc...

mds1 commented 3 years ago

zkSync Checkout: Tried contributing 0.5 DAI to two grants with optional tip, got a "Transactions batch summary fee is too low" error tx | error @mds1

@willsputra I noticed this also and messaged the zkSync team about it a few hours ago. We haven't been able to figure out how to reliably introduce it, so their checking the server for bugs on their end. They're also updating the UI with two changes:

  1. Show a minimum fee and recommended fee. Minimum is what may sometimes cause this bug. Recommended is ~1 cent higher and never will
  2. If this error happens, prompt the user with a button to continue checkout (this will pull the updated correct fee from the server)

zkSync Checkout: Are "Grants Round 8+ Dev Fund" and "Grants Round 7+ Development Fund" different? Current optional tip goes to "Grants Round 7+ Development Fund" screenshot

Good catch. Let me get a PR in to fix this

owocki commented 3 years ago
octavioamu commented 3 years ago
octavioamu commented 3 years ago

how the banner should work for mobile? I can reduce but that banner doesn't seems to work for mobile. image

owocki commented 3 years ago
kevinowocki@local /Users/kevinowocki~ % time curl https://gitcoin.co/grants/86/gitcoin-grants-round-8-dev-fund > /dev/null
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 6605k  100 6605k    0     0   789k      0  0:00:08  0:00:08 --:--:-- 1657k
curl https://gitcoin.co/grants/86/gitcoin-grants-round-8-dev-fund > /dev/null  0.04s user 0.04s system 0% cpu 8.393 total

here is the trace for the same https://bits.owocki.com/Z4uqzNAJ

octavioamu commented 3 years ago
owocki commented 3 years ago

@octavioamu i trust your judgement about how to resize the banner on mobile. i agree with your comment about the need to make it larger than it is in your screenshot

octavioamu commented 3 years ago

with the current banner design I don't see a way to fit it on a small screen more than showing the center or just making it small. Banner need a redesign to fit not more than 400 px height, in that way we can reduce it without losing information.

owocki commented 3 years ago
octavioamu commented 3 years ago

@owocki is using positive_round_contributor_count as the old one 🤔

owocki commented 3 years ago

then why does it show 233 Contributiors for a round that started today

octavioamu commented 3 years ago

seems the api is returning that 233 on positive_round_contributor_count https://gitcoin.co/grants/v1/api/grant/149/ there are 2 dapp clrs probably that is the reason, one yes started today, the other on september. https://gitcoin.co/_administrationgrants/grantclr/

vs77bb commented 3 years ago

image aditya: added comma here + limited this to landing page. Defer to @PixelantDesign cause we did bring this up during dev

owocki commented 3 years ago

cb4b75f13a0e2a1169fb26fde3f2cd2336858448 fixes the positive contributor count issue

corydickson commented 3 years ago

i just got 8 emails from making 8 contributions in 1 cart. is the bulk checkout email going to make it in?

aditya: Doubtful ! Given that subscription creation happens in celery. I'm not sure how we'd be able to wait for all celery tasks to be processed and only then send an email ! scope lift is working on it -> but I'm not sure how we'd actually do this

I think I've resolved the issue with celery using chords see: https://github.com/corydickson/web/tree/cd-bulk-email-cart and https://docs.celeryproject.org/en/stable/reference/celery.html#celery.chord

Before we were passing around the subscription that was created in the task, and now there is a bug retrieving the correct subscription when that task is completed, particularly when a user contributes to the same grant twice. When filtering for the correct object I'm using the grant and the contributor profile which is not enough to get the most recently created object. Hence the bug of improper balances being shown in the email.

I most likely have a poor understanding of the data model, any ideas for a unique identifier?

thelostone-mc commented 3 years ago

@corydickson well the unique identifier would be the pk of the subscription ! I'm not sure what else would qualify

corydickson commented 3 years ago

@thelostone-mc yeah retrieving that from each task is difficult as if a single task fails then the email task will receive an empty array

thelostone-mc commented 3 years ago

What you could do is make it into one celery task ! If you contribute to 5 grants -> fire one celery task -> loop through it there and let the code inside task run sync Once all subscriptions are created -> you can fire the bulk email. At that point -> you have access to subscriptions so you can pass those as well and get the email to fire aka no extra query

@corydickson

apbendi commented 3 years ago
corydickson commented 3 years ago

@thelostone-mc I. was hesitant to go in that direction in case one of the grants in the cart fails processing as it would block all other contributions from completing. There is also possibly a performance hit since we are no longer parallelizing tasks. If I understand your solution correctly, and you are comfortable with that trade off, I can start working on it

thelostone-mc commented 3 years ago

@corydickson well i think it's alright doing that cause

cc @danlipert

danlipert commented 3 years ago

the performance hit is negligible - its async which is all that matters. Just make the task itself robust against the expected failure here with a try block. @corydickson @thelostone-mc

vs77bb commented 3 years ago

🔵 scrolling bar on project descriptions are cutting off words, a bit more space on the right side would clean it up

image

mds1 commented 3 years ago