REST API: add functionality for saving Business Address widget in JPO (alternative) #8523

AnnaMag · 2018-01-14T23:18:48Z

An alternative approach to #8480 and #8426.

Changes proposed in this Pull Request:

adds functions required to inject a Business Address widget or update the existing one,
saves Business Address data in an option,
enables widget module if inactive ( throws an error on failure ).

Testing instructions:

Follow the instructions REST API: add functionality for saving Business Address in JPO #8426 tested on a site with a Business Plan ( or temporarily set isBusiness to true in jetpack-onboarding/main.jsx in Calypso).
To test that it works for the initially inactive widgets module:
use wp jetpack module deactivate to deactivate the module and follow the previous instructions. After, verify that the module is listed as active in wp jetpack module list and that the widget has been set correctly.
Verify that the jpo_business_address option is set i.e. using wp option get jpo_business_address.

This commit adds the functions required to inject a Business Address widget or update an exisiting one in the remote site during the JPO flow. Alternative approach to using Jetpack_Widgets.

tyxla

Looks awesome for a first version! I think this should be the easier and quicker way to go.

Left some feedback on things we need to iterate on from the JPO plugin version in order to adapt it to fit the Jetpack codebase.

tyxla · 2018-01-15T09:11:54Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

@@ -1053,6 +1053,10 @@ private function _process_onboarding( $data ) {
 			}
 		}

