REST API: add functionality for saving Business Address in JPO by AnnaMag · Pull Request #8426 · Automattic/jetpack

AnnaMag · 2017-12-28T13:13:37Z

Depends on #8480.

Save Business Address in a sidebar widget during the JPO flow.

Changes proposed in this Pull Request:

This patch extends the onboarding endpoint to inject and subsequently update Business Address info as a sidebar widget. It is not included in the current JPO version.

Testing instructions:

Checkout current master in your local Calypso
Checkout this branch on your JP sandbox
Go to https://YourJetpackSandbox.com/wp-admin/admin.php?page=jetpack&action=onboard&calypso_env=development, where YourJetpackSandbox.com is the domain of your JP sandbox
Skip through the flow till the Add a business address step
Add input data and move to the Next step
Go to https://YourJetpackSandbox.com/wp-admin/widgets.php and/or your site and verify you see the newly created BA widget in the list of widgets and a post sidebar
Go back to Add a business address and modify the BA input
Verify that changes apply on your JP site

Note: enabling widget module when not done so is a separate PR

eliorivero · 2017-12-28T15:25:49Z

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

Since there are only two keys, the lines from sidebar option fetch up to the loop could be written as

$active_sidebars = get_option( 'sidebars_widgets', array() ); unset( $active_sidebars[ 'wp_inactive_widgets' ], $active_sidebars[ 'array_version' ] );

The unset won't fail if the array item is not set.

It could also be written with an uglier one liner that ends up being longer :D

$active_sidebars = array_diff_key( get_option( 'sidebars_widgets', array() ), array_flip( array( 'wp_inactive_widgets', 'array_version' ) ) );

I settled for the first option to aid readability 👍 Thank a lot!

Using Jetpack_Widgets does not allow wp_inactive_widgets, so there is in fact a single key :)

Implement saving widget using activate and update `Jetpack_Widgets` functions to inject the widget onto the page and update its content.

code cosmetics as we only have 2 keys here, no changes in functionality.

`set_widget_settings` updates options without checking whether they exist, so this patch adds such a check `update_widget` unnecessarily adds widget to a sidebar patch adds helper function to look for a widget in a given sidebar by its `id_base`.

tyxla

This worked well upon initial testing 👍

Let's finish and merge #8480 and rebase this one against it.

Also, left some feedback, but looks like a really solid start 👍

tyxla · 2018-01-08T12:26:36Z

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

+			if ( empty( $active_sidebars ) ) {
+				return false;
+			}
+			return array_shift( array_keys( $active_sidebars ) );


This will throw a notice, because array_shift also updates the original referenced array (and there is no reference here):

PHP Notice: Only variables should be passed by reference in .../jetpack/_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php on line 1093

tyxla · 2018-01-08T12:33:13Z

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

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


Order seems a little weird to me - city, state, street, zip. In the original JPO plugin it was street, city, state, zip - https://github.com/Automattic/jetpack-onboarding/blob/master/class.jetpack-onboarding-end-points.php#L708.

tyxla · 2018-01-08T12:34:36Z

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

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


Do we want to store $data[ 'businessAddress' ] in a variable to make the code a little shorter by accessing all those fields with less code?

tyxla · 2018-01-08T12:35:38Z

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

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


Currently, we don't validate the presence of name, street or any of the others before using it; so we might end up using undeclared variables.

tyxla · 2018-01-08T12:38:41Z

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

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


Do we need those wp_unslash? Currently they might remove any backslashes that we have in the address.

tyxla · 2018-01-08T12:47:12Z

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

+			return array_shift( array_keys( $active_sidebars ) );
+		}
+
+		public function has_business_info_widget( $sidebar ) {


Can we make this a method in Jetpack_Widgets that accepts a widget base ID (widget_contact_info in our case)? Could be useful, and would be the right place to keep this code IMHO.

Also, can we use something from Jetpack_Widgets to make this one simpler? get_widget_by_id_base() looks like a good candidate.

@tyxla, I see the point, and at the same time thinking that it is pretty much a JPO-specific method. Are there other contexts in could be reused if defined in a Jetpack module? 🤔

No usages right now AFAIK, but it doesn't cost anything (other than just adding a parameter for the widget ID base) to do it, and in case we decide to use it in the future, it will be easier and will not require refactor PRs like this one #8464.

tyxla · 2018-01-08T12:49:05Z

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

+		}
+
+		public function update_widget( $id_base, $sidebar, $position, $widget_options ) {
+


Unnecessary whitespace in the beginning of a block.

tyxla · 2018-01-08T12:49:40Z

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

+			return false;
+		}
+
+		public function update_widget( $id_base, $sidebar, $position, $widget_options ) {


Should this method be in _inc/lib/widgets.php instead? Guess we should figure another name for it? update_widget_by_id_base maybe?

I put it in a separate method when at the beginning of getting it up and running the code was much longer. Looking at it now, I reckon it is not needed in fact. Slightly re-written it becomes a two-liner and can be used in-place. Do you agree?

Not sure what exactly you mean - could you please elaborate?

I removed that function and included: https://github.com/Automattic/jetpack/pull/8426/files#diff-2ccec9069213650359171727de9a2471R1073.

tyxla · 2018-01-08T12:51:03Z

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

+
+			$widget = Jetpack_Widgets::get_widget_by_id_base( $id_base );
+			$widget_id = $widget['id'];
+			Jetpack_Widgets::update_widget($widget_id,  $sidebar, $position, $widget_options );


Two consecutive spaces after $widget_id,.

tyxla · 2018-01-08T12:51:18Z

_inc/lib/widgets.php

 		return $found;
 	}

