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

feat(content-distribution): Sync authors #194

Merged
merged 37 commits into from
Jan 31, 2025

Conversation

naxoc
Copy link
Member

@naxoc naxoc commented Jan 20, 2025

This copies most of the existing code for distributed post authors.

I tried to make the naming of classes make a bit more sense (using outgoing/incoming) naming-wise, but I think I might have lost my way somewhere in the process. It's still pretty unwieldy. Suggestions very welcome!

One thing I still lack: when a post is new and I click "publish" and then distribute it (without having touched authors), the cap authors are not added to the payload. I think the best way to fix that is to save the post from JS before distributing the first time. What do you think @miguelpeixe ?

How to test

  • Try distributing a post without having CAP enabled. Check that the author is transferred in a way that makes sense
  • Try with an author that is a guest contributor. Also try mixing "normal" users and guest contributors.
  • Try with Guest Authors enabled. Try mixing here too.

Still remains:

@naxoc naxoc self-assigned this Jan 20, 2025
@naxoc naxoc marked this pull request as ready for review January 29, 2025 18:56
@naxoc naxoc requested a review from a team as a code owner January 29, 2025 18:56
@naxoc naxoc requested a review from miguelpeixe January 30, 2025 14:23
@naxoc
Copy link
Member Author

naxoc commented Jan 30, 2025

Thanks for the review @miguelpeixe

I implemented your suggestions. If you think the whole hooks thing for multiple authors is over engineering things, let me know and I can change them to function calls.

Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

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

Thank you for the revisions, @naxoc! It's looking great.

Maybe you missed this bit from my previous review:

I'm able to get multiple authors synced, but with the standard guest author approach from CAP I'm unable to get the guest authors distributed.

I'm activating it via define( 'NEWSPACK_ENABLE_CAP_GUEST_AUTHORS', true ); on all sites. Should I be doing something different?

I'm still unable to get it to work. The payload does include the data, though:

{
    "site_url": "http:\/\/localhost",
    "post_id": 827,
    "post_url": "http:\/\/localhost\/2025\/01\/my-new-cap-post\/",
    "network_post_id": "6dc7943077f7903f656015af0ebb029e",
    "sites": [
        "http:\/\/node1.test"
    ],
    "status_on_create": "draft",
    "post_data": {
        "multiple_authors": [
            {
                "type": "wp_user",
                "ID": 1,
                "display_name": "wordpress",
                "user_email": "[email protected]",
                "user_url": "http:\/\/localhost"
            },
            {
                "ID": "825",
                "display_name": "My CAP Guest Author",
                "first_name": "",
                "last_name": "",
                "user_login": "my-cap-guest-author",
                "user_email": "[email protected]",
                "linked_account": "",
                "website": "",
                "description": "",
                "newspack_job_title": "",
                "newspack_role": "",
                "newspack_employer": "",
                "newspack_phone_number": "",
                "user_nicename": "my-cap-guest-author",
                "type": "guest_author",
                "nickname": ""
            },
            {
                "type": "wp_user",
                "ID": 2,
                "display_name": "Contributor",
                "user_email": "[email protected]",
                "first_name": "Contributor",
                "last_name": "Contributor"
            }
        ],
        "date_gmt": "2025-01-30 17:37:01",
        "modified_gmt": "2025-01-30 17:39:22"
    },
    "partial": true
}

Comment on lines 55 to 57
* Action callback.
*
* Add a postmeta entry with the Co-Authors Plus authors for outgoing posts.
Copy link
Member

Choose a reason for hiding this comment

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

This docblock needs an update.

@@ -202,15 +202,20 @@ public function get_payload_hash( $payload = null ) {
* @return array|WP_Error The post payload or WP_Error if the post is invalid.
*/
public function get_payload( $status_on_create = 'draft' ) {
$post_author = $this->post->post_author ? Outgoing_Author::get_wp_user_for_distribution( $this->post->post_author ) : [];
$multiple_authors = apply_filters( 'newspack_network_multiple_authors_for_post', [], $this->post );
Copy link
Member

@miguelpeixe miguelpeixe Jan 30, 2025

Choose a reason for hiding this comment

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

Ah, I see. I didn't realize that's how the CAP data is being added to the payload.

Non-blocking suggestion: maybe instead of filtering this specific property we can go with a more generic approach and filter the payload post_data to let additional features determine their properties. That'll also generate a cleaner payload for sites that don't have CAP at all - no permanent empty array.

@naxoc
Copy link
Member Author

naxoc commented Jan 30, 2025

Woops. I didn't click comment on this one:

I'm able to get multiple authors synced, but with the standard guest author approach from CAP I'm unable to get the guest authors distributed.

I copied the behavior from the existing code that handles guest authors It would be great to also have it work in admin – at least in the posts listing 🤔 It works for me. They only show on the front end, so might that have been it? Let me know if the front end is not showing them for you.

@miguelpeixe
Copy link
Member

They only show on the front end, so might that have been it? Let me know if the front end is not showing them for you.

Ah! My apologies, that's indeed the case. I was expecting it to sync them exactly. Confirmed it's working as intended:

image

@naxoc
Copy link
Member Author

naxoc commented Jan 30, 2025

Thank you Miguel – I love the idea of filtering the whole post_data part instead. I'll add that tomorrow 👍

@naxoc
Copy link
Member Author

naxoc commented Jan 31, 2025

OK, @miguelpeixe I added the payload filtering. That is a great idea – it makes everything much more readable and extendable. Ready for re-review :)

@naxoc naxoc requested a review from miguelpeixe January 31, 2025 12:24
Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

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

Thanks, @naxoc! Looks good and tests well 💯

One small ✂️ and we're good to go!

@naxoc naxoc merged commit 3156d35 into trunk Jan 31, 2025
4 checks passed
Copy link

Hey @naxoc, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

@naxoc naxoc deleted the feat/content-distribution-authors branch January 31, 2025 13:35
matticbot pushed a commit that referenced this pull request Feb 7, 2025
# [2.6.0-alpha.1](v2.5.0...v2.6.0-alpha.1) (2025-02-07)

### Bug Fixes

* **content-distribution:** use payload hash on partial updates ([#207](#207)) ([31b342d](31b342d))
* **event-log:** data css overflow ([#206](#206)) ([f81adfe](f81adfe))

### Features

* **content-distribution:** migration tweaks ([#201](#201)) ([9c61fa8](9c61fa8))
* **content-distribution:** partial payload ([#205](#205)) ([b844d06](b844d06))
* **content-distribution:** remove "Quick Edit" from linked posts ([#200](#200)) ([6ad3a4a](6ad3a4a))
* **content-distribution:** Sync authors ([#194](#194)) ([3156d35](3156d35))
* increase pull frequency and amount ([#209](#209)) ([6791da1](6791da1))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.6.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Feb 17, 2025
# [2.6.0](v2.5.0...v2.6.0) (2025-02-17)

### Bug Fixes

* **content-distribution:** use payload hash on partial updates ([#207](#207)) ([31b342d](31b342d))
* **event-log:** data css overflow ([#206](#206)) ([f81adfe](f81adfe))

### Features

* **content-distribution:** migration tweaks ([#201](#201)) ([9c61fa8](9c61fa8))
* **content-distribution:** partial payload ([#205](#205)) ([b844d06](b844d06))
* **content-distribution:** remove "Quick Edit" from linked posts ([#200](#200)) ([6ad3a4a](6ad3a4a))
* **content-distribution:** Sync authors ([#194](#194)) ([3156d35](3156d35))
* increase pull frequency and amount ([#209](#209)) ([6791da1](6791da1))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.6.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants