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/distribute authors #17

Merged
merged 22 commits into from
Dec 15, 2023
Merged

Feat/distribute authors #17

merged 22 commits into from
Dec 15, 2023

Conversation

leogermani
Copy link
Contributor

@leogermani leogermani commented Nov 28, 2023

Distributes authors upon post distribution via the Distributor plugin

When a post is distributed via Distributor, all the authors (and co-authors if CAP is enabled) information will be attached to the post.

In the site where the post was distributed to, the authorship information will be stored in a post meta, and later will be used to override the bylines of the posts.

Also in the target site, every author that is a WP user in the origin site, will be created as an author if an user with that email yet doesn't exist. Then the post authorship will be assigned to the distributed authors, in the same way they are in the origin site.

Guest Authors will not be created and the authorship will not be established. Information about Guest Authors will only exist in the post_meta, and will be used to filter the byline on the fly (in a future PR).

How to test

  • Establish a network with 2 or 3 sites. (newspack_docker is your friend here, since it automatically creates a bunch of sites and connect them both on Newspack Network plugin and on Distributor plugin, creating needed the keys and application passwords)
  • On one of the sites, create a post, and assign it multiple authors, having at least one wp user and one guest author
  • Distribute this post via push to another site
  • In the target site, observe the WP_User was created and the authorship was properly assigned
  • Observe the newspack_network_authors post_meta and see all authors are there, including the guest author
  • Repeat the process, now pulling a post from a site instead of pushing
  • Make sure to fill all fields of the authors and confirm all fields are propagated (name, bio, site, job, etc)

@leogermani leogermani marked this pull request as ready for review December 1, 2023 21:58
@leogermani leogermani self-assigned this Dec 1, 2023
@leogermani leogermani requested a review from a team as a code owner December 6, 2023 19:28
@leogermani leogermani mentioned this pull request Dec 12, 2023
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.

The newspack_network_authors meta is being populated correctly and I was able to see the multiple WP_User authors after disabling this setting from Distributor:

image

A few questions/suggestions inline:

Comment on lines 96 to 99
if ( is_a( $co_author, 'WP_User' ) ) {
$authors[] = self::get_wp_user_for_distribution( $co_author );
}
$authors[] = self::get_guest_author_for_distribution( $co_author );
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't line 99 be inside an else block? If $co_author is a user, it's being added twice (through the wp_user and guest methods).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in df7f3d8

}

if ( ! $user ) {
return [];
Copy link
Member

Choose a reason for hiding this comment

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

Why not use WP_Error here for improved debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly because all this happens as side effects in the background and we don't have any opportunity to surface these errors to the user. How would you debug it?

But yes, I guess we could return a wp_error there... but then we need more lines in the method that call this to check what has been returned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really seeing a benefit of returning a WP_Error, but added some debugging on 2e26d03

public static function get_guest_author_for_distribution( $guest_author ) {

if ( ! is_object( $guest_author ) || ! isset( $guest_author->type ) || 'guest-author' !== $guest_author->type ) {
return [];
Copy link
Member

Choose a reason for hiding this comment

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

Same for debugging, a WP_Error could be good here.


Debugger::log( 'Ingesting author: ' . $author['user_email'] );

$user = User_Utils::get_or_create_user_by_email( $author['user_email'], get_post_meta( $post_id, 'dt_original_site_url', true ), $author['id'] );
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit confusing. Is the 3rd argument meant to be the author ID or the site ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The author id. Why confusing? When we create a user that has been propagated by another site we store their original ID

Copy link
Member

Choose a reason for hiding this comment

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

Because according to the code inline docs:

@param string $remote_site_id The ID of the remote site. Used only when a new user is created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I'll fix the docs. It should be The id ON the remote site... I'll make it clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 92c6b32

@miguelpeixe
Copy link
Member

One other thing, the display name is set as the author's email. Can we use the same from the origin site?

Original Distributed
image image

John Doe in the example above is a guest author, so it's expected to be missing from the distributed post.

@leogermani
Copy link
Contributor Author

One other thing, the display name is set as the author's email. Can we use the same from the origin site?

Good catch! Fixed in b529b36.

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!

@leogermani leogermani merged commit 62c69fe into master Dec 15, 2023
3 checks passed
@matticbot
Copy link
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.0.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.

3 participants