Subscriptions: Add unit tests and fixtures to validate block content parsing

[ramonjd]

Changes proposed in this Pull Request:

This PR adds unit and block fixture parsing tests to the Subscriptions block, and extricate the controls into a controls.js file.

No block functionality is affected.

Jetpack product discussion

p1HpG7-b3G-p2

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

Running the unit tests:

cd projects/plugins/jetpack && yarn fixtures:test extensions/blocks/subscriptions/test/

We should also ensure that the block works as expected in the editor and saved stats on a site.

To test the subscriptions block in a browser we need an active connection or have the plugin running on WPCOM in order to activate the module.

Apply D57958-code to your sandbox and add a subscriptions block to a new sandboxed test site.

Proposed changelog entry for your changes:

None. No functional changes made.

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello ramonjd! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D57958-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

[github-actions]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ✅ All commits were linted before commit.
  
• ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  
• ✅ Add testing instructions.
  
• ✅ Include a changelog entry for any meaningful change.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

---

If you are an automattician, once your PR is ready for review add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.undefined

---

jetpack plugin:

• Next scheduled release: April 6, 2021.
• Scheduled code freeze: March 29, 2021

[github-actions]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ❌ All commits were linted before commit.
  
• ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  
• ✅ Add testing instructions.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

---

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.undefined

---

Jetpack plugin:

• Next scheduled release: April 6, 2021.
• Scheduled code freeze: March 29, 2021

[andrewserong]

This is testing well for me @ramonjd, I tried the block out in a sandboxed wpcom environment and in my local Jetpack site, and the block still works as expected.

I left a tiny comment about the padding tests, but I also noticed with the edit.js tests it appears to be trying to fire the apiFetch. It doesn't seem to be breaking the tests, but does log out a warning when I run yarn jest extensions/blocks/subscriptions/test/:

Is it worth mocking apiFetch for the Edit tests?

[ramonjd]

Is it worth mocking apiFetch for the Edit tests?

Thanks for pointing this out @andrewserong! Totally agree we should remove those warnings.

It also gave me a chance to learn about act()

[glendaviesnz]

This tested well for me, and api notices are no longer logged.

For some reason the block content migrates every time you load a page in the editor with this block, even though the save function and saved content are the same:

New content generated by `save` function:

<div class="wp-block-jetpack-subscriptions wp-block-jetpack-subscriptions__supports-newline wp-block-jetpack-subscriptions__use-newline wp-block-jetpack-subscriptions__show-subs">[jetpack_subscription_form show_subscribers_total="true" button_on_newline="true" custom_font_size="36px" custom_border_radius="19" custom_border_weight="8" custom_border_color="#cf2e2e" custom_padding="30" custom_spacing="44" submit_button_classes="has-36-px-font-size has-cf-2-e-2-e-border-color has-text-color has-black-color has-background has-light-green-cyan-background-color" email_field_classes="has-36-px-font-size has-cf-2-e-2-e-border-color" show_only_email_and_button="true"]</div>

Content retrieved from post body:

<div class="wp-block-jetpack-subscriptions wp-block-jetpack-subscriptions__supports-newline wp-block-jetpack-subscriptions__use-newline wp-block-jetpack-subscriptions__show-subs">[jetpack_subscription_form show_subscribers_total="true" button_on_newline="true" custom_font_size="36px" custom_border_radius="19" custom_border_weight="8" custom_border_color="#cf2e2e" custom_padding="30" custom_spacing="44" submit_button_classes="has-36-px-font-size has-cf-2-e-2-e-border-color has-text-color has-black-color has-background has-light-green-cyan-background-color" email_field_classes="has-36-px-font-size has-cf-2-e-2-e-border-color" show_only_email_and_button="true"]</div>

I can't see an obvious reason why this is happening, but it looks like this is an existing bug as it happens on WPcom currently, so not sure if you want to investigate further or add a separate issue for it.

[aaronrobertshaw]

This tests well for me 👍

For some reason the block content migrates every time you load a page in the editor with this block, even though the save function and saved content are the same:

With empty/default attributes the current block version's shortcode matches the v1 deprecation. That deprecation implements isEligible which checks if a subscribeButton attribute is empty or not. It determines the deprecation to be eligible if that attribute is empty.

As the current default subscribe block content matches and is "valid", the isEligible is run. Given the attributes won't include subscribeButton, that explains why it is being mistakenly run.

If that isEligible function is deleted, the block deprecation doesn't run.

It appears that the point of this deprecation was to migrate from the subscribeButton attribute to submitButtonText. Instead of deleting isEligible I think updating the check to see if the attributes contain the subscribeButton key should still allow the deprecation to cover its intended use case.

	isEligible: attr => {
		if ( ! attr.hasOwnProperty( 'subscribeButton' ) || ! isEmpty( attr.subscribeButton ) ) {
			return false;
		}
		return true;
	},

Making a change like this might give an opportunity to run the fixture tests in earnest as well? 🙂

[ramonjd]

Thanks so much @glendaviesnz and @aaronrobertshaw for your reviews and explanations!

TIL

I've implemented the changes as recommended and can confirm that the content migration console message does not appear after the change. 🙇

[aaronrobertshaw]

I've pushed a commit updating all the fixtures and some of the deprecations. All the tests are green for me and all the deprecations' .json files show isValid: true.

Ran the tests as per the PR instructions but also via yarn jest extensions/blocks/subscriptions/test/ to execute only this block's tests.

There are a couple of things to note.

• Deprecation v4 was added to be able to migrate from shortcode that had localized default text strings embedded in it. I wasn't sure if I could set a locale for the fixture test so that is something that could be improved.
• This block will need testing in the post editor again to ensure I haven't caused a regression.
• I added a brief explanation as to what I understand the reasoning for each deprecation to be. Feel free to change those comments' wording or structure. Their only aim is to help our future selves and others.

[ramonjd]

Needs rebase after d001afc

Also, I suspect we need to update our dependency on @wordpress/components to get the latest UnitControl changes

We're pulling 9.2.6, but the latest is 12.0.7

[jeherve]

I suspect we need to update our dependency on @wordpress/components to get the latest UnitControl changes

We're pulling 9.2.6, but the latest is 12.0.7

Noting that this was planned in #16763, but we've run into some issues that have stopped from updating so far. :(

[ramonjd]

I've mocked the button width control component (similar to the Button tests) in the meantime to move this PR forward.

[stacimc]

Lgtm, tests are passing now 👍

[ramonjd]

This patch uses ButtonWidthControl which was included in #18809

To keep things in sync #18809 needs to be deployed to WPCOM before we deploy this patch to WPCOM (and after the next Jetpack release)

[github-actions]

Great news! One last step: head over to your WordPress.com diff, D57958-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

[jeherve]

r223841-wpcom