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.

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

REST API: JPO - save site type #8431

Merged
merged 2 commits into from Jan 4, 2018
Merged

REST API: JPO - save site type #8431

merged 2 commits into from Jan 4, 2018

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Dec 29, 2017

This updates the JPO onboarding request to save the site type in the onboarding request. Previously, the site type was not taken into consideration. Option name is consistent with the original JPO plugin (see https://github.com/Automattic/jetpack-onboarding/blob/master/class.jetpack-onboarding-end-points.php#L7).

To test:

Follow the instructions in Automattic/wp-calypso#21154

@tyxla tyxla added [Feature] WP REST API [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Dec 29, 2017
@tyxla tyxla self-assigned this Dec 29, 2017
@tyxla tyxla requested a review from a team as a code owner December 29, 2017 11:11
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

@oskosk oskosk added this to the 5.8 milestone Jan 3, 2018
@tyxla
Copy link
Member Author

tyxla commented Jan 3, 2018

@ockham replied here: Automattic/wp-calypso#21154 (comment)

@oskosk we'd appreciate a review of this one, it should be quick and straightforward but let us know if we can aid anyhow.

@@ -964,6 +964,12 @@ private function _process_onboarding( $data ) {
$site_title = get_option( 'blogname' );
$author = get_current_user_id() || 1;

if ( ! empty( $data['siteType'] ) ) {
if ( ! ( update_option( 'jpo_site_type', $data['siteType'] ) || get_option( 'jpo_site_type' ) == $data['siteType'] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this option be a Jetpack Option instead or prefixed with jetpack_?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately no, because we're mirroring how the plugin used to work, and mirroring the exact same option that it used to store the site type, see https://github.com/Automattic/jetpack-onboarding/blob/master/class.jetpack-onboarding-end-points.php#L7

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! Thanks for explaining @ebinnion do you have an opinion about how this impact the cleanup process on deactivation ?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to remove jpo_site_type on deactivation, then I'd add jpo_site_type to the array returned in Jetpack_Options::get_all_wp_options().

Jetpack_Options::delete_all_known_options() is called when Jetpack is uninstalled, and it will all Jetpack options that are stored in jetpack_options and jetpack_private_options as well as the options returned from Jetpack_Options::get_all_wp_options().

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @ebinnion! Added in 74309d2.

Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

LGTM and tests well

Copy link
Contributor

@AnnaMag AnnaMag left a comment

Choose a reason for hiding this comment

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

Thanks for keeping a close eye on how we port the existing functionality 💯
LGTM 👍

@tyxla
Copy link
Member Author

tyxla commented Jan 4, 2018

@oskosk in 74309d2 we've updated how we clean up the newly introduced option. This one is ready to 🚢 from my perspective, could you please merge it if it looks good to you too? Thanks!

@oskosk oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jan 4, 2018
@oskosk oskosk merged commit bc4f914 into master Jan 4, 2018
@oskosk oskosk deleted the add/jpo-save-site-type branch January 4, 2018 12:42
jeherve added a commit that referenced this pull request Jan 25, 2018
zinigor pushed a commit that referenced this pull request Jan 30, 2018
* Changelog 5.8: create base for changelog.

* Update 5.8 release post link

* fix 5.8 release date

* Updates to plugin description

* Changelog: add #8499

* Changelog: add #8506

* Changelog: add #8509

* Changelog: add #8516

* Changelog: add #8517

* Changelog: add #8523

* Changelog: add #8547

* Changelog: add #8496

* Changelog: add #8584

* Changelog: add #8595

* Changelog: add #8445

* Changelog: add #8431

* Changelog: add #8284

* Changelog: add #8270

* Changelog: add #8124

* Changelog: add #8581

* Changelog: add #8463

* Changelog: add #8568 (#8646)

* Updates to testing list and changelog

* Changelog: add #8443

* Changelog: add #8459

* Changelog: add #8469

* Changelog: add #8464

* Changelog: add #8478 and #8479

* Changelog: add #8483

* Changelog: add #8488

* Changelog: add #8513

* Changelog: add #8555

* Changelog: add #8565

* Changelog: add #8601

* Changelog: add #8612

* Changelog: add first pass at Search items.

* Changelog: add more info to help test Search.

* Changelog: add #8144

* Changelog: add #8313

* Changelog: add #8419

* Changelog: add #8465

* Changelog: add #8515

* Changelog: add #8587

* Changelog: add #8591

* Changelog: add #8659

* Changelog: add #8661

* Changelog: add #8671

* Changelog: add 5.7.1 to archived changelog too.

* Reverted changes to readme, removed entry about backups.
@tyxla tyxla removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants