Skip to content
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 Lists from UI Take 2 #10396

Merged
merged 13 commits into from
Oct 30, 2018
Merged

Decouple Publicize Connection Lists from UI Take 2 #10396

merged 13 commits into from
Oct 30, 2018

Conversation

mdawaffe
Copy link
Member

@mdawaffe mdawaffe commented Oct 24, 2018 •
edited
Loading

Take 2 of the excellent #9955 by @c-shultz. See #9039.

Updated to work on top of #10043.

Changes proposed in this Pull Request:

  • Adds a method to Publicize_Base to get the list of Publicize Connections suitable for use in UI.
  • Cleans up logic v. presentation boundary in Publicize_UI.

Testing instructions:

Automated Tests
  1. yarn docker:phpunit --filter=WP_Test_Publicize
Manual Tests
  1. Under an admin account, set up a test site with two publicize accounts: one "global" (available to all users of the site), one not global.
  2. As that admin, go to the Classic (non-Gutenberg) editor, and look at the Publicize UI for a new post.
  3. You should see two checked checkboxes: one for each connection. You should be able to uncheck each (the inputs should not be disabled).
  4. Publish the post.
  5. You should see the publicized content (tweet, FB update, etc.) on both connections.
  6. Under an author account, go to the Classic (non-Gutenberg) editor, and look at the Publicize UI or a new post.
  7. You should see one checked-but disabled checkbox for the global connection. You should not see any evidence at all of the admin's non-global connection.
  8. Publish a post as that author.
  9. See the publicized content (tweet, FB update, etc.) on the global connection.

Proposed changelog entry for your changes:

None? Part of a larger initiative to provide Publicize data via REST API.

@mdawaffe
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
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
Publicize: The comma separated list of connections should be separate…
234a2b0 
…d by commas :)

The JS does this. So should the PHP.
@mdawaffe mdawaffe added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Publicize Now Jetpack Social, auto-sharing [Status] In Progress DO NOT MERGE don't merge it! labels Oct 24, 2018
@mdawaffe mdawaffe added this to the 6.7 milestone Oct 24, 2018
@mdawaffe mdawaffe self-assigned this Oct 24, 2018
@mdawaffe mdawaffe requested review from ockham and a team October 24, 2018 21:41
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D19875-code. (newly created revision)

@jetpackbot
Copy link

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label and if possible have someone from your team review the code. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: November 6, 2018.
Scheduled code freeze: October 30, 2018

Generated by 🚫 dangerJS

@ockham ockham mentioned this pull request Oct 24, 2018
Closed
@mdawaffe
Publicize: Bump @since
32a0096
@mdawaffe
Publicize: Move ->done_sharing_post() out of Publicize_Base
7b9bd88 
It has Jetpack implementation details.
@mdawaffe
Publicize: use correct variable :)
180cfff
@mdawaffe
Publicize: Remove the visible property.
b5573a5 
Disabled checkboxes are never included in form submissions.

This is why we need the hidden form fields whenever we have a checked,
disabled checkbox.

Always show the checkbox (in the correct checked state). Look at
`toggleable` and `enabled` to see if we need the hidden form field.
@mdawaffe
Publicize: Fix tests
50234f8 
Be explicit about expected global/normal behavior.
@ockham ockham mentioned this pull request Oct 25, 2018
Merged
@mdawaffe
Publicize: Output Service Label and Connection Username separately.
3995e08 
Output both these human-readable properties separately so that clients can
decide for themselves how to display them.
@mdawaffe
Publicize: Fix the tests I broke in 3995e08
c4f653c
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D19875-code. (updated diff)

ockham
ockham previously approved these changes Oct 25, 2018
View reviewed changes
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments -- nothing major tho.

I tested this with the same instructions as #10043. LGTM! Ready to 🚢 IMO

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D19875-code. (updated diff)

@jeherve jeherve mentioned this pull request Oct 29, 2018
Closed
jeherve
jeherve previously approved these changes Oct 29, 2018
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran some tests with the different interfaces, with different Publicize connections, and with different statuses in wp-admin; I could not spot any issues. I think you can merge when ready.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D19875-code. (updated diff)

@mdawaffe
Publicize: Rename is_post_done_sharing() to post_is_done_sharing()
2d86791 
Consistency with other `post_is...()` functions.
oskosk
oskosk approved these changes Oct 30, 2018
View reviewed changes
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! thanks for the instructions!

dereksmart
dereksmart approved these changes Oct 30, 2018
View reviewed changes
Copy link
Member

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality tested and looked at the two commits since last review.

@mdawaffe mdawaffe added [Status] Ready to Merge Go ahead, you can push that green button! and removed DO NOT MERGE don't merge it! [Status] In Progress labels Oct 30, 2018
@mdawaffe mdawaffe merged commit 20da538 into master Oct 30, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 30, 2018
@jeherve jeherve deleted the update/publicize-decouple-ui-2 branch October 30, 2018 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dereksmart dereksmart dereksmart approved these changes

@oskosk oskosk oskosk approved these changes

@jeherve jeherve jeherve left review comments

@ockham ockham ockham left review comments

Assignees

@mdawaffe mdawaffe

Labels
[Feature] Publicize Now Jetpack Social, auto-sharing Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
6.7
Development

Successfully merging this pull request may close these issues.
7 participants
@mdawaffe @matticbot @jetpackbot @ockham @jeherve @oskosk @dereksmart