+	/**


Left some feedback in the other PR (#8480) about this one.

AnnaMag · 2018-01-08T16:42:37Z

Thanks for the reviews @tyxla 🙏

I see that a lot of the comments are concerned with what to keep in the endpoint and what to move to module's methods. I kept a lot in the former, not being sure how much of that extends beyond JPO itself. I'll have another 👀 after we merge the previous related PRs.

tyxla · 2018-01-09T21:25:14Z

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

+				$position = '0';
+				$id_base = 'widget_contact_info';
+
+				$has_ba_widget = self::has_business_info_widget( $first_sidebar );


@AnnaMag one thing to consider here: for the GET endpoint, we'll need to be able to:

Know whether business address has been set or not

Retrieve all those business address fields, in order to be able to return them in the endpoint.

Right now, with your PR, for 1, we can use self::has_business_info_widget( $first_sidebar ), but for 2. we'll need something else, for example storing $data[ 'businessAddress' ] in an option. If you decide to do that, make sure to also add the option to the list of known options, in order it to be cleaned up after uninstalling JP (as done here: https://github.com/Automattic/jetpack/pull/8495/files#diff-cb166c81692a76d1a92d53cb5e80d67cR537).

As agreed on Slack let's handle it in the follow-up. As always, 💯 point.

Previous commit in the patch removed it assuming that the sidebar already contains the widget to be updated. This will not be the case in general, e.g. when placing a widget in another sidebar. This commit brings it back conditional on the check that the widget is absent from the sidebar. In addition: - it adds a helper function and leaves a couple of comments for clarity. - small code refactor to avoid declaring unnecessary variables - change error code when no sidebar is present - updates error message when no widget is present in a sidebar.'

oskosk · 2018-01-11T13:01:16Z

_inc/lib/widgets.php

There's a missing parens here

Was fixed in #8480 and now also ported here. Thanks for reviewing!

Adding a check of validity of widget`s ID base for new widgets.

…module.

The logic in the function grew smaller with time and can be used in-place in a clear way.

ockham · 2018-01-11T16:46:47Z

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

+		// Add/update business address widget
+		if ( isset( $data['businessAddress'] ) ) {
+			//grab the first sidebar and proceed if it is present
+			$first_sidebar = Jetpack_Widgets::get_first_sidebar();


This gives me

PHP Deprecated: Non-static method Jetpack_Widgets::get_first_sidebar() should not be called statically in jetpack/_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php on line 1049

Since get_first_sidebar() doesn't seem to refer to the instance ( $this), I think we can actually make it static.

ockham · 2018-01-11T17:10:06Z

I can't seem to get this to work 🙁

How I'm testing: Using the Jetpack Beta Plugin, and selecting 'Bleeding Edge' -- which at this point was 'last updated 4 hours ago', so it doesn't include #8501. Thus, I'm manually applying #8501 and this PR's diff.

The error I'm getting is the 'Unexpected Error' notice, and checking /wp-admin/widgets.php, I see that no widget was added. My browser's 'Network' tab gives me

{"code":500,"headers":[{"name":"Content-Type","value":"application\/json"}],"body":{"error":"no_response_body","message":"Server could not read response."}}

The server debug log contains (a PHP Depracted and PHP Notice aside), the following PHP Warning:

PHP Warning: Cannot modify header information - headers already sent by (output started at wp-content/plugins/jetpack-dev/_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php:1069) in wp-includes/rest-api/class-wp-rest-server.php on line 1256

ockham · 2018-01-11T20:50:57Z

Tried this now with simply checking out the add/jpo-save-business-address branch on my sandbox (per @AnnaMag's suggestion -- I was previously assuming I'd have to build JP for it to work), and making sure I'm using the corresponding update/jpo-save-business-address Calypso branch, but unfortunately, I'm still getting the same error 🙁

AnnaMag · 2018-01-15T11:30:02Z

Closing in favor of #8523.

AnnaMag added the [Status] In Progress label Dec 28, 2017

AnnaMag self-assigned this Dec 28, 2017

AnnaMag requested a review from a team as a code owner December 28, 2017 13:13

eliorivero reviewed Dec 28, 2017

View reviewed changes

AnnaMag force-pushed the add/jpo-save-business-address branch from c4715a9 to a462a65 Compare December 29, 2017 00:23

jeherve added [Feature] WP REST API Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Dec 29, 2017

AnnaMag mentioned this pull request Jan 4, 2018

Jetpack Onboarding: save input from the Business Address step Automattic/wp-calypso#21083

Merged

AnnaMag force-pushed the add/jpo-save-business-address branch 4 times, most recently from d969051 to f1dbd45 Compare January 6, 2018 23:25

AnnaMag changed the title ~~Jetpack Onboarding: add functionality for saving Business Address~~ REST API: add functionality for saving Business Address in JPO Jan 7, 2018

AnnaMag force-pushed the add/jpo-save-business-address branch from f1dbd45 to 511caeb Compare January 7, 2018 20:27

AnnaMag added 3 commits January 7, 2018 22:17

JPO: save Business Address data using Jetpack_Widgets

7254f7f

Implement saving widget using activate and update `Jetpack_Widgets` functions to inject the widget onto the page and update its content.

REST API: simplify the function accessing first available sidebar

13b6fef

code cosmetics as we only have 2 keys here, no changes in functionality.

AnnaMag removed the [Status] In Progress label Jan 7, 2018

AnnaMag requested review from ockham, oskosk and tyxla January 7, 2018 22:52

AnnaMag mentioned this pull request Jan 7, 2018

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

Closed

AnnaMag added the [Status] Needs Review This PR is ready for review. label Jan 7, 2018

AnnaMag mentioned this pull request Jan 8, 2018

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

Closed

AnnaMag force-pushed the add/jpo-save-business-address branch from 511caeb to 2006537 Compare January 8, 2018 09:55

tyxla reviewed Jan 8, 2018

View reviewed changes

tyxla reviewed Jan 9, 2018

View reviewed changes

oskosk added this to the 5.8 milestone Jan 10, 2018

AnnaMag force-pushed the add/jpo-save-business-address branch from 8b4f0b6 to aa6ccf5 Compare January 10, 2018 23:48

oskosk reviewed Jan 11, 2018

View reviewed changes

AnnaMag force-pushed the add/jpo-save-business-address branch from 3367d09 to 3cd5b3a Compare January 11, 2018 13:27

AnnaMag added 4 commits January 11, 2018 13:28

Widgets: sanitize ID base of widget to be updated.

c0d3edf

Adding a check of validity of widget`s ID base for new widgets.

Widgets: add a method to retrieve first active sidebar.

78cc80a

JPO: move method to retrieve first active sidebar to Jetpack_Widgets …

a1eadff

…module.

JPO: remove separate function to update widget to use it in place.

3cd5b3a

The logic in the function grew smaller with time and can be used in-place in a clear way.

ockham reviewed Jan 11, 2018

View reviewed changes

AnnaMag mentioned this pull request Jan 11, 2018

Update/jpo save business address cont #8503

Closed

AnnaMag added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Jan 12, 2018

AnnaMag mentioned this pull request Jan 14, 2018

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

Merged

AnnaMag closed this Jan 15, 2018

matticbot removed the [Status] In Progress label Jan 15, 2018

tyxla deleted the add/jpo-save-business-address branch January 15, 2018 11:33

		}

		public function update_widget( $id_base, $sidebar, $position, $widget_options ) {

Conversation

AnnaMag commented Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Testing instructions:

Uh oh!

eliorivero Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaMag Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaMag Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyxla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaMag Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyxla Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaMag commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaMag Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaMag Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ockham commented Jan 11, 2018

Uh oh!

ockham commented Jan 11, 2018

Uh oh!

AnnaMag commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

AnnaMag commented Dec 28, 2017 •

edited

Loading

eliorivero Dec 28, 2017 •

edited

Loading

AnnaMag Jan 7, 2018 •

edited

Loading

AnnaMag Jan 11, 2018 •

edited

Loading

AnnaMag Jan 11, 2018 •

edited

Loading

tyxla Jan 8, 2018 •

edited

Loading

AnnaMag commented Jan 8, 2018 •

edited

Loading

AnnaMag Jan 9, 2018 •

edited

Loading

AnnaMag Jan 11, 2018 •

edited

Loading