Subscribe block: Hide social followers toggle when showSubscribersTotal is not on or isPublicizeEnabled is false #28944

TimBroddin · 2023-02-14T14:09:10Z

Proposed changes:

This PR hides the "Include social followers in count" when "Show subscribers count" is off, or when Publicize is not enabled.

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

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

No

Testing instructions:

Apply this PR
Create a new post
Add a Subscribe block
"Include social followers in count" should be hidden when "Show subscribers count" is off, or Publicize is disabled.

…tal is not toggled or isPublicizeEnabled is false

github-actions · 2023-02-14T14:10:18Z

Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run

bin/jetpack-downloader test jetpack fix/subscription-block-toggles

to get started. More details: p9dueE-5Nn-p2

github-actions · 2023-02-14T14:10:29Z

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.
✅ Add changelog entries to affected projects

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 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

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.

Jetpack plugin:

Next scheduled release: April 4, 2023.
Scheduled code freeze: March 28, 2023.

yansern

Works as described.

jeherve

Looks like bringing this in breaks the tests:
https://github.com/Automattic/jetpack/actions/runs/4174632935/jobs/7228468442

We'll consequently need to update those to be able to merge this.

TimBroddin · 2023-02-16T10:51:47Z

Hey @jeherve

This seems to break on something unrelated:

  [test-extensions]     Details:
  [test-extensions] 
  [test-extensions]     /home/runner/work/jetpack/jetpack/node_modules/.pnpm/@[email protected]_h4tp6taqtzz2k3o7nw5cx3at3a/node_modules/@automattic/social-previews/dist/cjs/facebook-preview/style.scss:12
  [test-extensions]     @import '../variables.scss';
  [test-extensions]     ^
  [test-extensions] 
  [test-extensions]     SyntaxError: Invalid or unexpected token
  [test-extensions] 
  [test-extensions]     > 1 | import { FacebookPreview, TwitterPreview, SearchPreview } from '@automattic/social-previews';
  [test-extensions]         | ^
  [test-extensions]       2 | import { __ } from '@wordpress/i18n';
  [test-extensions]       3 |
  [test-extensions]       4 | export const AVAILABLE_SERVICES = [``

What's the best way for me to fix this?

jeherve · 2023-02-17T17:56:34Z

What's the best way for me to fix this?

That's a good question! I don't know. I wonder if mocking the new hook you're bringing in would solve our problem, so the new package doesn't have to be pulled in.

@glendaviesnz Sorry for the ping! I wonder if you would have some pointers for us on this testing problem :)

glendaviesnz · 2023-02-26T20:45:19Z

@glendaviesnz Sorry for the ping! I wonder if you would have some pointers for us on this testing problem :)

No worries, sorry about the delay in replying, I have been AFK, will try and take a look today and get back to you.

glendaviesnz · 2023-02-27T01:19:04Z

This commit seems to get rid of the scss import errors. Taken from here jestjs/jest#3094 (comment) I also tried the other option of using identity-obj-proxy but that didn't work for me.

There are a couple of other @WordPress import errors still which I haven't got time to look at today, but let me know if you need any help with those and I will take a look tomorrow.

TimBroddin · 2023-02-27T08:25:10Z

Hi @glendaviesnz

🙏 Thanks for the fix. I mocked @wordpress/notices, and it got rid of this failure.

However, now I'm running into

You tried to opt-in to unstable APIs as module "@wordpress/block-editor" which is already registered. This feature is only for JavaScript modules shipped with WordPress core. Please do not use it in plugins and themes as the unstable APIs will be removed without a warning. If you ignore this error and depend on unstable features, your product will inevitably break on one of the next WordPress releases.

This seems the same thing that's blocking this PR, and I honestly don't have a clue about how to fix this. Don you have any suggestions?

glendaviesnz · 2023-02-28T04:00:11Z

@adamziel do you have any thoughts on the best way to suppress these errors? I did look at the docs but could not work out how to knowingly opt-in from the plugin end. In the case of the Jetpack blocks I think there is an understanding of the risks of using these experimental APIs, and backing out of them at this point may not be feasible.

adamziel · 2023-02-28T11:48:37Z

@glendaviesnz That check fails when the same module opts-in to the private APIs twice. When it fails, it could be a symptom of having two different versions of the @wordpress/block-editor package – I'd recommend reviewing the dependency tree as that's likely not what you want anyway.

That being said, this Gutenberg PR disables this strict check in @wordpress packages distributed on npm. It will only be active in WordPress core where every module is only loaded once. The next package release tomorrow will likely unblock you here.

glendaviesnz · 2023-02-28T21:14:40Z

@TimBroddin let me know if hunting out the duplicate dependency, or updating the @wordpress packages tomorrow doesn't sort this for you.

TimBroddin · 2023-03-06T12:06:05Z

The JS tests seem to pass now!

jeherve

This tests well for me. 🚢

…ck-toggles

manzoorwanijk · 2024-08-13T12:09:05Z

projects/plugins/jetpack/extensions/blocks/subscriptions/controls.js

+				{ showSubscribersTotal && isPublicizeEnabled ? (
+					<ToggleControl
+						disabled={ ! showSubscribersTotal }
+						label={ __( 'Include social followers in count', 'jetpack' ) }
+						checked={ includeSocialFollowers }
+						onChange={ () => {
+							setAttributes( { includeSocialFollowers: ! includeSocialFollowers } );
+						} }
+					/>
+				) : null }


