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

Jetpack Plan: Updated cached plan via common method; fix issues with plan caching #11626

Merged
merged 5 commits into from
Mar 22, 2019
Merged

Jetpack Plan: Updated cached plan via common method; fix issues with plan caching #11626

merged 5 commits into from
Mar 22, 2019

Conversation

ebinnion
Copy link
Contributor

@ebinnion ebinnion commented Mar 20, 2019 •
edited
Loading

Changes proposed in this Pull Request:

  • Add tests for Jetpack_Plan class
  • Refactor plans logic so that updates to the jetpack_active_plan option go through the Jetpack_Plan class
  • Change how the active plan is cached so that it is a static variable on the Jetpack_Plan cache and add some functionality to clear that cache when the option is updated.
  • Fix an issue where the active plan might not be updated if there was a mismatch in cache. See p1HpG7-5GG-p2 for backstory.

Testing instructions:

  • Checkout branch
  • For each of the plans, including free, check the following:
    • Load up the My Plan view (/wp-admin/admin.php?page=jetpack#/my-plan` and ensure the plan shows correctly
    • Ensure that there are no errors or warnings in logs
    • Run wp option delete jetpack_active_plan and then reload admin page. Ensure plan is reflected correctly
    • After waiting some seconds, ensure that you get a new plan welcome modal

Proposed changelog entry for your changes:

  • Fix an issue where the Jetpack plan was not always reflected correctly.

@ebinnion ebinnion added [Status] Needs Review This PR is ready for review. Plans labels Mar 20, 2019
@ebinnion ebinnion added this to the 7.2 milestone Mar 20, 2019
@ebinnion ebinnion self-assigned this Mar 20, 2019
@ebinnion ebinnion requested review from oskosk, rcoll and a team March 20, 2019 15:45
@ebinnion ebinnion added [Status] In Progress [Status] Needs Review This PR is ready for review. and removed [Status] Needs Review This PR is ready for review. [Status] In Progress labels Mar 20, 2019
@jetpackbot
Copy link
Collaborator

jetpackbot commented Mar 20, 2019 •
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 2, 2019.
Scheduled code freeze: March 26, 2019

Generated by 🚫 dangerJS against 8313663

@kraftbj kraftbj added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Mar 20, 2019
@ebinnion ebinnion force-pushed the update/plan-refactor-logic-to-set-plan branch from 7c4dc90 to d6eab2a Compare March 20, 2019 19:44
@ebinnion ebinnion force-pushed the update/plan-refactor-logic-to-set-plan branch from d6eab2a to cc04c7e Compare March 20, 2019 19:45
@ebinnion ebinnion added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Mar 20, 2019
@ebinnion ebinnion added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Mar 20, 2019
ebinnion
ebinnion commented Mar 20, 2019
View reviewed changes
tests/php/test_class.jetpack-plan.php
);
}

private function get_free_plan() {
Copy link
Contributor Author

@ebinnion ebinnion Mar 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Arguably, we could probably minimize the array in this method and the next since our current logic focuses on the product slug. BUT, we may want to write tests that rely on the other information in these arrays in the near future. So, I have a slight preference to leave as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense and sounds good.

@ebinnion ebinnion changed the title Jetpack Plan: Factor out logic for actually setting the plan option Jetpack Plan: Updated cached plan via common method; fix issues with plan caching Mar 21, 2019
jeherve
jeherve previously requested changes Mar 21, 2019
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 looks good. I only have a minor comment.

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Mar 21, 2019
@ebinnion ebinnion requested a review from jeherve March 21, 2019 20:14
@ebinnion ebinnion dismissed jeherve’s stale review March 21, 2019 20:14

Addressed in a comment above.

@ebinnion ebinnion added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 21, 2019
kraftbj
kraftbj approved these changes Mar 21, 2019
View reviewed changes
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

It worked as described for me. I didn't see any warnings or errors, expected plan result provided.

@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 21, 2019
@kraftbj kraftbj merged commit 30c3fec into master Mar 22, 2019
@ghost ghost removed [Status] Needs Changelog [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 22, 2019
@kraftbj kraftbj deleted the update/plan-refactor-logic-to-set-plan branch March 22, 2019 15:27
kraftbj added a commit that referenced this pull request Mar 26, 2019
@kraftbj
CL #11626
5b574a8
kraftbj added a commit that referenced this pull request Mar 27, 2019
@kraftbj
Add initial 7.2 changelog to master (#11690)
2227bfd 
* Initial Changelog for 7.2

* Testing list: add mention of IE11 testing

* Initial Changelog for 7.2

* Testing list: add mention of IE11 testing

* Add CL for #11224

* Add CL for #11426

* Add CL for #11442

* Add testing instructions for #11224

* Add CL for #11451

* Reclassify CL item

* Add testing instructions for #11451

* Add CL for #11486

* Add CL for #11418

* Add CL for #11524

* Add CL and testing instructions for #11449

* Add CL for #11460

* Add CL for #11520 and #11582

* Add CL for #11531

* Add CL #11644

* Add testing instructions for #11644

* Add testing instructions for #11644

* Add CL for #11618

* Uniform changelog lines

* CL #11679

* CL #11661

* CL #11654

* CL #11645

* CL #11643

* CL #11636

* CL #11635 and for other PHPCS commits

* CL #11627

* CL #11626

* CL #11598

* CL #11596

* Remove nested items for shortcopy. I don't believe the detailed list is helpful

* CL #11570

* CL #11569

* CL #11560

* CL #11558

* CL #11555

* CL #6704

* CL #11298

* CL #11324

* CL #11443

* CL #11484

* CL #11516

* CL #11529

* Expand Ads block enhancement CL item
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kraftbj kraftbj kraftbj approved these changes

@oskosk oskosk Awaiting requested review from oskosk

@rcoll rcoll Awaiting requested review from rcoll

@jeherve jeherve Awaiting requested review from jeherve

Assignees

@ebinnion ebinnion

Labels
Plans [Type] Bug When a feature is broken and / or not performing as intended [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
7.2
Development

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