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(donate): streamlined block option #784

Merged
merged 8 commits into from
Jul 15, 2021
Merged

Conversation

adekbadek
Copy link
Member

@adekbadek adekbadek commented Jul 1, 2021

All Submissions:

Changes proposed in this Pull Request:

Introduces an experimental, streamlined option for the Donate block. When activated, the donation flow will be as short as possible - the donor will only have to provide credit card details & email address.

TODOs

  • Listen to Stripe successful payment via webhooks to send custom event to GA and update Campaigns data)

How to test the changes in this Pull Request:

  1. Set newspack-plugin to feat/donation-block-streamlined branch (Add Stripe connection for streamlined Donate block newspack-plugin#1029)
  2. Nothing should change yet, so test both donation platforms (WooCommerce (Newspack) and NRH) and confirm that donation flow works as before
  3. Configure the site to use AMP Plus by adding the following env variable to wp-config.php: define( 'NEWSPACK_AMP_PLUS_ENABLED', true );
  4. Set the Reader Revenue platform to NRH in the Reader Revenue wizard
  5. Fill in the Stripe keys and choose a currency
  6. On a page, insert a Donate block. Observe a new panel in editor sidebar - "Streamlined". Toggle the option on and publish the page
  7. Observe that the Donate block features credit card & email inputs
  8. Test a successful transaction with a 4242 4242 4242 4242 card (and any valid other details) and observe a green "Thank you" notice and a successful charge in Stripe (and a Customer created)
  9. Test a 3D Secure (requiring authentication) transaction with a 4000 0027 6000 3184 card
  10. Test a failed transaction with a 4000000000000002 card

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

I'm not able to fully test this yet. When viewing a page with a streamlined Donate block on the front-end, I see this JS error in the console:

Screen Shot 2021-07-01 at 11 34 56 AM

And the Stripe form doesn't load. Seems like it's a webpack build error. This is after running npm ci && npm run build in this repository. Also tried clearing package-lock.json and running npm i && npm run build with the same result. I'm running node v12.21.0.

UPDATE: Strangely, I'm not seeing this error when installing the built plugins on a fresh ephemeral test site. Seems to be only happening in my local environment. Investigating.

UPDATE 2: I was able to get past this error and complete testing in both my local environment and the fresh ephemeral test site. Details below. My two questions about sanitization still apply.

UPDATE 3: After further investigation I confirmed that the JS error is related to the WP v5.8 beta. I had the beta installed on my dev environment when I first started testing this, but downgraded to v5.7.2 in order to continue testing, which fixed the issue. After re-installing v5.8, I'm seeing the error again. I'm not sure we need to take action at this point since it could be a beta thing, but we should re-test after the v5.8 production release.

src/blocks/donate/view.php Show resolved Hide resolved
src/blocks/donate/view.php Show resolved Hide resolved
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Ok, I was able to get past the webpack error after rebuilding a few more times. I have no idea what caused it in the first place, but after clearing node_modules and package-lock.json, running npm i, then clearing them out again, restoring the checked-in package-lock.json and running npm ci again, I'm no longer seeing the error.

The streamlined flow works great and is so much more user-friendly than redirecting to a separate checkout page. I have a few suggestions below about the messaging and UI. Since payment processing can be a sensitive user experience I think we want to be as communicative as possible. We may even want to offer admins the ability to write their own success and error messaging (which would let them tailor their messaging according to their audience), but that could be a future enhancement.

Also, do we need to do anything to ensure that the payment details are being submitted to Stripe from an TLS-secured page? That should be the case for all Newspack-hosted sites, but it may not be for self-hosted sites. Maybe if the current page isn't secured with TLS, we don't show or disable the payment form? Answering my own question here. According to Stripe documentation, since we're using Stripe.js to render the payment form, the payment details never actually touch the site—they're submitted in a form hosted by Stripe in a secure iframe. This takes care of most PCI compliance and security concerns. However, in order to fully meet PCI compliance, the payment page hosting the Stripe form must be secured with TLS/HTTPS. This is on the site owner to ensure they're compliant. Maybe we could include a link to this Stripe security guide in the "Stripe" tab in the dashboard if we want to educate users about security requirements, but I don't think we need to do anything beyond this in the code.

src/blocks/donate/edit.js Outdated Show resolved Hide resolved
src/blocks/donate/streamlined.js Outdated Show resolved Hide resolved
src/blocks/donate/streamlined.js Show resolved Hide resolved
@adekbadek
Copy link
Member Author

We may even want to offer admins the ability to write their own success and error messaging (which would let them tailor their messaging according to their audience), but that could be a future enhancement.

Good idea, we should do that later.

This is on the site owner to ensure they're compliant. Maybe we could include a link to this Stripe security guide in the "Stripe" tab in the dashboard if we want to educate users about security requirements, but I don't think we need to do anything beyond this in the code.

Added an SSL check & a warning in the Plugin part of this feature 👍

@adekbadek adekbadek requested a review from dkoo July 2, 2021 12:08
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

Copy link
Contributor

@thomasguillot thomasguillot left a comment

Choose a reason for hiding this comment

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

I pushed a few design changes (nothing major -- just to be a little be more inline with the theme) 1d2c155

@adekbadek adekbadek merged commit 10bfb0b into master Jul 15, 2021
@adekbadek adekbadek deleted the add/stripe-donations branch July 15, 2021 10:46
matticbot pushed a commit that referenced this pull request Jul 19, 2021
# [1.30.0-alpha.1](v1.29.2...v1.30.0-alpha.1) (2021-07-19)

### Bug Fixes

* **class-newspack-blocks.php:** fixes assets meta file path for scripts ([#780](#780)) ([a37ff23](a37ff23)), closes [#779](#779)
* allow swiper to reinitialize carousel on attribute changes ([#807](#807)) ([cc9fa30](cc9fa30))
* do not initialize swiper instances if component is hidden ([#804](#804)) ([fe599e1](fe599e1))
* **carousel:** preview performance ([#803](#803)) ([e5fc989](e5fc989))

### Features

* **donate:** streamlined block option w/ Stripe ([#784](#784)) ([10bfb0b](10bfb0b))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.30.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Jul 19, 2021
# [1.30.0](v1.29.2...v1.30.0) (2021-07-19)

### Bug Fixes

* remove autoload dependency from donation class ([#811](#811)) ([c37c885](c37c885))
* **class-newspack-blocks.php:** fixes assets meta file path for scripts ([#780](#780)) ([a37ff23](a37ff23)), closes [#779](#779)
* allow swiper to reinitialize carousel on attribute changes ([#807](#807)) ([cc9fa30](cc9fa30))
* do not initialize swiper instances if component is hidden ([#804](#804)) ([fe599e1](fe599e1))
* **carousel:** preview performance ([#803](#803)) ([e5fc989](e5fc989))

### Features

* **donate:** streamlined block option w/ Stripe ([#784](#784)) ([10bfb0b](10bfb0b))
* **nrh:** donations handling ([#805](#805)) ([821f6db](821f6db))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.30.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants