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.

Sign up for GitHub

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

Jump to bottom

Decouple Publicize connection list access from UI scripts #9466

Closed

c-shultz wants to merge 12 commits into Automattic:master from c-shultz:try/publicize-decouple-ui

c-shultz commented May 2, 2018 •

edited

Loading

Fixes #9039 (along with PR #9144)

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!

c-shultz requested a review from a team as a code owner

May 2, 2018 05:39

c-shultz mentioned this pull request

Add Gutenberg Publicize Support #9144

Closed

jeherve added [Type] Enhancement [Feature] Publicize [Status] Needs Review labels

kraftbj reviewed

View reviewed changes

Contributor

kraftbj left a comment

Just made a really quick look for small fish. It reads well with some minor nitpicks. Not approving since I don't have a chance right now to actually test it.

modules/publicize/publicize-jetpack.php Outdated

    
              	 * data directly as array so it can be retrieved for static HTML generation

              	 * or JSON consumption.

              	 *

              	 * @since 5.9.1

Contributor

kraftbj May 2, 2018

Nitpick, probably should be 6.2 now.

modules/publicize/publicize-jetpack.php Outdated

    
              	public function get_filtered_connection_data( $post_id = null ) {

              		$connection_list = array();

              		$post = get_post( $post_id ); // Defaults to current post if $post_id is null.

Contributor

kraftbj May 2, 2018

Do we need a null check in case there is no current post?

Author

c-shultz May 3, 2018 •

edited

Loading

Hmm. Very good point. I need to work out what needs to happen in this case.

I'm just thinking out loud here: Post ID is used to disable the connection checkbox if the post has already been shared to some or all connections. A null post ID may come in to play with a new post. In that case, I think we should just leave all connection checkboxes enabled, since the new post clearly hasn't been shared yet.

I'm going to do some testing around this.

Author

c-shultz May 3, 2018

This was indeed an issue. Just pushed in a fix with associated test cases. Thanks!

modules/publicize/publicize-jetpack.php Outdated

    
              				// those connections, don't let them change it

              				$cmeta = $this->get_connection_meta( $connection );

              				$hidden_checkbox = false;

              				if ( !$done && ( 0 == $cmeta['connection_data']['user_id'] && !current_user_can( $this->GLOBAL_CAP ) ) ) {

Contributor

kraftbj May 2, 2018

Nitpick, coding standards (need space)

modules/publicize/publicize-jetpack.php Outdated

    
              	 * far as Publicize is concerned. Jetpack uses this approach. All published posts in Jetpack

              	 * have Publicize disabled.

              	 *

              	 * @since 5.9.1

Contributor

kraftbj May 2, 2018

Bump needed.

modules/publicize/publicize-jetpack.php Outdated

    
              	 *

              	 * @return bool True if post has already been shared by Publicize, false otherwise.

              	 */

              	function done_sharing_post( $post_id = null )

Contributor

kraftbj May 2, 2018

Nitpick, braces needed on the function declaration line

modules/publicize/publicize-jetpack.php Outdated

    
              	function done_sharing_post( $post_id = null )

              	{

              		global $publicize_ui;

              		$post = get_post( $post_id ); // Defaults to current post if $post_id is null

Contributor

kraftbj May 3, 2018

another null check?

modules/publicize/publicize-jetpack.php Outdated

    
              	 * Retrieves current available publicize service connections

              	 * with associated labels and URLs.

              	 *

              	 * @since 5.9.1

Contributor

kraftbj May 3, 2018

Bump

tests/php/modules/publicize/test_class.publicize.php Outdated

    
              	/**

              	 * Current test user id produced by factory method.

              	 *

              	 * @since 5.9.1

Contributor

kraftbj May 3, 2018

few more bumps in this file

c-shultz force-pushed the try/publicize-decouple-ui branch 2 times, most recently from 8f9b81d to 0d64022 Compare

May 3, 2018 04:08

oskosk added this to the 6.2 milestone

Contributor

oskosk commented May 28, 2018

This PR needs a rebase :)

oskosk added [Status] Needs Author Reply and removed [Status] Needs Review labels

oskosk removed this from the 6.2 milestone

Contributor

oskosk commented May 28, 2018

We'll have to punt this one for the next release maybe

oskosk added this to the 6.3 milestone

oskosk modified the milestones: 6.3, 6.4

Contributor

oskosk commented Jul 11, 2018

@c-shultz could you create a version of this branch but on this same repo instead of being a branch on a fork ?

Author

c-shultz commented Jul 12, 2018

Sure. I should be able to take care of that today!

c-shultz added 11 commits

July 12, 2018 15:35


          Publicize: Move connection access out of ui.php

e8ba2b9

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.


          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: Adding test coverage for get_filtered_connection_data filters

fdb4985

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.


          Publicize: Modifying references for get_filtered_connection_data

d43d804

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.


          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: handle no current post case

ed05b85

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


          Publicize: Long line formatting

2d53c34

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

c-shultz force-pushed the try/publicize-decouple-ui branch from d66b561 to 2d53c34 Compare

July 12, 2018 22:42

Author

c-shultz commented Jul 12, 2018

@oskosk, I rebased the forked branch and then merged it onto a fresh branch in this repo: https://github.com/Automattic/jetpack/compare/update/publicize-decouple-ui

c-shultz removed the [Status] Needs Author Reply label

c-shultz mentioned this pull request

Adding Gutenberg support to Publicize #9934

Closed

Contributor

brbrr commented Jul 24, 2018

@oskosk do we need a new PR for this?

brbrr added the [Status] Needs Review label

Member

zinigor commented Jul 26, 2018

Thank you for creating a separate branch, I'm going to close this in favor of the new one: #9955

zinigor closed this

zinigor mentioned this pull request

Decouple Publicize connection list access from UI scripts. #9955

Closed

kraftbj removed the [Status] Needs Review label

matticbot added the Touches WP.com Files label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Publicize Touches WP.com Files [Type] Enhancement