Subscriptions Block: Remove logged in view for wpcom #18550

apeatling · 2021-01-26T18:54:09Z

Changes proposed in this Pull Request:

Remove the logged in view for WordPress.com so that the Subscriptions block always renders what is in the editor. When a user is logged in to WordPress.com they will still see the email subscription form and can subscribe as normal.

Before	After

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

No

Testing instructions:

Apply D56029-code to your sandbox.
Apply this branch to a local Jetpack install
Confirm that this works correctly on WordPress.com based on testing instructions on D56029-code
Confirm the subscriptions form works on your Jetpack site still (you will need it to be a publicly accessible site via jurrassic.tube or ngrok) and the subscriptions Jetpack setting turned on.
Try subscribing and confirm that you can still subscribe to a site.

Proposed changelog entry for your changes:

Not needed.

matticbot · 2021-01-26T18:54:15Z

Caution: This PR has changes that must be merged to WordPress.com
Hello apeatling! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D56029-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

jetpackbot · 2021-01-26T18:55:06Z

Scheduled Jetpack release: February 16, 2021.
Scheduled code freeze: February 8, 2021

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Generated by 🚫 dangerJS against 5c61b44

stacimc

On wpcom, I was able to see the subscription form both as a logged-in and a not-logged-in user.

When I submitted the form as a logged-in user, it worked as expected, and I received the follow up email at the address I used:

~~However when I tested as a logged-out user, I got a message saying I was already subscribed (I made 100% sure the email I was using was not already subscribed), and received no email:~~ <-- After rebuilding jetpack, this is no longer happening. It is now working for both logged-in and logged-out users.

stacimc · 2021-01-28T19:31:54Z

projects/plugins/jetpack/modules/subscriptions/views.php

