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

Enable filtering whether Distributor should update distributed post statuses when the origin post status changes #446

Open
wants to merge 35 commits into
base: develop
Choose a base branch
from
Open

Enable filtering whether Distributor should update distributed post statuses when the origin post status changes #446

wants to merge 35 commits into from

Conversation

adamsilverstein
Copy link

@adamsilverstein adamsilverstein commented Aug 19, 2019 •
edited by dinhtungdu
Loading

Description of the Change

Filter whether Distributor should update distributed post statuses when the origin post status changes.

Closes #112.

Benefits

Enables distributing post status when desired, eg. takedowns.

Verification Process

Code review without whitespace: https://github.com/10up/distributor/pull/446/files?w=1

Repeat the following tests with a Network Connection and an External Connection

  • Enable filter:
    add_filter( 'dt_distribute_post_status', '__return_true' );

  • Pull post subscription - pull published post then test changing origin post from published to draft also changes pulled copies status

  • Push post - push published post then test changing origin post from published to draft also changes pushed copies status

  • Disable filter:
    add_filter( 'dt_distribute_post_status', '__return_false' );

  • Pull post subscription - pull published post then test changing origin post from published to draft also changes pulled does not copy status

  • Push post - push published post then test changing origin post from published to draft also changes pushed does not copy status

Unit tests

  • Network Connection

    • Pull
      • Filter enabled
      • Filter disabled
    • Push
      • Filter enabled
      • Filter disabled

  • External Connection

    • Pull
      • Filter enabled
      • Filter disabled
    • Push
      • Filter enabled
      • Filter disabled

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

@adamsilverstein adamsilverstein mentioned this pull request Aug 19, 2019
Open
@jeffpaul jeffpaul added the type:enhancement New feature or request. label Aug 19, 2019
@jeffpaul jeffpaul added this to the 2.0.0 milestone Aug 19, 2019
@adamsilverstein adamsilverstein requested a review from helen August 27, 2019 18:40
@adamsilverstein adamsilverstein changed the title dt_distribute_post_status first pass Enable filtering whether Distributor should update distributed post statuses when the origin post status changes Aug 27, 2019
@adamsilverstein adamsilverstein changed the title Enable filtering whether Distributor should update distributed post statuses when the origin post status changes [WIP] Enable filtering whether Distributor should update distributed post statuses when the origin post status changes Aug 27, 2019
@adamsilverstein adamsilverstein changed the title [WIP] Enable filtering whether Distributor should update distributed post statuses when the origin post status changes Enable filtering whether Distributor should update distributed post statuses when the origin post status changes Sep 16, 2019
Merge branch 'develop' into feature/distribute_post_status
0297cdc 
# Conflicts:
#	includes/classes/Authentications/WordPressDotcomOauth2Authentication.php
#	includes/syndicated-post-ui.php
barryceelen
barryceelen reviewed Sep 27, 2019
View reviewed changes
includes/classes/ExternalConnections/WordPressExternalConnection.php Outdated
@@ -556,7 +559,7 @@ public function push( $post_id, $args = array() ) {
'slug' => $post->post_name,
'content' => Utils\get_processed_content( $post->post_content ),
'type' => $post->post_type,
'status' => ( ! empty( $args['post_status'] ) ) ? $args['post_status'] : 'publish',
'status' => ( ! empty( $args['post_status'] ) ) ? $args['post_status'] : $distribute_status,
Copy link
Member

Choose a reason for hiding this comment

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

@adamsilverstein If I mark $distribute_post_status as true, the source post status will only be distributed if $args['post_status'] is empty. $args['post_status'] comes from the connection map for the origin post, how is that populated or changed? In any case, going off the filter name I would expect $distribute_post_status to always take precedence, but it's very possible I'm woefully missing context here.

Copy link
Author

Choose a reason for hiding this comment

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

In this case of push, the post_status comes from whether you have checked the 'distribute as draft' checkbox, if you have that setting overrides the filter. I do need to test subscriptions and pull, i'm not sure that works correctly. See

distributor/includes/push-ui.php

Lines 248 to 250 in b1dd60f

if ( ! empty( $_POST['postStatus'] ) ) {
$push_args['post_status'] = $_POST['postStatus'];
}

@dinhtungdu
Copy link
Contributor

@jeffpaul, I fixed the test, only one test is failing because of #516, waiting for #534 to pass WPA tests

@jeffpaul
Copy link
Member

In reviewing open PRs in the 1.6.0 milestone, this PR seemed still too risky and in need of testing/confirmation before merging so I'm going to punt this to our next release in hopes of having things in a more stable state by then.

@jeffpaul jeffpaul modified the milestones: 1.6.0, 2.0.0 May 27, 2020
@jeffpaul jeffpaul modified the milestones: 2.0.0, 1.7.0 Sep 15, 2020
@vikrampm1 vikrampm1 added needs:documentation This requires documentation. and removed needs documentation labels Dec 3, 2021
@jeffpaul jeffpaul modified the milestones: 1.7.0, 2.1.0 Apr 27, 2022
@jeffpaul jeffpaul removed the request for review from dkotter January 16, 2023 18:02
@jeffpaul
Copy link
Member

Unassigning reviewers here while we focus on the v2 refactoring and can then reassess how best to handle the root issue here.

@github-actions github-actions bot added the needs:refresh This requires a refreshed PR to resolve. label Feb 9, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 9, 2024

@adamsilverstein thanks for the PR! Could you please rebase your PR on top of the latest changes in the base branch?

@leogermani leogermani mentioned this pull request Feb 9, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barryceelen barryceelen barryceelen left review comments

@dinhtungdu dinhtungdu dinhtungdu requested changes

At least 1 approving review is required to merge this pull request.

Assignees

@adamsilverstein adamsilverstein

Labels
needs:documentation This requires documentation. needs:refresh This requires a refreshed PR to resolve. type:enhancement New feature or request.
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.

Improve/verify handling of post status changes
5 participants
@adamsilverstein @dinhtungdu @jeffpaul @barryceelen @vikrampm1