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

WP authors migration command - post type argument. #122

Merged
merged 10 commits into from
Sep 14, 2023

Conversation

mattheu
Copy link
Member

@mattheu mattheu commented Jan 19, 2023

The recently added wp authors migration command uses query_posts, but does not provide a way to query multiple post types.

New param defaults to just posts, but supports passing another post type, or comma separated list of post types.

e.g.

wp authorship migrate wp-authors // default post
wp authorship migrate wp-authors --post-type=page
wp authorship migrate wp-authors --post-type=post,page

Copy link
Contributor

@mikelittle mikelittle left a comment

Choose a reason for hiding this comment

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

The code looks good.
But there should probably be one or more tests for this new functionality. I'd suggest testing:

  1. the default value works
  2. posts and pages work
  3. maybe a cpt too

@mattheu
Copy link
Member Author

mattheu commented Sep 5, 2023

Hey @mikelittle - digging up an old pull request here! But the need for this functionality came up again.

We don't have any precedence in the Authorship plugin for writing tests against CLI command functionaltiy... so I did something very basic by calling the function directly and verifying the data changed as expected.

To the cases you suggest...

  1. default - as I'm calling the function directly, the default is not set. But can re-create by passing post
  2. Posts and pages. This is passed as comma separated string, so a case I reckon we should test for.
  3. CPT. I actually don't think this is necessary and a bit of a hassle to register a CPT etc.

Copy link
Contributor

@mikelittle mikelittle left a comment

Choose a reason for hiding this comment

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

I couldn't get the tests to run in my brief attempt at a local set up, but it all looks good to me.

@mattheu
Copy link
Member Author

mattheu commented Sep 9, 2023

Ha me neither. I didn’t really try TBH, I just relied on the CI to run them!

@mikelittle
Copy link
Contributor

All the tests passed. (though they were marked risky because of the output).

@mikelittle mikelittle merged commit bca9c79 into develop Sep 14, 2023
4 checks passed
@mikelittle mikelittle deleted the migrate-authors-post-type branch September 14, 2023 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants