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

Subscriptions Block: Invalidates when edited in different language #15776

Closed
sirreal opened this issue May 13, 2020 · 18 comments · Fixed by #16361
Closed

Subscriptions Block: Invalidates when edited in different language #15776

sirreal opened this issue May 13, 2020 · 18 comments · Fixed by #16361
Assignees
Labels
[Block] Calendly [Block] Eventbrite [Block] Revue [Block] Subscriptions [Feature] Contact Form [Focus] i18n Internationalization / i18n, adaptation to different languages [Pri] High [Size] M [Type] Bug When a feature is broken and / or not performing as intended

Comments

@sirreal
Copy link
Member

sirreal commented May 13, 2020

If a Subscription Block is added to a post with one localization, then subsequently edited in another localization, the block will be invalidated with no good way of recovering. The default texts in the block must be maintained, customization of the texts will prevent invalidation.

Core Gutenberg is working out how to handle situations like this:

Some of the default block attributes are localized with __:

subscribePlaceholder: {
type: 'string',
default: __( 'Enter your email address', 'jetpack' ),
},

submitButtonText: {
type: 'string',
default: __( 'Sign Up', 'jetpack' ),
},

These attributes are stored directly in the save function:

const {
subscribePlaceholder,
showSubscribersTotal,
buttonOnNewLine,
submitButtonText,
emailFieldBackgroundColor,
customEmailFieldBackgroundColor,
emailFieldGradient,
customEmailFieldGradient,
buttonBackgroundColor,
customButtonBackgroundColor,
buttonGradient,
customButtonGradient,
textColor,
customTextColor,
fontSize,
customFontSize,
borderRadius,
borderWeight,
borderColor,
customBorderColor,
padding,
spacing,
} = attributes;

return (
<div className={ getBlockClassName() }>
<RawHTML>
{ `
[jetpack_subscription_form
subscribe_placeholder="${ subscribePlaceholder }"
show_subscribers_total="${ showSubscribersTotal }"
button_on_newline="${ buttonOnNewLine }"
submit_button_text="${ submitButtonText }"
custom_background_emailfield_color="${ emailFieldBackgroundStyle }"
custom_background_button_color="${ buttonBackgroundStyle }"
custom_text_button_color="${ customTextColor }"
custom_font_size="${ customFontSize || DEFAULT_FONTSIZE_VALUE }"
custom_border_radius="${ borderRadius || DEFAULT_BORDER_RADIUS_VALUE }"
custom_border_weight="${ borderWeight || DEFAULT_BORDER_WEIGHT_VALUE }"
custom_border_color="${ customBorderColor }"
custom_padding="${ padding || DEFAULT_PADDING_VALUE }"
custom_spacing="${ spacing || DEFAULT_SPACING_VALUE }"
submit_button_classes="${ submitButtonClasses }"
email_field_classes="${ emailFieldClasses }"
show_only_email_and_button="true"
]` }
</RawHTML>
</div>
);

The block may be saved with a localized text Enter your email address, then when the editor loads and validates the block content, it finds an unexpected localization (e.g. Dirección de correo electrónico) and determines that the block content is not valid.

I wrote about the problem of "impure save" here in more detail: pafL3P-kL-p2
In this case, translations implicitly leak into the block's save, rendering them impure and susceptible to invalidation.

This causes the following issues:

This has been present since the very first iteration of the block: Automattic/wp-calypso#28887

cc: @obenland @iamtakashi

