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

Fix Yoast primary category #41

Merged
merged 5 commits into from
Feb 9, 2024
Merged

Fix Yoast primary category #41

merged 5 commits into from
Feb 9, 2024

Conversation

adekbadek
Copy link
Member

All Submissions:

Changes proposed in this Pull Request:

With this change, Yoast's primary category will be interpreted on the Node site. Even though the taxonomies are sync'd between sites by Distributor, Yoast's primary category – stored as post meta – was not handled, and its ID still pointed to the ID of the category on the Hub site.

See 1200550061930446-as-1206399433499334/f

How to test the changes in this Pull Request:

  1. On the hub site, create a new category and publish a post after setting the new category as the primary category
  2. Observe that the distributed post on the Node site has the correct primary category set*
  3. Create yet another new category and update that same post so now it has this new category as the primary category
  4. Again observe that the distribute post on the Node site has the correct primary category set

* this is stored as the _yoast_wpseo_primary_category post meta

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@adekbadek adekbadek requested a review from a team as a code owner February 6, 2024 10:10
@leogermani
Copy link
Contributor

Didn't work for me.

I've tested both pushing and pulling posts and the value of the _yoast_wpseo_primary_category post meta still shows the cat ID as it is in the site where the post was originally published.

Unrelated to this PR, once we get this merged I'll get this file refactor.

To have a file like this seemed like a good idea for small snippets, to avoid all the overhead of new files and classes for simple and small tweaks. But it's starting to grow too much. Let's break this into a class for each thing...

@adekbadek
Copy link
Member Author

once we get this merged I'll get this file refactor.

Already done in #42

Didn't work for me.

I'll work on JN test site(s), so it can be debugged.

@adekbadek
Copy link
Member Author

@leogermani – should be fixed with 1da862c

@leogermani
Copy link
Contributor

leogermani commented Feb 6, 2024 •
edited
Loading

@adekbadek It fixed things for when pushing a post, but not for pulling. You also need to add the cat slug to dt_pull_post_args filter.

once we get this merged I'll get this file refactor.
Already done in #42

Nice! I'll go even further and will create one file/class for each thing

@adekbadek
Copy link
Member Author

Thanks @leogermani ! 9403053 should fix it 👌

@leogermani
Copy link
Contributor

Sorry, but it didn't work yet. Pulling content still gives me the wrong (original) cat id

leogermani
leogermani reviewed Feb 8, 2024
View reviewed changes
includes/distributor-customizations/global.php Outdated
*/
function newspack_network_get_primary_category_slug( $post, $is_pulling = false ) {
if ( $is_pulling ) {
// When pulling content, the post will be the Node post.
Copy link
Contributor

@leogermani leogermani Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The "Hub" and "Node" terms don't apply here. Posts can be distributed from any site to any site (node to node, node to hub, etc).

It's better to say the origin and target sites, or something like that

@adekbadek
Copy link
Member Author

@leogermani – 3786e46 should do it

leogermani
leogermani approved these changes Feb 8, 2024
View reviewed changes
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

It works.

I tested all possible combinations, and also confirmed that the category is created in the site before we attempt to fix the metadata if the cat doesnt exist.

I left two very minor comments.

includes/distributor-customizations/global.php
*
* @param WP_Post $post The post object.
*/
function newspack_network_fix_primary_category( $post ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove the "hub" jargon from this method

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 29ee85f

includes/distributor-customizations/global.php Outdated
* @param WP_Post $post The post object.
*/
function newspack_network_fix_primary_category( $post ) {
$primary_category_slug = get_post_meta( $post->ID, 'yoast_primary_category_slug', true );
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd add a newspack_network prefix to this meta name (and maybe make it shorter. it confuses with the actual yoast meta.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 29ee85f

@adekbadek adekbadek merged commit 3457d19 into trunk Feb 9, 2024
3 checks passed
@adekbadek adekbadek deleted the fix/yoast-primary-cat branch February 9, 2024 07:48
matticbot pushed a commit that referenced this pull request Feb 15, 2024
@semantic-release-bot
chore(release): 1.2.0-alpha.2 [skip ci]
a07cc1d 
# [1.2.0-alpha.2](v1.2.0-alpha.1...v1.2.0-alpha.2) (2024-02-15)

### Bug Fixes

* assorted error handling fixes ([#52](#52)) ([234f883](234f883))
* dynamic class property deprecated warnings ([#47](#47)) ([693865c](693865c))
* prevent coauthors capability check infinite loop ([#46](#46)) ([9565cef](9565cef))
* set Yoast primary category ([#41](#41)) ([3457d19](3457d19))

### Features

* sync billing and shipping addresses ([#50](#50)) ([6a05580](6a05580))
* sync publish, trash post statuses ([#42](#42)) ([fd5d8b9](fd5d8b9))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.2.0-alpha.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Feb 20, 2024
@semantic-release-bot
chore(release): 1.2.0 [skip ci]
7475bb9 
# [1.2.0](v1.1.0...v1.2.0) (2024-02-20)

### Bug Fixes

* assorted error handling fixes ([#52](#52)) ([234f883](234f883))
* **data-listeners:** handle no user in wc data update ([4eeefc0](4eeefc0))
* dynamic class property deprecated warnings ([#47](#47)) ([693865c](693865c))
* prevent coauthors capability check infinite loop ([#46](#46)) ([9565cef](9565cef))
* race condition when creating a Node ([1946473](1946473))
* set Yoast primary category ([#41](#41)) ([3457d19](3457d19))

### Features

* **ci:** add epic/* release workflow and rename `master` to `trunk` ([#39](#39)) ([9cee51d](9cee51d))
* sync billing and shipping addresses ([#50](#50)) ([6a05580](6a05580))
* sync publish, trash post statuses ([#42](#42)) ([fd5d8b9](fd5d8b9))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.2.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leogermani leogermani leogermani approved these changes

Assignees
No one assigned
Labels
Distributor released on @alpha released [Status] Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adekbadek @leogermani @matticbot