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

Show all users in post author column. #106

Merged
merged 6 commits into from
Aug 18, 2022
Merged

Show all users in post author column. #106

merged 6 commits into from
Aug 18, 2022

Conversation

robindevitt
Copy link
Contributor

@robindevitt robindevitt commented Aug 15, 2022

Related issue : https://github.com/humanmade/product-dev/issues/993

Issue

When viewing the posts screen you don't see all the authors of a given post.

Solution

The function get_author_ids wasn't returning the author ID's of all posts. This PR now shows all users in the column even when the user hasn't been assigned a role.

Steps to test

  1. Ensure you have a post where the post author is a super admin that hasn't been added to the site.
  2. Viewing all posts this will be identifiable by a post which has no author listed. It will be blank.
    Screenshot 2022-08-16 at 10 11 51
  3. Applying the PR allows the site to reference a user despite them having no assigned role on the site, Altis Dev is a super admin and isn't assigned to the current site with any role/s :
    Screenshot 2022-08-16 at 10 12 22

@robindevitt robindevitt marked this pull request as ready for review August 16, 2022 07:57
@robindevitt
Copy link
Contributor Author

@roborourke when you get a chance please can you review my current suggested fix for this.

Copy link
Contributor

@roborourke roborourke left a comment

Choose a reason for hiding this comment

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

Couple of questions, may just need to add code comments explaining the logic

inc/template.php Show resolved Hide resolved
inc/template.php Outdated
// If $authors is an error or empty.
if ( is_wp_error( $authors ) || empty( $authors ) ) {
// Check if the post_type supports Authorship.
if ( is_post_type_supported( $post->post_type ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the existing logic but it’s not clear why. For any post type that doesn’t support authorship the function will now always return an empty array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll test some more and see if I can tweak this further

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this change was needed at all to be honest, what lead you to altering it?

Copy link
Contributor Author

@robindevitt robindevitt Aug 16, 2022

Choose a reason for hiding this comment

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

The function get_author_ids currently checks if a post type is not supported and then followed by doing a wp_get_post_terms which if the user is a super admin it returns an empty array.

So my thinking was to perform the check first for wp_get_post_terms if it returns an error or was empty then it would return the post author details. Looking at it now it could probably do with dropping the is_post_type_supported check too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it can just be put back how it was - the problem was fixed just adding the blog_id => 0 argument right?

The change you have here will now cause a DB query even if the post type does not support multiple authors. Sometimes condensing logic like this doesn't help with performance or readability.

It's always good to do one thing at a time, ideally starting with conditionals that are the least effort to process, e.g. not checking the db.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

blog_id => 0 allows us to get the user who isn't assigned to the existing site when we have an ID. However with the current code in place for get_author_ids it returns an empty array still which will still show the reported issue.

So this change will ensure an author ID is returned.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could the problem be that when saving the author terms they aren't being stored if the user doesn't have a role on the current site?

This is still hiding the real issue I think - wp_get_post_terms( $post->ID, TAXONOMY ); should be returning IDs even for users that don't strictly have a role on the site.

@roborourke
Copy link
Contributor

Cheers both

@roborourke roborourke merged commit ad162b1 into develop Aug 18, 2022
@roborourke roborourke deleted the show-all-authors branch August 18, 2022 09:36
@roborourke roborourke mentioned this pull request Aug 18, 2022
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.

4 participants