@@ -108,11 +108,7 @@ function widget( $args, $instance ) {

 		self::render_widget_status_messages( $instance );

-		if ( self::is_current_user_subscribed() ) {
-			self::render_widget_already_subscribed( $instance );


Double checking -- is removing this already-subscribed view meant to be included with removing the logged-in view? If so, can we get rid of the now unused render_widget_already_subscribed function?

Yes, I think that would be a good idea. Done in 5c61b44

apeatling · 2021-01-28T20:15:40Z

**Update -- I just tried this on a different sandboxed site, and it worked even as a logged-out user. I'm not sure what was different.

Just checked this on a sandboxed site, and it seemed to work okay logged in and logged out.

stacimc

Tested well for me on Jetpack, and the error I saw on wpcom for logged-out is working fine after a rebuild. LGTM!

jeherve

The linting errors will stop us from merging this I'm afraid.

apeatling · 2021-01-29T18:11:48Z

The linting errors will stop us from merging this I'm afraid.

I think I'll need to mark to ignore these errors because removing these extra escapes was breaking the reg ex.

Edit: Wrong PR :)

apeatling · 2021-01-29T18:37:03Z

I've added the jetpack textdomain since the ignore does not work. The wpautop() pre-existed this PR, but I can't escape html with this since, or remove it since it will break existing usage.

apeatling · 2021-02-09T18:56:09Z

@jeherve This one look good now?

apeatling · 2021-02-10T18:25:13Z

Nevermind! Seeing one last lint error 🤦🏻 Fixing.

…ock-alignment

See https://github.com/Automattic/jetpack/pull/18550/files?diff=unified&w=1#r566354742

jeherve

This looks good to me. It's a big change for WordPress.com so I think it may be good to warn Happiness about the change, but I think it makes for a good change.

jeherve · 2021-02-11T15:32:08Z

projects/plugins/jetpack/modules/subscriptions/views.php

@@ -108,11 +108,7 @@ function widget( $args, $instance ) {

 		self::render_widget_status_messages( $instance );

-		if ( self::is_current_user_subscribed() ) {
-			self::render_widget_already_subscribed( $instance );


Yes, I think that would be a good idea. Done in 5c61b44

jeherve · 2021-03-04T17:36:56Z

r222134-wpcom

Remove logged in view for wpcom and match terminology to editor.

934ff88

apeatling added [Feature] Subscriptions All subscription-related things such as paid and unpaid, user management, and newsletter settings. [Block] Subscriptions [Status] Needs Team Review labels Jan 26, 2021

apeatling requested a review from a team January 26, 2021 18:54

apeatling self-assigned this Jan 26, 2021

matticbot added Touches WP.com Files labels Jan 26, 2021

apeatling requested review from a team and removed request for a team January 26, 2021 19:07

stacimc self-requested a review January 28, 2021 19:16

stacimc suggested changes Jan 28, 2021

View reviewed changes

stacimc previously approved these changes Jan 28, 2021

View reviewed changes

apeatling requested a review from a team January 28, 2021 20:18

apeatling added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Team Review labels Jan 28, 2021

jeherve requested changes Jan 29, 2021

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 Jan 29, 2021

Fix linting errors.

a1acf2b

apeatling dismissed stacimc’s stale review via a1acf2b January 29, 2021 18:30

apeatling requested a review from jeherve January 29, 2021 18:30

Ignore escape error because this pre existed and we need autop.

2e20e05

apeatling 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! labels Jan 29, 2021

jeherve added 2 commits February 11, 2021 16:16

Merge remote-tracking branch 'origin/master' into fix/subscription-bl…

a913c37

…ock-alignment

remove unused variable

4d40e81

github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Feb 11, 2021

Remove unused function

5c61b44

See https://github.com/Automattic/jetpack/pull/18550/files?diff=unified&w=1#r566354742

jeherve approved these changes Feb 11, 2021

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 Feb 11, 2021

jeherve added this to the 9.5 milestone Feb 11, 2021

apeatling merged commit 32919e1 into master Feb 11, 2021

apeatling deleted the fix/subscription-block-alignment branch February 11, 2021 19:10

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Feb 11, 2021

jeherve removed the [Status] Needs Changelog label Feb 12, 2021

aaronrobertshaw mentioned this pull request Feb 17, 2021

Subscription Form Block: block validation errors cause by "Subscribe" / "Follow" text #18518

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscriptions Block: Remove logged in view for wpcom #18550

Subscriptions Block: Remove logged in view for wpcom #18550

apeatling commented Jan 26, 2021 •

edited by jeherve

Loading

matticbot commented Jan 26, 2021

jetpackbot commented Jan 26, 2021 •

edited

Loading

stacimc left a comment •

edited

Loading

stacimc Jan 28, 2021

jeherve Feb 11, 2021

apeatling commented Jan 28, 2021

stacimc left a comment

jeherve left a comment

apeatling commented Jan 29, 2021 •

edited

Loading

apeatling commented Jan 29, 2021

apeatling commented Feb 9, 2021

apeatling commented Feb 10, 2021

jeherve left a comment

jeherve Feb 11, 2021

jeherve commented Mar 4, 2021

Subscriptions Block: Remove logged in view for wpcom #18550

Subscriptions Block: Remove logged in view for wpcom #18550

Conversation

apeatling commented Jan 26, 2021 • edited by jeherve Loading

Changes proposed in this Pull Request:

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

Testing instructions:

Proposed changelog entry for your changes:

matticbot commented Jan 26, 2021

jetpackbot commented Jan 26, 2021 • edited Loading

stacimc left a comment • edited Loading

Choose a reason for hiding this comment

stacimc Jan 28, 2021

Choose a reason for hiding this comment

jeherve Feb 11, 2021

Choose a reason for hiding this comment

apeatling commented Jan 28, 2021

stacimc left a comment

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

apeatling commented Jan 29, 2021 • edited Loading

apeatling commented Jan 29, 2021

apeatling commented Feb 9, 2021

apeatling commented Feb 10, 2021

jeherve left a comment

Choose a reason for hiding this comment

jeherve Feb 11, 2021

Choose a reason for hiding this comment

jeherve commented Mar 4, 2021

apeatling commented Jan 26, 2021 •

edited by jeherve

Loading

jetpackbot commented Jan 26, 2021 •

edited

Loading

stacimc left a comment •

edited

Loading

apeatling commented Jan 29, 2021 •

edited

Loading