This will not work for published posts on simple sites with a free plan because isPublicizeEnabled is more like a local state whether sharing is enabled for the post or not. For published posts, this flag is false if re-sharing is not enabled. For simple sites, resharing is not enabled with free plan.

You can check its source in usePublicizeConfig.

The correct solution here might to check if publicize module is active.

CC: @jeherve

The correct solution here might to check if publicize module is active.

This may indeed be a better solution, and we now have useModuleStatus to help with that. @Automattic/zap Is that something you could look into?

Thank you!

TimBroddin added 2 commits February 14, 2023 14:12

Don't show "Include social followers in count" when showSubscribersTo…

e2b3533

…tal is not toggled or isPublicizeEnabled is false

Don't show "Include social followers in count" when showSubscribersTo…

a300a83

…tal is not toggled or isPublicizeEnabled is false

TimBroddin requested a review from a team February 14, 2023 14:09

TimBroddin self-assigned this Feb 14, 2023

github-actions bot added [Block] Subscriptions [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Feb 14, 2023

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Feb 14, 2023

TimBroddin added [Status] Needs Team Review and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Feb 14, 2023

yansern previously approved these changes Feb 15, 2023

View reviewed changes

yansern added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Team Review labels Feb 15, 2023

jeherve requested changes Feb 15, 2023

View reviewed changes

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Feb 15, 2023

Merge branch 'trunk' into fix/subscription-block-toggles

ccb0b2a

Mock scss imports in jest

a1e270a

glendaviesnz dismissed yansern’s stale review via a1e270a February 27, 2023 01:15

github-actions bot added the [Status] Needs Test Review label Feb 27, 2023

glendaviesnz and others added 2 commits February 27, 2023 14:21

Fix linting issues

9a461a3

Mock @wordpress/notices

b83b623

TimBroddin added 3 commits March 1, 2023 09:53

Merge branch 'trunk' into fix/subscription-block-toggles

566c540

Merge branch 'trunk' into fix/subscription-block-toggles

f1baa30

Mock @wordpress/notices in the tests

abf6326

TimBroddin added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] Needs Test Review labels Mar 6, 2023

github-actions bot added the [Status] Needs Test Review label Mar 6, 2023

jeherve approved these changes Mar 6, 2023

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Mar 6, 2023

jeherve added this to the jetpack/12.0 milestone Mar 6, 2023

Merge remote-tracking branch 'origin/trunk' into fix/subscription-blo…

2058fe1

…ck-toggles

kraftbj enabled auto-merge (squash) March 9, 2023 19:37

kraftbj merged commit 51c65fe into trunk Mar 9, 2023

kraftbj deleted the fix/subscription-block-toggles branch March 9, 2023 20:00

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 9, 2023

manzoorwanijk reviewed Aug 13, 2024

View reviewed changes

manzoorwanijk mentioned this pull request Nov 27, 2024

Fix incorrect usage of publicize config in subscribe block #40355

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscribe block: Hide social followers toggle when showSubscribersTotal is not on or isPublicizeEnabled is false #28944

Subscribe block: Hide social followers toggle when showSubscribersTotal is not on or isPublicizeEnabled is false #28944

TimBroddin commented Feb 14, 2023 •

edited

Loading

github-actions bot commented Feb 14, 2023 •

edited

Loading

github-actions bot commented Feb 14, 2023 •

edited

Loading

yansern left a comment

jeherve left a comment

TimBroddin commented Feb 16, 2023

jeherve commented Feb 17, 2023

glendaviesnz commented Feb 26, 2023

glendaviesnz commented Feb 27, 2023 •

edited

Loading

TimBroddin commented Feb 27, 2023

glendaviesnz commented Feb 28, 2023 •

edited

Loading

adamziel commented Feb 28, 2023 •

edited

Loading

glendaviesnz commented Feb 28, 2023

TimBroddin commented Mar 6, 2023

jeherve left a comment

manzoorwanijk Aug 13, 2024 •

edited

Loading

jeherve Aug 13, 2024

Subscribe block: Hide social followers toggle when showSubscribersTotal is not on or isPublicizeEnabled is false #28944

Subscribe block: Hide social followers toggle when showSubscribersTotal is not on or isPublicizeEnabled is false #28944

Conversation

TimBroddin commented Feb 14, 2023 • edited Loading

Proposed changes:

Other information:

Jetpack product discussion

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

Testing instructions:

github-actions bot commented Feb 14, 2023 • edited Loading

github-actions bot commented Feb 14, 2023 • edited Loading

yansern left a comment

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

TimBroddin commented Feb 16, 2023

jeherve commented Feb 17, 2023

glendaviesnz commented Feb 26, 2023

glendaviesnz commented Feb 27, 2023 • edited Loading

TimBroddin commented Feb 27, 2023

glendaviesnz commented Feb 28, 2023 • edited Loading

adamziel commented Feb 28, 2023 • edited Loading

glendaviesnz commented Feb 28, 2023

TimBroddin commented Mar 6, 2023

jeherve left a comment

Choose a reason for hiding this comment

manzoorwanijk Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

jeherve Aug 13, 2024

Choose a reason for hiding this comment

TimBroddin commented Feb 14, 2023 •

edited

Loading

github-actions bot commented Feb 14, 2023 •

edited

Loading

github-actions bot commented Feb 14, 2023 •

edited

Loading

glendaviesnz commented Feb 27, 2023 •

edited

Loading

glendaviesnz commented Feb 28, 2023 •

edited

Loading

adamziel commented Feb 28, 2023 •

edited

Loading

manzoorwanijk Aug 13, 2024 •

edited

Loading