feat(reader-revenue): make NYP and Stripe Gateway optional #1645

dkoo · 2024-01-19T21:00:11Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Makes Stripe Gateway and Name Your Price extensions optional.

How to test the changes in this Pull Request:

See Automattic/newspack-plugin#2866 for testing instructions.

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?

miguelpeixe

The fallback is working great! I have a few comments inline.

Non-blocking, but I miss having a greyed-out style here:

Even with the message, I still want to click the button.

miguelpeixe · 2024-01-25T20:40:21Z

src/blocks/donate/frontend/class-newspack-blocks-donate-renderer-base.php

@@ -33,8 +50,10 @@ public static function get_configuration( $attributes ) {

 		$configuration['defaultFrequency'] = $attributes['defaultFrequency'];

+		$is_manual = Newspack_Blocks::can_use_name_your_price() && $attributes['manual'];


This line is throwing Warning: Undefined array key "manual" because it's not an attribute that has a default value. It can be undefined.

Fixed in a985f00

src/blocks/donate/frontend/class-newspack-blocks-donate-renderer-base.php

…er-base.php Co-authored-by: Miguel Peixe <[email protected]>

chickenn00dle

Nice work @dkoo!

Tested and everything works as expected. I left a few comments in the code, mostly nitpicks that can be ignored, but one question regarding translations that should be answered before moving forward with this.

chickenn00dle · 2024-01-29T20:23:02Z

src/blocks/donate/edit/FrequencyBasedLayout.tsx

+										>
+											{ settings.currencySymbol +
+												untieredAmount.toFixed( 2 ).replace( /\.?0*$/, '' ) +
+												getFrequencyLabel( frequencySlug ) }


I wonder if this might cause an issue for languages where the amount should not precede the per month/year text? Might be a better idea to have the getFrequencyLabel method accept the amount and return a translated string which includes the amount in it?

That's a good point, although I had some trouble conceptualizing how we'd translate such a string when it has to include HTML tags for styling, too. 3701068 is my best attempt—how's it look?

Thanks @dkoo! I left some additional review comments for this 👍

Great suggestions here! Thank you for the guidance on this part—admittedly our work around making sure things are translatable has been lacking. 😅

chickenn00dle · 2024-01-29T20:26:25Z

src/blocks/donate/frontend/class-newspack-blocks-donate-renderer-base.php

+	 * @param string $frequency_slug Frequency slug.
+	 * @param bool   $hide_once_label Whether to hide the "once" label.
+	 */
+	public static function get_frequency_label( $frequency_slug, $hide_once_label = false ) {


Same comment as above re including amount in the translations here

See #1645 (comment)

chickenn00dle · 2024-01-29T20:27:15Z

src/blocks/donate/frontend/class-newspack-blocks-donate-renderer-base.php

@@ -33,8 +50,10 @@ public static function get_configuration( $attributes ) {

 		$configuration['defaultFrequency'] = $attributes['defaultFrequency'];

+		$is_manual = Newspack_Blocks::can_use_name_your_price() && ! empty( $attributes['manual'] );


nitpick: same comment re creating variable only used in one place

Nope, not necessary—updated in 42e145b.

chickenn00dle · 2024-01-29T20:28:43Z

includes/class-modal-checkout.php

-				$price     = ! empty( $min_price ) ? max( $price, $min_price ) : $price;
-
-				$cart_item_data['nyp'] = (float) \WC_Name_Your_Price_Helpers::standardize_number( $price );
+		$is_product_nyp = \Newspack_Blocks::can_use_name_your_price() ? \WC_Name_Your_Price_Helpers::is_nyp( $product_id ) : false;


nitpick: is it necessary to create a variable here considering this is only used in one place?

Nope, not necessary—updated in 42e145b.

chickenn00dle · 2024-01-30T15:50:12Z

src/blocks/donate/frontend/class-newspack-blocks-donate-renderer-base.php

+				'%1$s is the amount (with currency symbol). %2$s is the frequency of the donation. Inludes embedded HTML tags for styling.',
+				'newspack-blocks'
+			),
+			'<h3 class="wpbnbd__tiers__amount__number">' . $currency_symbol . $amount . '</h3>',


I think this pattern for translating is fine, but I think we will still run into issues with languages where currencies and amounts might need to be swapped. Can we use something like the wc_price helper function for this?

Alternatively, I'm not sure how tied we are to the specific wording here, but WC Subscriptions has done alot of the heavy lifting for formatting price strings, which can get super tricky. Can we rely on something like wcs_price_string?

5f9479a implements wcs_price_string with a fallback to wc_price if not available (if that's not available, the site has bigger problems).

chickenn00dle · 2024-01-30T15:57:30Z

src/blocks/donate/edit/FrequencyBasedLayout.tsx

+										>
+											<div
+												dangerouslySetInnerHTML={ {
+													__html: getFrequencyLabel(


I wonder if we can have this string passed from the backend via newspack_blocks_data since the logic for creating the string already lives there? Then we can remove the duplicate logic from the frontend.

Done in 5f9479a. Because the block needs to be able to dynamically render values from attributes, this may look a little janky with the string replacement, but I'm open to better suggestions.

Ah. I see what you mean. It definitely does seem a bit overly complex. I'm not sure of a cleaner way to handle this with the existing design for the donate block. My suggestion would be to remove the per X/once from the amount strings altogether as this is implied based on the category tab selection at the top (one-time/monthly/annually). But I think changing the design of this might be outside of the scope of this PR.

I think what we have here is fine, although there is an edge case where the publisher has an empty value set, for example while editing an amount. The block shows NaN:

5b8470c forces the value to be 0 in this case. Maybe still not ideal, but better than $NaN?

Yeah. I think it makes sense to set this to 0 when the input is empty. One other issue I noticed just now while retesting this is if the input is empty, we need to make sure the below $X validation still functions and prevents us from saving.

Here it is with an input value less than 5 for me:

And while the input is empty:

I think instead of forcing how the null value is rendered in the preview, we can force null values to be stored as 0 somewhere around here:

newspack-blocks/src/blocks/donate/edit/index.tsx

Line 109 in 5b8470c

setAttributes( { amounts: { ...donationSettings.amounts, ...attributes.amounts } } );

I also want to note while I was retesting this, I did some more thinking about the complexity of generating the translation string with dynamic pricing and HTML elements, and realized we can probably reduce the complexity by removing the span wrappers from the currency symbol and price, and instead placing them only around the frequency string.

We can achieve the same style results via CSS this way by setting the .wpbnbd__tiers__amount__value text to have the bolded and enlarged size and set the frequency span to have the unbolded smaller size.

This is how it is rendered right now:

 <bdi> $5 </bdi> once 

We can probably achieve the same result with this:

 $5 once 

I'm okay if we tackle this another time though since this PR has become quite involved.

I also want to note while I was retesting this, I did some more thinking about the complexity of generating the translation string with dynamic pricing and HTML elements, and realized we can probably reduce the complexity by removing the span wrappers from the currency symbol and price, and instead placing them only around the frequency string.

That's a good call, but yes, let's leave for another PR.

I think instead of forcing how the null value is rendered in the preview, we can force null values to be stored as 0 somewhere around here:

At first I tried forcing empty input values to be stored as 0, but this results in the input itself showing 0 when you delete the values in the input, which is potentially confusing to the user. Agree that we should still validate against the minimum donation value, though.

Agree that we should still validate against the minimum donation value, though.

Implemented in 9f041db

chickenn00dle

Sorry for the delay on getting back to this one @dkoo. Almost there. Just one final issue commented here: #1645 (comment)

chickenn00dle

Nice! thanks for dealing with all the back and forth on this one!

# [3.0.0-alpha.1](v2.6.2...v3.0.0-alpha.1) (2024-02-08) ### Bug Fixes * **homepage-articles:** use map_deep to construct articles_rest_url and resolve PHP 8.1 warnings ([#1655](#1655)) ([24085d8](24085d8)) * prevent error in modal-checkout check ([7e2a6c7](7e2a6c7)) ### Features * **ci:** add `epic/*` release workflow and rename `master` to `trunk` ([#1656](#1656)) ([c788e55](c788e55)) * deprecate streamlined (Stripe) Donate block version ([#1638](#1638)) ([11bd0d6](11bd0d6)) * **homepage-posts:** add custom taxonomy exclusions filter ([#1641](#1641)) ([b140a99](b140a99)) * **reader-revenue:** make NYP and Stripe Gateway optional ([#1645](#1645)) ([1322d7c](1322d7c)) ### BREAKING CHANGES * streamlined (Stripe) Donate block version is no more

matticbot · 2024-02-08T23:27:23Z

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

The release is available on:

Your semantic-release bot 📦🚀

matticbot · 2024-02-15T08:19:02Z

🎉 This PR is included in version 3.0.0-epic-ras-acc.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [3.0.0](v2.6.2...v3.0.0) (2024-02-20) ### Bug Fixes * **homepage-articles:** use map_deep to construct articles_rest_url and resolve PHP 8.1 warnings ([#1655](#1655)) ([24085d8](24085d8)) * prevent error in modal-checkout check ([7e2a6c7](7e2a6c7)) ### Features * **ci:** add `epic/*` release workflow and rename `master` to `trunk` ([#1656](#1656)) ([c788e55](c788e55)) * deprecate streamlined (Stripe) Donate block version ([#1638](#1638)) ([11bd0d6](11bd0d6)) * **homepage-posts:** add custom taxonomy exclusions filter ([#1641](#1641)) ([b140a99](b140a99)) * **reader-revenue:** make NYP and Stripe Gateway optional ([#1645](#1645)) ([1322d7c](1322d7c)) ### BREAKING CHANGES * streamlined (Stripe) Donate block version is no more

matticbot · 2024-02-20T19:24:47Z

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

dkoo added 2 commits January 16, 2024 18:30

fix(donate): don’t require Name Your Price extension

79c2612

feat(donate): force untiered and non-manual when NYP is available

db8797e

dkoo added the [Status] Needs Review label Jan 19, 2024

dkoo self-assigned this Jan 19, 2024

dkoo requested a review from a team as a code owner January 19, 2024 21:00

dkoo changed the title ~~Fix/optional nyp stripe gateway~~ feat(reader-revenue): make NYP and Stripe Gateway optional Jan 19, 2024

dkoo mentioned this pull request Jan 19, 2024

feat(reader-revenue): make NYP and Stripe Gateway optional Automattic/newspack-plugin#2866

Merged

6 tasks

dkoo added 2 commits January 19, 2024 14:03

fix: typescript lint errors

c7c5b5c

Merge branch 'master' into fix/optional-nyp-stripe-gateway

bb3b2e1

miguelpeixe requested changes Jan 25, 2024

View reviewed changes

dkoo and others added 4 commits January 25, 2024 14:56

Update src/blocks/donate/frontend/class-newspack-blocks-donate-render…

9f528b8

…er-base.php Co-authored-by: Miguel Peixe <[email protected]>

Merge branch 'master' into fix/optional-nyp-stripe-gateway

a991eb8

fix: undefined array key warning

a985f00

fix: hide layout options if NYP not available

308181d

chickenn00dle self-requested a review January 26, 2024 22:24

chickenn00dle reviewed Jan 29, 2024

View reviewed changes

dkoo added 3 commits January 29, 2024 17:38

fix: make amount + frequency label translatable

3701068

chore: remove unused variables

42e145b

fix: eslint errors

3a15ae5

dkoo requested a review from chickenn00dle January 30, 2024 00:44

chickenn00dle reviewed Jan 30, 2024

View reviewed changes

dkoo added 2 commits January 31, 2024 16:55

refactor: use wc and wcs methods to get formatted price

5f9479a

fix: add missing TS type

977db4b

dkoo requested a review from chickenn00dle January 31, 2024 23:59

fix: add missing TS type

2b65371

chickenn00dle requested changes Feb 2, 2024

View reviewed changes

dkoo changed the base branch from master to trunk February 5, 2024 22:07

dkoo added 2 commits February 5, 2024 15:07

Merge branch 'trunk' into fix/optional-nyp-stripe-gateway

c388896

fix: avoid NaN with empty number inputs

5b8470c

dkoo requested a review from chickenn00dle February 5, 2024 23:55

fix: show minimum suggestion warning for null values

9f041db

dkoo requested a review from miguelpeixe February 6, 2024 18:19

chickenn00dle approved these changes Feb 6, 2024

View reviewed changes

github-actions bot added [Status] Approved and removed [Status] Needs Review labels Feb 6, 2024

Merge branch 'trunk' into fix/optional-nyp-stripe-gateway

8c01e69

dkoo merged commit 1322d7c into trunk Feb 6, 2024
6 checks passed

dkoo deleted the fix/optional-nyp-stripe-gateway branch February 6, 2024 20:09

matticbot added the released on @alpha label Feb 8, 2024

matticbot added the released on @epic/ras-acc label Feb 15, 2024

matticbot added the released label Feb 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(reader-revenue): make NYP and Stripe Gateway optional #1645

feat(reader-revenue): make NYP and Stripe Gateway optional #1645

dkoo commented Jan 19, 2024

miguelpeixe left a comment

miguelpeixe Jan 25, 2024

dkoo Jan 26, 2024

chickenn00dle left a comment

chickenn00dle Jan 29, 2024

dkoo Jan 30, 2024

chickenn00dle Jan 30, 2024

dkoo Jan 30, 2024

chickenn00dle Jan 29, 2024

dkoo Jan 30, 2024

chickenn00dle Jan 29, 2024

dkoo Jan 30, 2024

chickenn00dle Jan 29, 2024

dkoo Jan 30, 2024

chickenn00dle Jan 30, 2024

chickenn00dle Jan 30, 2024

dkoo Jan 31, 2024

chickenn00dle Jan 30, 2024

dkoo Jan 31, 2024

chickenn00dle Feb 2, 2024 •

edited

Loading

dkoo Feb 5, 2024

chickenn00dle Feb 6, 2024

chickenn00dle Feb 6, 2024

dkoo Feb 6, 2024

dkoo Feb 6, 2024

chickenn00dle left a comment

chickenn00dle left a comment

matticbot commented Feb 8, 2024

matticbot commented Feb 15, 2024

matticbot commented Feb 20, 2024

		@@ -33,8 +50,10 @@ public static function get_configuration( $attributes ) {

		$configuration['defaultFrequency'] = $attributes['defaultFrequency'];

		$is_manual = Newspack_Blocks::can_use_name_your_price() && $attributes['manual'];

feat(reader-revenue): make NYP and Stripe Gateway optional #1645

feat(reader-revenue): make NYP and Stripe Gateway optional #1645

Conversation

dkoo commented Jan 19, 2024

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

miguelpeixe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chickenn00dle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chickenn00dle Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chickenn00dle left a comment

Choose a reason for hiding this comment

chickenn00dle left a comment

Choose a reason for hiding this comment

matticbot commented Feb 8, 2024

matticbot commented Feb 15, 2024

matticbot commented Feb 20, 2024

chickenn00dle Feb 2, 2024 •

edited

Loading