Decouple Publicize connection list access from UI scripts. #9955

zinigor · 2018-07-26T20:55:31Z

Fixes #9039 (along with PR #9144)

Replaces the original #9466 by @c-shultz , thanks for all the effort!

Note: These changes are standalone but they are a dependency of #9144. They were originally contained in #9144, but are being pulled out for for clarity/organization because that PR was getting too large and complex.

Changes proposed in this Pull Request:

This PR separates the Publicize connection list processing from the HTML generation for the UI form.

Before this PR:

The method Publicize_UI::get_metabox_form_connected() in ui.php retrieved and filtered the connection list and then also generated the HTML form.

After this PR:

This PR leaves the HTML generation in Publicize_UI::get_metabox_form_connected() but moves the connection gathering/filtering to new method Publicize::get_filtered_connection_data() in publicize-jetpack.php.

Why this is needed:

PR #9144 needs to access the connection list so it can be JSON encoded and utilized in new React-based frontend for Gutenberg. Splitting the connection list retrieval to a separate method allows code reuse between the classic editor HTML generation and the Gutenberg front-end to gather.

The final diff view of these changes looks a little muddled, but the first commit pretty effectively shows the basics of what's being moved where in this PR.

This PR should not affect the Classic Editor in any way.

Unit tests have been added to test connection data fields and to test the three potentially affected filters:

'wpas_submit_post?'
'publicize_checkbox_global_default'
'publicize_checkbox_default'

Testing instructions:

Checkout and test Publicize in Classic Editor to confirm there are no functional changes.
See #9144 for additional testing instructions.

props to @ehg for the design suggestions!

Connection data access is intermingled with HTML generation. This moves connection data access out of ui.php and into publicize-jetpack.php to decouple it from the UI. This is a cleaner design and allows the data to be accessed for other purposes.

Adding test coverage for: - done_sharing_post: testing flagged and published posts - get_services_connected: testing service list - get_filtered_connection_data: testing simple connection data with no filtering

Adding unit tests for three existing filters in Publicize: - 'wpas_submit_post?' - 'publicize_checkbox_global_default' - 'publicize_checkbox_default' These filters were previously difficult to test since they were being applied within HTML generation code. Now that HTML generation is separated from the connection retrieving/filtering, it is a good time to add unit tests.

A previous iteration had method 'get_filtered_connection_data' in Publicize_UI, so the test and other references were calling it from there. This updates those references now that the method has been moved into Publicize class.

'disabled' parameter value was from 'get_filtered_connection_data' return was an HTML string an an early iteration but is now a boolean. This commit updates the associated test case to check new data format.

HTML indenting was incorrect in Publicize_UI::get_metabox_form_connected(). This commit fixes the whitespace.

Updated Publicize form generation to use new boolean parameter value to correctly disable checkboxes when required.

Cleaning up some minor issues identified by phpcs in get_filtered_connection_data. This function is legacy code that has been moved over, so it continues to have code style issues. More major refactoring would be necessary to fix the rest...

Cleaning up low-hanging fruit on coding styles, including: - whitespace - comments - yoda conditions

@SInCE

'@SInCE' changing from 5.9.1 to 6.2.0. '@SInCE' version number being removed from Publicize_UI::get_metabox_form_connected() since this method is not new in this PR. props @kraftbj

@kraftbj

Modifies Publicize_UI::get_filtered_connection_data() to gracefully handle the case where there is no current post and the post_id parameter is unset. This condition is now handled by not disabling any of the connection checkboxes. Previously, this case would cause a PHP error. Also adding test cases to exercise this case. props @kraftbj

Reining in some especially long lines in Publicize_UI::get_filtered_connection_data(). No functional changes intended.

brbrr · 2018-07-31T12:14:31Z

Since our plan is to build JS (and maybe CSS) within Calypso env - we shouldn't merge this PR as is. JS/CSS part already merged here: Automattic/wp-calypso#26389, the remaining part is to ship PHP code for this block.

Moving this PR into next release.

oskosk · 2018-08-24T12:28:52Z

Pushed a commit updating all @since tag reading 6.2.0 to read 6.5.0

oskosk · 2018-08-24T12:29:19Z

Still this needs a rebase

mdawaffe · 2018-08-24T16:38:14Z

Let's hold off on this until #10043 is done

gravityrail · 2018-08-31T19:08:06Z

You might want to check out #10065, which is a small refactor that moves URL logic into a "Jetpack Connection Helpers" library.

The reason we did it was so that we could reuse the keyring code for our "One-click site verify" project without needing publicize to be enabled.

gravityrail · 2018-08-31T19:08:21Z

Whoops accidentally closed. Reopened now.

mdawaffe

Now needs rebasing. After #10043, needs some functions moved to the base class.

Also, let's look at the function and connection property names.

mdawaffe · 2018-10-24T03:47:32Z

modules/publicize/publicize-jetpack.php

+	 *     @type string 'display_name'    Username for sharing account.
+	 * }
+	 */
+	public function get_filtered_connection_data( $selected_post_id = null ) {


After #10043 is in, let's move this to publicize.php's Publicize_Base.

This function also needs to be "rebased" with the current logic from prior to #10043. (There have been a couple changes.)

mdawaffe · 2018-10-24T03:58:19Z

modules/publicize/publicize-jetpack.php

+				$connection_list[] = array(
+					'unique_id'       => $unique_id,
+					'name'            => $name,
+					'checked'         => $checked,


If we want to decouple this from HTML, should we still call this "checked"? "default?"

Do we even need checked/default? We could leave it up to the UI to determine if the post title === publicize msg, no?

As @mdawaffe just clarified per DM, this property isn't equivalent to post title === publicize msg. Rather:

The checked/default stuff is about which publicize connections [...] to use. Especially for posts that have already been saved as drafts, the client needs to know the state of those checkboxes (that is: the state of the author’s desire regarding which connections to use).

Maybe call that property enabled then? Which is hopefully more UI agnostic than checked. Seems like default had me a bit confused about its meaning...

mdawaffe · 2018-10-24T04:10:16Z

modules/publicize/publicize-jetpack.php

+					'name'            => $name,
+					'checked'         => $checked,
+					'disabled'        => $disabled,
+					'active'          => $active,


I'm not sure we need active at all.

checked is used in the list of checkboxes when the Publicize UI is open. active is used in the comma separated list of connections when the Publicize UI is closed.

Technically, it's possible for active to be different than checked, but only via the publicize_checkbox_default filter. But why would we ever want to display different information in the two lists?

Perhaps checked/default, already/done, and disabled are a better set of properties?

But why would we ever want to display different information in the two lists?

Go with YAGNI and ditch this property?

mdawaffe · 2018-10-24T04:10:39Z

modules/publicize/publicize-jetpack.php

+					'checked'         => $checked,
+					'disabled'        => $disabled,
+					'active'          => $active,
+					'hidden_checkbox' => $hidden_checkbox,


Maybe global instead of hidden_checkbox? (What it means, not how it's used.)

See #9955 (comment)

mdawaffe · 2018-10-24T04:12:12Z

modules/publicize/publicize-jetpack.php

+	 *
+	 * @return bool True if post has already been shared by Publicize, false otherwise.
+	 */
+	public function done_sharing_post( $post_id = null ) {


I'm not sure about this name. is_post_already_publicized()?

mdawaffe · 2018-10-24T04:12:31Z

modules/publicize/publicize-jetpack.php

+	 *     @type string 'url'   URL for adding connection to service.
+	 * }
+	 */
+	function get_available_service_data() {


This function is never used in this PR. Do we need it now?

mdawaffe · 2018-10-24T04:13:50Z

modules/publicize/ui.php

+						?> />
+						<?php
+						if ( $c['hidden_checkbox'] ) {
+							// Need to submit a value to force a global connection to post


This logic doesn't change in this PR, but we should check and see if it's this hidden form field that controls the server's behavior regarding global connections. If that's true, it's strange.

I agree. If we're able to ditch the hidden field, I guess we should also be able to remove the field from get_filtered_connection_data()'s return value, and filter the corresponding connection from that array instead?

ockham · 2018-10-24T17:16:28Z

Thanks for reviewing this, @mdawaffe! I'll take over from @zinigor and will either rebase on #10043, or maybe start a new branch (mostly because 'rebasing' the functions that have been moved to a different file in this PR and have been modified by #10043 might be a bit of a hassle).

mdawaffe · 2018-10-24T17:43:49Z

Thanks for reviewing this, @mdawaffe! I'll take over from @zinigor and will either rebase on #10043, or maybe start a new branch (mostly because 'rebasing' the functions that have been moved to a different file in this PR and have been modified by #10043 might be a bit of a hassle).

I can work on this if you like, @ockham.

@c-shultz

Based on #9955 by @c-shultz. "Rebased" on top of #10043 Moves Connection List Data "getter" out of `Publicize_UI` and in to `Publicize_Base`.

They fail :) But at least they now look at the right data since the data structures changd from #9955 to b17be1f.

jeherve · 2018-10-29T09:48:05Z

Closing this in favor of #10396

@c-shultz

Publicize: Decouple Connection List from UI Based on #9955 by @c-shultz. See #9039. Updated to work on top of #10043. * Move Connection List Data "getter" out of `Publicize_UI` and in to `Publicize_Base` to clean up logic v. presentation boundary in `Publicize_UI`. * In the wp-admin/ Publicize UI, separate the comma separated list of connections by commas :) The JS does this. So should the PHP.

c-shultz added 12 commits July 12, 2018 15:35

Publicize: Adding simple test coverage for Publicize helper methods

a500a21

Adding test coverage for: - done_sharing_post: testing flagged and published posts - get_services_connected: testing service list - get_filtered_connection_data: testing simple connection data with no filtering

Publicize: update 'disabled' test case to boolean

553760c

'disabled' parameter value was from 'get_filtered_connection_data' return was an HTML string an an early iteration but is now a boolean. This commit updates the associated test case to check new data format.

Publicize: Fix indenting in Publicize_UI

44525b5

HTML indenting was incorrect in Publicize_UI::get_metabox_form_connected(). This commit fixes the whitespace.

Publicize: Use boolean 'disabled' flag for form

9bb8150

Updated Publicize form generation to use new boolean parameter value to correctly disable checkboxes when required.

Publicize: Minor whitespace cleanup

776bdee

Cleaning up some minor issues identified by phpcs in get_filtered_connection_data. This function is legacy code that has been moved over, so it continues to have code style issues. More major refactoring would be necessary to fix the rest...

Publicize: clean up code styles.

0dbe8b4

Cleaning up low-hanging fruit on coding styles, including: - whitespace - comments - yoda conditions

Publicize: Updating @SInCE of changes.

4b29a7c

'@SInCE' changing from 5.9.1 to 6.2.0. '@SInCE' version number being removed from Publicize_UI::get_metabox_form_connected() since this method is not new in this PR. props @kraftbj

Publicize: Long line formatting

2d53c34

Reining in some especially long lines in Publicize_UI::get_filtered_connection_data(). No functional changes intended.

zinigor requested a review from a team as a code owner July 26, 2018 20:55

zinigor mentioned this pull request Jul 26, 2018

Decouple Publicize connection list access from UI scripts #9466

Closed

zinigor added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Publicize Now Jetpack Social, auto-sharing [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jul 26, 2018

zinigor added this to the 6.4 milestone Jul 26, 2018

tyxla mentioned this pull request Jul 30, 2018

Demo: Try externally built Publicize extension for Gutenberg #9963

Closed

brbrr modified the milestones: 6.4, 6.5 Jul 31, 2018

ZebulanStanphill mentioned this pull request Aug 11, 2018

How do I set a featured image, share to social media, and add captions below photos instead of on top of them? WordPress/gutenberg#8860

Closed

mdawaffe self-assigned this Aug 23, 2018

update @SInCE tags to read 6.5.0

3484a4a

oskosk added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Aug 24, 2018

oskosk removed this from the 6.5 milestone Aug 30, 2018

gravityrail closed this Aug 31, 2018

gravityrail reopened this Aug 31, 2018

ockham mentioned this pull request Oct 22, 2018

Try/publicize api #10367

Closed

mdawaffe requested changes Oct 24, 2018

View reviewed changes

mdawaffe added a commit that referenced this pull request Oct 24, 2018

Publicize: Decouple Connection List from UI

b17be1f

Based on #9955 by @c-shultz. "Rebased" on top of #10043 Moves Connection List Data "getter" out of `Publicize_UI` and in to `Publicize_Base`.

mdawaffe added a commit that referenced this pull request Oct 24, 2018

Publicize: Tweak Tests

6b8b2d0

They fail :) But at least they now look at the right data since the data structures changd from #9955 to b17be1f.

mdawaffe mentioned this pull request Oct 24, 2018

Decouple Publicize Connection Lists from UI Take 2 #10396

Merged

kraftbj added [Status] In Progress and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Oct 25, 2018

matticbot added the Touches WP.com Files label Oct 25, 2018

jeherve closed this Oct 29, 2018

jeherve deleted the update/publicize-decouple-ui branch October 29, 2018 09:48

kraftbj removed the [Status] In Progress label Oct 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple Publicize connection list access from UI scripts. #9955

Decouple Publicize connection list access from UI scripts. #9955

zinigor commented Jul 26, 2018 •

edited

Loading

brbrr commented Jul 31, 2018

oskosk commented Aug 24, 2018

oskosk commented Aug 24, 2018

mdawaffe commented Aug 24, 2018

gravityrail commented Aug 31, 2018

gravityrail commented Aug 31, 2018

mdawaffe left a comment

mdawaffe Oct 24, 2018

mdawaffe Oct 24, 2018

mdawaffe Oct 24, 2018

ockham Oct 24, 2018

ockham Oct 24, 2018

ockham Oct 24, 2018

mdawaffe Oct 24, 2018

ockham Oct 24, 2018

mdawaffe Oct 24, 2018

ockham Oct 24, 2018

mdawaffe Oct 24, 2018

mdawaffe Oct 24, 2018

mdawaffe Oct 24, 2018

ockham Oct 24, 2018

ockham commented Oct 24, 2018

mdawaffe commented Oct 24, 2018

jeherve commented Oct 29, 2018

Decouple Publicize connection list access from UI scripts. #9955

Decouple Publicize connection list access from UI scripts. #9955

Conversation

zinigor commented Jul 26, 2018 • edited Loading

Changes proposed in this Pull Request:

Before this PR:

After this PR:

Why this is needed:

Testing instructions:

brbrr commented Jul 31, 2018

oskosk commented Aug 24, 2018

oskosk commented Aug 24, 2018

mdawaffe commented Aug 24, 2018

gravityrail commented Aug 31, 2018

gravityrail commented Aug 31, 2018

mdawaffe 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham commented Oct 24, 2018

mdawaffe commented Oct 24, 2018

jeherve commented Oct 29, 2018

zinigor commented Jul 26, 2018 •

edited

Loading