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

Debugger: Add plan info to debug info #6171

Merged
merged 5 commits into from
Feb 17, 2017
Merged

Debugger: Add plan info to debug info #6171

merged 5 commits into from
Feb 17, 2017

Conversation

kraftbj
Copy link
Contributor

@kraftbj kraftbj commented Jan 25, 2017

To aid HEs in debugging issues.

@kraftbj kraftbj added General [Status] Needs Review This PR is ready for review. [Type] Bug When a feature is broken and / or not performing as intended labels Jan 25, 2017
@kraftbj kraftbj added this to the 4.5.1 milestone Jan 25, 2017
jeherve
jeherve approved these changes Jan 26, 2017
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.

Works well in my tests! LGTM! 🚢

@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 Jan 26, 2017
georgestephanis
georgestephanis reviewed Jan 26, 2017
View reviewed changes
class.jetpack-debugger.php Outdated
case 'jetpack_premium_monthly' :
$plan = 'Premium';
break;
case 'jetpack_personal' :
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, is there a reason that we're ordering Personal after Premium? Wouldn't it make more sense to order from lowest to highest?

Also, would it be at all useful for happiness to be able to easily tell if it's monthly/annual, and the expiration date? Or do y'all just care what it is now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just the plan that the site thinks it has is enough for us. I had Personal first for performance (started this with if statements and Personal, I'd expect, to be the most purchased plan.

The KISS update negates the need for that though.

@samhotchkiss
Copy link
Contributor

I super simplified this... I don't see any reason for us to modify the plan names-- why not just pass the plan name string? This is more futureproof and would potentially be more helpful in figuring an issue out.

cc: @kraftbj @jeherve

@samhotchkiss
Copy link
Contributor

If y'all are good with this, merge at will.

@jeherve jeherve added [Status] Needs Review This PR is ready for review. [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] Ready to Merge Go ahead, you can push that green button! [Status] Needs Review This PR is ready for review. labels Jan 27, 2017
@kraftbj
Copy link
Contributor Author

kraftbj commented Jan 29, 2017

After further reflection, I'd like to change this a bit to standardize plan type by working with

jetpack/class.jetpack.php

Line 1232 in 997a48a

if ( in_array( $plan['product_slug'], $personal_plans ) ) {
to include the detail we'd need here.

I'll tag for review tomorrow/Monday after tweaking it.

@jeherve jeherve modified the milestones: 4.5.1, 2/17 - February Jan 30, 2017
@zinigor
Copy link
Member

zinigor commented Feb 1, 2017

@kraftbj I have added supported features to the output, is that what you had in mind?

@samhotchkiss samhotchkiss removed this from the 2/17 - February milestone Feb 3, 2017
@kraftbj
Copy link
Contributor Author

kraftbj commented Feb 6, 2017

@zinigor No, I'm going to put this on hold until I have a chance to make the changes to add to the discussion.

@kraftbj kraftbj added [Status] In Progress 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 Feb 6, 2017
@kraftbj kraftbj added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Feb 14, 2017
@kraftbj kraftbj force-pushed the add/jetpack-plan-support branch from c36d231 to 95100bf Compare February 14, 2017 21:13
@kraftbj
Copy link
Contributor Author

kraftbj commented Feb 14, 2017

@zinigor I reworked it some. The thing I'm trying to balance out is simplifying what plan gets passed in (just the type of plan, annual vs monthly doesn't matter) to save the parser on our end from needing to be tweaked too much. What do you think?

jeherve
jeherve approved these changes Feb 15, 2017
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 works well in my tests! LGTM!

@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 Feb 15, 2017
georgestephanis
georgestephanis approved these changes Feb 15, 2017
View reviewed changes
Copy link
Member

@georgestephanis georgestephanis left a comment

Choose a reason for hiding this comment

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

Minor thought -- but I'd be fine merging it as is.

class.jetpack-debugger.php Outdated
@@ -18,6 +18,12 @@ private static function is_jetpack_support_open() {
}
}

