Conversation
ockham
left a comment
There was a problem hiding this comment.
Looks good, but see Automattic/wp-calypso#21154 (review)
|
@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. |
| $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'] ) ) { |
There was a problem hiding this comment.
Can this option be a Jetpack Option instead or prefixed with jetpack_?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Got it! Thanks for explaining @ebinnion do you have an opinion about how this impact the cleanup process on deactivation ?
There was a problem hiding this comment.
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().
AnnaMag
left a comment
There was a problem hiding this comment.
Thanks for keeping a close eye on how we port the existing functionality 💯
LGTM 👍
* 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.
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