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

Donation page layout #58

Merged
merged 29 commits into from
Jul 17, 2023
Merged

Conversation

vuphuctho
Copy link
Collaborator

@vuphuctho vuphuctho commented Jun 3, 2023

  • Create page based on Figma design
  • Integrate with newly created Prismic content

TODO:

TODO: design the two page donate success and donate failure

@vuphuctho vuphuctho requested a review from shuesken June 3, 2023 16:11
@vuphuctho
Copy link
Collaborator Author

@shuesken
Copy link
Contributor

shuesken commented Jun 5, 2023

Hey this looks good, two minor design bits:

  • When hovering over the donation buttons, we should have cursor: pointer.
  • The links should read "Donate" rather than "Donation"

My understanding is we have to wait for the Stripe integration to fully review & merge this?

@vuphuctho
Copy link
Collaborator Author

vuphuctho commented Jun 5, 2023

My understanding is we have to wait for the Stripe integration to fully review & merge this?

For now it's the UI alone. We would need API key to integrate Stripe - I have put details in the ticket.

@vuphuctho
Copy link
Collaborator Author

vuphuctho commented Jun 5, 2023

  • When hovering over the donation buttons, we should have cursor: pointer.
  • The links should read "Donate" rather than "Donation"

I have fixed these issues with latest commit @shuesken

@vuphuctho vuphuctho marked this pull request as draft June 8, 2023 16:07
@vuphuctho vuphuctho removed the request for review from shuesken June 8, 2023 16:07
@vuphuctho vuphuctho requested a review from shuesken June 12, 2023 16:30
@vuphuctho vuphuctho marked this pull request as ready for review June 12, 2023 16:30
@shuesken
Copy link
Contributor

shuesken commented Jun 13, 2023

@vuphuctho
Picking "one-time donation" seems to be less reliable, i.e. I click the "Donate Now" button and nothing happens. I believe it's related to the else if (isStripeUpdated.value) { check, which I don't really understand. Why is Stripe providing multiple methods of doing a thing?

I think the redirect URL should be /donate-complete rather than /donate-success (or the donate-complete.vue component should be renamed)

@vuphuctho
Copy link
Collaborator Author

Picking "one-time donation" seems to be less reliable, i.e. I click the "Donate Now" button and nothing happens. I believe it's related to the else if (isStripeUpdated.value) { check, which I don't really understand. Why is Stripe providing multiple methods of doing a thing?

The issue is that Stripe's element requires an initial price to be render, so whenever user reselects amount of donation we have to call Stripe again. Also one-time and recurring use different Stripe APIs, hence I have to track isStripeUpdated.

I understand the issue you observe - let me see if there is better way to handle user's reselection.

@vuphuctho
Copy link
Collaborator Author

vuphuctho commented Jun 20, 2023

@shuesken I have fixed the flow for Stripe payment:

  • For recurring payment: the app would redirect to stripe payment page. No change.
  • For one-time payment: the app would display Stripe element inside the page. Now reselection of donation amount would not need user to click "Submit" twice, but the Stripe element would re-render itself every reselection.
    image
    Problem we have is, one-time payment requires us to send selected amount to Stripe to render the element, hence for every reselection, we have to re-render again. I don't know if there is any better way to avoid that.
    Card numbers for testing can be founded here https://stripe.com/docs/testing#cards

@shuesken
Copy link
Contributor

@vuphuctho I updated the donate-complete page to be a little more cheery. The flow works fine now, but I run into issues when running this with npm run build and npm run preview. Debugging showed that const body: DonationRequestBody = await readBody(event); in server/api/create-payment-intent.post.ts yields a buffer instead of a json. I believe this may be the same error as in this question: https://stackoverflow.com/questions/75931698/nuxt3-cant-use-readbody-in-server-directory

It's annoying there appears to be different behaviour here depending on runtime environment :/

@vuphuctho
Copy link
Collaborator Author

@shuesken I tried the Stackoverflow solution but it's not working - then I applied the solution in contact.post.js and the fix seems to work - body is pasted as JSON now. This fix is also mentioned in a related thread in Nuxt repo.

There are a few more things they suggested (update package nitro to apply their recent fix, force package h3 to be minimum 1.6.3 to include this fix) but none helps to fix the issue.

Problem I have right now is 2 server endpoints create-checkout-session and create-payment-intent don't return any response when running the app with npm run build && npm run preview. If you manage to produce something different on your end, please let me know.

@shuesken
Copy link
Contributor

@vuphuctho I've investigated this and it's some issue with Cloudflare. The handler times out at await stripe.checkout.sessions.create(…) If we change the Nitro preset to 'node-server', it works fine. CloudFlare does say that the Stripe API works with CF workers; https://blog.cloudflare.com/announcing-stripe-support-in-workers/

I looked at their sample implementation and noticed they explicitly specify an httpClient:

const stripe = new Stripe(STRIPE_API_KEY, {
	// Cloudflare Workers use the Fetch API for their API requests.
	httpClient: Stripe.createFetchHttpClient(),
	apiVersion: '2020-08-27',
});

I could imagine that our issue is somewhere there. I tried using that code though and didn't get better results either. When running this on test.bank.green, the workers yield this error "The script will never generate a response.". Which means CF is tracking the event loop and noticing that that Stripe promise will never resolve.

I'll keep looking at this!

@vuphuctho vuphuctho marked this pull request as draft July 3, 2023 15:49
@shuesken shuesken marked this pull request as ready for review July 10, 2023 12:11
@shuesken
Copy link
Contributor

I removed the Stripe SDK in favour of direct HTTP calls, deployed to test.bank.green and asked Zak for feedback.

@RogerTangos
Copy link
Contributor

Looks ok to me.

Here are some notes. I'm happy to work on these next week or to have someone else pick them up.

  • When a new payment amount is selected, the user has to re-enter the credit card number. This isn't ideal and should be fixed before release.
  • Selecting an amount with a one-time-donation should trigger the opening of the payment information page. The user shouldn't have to click "donate now" to triger that.
  • It's a bit of a shame that we're not supporting Paypal/Klarna/Maestro/Girocard/iDEAL/etc, which is going to limit our fundraising to people who have credit cards, aka the US. That is certainly one thing I was hoping using the SDK would take care of. It this a limitation of using http calls, @shuesken ? I suppose it's ok to launch without this, just so we get something out there.

@shuesken
Copy link
Contributor

shuesken commented Jul 16, 2023

@RogerTangos

  • When a new payment amount is selected, the user has to re-enter the credit card number. This isn't ideal and should be fixed before release.

I am not sure this is possible. As far as Stripe is concerned, a new amount is a new product and the Stripe process starts by telling Stripe "A customer wants to purchase these products, please start the flow" and they return you a session and a payment element to render and handle the rest. But e.g. you cannot auto-fill sensitive details of that element (like CVC or Number) and you cannot update the products. You can allow a customer to change the quantity of a line item if you explicitly set that flag.

  • Selecting an amount with a one-time-donation should trigger the opening of the payment information page. The user shouldn't have to click "donate now" to triger that.

Changed this.

  • It's a bit of a shame that we're not supporting Paypal/Klarna/Maestro/Girocard/iDEAL/etc, which is going to limit our fundraising to people who have credit cards, aka the US. That is certainly one thing I was hoping using the SDK would take care of. It this a limitation of using http calls, @shuesken ? I suppose it's ok to launch without this, just so we get something out there.

Payment methods availability is orthogonal to SDK vs HTTP; the SDK is just a wrapper around the HTTP calls with typing and some convenience error handling. We still use what's called the Stripe Payment Element (which is maybe what you thought of just now).

In general, Stripe automatically supports most common payment methods and displays the appropriate ones, including e.g. iDEAL for Netherlands or Giropay in Germany. For me, for example, it always displays "Cash App" as a way of paying. However, it comes with restrictions that apply in our case: E.g. iDEAL is only offered when the presentment currency is Euro and the business location is in particular places (which do not include the UK). Giropay supports UK businesses, but also requires the amount to be presented in Euro. (Stripe "presentment currency" is what the customer is shown and what is charged from them, "settlement currency" is what ends up in your account, possibly after conversion fees if that was necessary). You can get an overview of what methods are automatically supported and under what restrictions they're offered on our stripe dashboard.

Klarna is supported and works when I turn it on. I don't know if we actually want to support them though, they're backed by WebBank, which provides Fintech's with banking services, makes loans to individuals and businesses, and almost certainly doesn't care about climate change since they have no consumer brand. Paypal seems to be supported but I couldn't find it to enable in our payment methods, so maybe there's something restricting it.

So we could make changes to allow for payment in Euros or connect other business accounts, but it's not trivial and should definitely not be part of this PR.

For what it's worth, most cards issued in Europe today are (also) debit cards and work just fine with the card payment flow, the Netherlands is an outlier in this as far as I understand.

@RogerTangos
Copy link
Contributor

It's a shame with Paypal. It seems like it's not enabled for business based outside of the EU.

LGTM though. Let's do it.

@shuesken shuesken merged commit 2a2f45f into main Jul 17, 2023
@shuesken shuesken deleted the robert-vu947/pe-579-donation-page-layout branch July 17, 2023 18:10
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