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

Add view all post types filter to External Connections. #1003

Closed
wants to merge 9 commits into from

Conversation

ankitrox
Copy link

@ankitrox ankitrox commented Jan 12, 2023

Description of the Change

Introduce a "View All" post type view to the pull screen for external connections for parity with the internal connections. The new view is the default when visiting Distributor > Pull Content > [External connection].

Closes #861

How to test the Change

  1. Create two separate sites using insta wp.
  2. Install distributor plugin on both the sites. Connect the sites.
  3. Create a custom post type "Book" on both the sites.
  4. On pull screen ensure that "View All" option is available.
  5. Pull content from CPT and it should pull the posts from external sites.

Changelog Entry

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

Credits

Props @ankitrox, @Sidsector9, @ravinderk, @jeffpaul, @peterwilsoncc

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.

@ankitrox ankitrox self-assigned this Jan 12, 2023
@jeffpaul jeffpaul added this to the Future Release milestone Jan 16, 2023
@ankitrox ankitrox marked this pull request as ready for review January 23, 2023 19:19
@ankitrox ankitrox requested a review from a team as a code owner January 23, 2023 19:19
@ankitrox ankitrox requested review from Sidsector9 and removed request for a team January 23, 2023 19:19
@jeffpaul jeffpaul requested review from cadic and removed request for Sidsector9 February 6, 2023 17:06
@jeffpaul
Copy link
Member

jeffpaul commented Feb 6, 2023

Tagging Max on this one as he's already reviewed a separate PR #1002 for the same issue, so worth checking here to see which approach is most optimal (and can close the lessor of the two)

@Sidsector9
Copy link
Member

@ankitrox can you please fill up the description and testing instructions for clarity?

@ankitrox
Copy link
Author

ankitrox commented May 5, 2023

@ankitrox can you please fill up the description and testing instructions for clarity?

Added the steps to test

@jeffpaul jeffpaul requested review from peterwilsoncc and removed request for cadic June 29, 2023 16:03
@jeffpaul jeffpaul modified the milestones: Future Release, 2.0.1 Jun 29, 2023
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.

Hi @ankitrox

I followed your instructions and tested this branch, I faced some issues. The Posts filter shows Pages as well.

I tested by resolving the conflicts locally as well and still faced the same issue.

Please watch the video below:

Screen-2023-07-13-154244.mp4

Let me know if I'm missing any steps.

@peterwilsoncc
Copy link
Collaborator

The changes in develop to the includes/rest-api.php file are causing a few conflicts for this.

However, the good news is that the changes allow for requesting any as the post type to get all the post types so you won't need to make any changes to the file -- the other files will need to specify the post type as any rather than all.

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 pushed a few changes to account for updates to the REST API endpoint in Distributor 2.0.0

For external connections, I am wondering if it would be best to send the local post types individually when requesting all post types: this will allow for situations in which the external connection has post types that are not supported locally.

However, I think that might be best left fro another ticket.

Copy link
Contributor

@ravinderk ravinderk left a comment

Choose a reason for hiding this comment

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

@peterwilsoncc I am not getting a correct result for the View all filter.

I noticed that in the final SQL query, WordPress adds post_author to the current user id (rest API) when doing a query for multiple posts (i.e. all), which means I am not getting the list of all editable posts from all post types.

Can you check?

image image

@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
@jeffpaul jeffpaul requested a review from peterwilsoncc April 12, 2024 18:28
@github-actions github-actions bot added the needs:feedback This requires reporter feedback to better understand the request. label Apr 15, 2024
Copy link

@ankitrox 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?

peterwilsoncc
peterwilsoncc previously approved these changes Apr 16, 2024
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 noticed that in the final SQL query, WordPress adds post_author to the current user id (rest API) when doing a query for multiple posts (i.e. all), which means I am not getting the list of all editable posts from all post types.

I'm not seeing this issue when connected as an administrator. If Ravinder had connected the sites as a non-admin it may be an effect of the security fixes in GHSA-q43c-v867-4cfp

I think this code is good or merging.

@peterwilsoncc peterwilsoncc changed the title Add ability to view all post types when Pulling from an External Conn… Add view all post types filter to External Connections. Apr 16, 2024
@peterwilsoncc peterwilsoncc dismissed their stale review April 16, 2024 00:17

Noticed bugs post review. NOT yet merge ready.

@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 Apr 16, 2024
@jeffpaul
Copy link
Member

jeffpaul commented Jul 5, 2024

Closing in favor of #1002.

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
5 participants