+		if ( isset( $data['businessAddress'] ) ) {
+			self::add_business_address( $data );


We still need to handle the errors in the same way that we handle them for the rest of the onboarding settings (adding them to the error array). So you might want to update add_business_address() to return either an array of errors, a WP_Error object, or true on success; and if there are any errors, merge them with $errors.

tyxla · 2018-01-15T09:13:03Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

@@ -1066,6 +1070,126 @@ private function _process_onboarding( $data ) {
 			: join( ', ', $error );
 	}

+	static function get_first_sidebar() {


How about just moving this one the widgets lib and using it statically from there?

tyxla · 2018-01-15T09:13:48Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

+ 	 *
+ 	 * @param string  $sidebar ID of the sidebar to which the widget will be added.
+	 */
+	static function insert_widget_in_sidebar( $widget_id, $widget_options, $sidebar ) {


How about just moving this one the widgets lib and using it statically from there?

tyxla · 2018-01-15T09:13:54Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

+ 	 *
+ 	 * @param string  $sidebar ID of the sidebar to which the widget will be added.
+	 */
+	static function update_widget_in_sidebar( $widget_id, $widget_options, $sidebar ) {


How about just moving this one the widgets lib and using it statically from there?

tyxla · 2018-01-15T09:16:39Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

+			} else {
+				self::update_widget_in_sidebar( 'widget_contact_info', $widget_options, $first_sidebar );
+			}
+			wp_send_json_success( array( 'updated' => true ) );


We're not supposed to call wp_send_json_success here, because it'll end the request in an unexpected way. Jetpack_Core_API_Data::update_data() already handles this by either calling a rest_ensure_response() on success, or returning a WP_Error object on failure, so we only need to pass on the errors, if any, and success messages, if any (following the pattern that _process_onboarding already implements).

tyxla · 2018-01-15T09:17:50Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

+			$sidebars_widgets[ $sidebar ] = array();
+		}
+		$sidebars_widgets[ $sidebar ][] = $widget_id . '-' . $next_key;
+		// Add the new widget instance


Nit: Adding a newline before each of those // comments, will make the methods you introduce more readable.

tyxla · 2018-01-15T09:18:53Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

+		if ( $first_sidebar ) {
+			$title = $data['businessAddress'][ 'name' ];
+			$address =
+					$data[ 'businessAddress' ][ 'city' ] . ' ' .


Indentation appears a little off here. Do we want to make this one a little smarter, by grabbing all values, then filtering out any empty ones, and then building the string?

Where are you prettify? 😆

tyxla · 2018-01-15T09:20:50Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

+		$first_sidebar = self::get_first_sidebar();
+
+		if ( $first_sidebar ) {
+			$title = $data['businessAddress'][ 'name' ];


Code style: we don't wrap specific array keys with leading/trailing spaces, so instances like [ 'name' ] need to become like this: ['name'].

Contrary to this, variable keys need to have trailing/leading spaces, like this: [ $key ].

tyxla · 2018-01-15T09:21:55Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

+				'email' => ''
+			);
+
+			if ( ! self::has_business_address_widget( $first_sidebar ) ) {


Do we also want in this PR to store the business address in an option to make it available for the REST API? Should be a quick one.

tyxla · 2018-01-15T09:22:34Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

+			if ( ! self::has_business_address_widget( $first_sidebar ) ) {
+				self::insert_widget_in_sidebar( 'widget_contact_info', $widget_options, $first_sidebar );
+			} else {
+				self::update_widget_in_sidebar( 'widget_contact_info', $widget_options, $first_sidebar );


Let's make sure to capture errors from those methods and pass them on to $errors.

tyxla · 2018-01-15T11:30:06Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

+				'email' => ''
+			);
+
+			if ( ! self::has_business_address_widget( $first_sidebar ) ) {


We need to make sure the widgets module is active, you can borrow the code from here:

https://github.com/Automattic/jetpack/pull/8481/files#diff-2ccec9069213650359171727de9a2471R1048

We already have that in the pipeline, I believe #8481. Will need to be adapted though as it served the other approach.

How do you feel about handling these in the same PR? It's only those 4 lines.

Yes, we can 👍 closing it then.

…usiness Ad. widget style fixes: separating comments, fixing spacing, adding f-ns definitions. pass only relevant part of data to the function handling business address. error handle: specify default values to handle case where no errors are thrown.

AnnaMag · 2018-01-15T22:55:13Z

Thanks for reviewing @tyxla. Nice sugestions!

The patch has been adapted 👍 I'd appreciate another look when you get a chance. Thanks!

Add to known options.

This funcionality is generic and not directly related to the logic of inserting a business address widget.

…aved option. Done to avoid passing empty fields to the widget. This commit also sanitizes the option storing widget content.

AnnaMag · 2018-01-16T22:13:07Z

Feedback incorporated 👍 Thanks @tyxla!

tyxla

This works like a charm, awesome work 👍 ❤️

Left several comments, but all of them are super minor styling things, nothing blocking.

tyxla · 2018-01-17T08:32:59Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

+			$widgets_module_active = Jetpack::activate_module( 'widgets', false, false );
+		}
+		if ( ! $widgets_module_active ) {
+			return new WP_Error( 'module_activation_failed', 'Failed to activate module.', 400 );


Maybe worth clarifying which module we failed to activate: Failed to activate the widgets module.

tyxla · 2018-01-17T08:33:42Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

+		if ( $first_sidebar ) {
+			$title = isset( $address['name'] ) ? sanitize_text_field( $address['name'] ) : '';
+			$street = isset( $address['street'] ) ? sanitize_text_field( $address['street'] )
+			: '';


Let's keep this on the same line.

tyxla · 2018-01-17T08:41:27Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

+			);
+
+			$widget_inserted = '';
+			$widget_updated = '';


Why do we need 2 separate variables for handling insert and update if we return the same error message?

tyxla · 2018-01-17T08:41:49Z

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php

+		}
+
+		// No sidebar to place the widget
+		return new WP_Error( 'sidebar_failed', 'No sidebar.', 400 );


I'd rephrase sidebar_failed to sidebar_not_found.

tyxla · 2018-01-17T08:44:02Z

_inc/lib/widgets.php

+	}
+
+	/**
+	 * Update the content of an exisitng widget in a given sidebar.


We've got a typo here: exisitng

tyxla · 2018-01-17T08:44:18Z

_inc/lib/widgets.php

+ 	 * @param string $sidebar ID of the sidebar to which the widget will be added.
+ 	 *
+ 	 * @return WP_Error|true True when data has been updated correctly,
+ 	 * error otherwise.


Let's keep this on the same line.

tyxla · 2018-01-17T08:44:25Z

_inc/lib/widgets.php

+ 	 * @param string $sidebar ID of the sidebar to which the widget will be added.
+ 	 *
+ 	 * @return WP_Error|true True when data has been saved correctly,
+ 	 * error otherwise.


Let's keep this on the same line.

tyxla · 2018-01-17T08:45:08Z

_inc/lib/widgets.php

+		$widget_key = false;
+		foreach ( $sidebars_widgets[ $sidebar ] as $widget ) {
+			if ( strpos( $widget, 'widget_contact_info' ) !== false ) {
+				$widget_key = absint( str_replace( 'widget_contact_info-', '', $widget ) );


Should we use $widget_id instead of 'widget_contact_info' here?

tyxla · 2018-01-17T08:54:05Z

@dereksmart @zinigor can we please get a review of this one? Thank you!

Fixes typo, removes redundant var declaration, error message or description re-write, removing redundant lines.

AnnaMag · 2018-01-17T14:49:22Z

All points addressed 👍 . Thanks for an awesome review @tyxla !

tyxla

LGTM 👍 Great work Anna ❤️

tyxla · 2018-01-17T18:50:04Z

_inc/lib/widgets.php

@@ -715,14 +714,13 @@ static function insert_widget_in_sidebar( $widget_id, $widget_options, $sidebar
 	}

 	/**
-	 * Update the content of an exisitng widget in a given sidebar.
+	 * Update the content of an exisiting widget in a given sidebar.


Should be existing instead of exisiting

🤦‍♀️

zinigor

Tested: widget successfully added with the correct address, code LGTM, but I have one small nitpick.

zinigor · 2018-01-18T20:14:01Z

_inc/lib/widgets.php

+
+		// Store updated sidebars, widgets and their instances
+		if ( ! ( update_option( 'sidebars_widgets', $sidebars_widgets ) )
+		|| ( ! ( update_option( 'widget_' . $widget_id, $widget_instances ) ) ) )  {


Minor nitpick: can we please have the if indented this way for readability?

if ( condition || condition2 ) {

AnnaMag · 2018-01-18T20:44:40Z

Thanks for the review @zinigor! Styling addressed 👍
Could you have another look, please? Feel free to merge :)

…ment.

zinigor · 2018-01-18T20:58:24Z

Thanks! Please merge once Travis passes.

* 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.

AnnaMag added [Status] In Progress [Feature] WP REST API labels Jan 14, 2018

AnnaMag self-assigned this Jan 14, 2018

AnnaMag requested a review from a team as a code owner January 14, 2018 23:18

AnnaMag force-pushed the update/jpo-enable-widgets-no-lib branch 2 times, most recently from a05716e to 6d845d0 Compare January 14, 2018 23:41

JPO: extend JPO endpoint to insert and update Business Address widget.

6d845d0

This commit adds the functions required to inject a Business Address widget or update an exisiting one in the remote site during the JPO flow. Alternative approach to using Jetpack_Widgets.

AnnaMag requested review from tyxla and ockham January 14, 2018 23:46

AnnaMag added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Jan 14, 2018

tyxla reviewed Jan 15, 2018

View reviewed changes

tyxla added [Status] In Progress and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 15, 2018

AnnaMag added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 15, 2018

tyxla added this to the 5.8 milestone Jan 15, 2018

This was referenced Jan 15, 2018

Widget housekeeping: extend functionality and add check for existing data upon update #8480

Closed

REST API: add functionality for saving Business Address in JPO #8426

Closed

tyxla reviewed Jan 15, 2018

View reviewed changes

AnnaMag mentioned this pull request Jan 15, 2018

REST API: enable widgets module when inserting Business Address widget during JPO #8481

Closed

AnnaMag force-pushed the update/jpo-enable-widgets-no-lib branch 2 times, most recently from bfa5b5d to 23dd7c7 Compare January 15, 2018 13:36

AnnaMag added 2 commits January 15, 2018 13:38

JPO: add error handling to processing Business Address widget.

23dd7c7

AnnaMag added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Jan 15, 2018

AnnaMag force-pushed the update/jpo-enable-widgets-no-lib branch from 4e16702 to 370809e Compare January 15, 2018 22:54

JPO: save Business address data in an option.

a4d8fe2

Add to known options.

AnnaMag force-pushed the update/jpo-enable-widgets-no-lib branch from bcd0652 to 217ee22 Compare January 16, 2018 21:58

AnnaMag added 2 commits January 16, 2018 22:00

JPO: move function fetching first active sidebar to the widget lib.

217ee22

This funcionality is generic and not directly related to the logic of inserting a business address widget.

JPO: filter non-empty fields when saving the BA widget and sanitize s…

7b88f5a

…aved option. Done to avoid passing empty fields to the widget. This commit also sanitizes the option storing widget content.

tyxla approved these changes Jan 17, 2018

View reviewed changes

tyxla requested review from zinigor and dereksmart January 17, 2018 08:53

JPO: styling corrections from reviews.

6ee7069

Fixes typo, removes redundant var declaration, error message or description re-write, removing redundant lines.

tyxla approved these changes Jan 17, 2018

View reviewed changes

JPO: typo correction exisiting->existing.

aec52b8

tyxla mentioned this pull request Jan 18, 2018

REST API: Include business address in the JPO GET endpoint #8558

Merged

ockham mentioned this pull request Jan 18, 2018

JP Onboarding: Prepopulate Business Address Fields Automattic/wp-calypso#21645

Merged

zinigor requested changes Jan 18, 2018

View reviewed changes

JPO: addressing style nit- improve readibility in a conditional state…

d1cfbfe

…ment.

zinigor approved these changes Jan 18, 2018

View reviewed changes

zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 18, 2018

AnnaMag merged commit 4531d28 into master Jan 18, 2018

matticbot added the [Status] Needs Changelog label Jan 18, 2018

AnnaMag deleted the update/jpo-enable-widgets-no-lib branch January 18, 2018 21:18

tyxla removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 18, 2018

AnnaMag mentioned this pull request Jan 23, 2018

JPO: skip Business Address widget instance and option update if the content has not changed. #8596

Merged

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Jan 25, 2018

jeherve added a commit that referenced this pull request Jan 25, 2018

Changelog: add #8523

b1bf0cb

REST API: add functionality for saving Business Address widget in JPO (alternative) #8523

REST API: add functionality for saving Business Address widget in JPO (alternative) #8523

Conversation

AnnaMag commented Jan 14, 2018 • edited Loading

Changes proposed in this Pull Request:

Testing instructions:

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaMag Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaMag commented Jan 15, 2018

AnnaMag commented Jan 16, 2018

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla commented Jan 17, 2018

AnnaMag commented Jan 17, 2018

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zinigor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaMag commented Jan 18, 2018

zinigor commented Jan 18, 2018

AnnaMag commented Jan 14, 2018 •

edited

Loading

AnnaMag Jan 15, 2018 •

edited

Loading