Steps to reproduce the issue

  1. Create a new site
  2. Connect Jetpack
  3. Enable Subscriptions (/wp-admin/admin.php?page=jetpack#/discussion)
  4. Create a new post (/wp-admin/post-new.php)
  5. Add a Subscription Form block (Start typing /subscription)
  6. Save the post
  7. Change the site language (e.g. Español) (/wp-admin/options-general.php)
  8. Update localizations (/wp-admin/update-core.php)
  9. Open the previous post
  10. The block will be invalidated with errors in the console.

Save (English):

Screen Shot 2020-05-13 at 11 46 57

Load (Spanish):

Screen Shot 2020-05-13 at 11 47 40

Block validation: Block validation failed for `jetpack/subscriptions` ({name: "jetpack/subscriptions", icon: {…}, …

Content generated by `save` function:

<div class="wp-block-jetpack-subscriptions">[jetpack_subscription_form show_subscribers_total="false" show_only_email_and_button="true"]</div>

Content retrieved from post body:

<div class="wp-block-jetpack-subscriptions">[jetpack_subscription_form show_only_email_and_button="true" custom_background_button_color="undefined" custom_text_button_color="undefined" submit_button_text="Subscribe" submit_button_classes="undefined" show_subscribers_total="false" ]</div>
@sirreal sirreal added [Type] Bug When a feature is broken and / or not performing as intended [Focus] i18n Internationalization / i18n, adaptation to different languages [Block] Subscriptions labels May 13, 2020
@sirreal sirreal added this to the 8.6 milestone May 13, 2020
@sirreal
Copy link
Member Author

sirreal commented May 13, 2020

I added 8.6 milestone from #15475.

@jeherve
Copy link
Member

jeherve commented May 13, 2020

cc @apeatling who's been working on some improvements to the Subscriptions block.

@sirreal
Copy link
Member Author

sirreal commented May 13, 2020

I was doing some additional testing and it doesn't seem like you can modify the placeholder text through the block UI at all.

@simison
Copy link
Member

simison commented May 13, 2020

Ah bummer, this was tested (#15107 (comment)) but somehow slipped still through.

@apeatling
Copy link
Member

apeatling commented May 13, 2020

@sirreal Did you test this with the updates to the block in master? I couldn't reproduce this when I was working on it, but I probably wasn't doing it correctly. I'll try again today.

@apeatling apeatling added this to Backlog in Blocks V2 via automation May 13, 2020
@sirreal
Copy link
Member Author

sirreal commented May 13, 2020

@apeatling Yep, I just spun up a new site with Jetpack beta "Bleeding edge" build and was able to reproduce the issue following the instructions in the description.

@apeatling apeatling moved this from Backlog to In progress in Blocks V2 May 13, 2020
@apeatling
Copy link
Member

apeatling commented May 13, 2020

Couple of options I can think of:

  1. Remove the defaults as translatable for these two strings. The user can update the input button text on their own, and we can make the placeholder text editable.

  2. Remove the defaults as translatable and add logic to the edit component to check for a different translation and update it once the block has been loaded (assuming the value hasn't been edited).

  3. ??

@apeatling apeatling removed this from In progress in Blocks V2 May 14, 2020
@akirk
Copy link
Member

akirk commented May 15, 2020

I think source: html would be a solution here, since by default the text is stored in the HTML comment which can lead to incompatible data (English attribute, translated HTML).

@jeherve jeherve removed this from the 8.6 milestone May 25, 2020
@creativecoder
Copy link
Contributor

creativecoder commented May 28, 2020

Note that this affects several blocks

(and maybe others--I only searched through the attributes.js files, so this may have missed blocks that set default attributes in other ways)

@iamtakashi
Copy link
Contributor

Note that this affects several blocks

I'm not sure if it was the same reason, but the Map block and the Contact Info block were also broken (invalidated) with the translations for all the Mag 16, and I had to manually fix in the json files. It was really time-consuming, and the issue blocks us to deploy layouts regularly.

@creativecoder
Copy link
Contributor

Note, I updated my comment above to indicate that it's not an exhaustive list

(and maybe others--I only searched through the attributes.js files, so this may have missed blocks that set default attributes in other ways)

@apeatling
Copy link
Member

I think source: html would be a solution here, since by default the text is stored in the HTML comment which can lead to incompatible data (English attribute, translated HTML).

In the case of the contact form and subscription blocks, the source: html is not going to work because the save function creates a shortcode that is then rendered via the backend at runtime.

It could work for the other blocks that do not do this however.

@apeatling
Copy link
Member

apeatling commented Jun 1, 2020

I tried this with the Calendly block, but I'm not having any luck with selecting via source: html or text because of the server side rendering.

@creativecoder
Copy link
Contributor

@lessbloat This might be a good candidate for the Manage Janitorial rotation

@davemart-in
Copy link
Contributor

@creativecoder by the looks of it, this is likely too complex for the Janitor rotation.

@Copons
Copy link
Contributor

Copons commented Jul 1, 2020

Note that this affects several blocks

* [Subscriptions](https://github.com/Automattic/jetpack/blob/3466b0681a195db76416077b8d3647f877f41448/extensions/blocks/subscriptions/attributes.js#L9)

* [Eventbrite](https://github.com/Automattic/jetpack/blob/3466b0681a195db76416077b8d3647f877f41448/extensions/blocks/subscriptions/attributes.js#L9)

* [Calendly](https://github.com/Automattic/jetpack/blob/3466b0681a195db76416077b8d3647f877f41448/extensions/blocks/calendly/attributes.js#L21)

* [Revue](https://github.com/Automattic/jetpack/blob/3466b0681a195db76416077b8d3647f877f41448/extensions/blocks/revue/attributes.js#L13)

* [Contact Form](https://github.com/Automattic/jetpack/blob/5fbf71770770dc16d20eae74c07946493a89945a/extensions/blocks/contact-form/attributes.js#L9)

(and maybe others--I only searched through the attributes.js files, so this may have missed blocks that set default attributes in other ways)

I've just run a quick test, and of these only Subscriptions seems to be affected.
(TBH I haven't tried all Contact Form variations, but it shouldn't matter)

The difference is that those blocks don't save the localized attributes in the block content, whereas Subscriptions does.

Examples

<!-- wp:jetpack/revue {"revueUsername":"test"} -->
<div class="wp-block-jetpack-revue">
	<!-- wp:jetpack/button {"element":"button","text":"Subscribe"} /-->
	<a class="wp-block-jetpack-revue__fallback" href="https://www.getrevue.co/profile/test">
		https://www.getrevue.co/profile/test
	</a>
</div>
<!-- /wp:jetpack/revue -->

Revue has all the fields labels and placeholders as localized attributes, plus the button text, which is an attribute of the Button inner block.

No field labels/placeholders are saved in the block content, as the form is built server-side.

The inner Button doesn't break because the parent block is actually defining it with the localized attribute: in other words, we are actually setting its text attribute with the localized string.
The side effect is that changing the site language won't also change the Button text.
This might be considered an issue, though, but a different one.

<!-- wp:jetpack/subscriptions -->
<div class="wp-block-jetpack-subscriptions wp-block-jetpack-subscriptions__supports-newline">
	[jetpack_subscription_form
		subscribe_placeholder="Enter your email address"
		submit_button_text="Sign Up"
	]</div>
<!-- /wp:jetpack/subscriptions -->

Subscriptions (example trimmed for readability), instead, saves the defaults in the block content.
As pointed out by @sirreal, when the block loads with a different translation, it tries to match it with the block content, and fails, breaking the block.

I'm still trying to figure out a good solution, though.
SSR-ing the block is not a good one, as we want the shortcode to stay in the post content.
Maybe instead of having attribute defaults, we could actually save them as attribute values?
We would end up with the same issue I've pointed out earlier about the Button inner block, but I'd argue it's better than having the block break? 🤔

@Copons Copons self-assigned this Jul 1, 2020
@Copons
Copy link
Contributor

Copons commented Jul 1, 2020

Actually I think I've found a nice solution! ✨

@Copons
Copy link
Contributor

Copons commented Jul 6, 2020

I've unassigned myself, but of course feel free to pick #16361 up! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Calendly [Block] Eventbrite [Block] Revue [Block] Subscriptions [Feature] Contact Form [Focus] i18n Internationalization / i18n, adaptation to different languages [Pri] High [Size] M [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet