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.

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

Added ability to view all post types when Pulling from an External Connection #1002

Merged
merged 22 commits into from
Jul 30, 2024

Conversation

mehul0810
Copy link
Contributor

@mehul0810 mehul0810 commented Jan 12, 2023

Description of the Change

Add the ability to view all post types when pulling content from external connections.

Closes #861

How to test the Change

  • Go to Distributor > Pull Content (Make sure you have added an internal as well as external connection)
  • Check and confirm that "View All" filter is available for both internal and external connection
  • Check and confirm that filtering with "View All", displays posts from all the post types for both internal and external connection

Changelog Entry

Added - Introduce "View all" post type view when pulling content from external connections.

Credits

Props @mehul0810, @ravinderk, @cadic, @peterwilsoncc, @ankitrox, @Sidsector9, @jeffpaul.

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@mehul0810 mehul0810 marked this pull request as ready for review January 14, 2023 09:44
@mehul0810 mehul0810 requested a review from a team as a code owner January 14, 2023 09:44
@mehul0810 mehul0810 requested review from cadic and removed request for a team January 14, 2023 09:44
@cadic cadic added this to the 2.0.0 milestone Jan 16, 2023
Copy link
Contributor

@cadic cadic left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @mehul0810

I've left couple non-blocking questions and a suggestion for the $args in get_pull_content()

includes/classes/PullListTable.php Outdated Show resolved Hide resolved
includes/classes/PullListTable.php Outdated Show resolved Hide resolved
includes/rest-api.php Outdated Show resolved Hide resolved
includes/rest-api.php Outdated Show resolved Hide resolved
@peterwilsoncc
Copy link
Collaborator

In #1017 (awaiting merge) I've made a bunch of modifications to the rest api endpoint that should make this more achievable. It will probably cause a lot of conflicts to this PR though.

You'll be able to use post_type => any once that PR is merged as the custom endpoint will modify the value to only include post types the user can read, either because they have permission or the post type is publicly viewable.

@peterwilsoncc peterwilsoncc modified the milestones: 2.0.0, 2.0.1 May 24, 2023
@jeffpaul jeffpaul mentioned this pull request Sep 14, 2023
16 tasks
@peterwilsoncc peterwilsoncc modified the milestones: 2.0.1, 2.1.0 Sep 19, 2023
@github-actions github-actions bot added the needs:refresh This requires a refreshed PR to resolve. label Feb 9, 2024
@github-actions github-actions bot removed the needs:refresh This requires a refreshed PR to resolve. label Jun 26, 2024
Copy link

@mehul0810 thanks for the PR! Could you please fill out the PR template with description, changelog, and credits information so that we can properly review and merge this?

Copy link
Collaborator

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

I've left on question/comment inline.

I was getting a bunch of array to string notices so I pushed 7cdc43b to resolve that. WP_Query accepts all to query all of the post types so that can be used for the option.

The custom endpoint also accepts All as a shorthand.

@@ -623,10 +623,11 @@ function check_post_types_permissions() {
* @return \WP_REST_Response|\WP_Error
*/
function get_pull_content_list( $request ) {
$args = [
$post_type = ! empty( $request['post_type'] ) ? $request['post_type'] : array( 'post', 'page' );
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default is set to post in get_pull_content_list_args() so it's unlikely this will ever be set to [post, page].

I included it back when I was rewriting this as a safety check just in case someone messes with the URL.

Was the intent with this change to default to the View All view? For internal connections Distributor offers a View All view but defaults to posts only, so I think it's best to match that for the time being.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peterwilsoncc Replaced it to show only the post by default.

The intent with this change was to get the list of post type that needs to be listed or else default to post. But, we are not defaulting the post type filter to View All. It is still Post

@jeffpaul jeffpaul requested a review from peterwilsoncc July 8, 2024 13:10
Copy link
Member

@Sidsector9 Sidsector9 left a comment

Choose a reason for hiding this comment

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

I am seeing some unusual behaviour with the "View All" selection. Watch the video below:

Screen-2024-07-10-60918.PM.mp4

Additionally, only posts are being pulled.

@kirtangajjar
Copy link
Member

I can confirm that I can see something strange as well. It looks like when we select "view all" only posts by "admin" users are shown. That's what it looks on the surface(which may not be the case as well), needs more investigation.

Screen.Recording.2024-07-13.at.10.30.18.AM.1.mp4

@kirtangajjar
Copy link
Member

I think what I and sid observed was perhaps a security fix, so it's all good? #1003 (review)

@jeffpaul
Copy link
Member

@peterwilsoncc if this looks good to you, please merge, thanks!

@peterwilsoncc
Copy link
Collaborator

I think what I and sid observed was perhaps a security fix, so it's all good? #1003 (review)

It looks like it's a bug in WP_Query when querying multiple post types using the perm argument added as part of the security fix.

$args['perm'] = 'editable';

For multiple post types the edit_others_posts capability is ignored, for single post types it's checked and all posts are shown. I'm not sure whether it's by design to avoid overly complex SQL queries or an oversight.

Copy link
Collaborator

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

LGTM and tests well.

Thanks.

@github-actions github-actions bot added needs:code-review This requires code review. and removed needs:feedback This requires reporter feedback to better understand the request. labels Jul 30, 2024
@peterwilsoncc
Copy link
Collaborator

E2E tests are failing on the main branch so bypassing the merge requirements.

@peterwilsoncc peterwilsoncc merged commit ae90450 into develop Jul 30, 2024
13 of 19 checks passed
@peterwilsoncc peterwilsoncc deleted the feature/861 branch July 30, 2024 03:08
@dkotter dkotter modified the milestones: 2.1.0, 2.0.5 Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review. nice-to-have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to view all post types when Pulling from an External Connection
9 participants