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.

Sign up for GitHub

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

Jump to bottom

Connection: Do not show plans grid if site has pending plan #14916

Merged
merged 2 commits into from
Mar 12, 2020
Merged

Connection: Do not show plans grid if site has pending plan #14916

merged 2 commits into from
Mar 12, 2020

Conversation

ebinnion
Copy link
Contributor

@ebinnion ebinnion commented Mar 6, 2020 •
edited
Loading

The in-place connection creates a regression in the connection flow for users that have a pending plan. Specifically, users with a pending plan, should be treated as users that already have a plan, and as such, those users should not see the plans grid after connecting their site.

Changes proposed in this Pull Request:

  • Do not redirect the user to a plans grid after in-place connection if the user has a pending plan.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This is a bug fix for a relatively new connection flow.

Testing instructions:

  • Checkout D40032-code in your sandbox
  • Ensure that your Jetpack site is sandboxing the API
  • Ensure that your Jetpack site has a pending plan. Ping @Automattic/jetpack-infinity to help with this
  • Connect your test site through the in-place connection flow
  • Ensure that you are not shown a plans grid after the connection

Proposed changelog entry for your changes:

  • Fixed a connection flow issue for sites that had a pending plan that was provisioned by a partner.

@ebinnion ebinnion added this to the 8.4 milestone Mar 6, 2020
@ebinnion ebinnion requested a review from a team March 6, 2020 23:46
@ebinnion ebinnion self-assigned this Mar 6, 2020
@jetpackbot
Copy link
Collaborator

jetpackbot commented Mar 6, 2020 •
edited
Loading

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.

Scheduled Jetpack release: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against e070e4d

gravityrail
gravityrail previously approved these changes Mar 7, 2020
View reviewed changes
Copy link
Contributor

@gravityrail gravityrail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love how simple this is. Minor, optional nit but overall this approach is great.

Also - tests?

_inc/connect-button.js Outdated
@@ -138,7 +138,7 @@ jQuery( document ).ready( function( $ ) {
},
success: function( data ) {
var siteData = JSON.parse( data.data );
jetpackConnectButton.isPaidPlan = ! siteData.plan.is_free;
jetpackConnectButton.isPaidPlan = ! siteData.plan.is_free || siteData.is_pending_plan;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you reverse the order of the RHS? in it's current state it's easy to mis-read the logic if you don't know operator precedence by heart

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reversed the order as requested. 👍

@ebinnion ebinnion force-pushed the fix/plans-redirect-pending-plan branch from bb48d91 to f86c45d Compare March 7, 2020 03:45
@ebinnion ebinnion added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Mar 7, 2020
@ebinnion
Copy link
Contributor Author

ebinnion commented Mar 7, 2020

Also - tests?

Do you know if there are existing tests that I can leverage? I didn't see any tests. But, it's also been quite a while since I've been active in the Jetpack plugin.

@gravityrail
Copy link
Contributor

gravityrail commented Mar 7, 2020 via email

kraftbj
kraftbj previously approved these changes Mar 9, 2020
View reviewed changes
@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 9, 2020
@kraftbj
Copy link
Contributor

kraftbj commented Mar 9, 2020

Need to go out with D40032-code .

@ebinnion
Copy link
Contributor Author

I've updated the patch to look for the is_pending_plan flag in siteData.options to be more in line with how other flags are handled. I will be deploying the WPCOM patch shortly.

@ebinnion ebinnion added [Status] Needs Review This PR is ready for review. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 11, 2020
@ebinnion
Copy link
Contributor Author

D40032-code is now deployed.

@github-actions
Copy link
Contributor

Howdy! The Jetpack team has disappeared for a few days to a secret island lair to concoct new ways to make Jetpack one hundred billion percent better. As a result, your Pull Request may not be reviewed right away. Do not worry, we will be back next week to look at your work! Thank you for your understanding.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 12, 2020
jeherve
jeherve approved these changes Mar 12, 2020
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests well, it should be good to merge!

@gravityrail gravityrail merged commit 6ab22b4 into master Mar 12, 2020
@gravityrail gravityrail deleted the fix/plans-redirect-pending-plan branch March 12, 2020 13:28
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 12, 2020
jeherve added a commit that referenced this pull request Mar 20, 2020
@jeherve
Changelog: add #14916
2f89ffd
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
8.4 Changelog (#15075)
0eb0b35 
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve approved these changes

@gravityrail gravityrail gravityrail left review comments

@kraftbj kraftbj kraftbj left review comments

Assignees

@ebinnion ebinnion

Labels
Connect Flow Connection banners, buttons, ... Jetpack Start
Projects
None yet
Milestone
8.4
Development

Successfully merging this pull request may close these issues.
6 participants
@ebinnion @jetpackbot @gravityrail @kraftbj @jeherve @matticbot