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 categories to pull screen - 1 #1237

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

Conversation

kirtangajjar
Copy link
Member

@kirtangajjar kirtangajjar commented Jun 23, 2024

Description of the Change

Add categories to the pull screen. This is a part 1 of 2 of this issue. This PR only lists categories from posts.

Closes #428

How to test the Change

  1. Open a pull screen and select any site
  2. Ensure that you see "Categories" column on it.
  3. Ensure that the categories link to their original site.
  4. Ensure step 1-3 for both internal and external connections.

Changelog Entry

Added - categories to pull screen

Credits

Props @kirtangajjar

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.

@kirtangajjar kirtangajjar requested a review from a team as a code owner June 23, 2024 01:22
@kirtangajjar kirtangajjar requested review from faisal-alvi and removed request for a team June 23, 2024 01:22
@faisal-alvi faisal-alvi changed the base branch from trunk to develop July 2, 2024 14:07
@github-actions github-actions bot added this to the 2.1.0 milestone Jul 2, 2024
@github-actions github-actions bot added the needs:code-review This requires code review. label Jul 2, 2024
Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

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

@kirtangajjar Thanks for the PR.

Targeted branch

I noticed the PR branch was targeting the trunk due to which the workflows like PHPCS did not trigger. I updated the target branch to be develop.

Testing

While testing I noticed the category link is broken because a '/' is missing:

image

Actual requirement

As per the requirement,

make the plugin very useful is if it was possible to view the category of the posts and select which categories to pull.

It seems like it says a user should be able to select categories but this PR only displays them. I'm not sure how do we achieve pulling posts via selected categories in the same UI. But is displaying them enough to should we worry about selection as well? cc @jeffpaul

E2E failing

Can you please check and fix the E2E failures? Try upgrading the wp-env version.

Suggestions

I have also added a few suggestions below, mostly related to formatting.

includes/utils.php Outdated Show resolved Hide resolved
includes/classes/PullListTable.php Outdated Show resolved Hide resolved
includes/classes/PullListTable.php Outdated Show resolved Hide resolved
includes/classes/PullListTable.php Show resolved Hide resolved
includes/utils.php Show resolved Hide resolved
includes/utils.php Show resolved Hide resolved
includes/utils.php Show resolved Hide resolved
includes/utils.php Show resolved Hide resolved
includes/utils.php Show resolved Hide resolved
includes/utils.php Show resolved Hide resolved
@vikrampm1 vikrampm1 removed the request for review from peterwilsoncc July 3, 2024 21:06
@kirtangajjar
Copy link
Member Author

While testing I noticed the category link is broken because a '/' is missing

This should be fixed now.

It seems like it says a user should be able to select categories but this PR only displays them. I'm not sure how do we achieve pulling posts via selected categories in the same UI

That will happen in another PR. I'll ping that PR here when it's ready.

@kirtangajjar
Copy link
Member Author

Can you please check and fix the E2E failures? Try upgrading the wp-env version.

Which version of wp-env works fine?

@faisal-alvi faisal-alvi removed their request for review July 4, 2024 12:26
@kirtangajjar
Copy link
Member Author

kirtangajjar commented Jul 4, 2024

@faisal-alvi I worked fixing tests for a while and I was able to fix many things, but it looks like it'll require more work.

Work done so far:

https://github.com/10up/distributor/pull/1237/files#diff-4c9c322e893113577fd895f8ac7bf59b02ac95aaae52b64177f24a996f706c22
https://github.com/10up/distributor/pull/1237/files#diff-5da9a41590fb261de0e464c95862528100c42c77dffad53b7c6de228b6afd2bd
https://github.com/10up/distributor/pull/1237/files#diff-fb5314afdcfb013b9945785877b0501d5b8fdfbe6d379a51fd4690bf5f00feca

Work remaining:
Here is a sample error that's occuring:

            {
              "title": "Should distribute blocks when pushing to network connections.",
              "fullTitle": "Distributed content block tests Should distribute blocks when pushing to network connections.",
              "timedOut": null,
              "duration": 16925,
              "state": "failed",
              "speed": null,
              "pass": false,
              "fail": true,
              "pending": false,
              "context": null,
              "code": "const postTitle = 'Post to push ' + randomName();\ncy.createPost({\n  title: postTitle\n}).then(sourcePost => {\n  cy.distributorPushPost(sourcePost.id, 'second', '', 'publish').then(distributedPost => {\n    cy.postContains(distributedPost.distributedPostId, '<!-- wp:paragraph -->', 'http://localhost/second/');\n  });\n});",
              "err": {
                "message": "CypressError: `cy.exec('npm --silent run env run tests-cli \"post get 33 --field=content --url=http://localhost/second/\"')` failed because the command exited with a non-zero code.\n\nPass `{failOnNonZeroExit: false}` to ignore exit code failures.\n\nInformation about the failure:\nCode: 1\n\nStdout:\nOCI runtime exec failed: exec failed: unable to start container process: exec: \"post get 33 --field=content --url=http://localhost/second/\": stat post get 33 --field=content --url=http://localhost/...\nStderr:\n(node:1726) Warning: Setting the NODE_TLS_REJECT_UNAUTHORIZED environment variable to '0' makes TLS connections and HTTPS requests insecure by disabling certificate verification.\n(Use `node --trace...\n\nhttps://on.cypress.io/exec",
                "estack": "CypressError: `cy.exec('npm --silent run env run tests-cli \"post get 33 --field=content --url=http://localhost/second/\"')` failed because the command exited with a non-zero code.\n\nPass `{failOnNonZeroExit: false}` to ignore exit code failures.\n\nInformation about the failure:\nCode: 1\n\nStdout:\nOCI runtime exec failed: exec failed: unable to start container process: exec: \"post get 33 --field=content --url=http://localhost/second/\": stat post get 33 --field=content --url=http://localhost/...\nStderr:\n(node:1726) Warning: Setting the NODE_TLS_REJECT_UNAUTHORIZED environment variable to '0' makes TLS connections and HTTPS requests insecure by disabling certificate verification.\n(Use `node --trace...\n\nhttps://on.cypress.io/exec\n    at <unknown> (http://localhost/__cypress/runner/cypress_runner.js:119010:77)\n    at tryCatcher (http://localhost/__cypress/runner/cypress_runner.js:1807:23)\n    at Promise._settlePromiseFromHandler (http://localhost/__cypress/runner/cypress_runner.js:1519:31)\n    at Promise._settlePromise (http://localhost/__cypress/runner/cypress_runner.js:1576:18)\n    at Promise._settlePromise0 (http://localhost/__cypress/runner/cypress_runner.js:1621:10)\n    at Promise._settlePromises (http://localhost/__cypress/runner/cypress_runner.js:1701:18)\n    at _drainQueueStep (http://localhost/__cypress/runner/cypress_runner.js:2407:12)\n    at _drainQueue (http://localhost/__cypress/runner/cypress_runner.js:2400:9)\n    at Async._drainQueues (http://localhost/__cypress/runner/cypress_runner.js:2416:5)\n    at Async.drainQueues (http://localhost/__cypress/runner/cypress_runner.js:2286:14)\nFrom Your Spec Code:\n    at Context.wpCli (webpack:///../../node_modules/@10up/cypress-wp-utils/lib/commands/wp-cli.js:23:0)",
                "diff": null
              },
              "uuid": "bc33304f-9d27-47f7-91bd-fc8b95dcff4b",
              "parentUUID": "ea14640d-eb3f-43e4-a81e-0e5b13765920",
              "isHook": false,
              "skipped": false
            }

Here we need to change

cy.exec('npm --silent run env run tests-cli \"post get 33 --field=content --url=http://localhost/second/\"')

to

cy.exec('npx wp-env run tests-cli post get 33 --field=content --url=http://localhost/second/')

That command is coming from here - https://github.com/10up/cypress-wp-utils/blob/develop/src/commands/wp-cli.ts#L21.

I think we should create a separate issue to fixing build pipeline given the work it's requiring so it can be completed in parallel to this PR.

Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

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

@kirtangajjar thanks for the fixes, just a couple of minor suggestions below.

I think we should create a separate issue for fixing the build pipeline given the work it requires so it can be completed in parallel to this PR.

Agree, please create a separate ticket and we can put this ticket on hold until E2Es are fixed in the other PR, just to confirm that E2Es are not failing due to this PR.

includes/utils.php Outdated Show resolved Hide resolved
includes/utils.php Show resolved Hide resolved
This was referenced Jul 5, 2024
@kirtangajjar
Copy link
Member Author

@faisal-alvi I've resolved all your comments. LMK there is any additional feedback.

@kirtangajjar kirtangajjar changed the title Add categories to pull screen Add categories to pull screen - 1 Jul 9, 2024
@faisal-alvi
Copy link
Member

@kirtangajjar Thanks for the update. Let's wait on the review/merge until the E2E tests are fixed in #1242 and merged. After that, we'll merge develop here to run the E2E tests and verify if they pass.

@faisal-alvi faisal-alvi removed their request for review July 16, 2024 09:26
@github-actions github-actions bot added the needs:refresh This requires a refreshed PR to resolve. label Aug 5, 2024
@faisal-alvi
Copy link
Member

@kirtangajjar can you please resolve the conflicts?

@github-actions github-actions bot removed the needs:refresh This requires a refreshed PR to resolve. label Aug 5, 2024
@kirtangajjar
Copy link
Member Author

@faisal-alvi Strange, I don't see any conflicts. Only some flakey tests are failing

@Sidsector9
Copy link
Member

@kirtangajjar there's a syntax error in the oembed test. Can you have a look at that?

Error: Timed out retrying after 20000ms: Syntax error, unrecognized expression: #[object Object] input[aria-label="Twitter URL"]

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.

All tests are passing locally:

Screenshot 2024-08-19 at 8 59 08 PM

Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

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

@kirtangajjar Thanks for the updates. I tested the changes, and they work well with external connections. However, I noticed incorrect category links on internal sites.

Here’s the issue:

Main Site: When pulling posts from Sub Site 1, the categories link back to the Main Site instead of Sub Site 1.

image

Sub Site 1: When pulling posts from Sub Site 2, the categories link to Sub Site 1 instead of Sub Site 2.

image

Comment on lines -257 to +276
* @since 0.8
* @since 2.0.5
Copy link
Member

Choose a reason for hiding this comment

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

Why is the version bumped for this existing function?

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add Category to bulk pull screen
4 participants