private static function what_jetpack_plan() {
$plan = Jetpack::get_active_plan();
$plan = $plan['class'];
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a fallback value if $plan['class'] is empty? Like if someone adds a new plan down the road and forgets to include a class?

Copy link
Member

Choose a reason for hiding this comment

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

I agree. The way get_active_plan() is written it makes it possible for this value to not be set, and in that case would throw a notice. Let's add a fallback of unknown if not set to future-proof.

@dereksmart
Copy link
Member

@kraftbj pushed 261a114 to address george's comment, which I agree with. Feel free to merge if you're happy.

@samhotchkiss samhotchkiss merged commit f17463c into master Feb 17, 2017
@samhotchkiss samhotchkiss deleted the add/jetpack-plan-support branch February 17, 2017 03:09
@samhotchkiss samhotchkiss removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 17, 2017
jeherve added a commit that referenced this pull request Feb 21, 2017
@jeherve
Changelog: add #6171
f0e0798
dereksmart pushed a commit that referenced this pull request Feb 28, 2017
@jeherve @dereksmart
Jetpack 4.7: Changelog (#6331)
a9adf86 
* Changelog: update stable tag and move changelog to changelog.txt

Also remove old releases from readme.txt to keep the changelog tab short.

* Changelog: add #5883

Also update the filter's docblock to match new version.

* Changelog: add #5938

* Changelog: add #6298

* Changelog: add #3405

* Changelog: add #5941

* Changelog: add #6239

* Changelog: add #6281

* Changelog: add #6303

* Changelog: add #6018

* Changelog: add #6300

* Changelog: add #6296

* Changelog: add #6130

* Changelog: add #6292

* Readme: remove extra "on".

* Changelog: add #6307

* Changelog: add #3297

* Changelog: add #6275

* Changelog: add #6321

* Changelog: add #6297

* Readme: update the support forum link anchor.

Anchor changed when WordPress.org forums were updated to bbPress 2

* Readme: update list of a12s, it wasn't up to date anymore!

* Changelog: add #6338

* Changelog: add #6337

* Changelog: add #6335

* Changelog: add #6333

* Testing List: first version of the 4.7 testing list.

* Changelog: add #6332

* Changelog: add #6325

* Changelog: add #6326

* Changelog: add #6339

* Changelog: add #6342

* Changelog: add #6343

* Changelog: add #6346

* Changelog: add #6347

* Changelog: add #6279

* Changelog: add #6306

* Changelog: add #6312

* Changelog: add #6316

* Changelog: add #6171

* Changelog: add #6317

* Changelog: add #6246

* Changelog: add #6263

* Changelog: add #4220

* Changelog: add #5888

* Changelog: add #3406

* Changelog: add #3637

* Changelog: add #6320

* Changelog: add #5992

* Changelog: add #6322

* Changelog: add #6324

* Changelog: add #6352

* Changelog: add #6355

* Changelog: add #6360

* Changelog: add #6362

* Changelog: add #6369, #6382

* Changelog: add #6370

* Changelog: add #6375

* Changelog: add #6383

* Changelog: add #6384

* Changelog: add #6386

* Changelog: add #6395

* Changelog: add #6403

* Changelog: add #6406

* Changelog: add #6418

* Changelog: add #6419

* Changelog: add #6434

* Changelog: add #6446

* Changelog: add #6006

* Changelog: add #6096

* Changelog: add #6399

* Changelog: fix typo.

@see #6331 (comment)

* Changelog: add #6440

* Changelog: add #6443

* Changelog: add #6445

* Changelog: add #6463

* Changelog: add #6468

* Changelog: add #6471

* Changelog: add #6474

* Changelog: add #6480

* Changelog: add #6497

* Changelog: add #6499

* Changelog: add #6514

* Changelog: add #6267

* Changelog: add #5940

* Changelog: add #6492

* Changelog: add #5281

* Changelog: add #6327

* Changelog: add #6451

* Changelog: add #6525

* Changelog: add #6530
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

@georgestephanis georgestephanis georgestephanis approved these changes

@dereksmart dereksmart dereksmart approved these changes

Assignees
No one assigned
Labels
General [Pri] BLOCKER [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kraftbj @samhotchkiss @zinigor @dereksmart @jeherve @georgestephanis