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

Switch back to getAuthors #26554

Merged
merged 11 commits into from
Nov 19, 2020
Merged

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Oct 28, 2020

Description

Fixes #26476

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@adamsilverstein adamsilverstein marked this pull request as ready for review October 29, 2020 21:43
const users = yield apiFetch( {
path: `/wp/v2/users?who=authors&include=${ id }`,
} );
yield receiveUserQuery( 'author', users );
Copy link
Contributor

Choose a reason for hiding this comment

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

should the first argument here be "authors" to match the same entity as "getAuthors"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? I think that results in the fetched author overwriting the authors? I can test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested changing this and confirmed using the same key results in the query interfering with the getAuthors query, causing errors or unexpected results as the results from the two queries overwrite each other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm, so if I'm reading this right getAuthor(1) and getAuthor(2) are using the same query key which means if I trigger both concurrently, they'll mess with each other's result.

Copy link
Member Author

Choose a reason for hiding this comment

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

@youknowriad - Oh, good point. I didn't think about concurrent requests. Should we build a dynamic key here that includes the requested id? Do we have an existing component that uses this approach/a helper to create the query key?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I did add dynamic query keys for both getAuthor and getAuthors, I think it will solve both issues so I reverted the change you did with useMemo

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 30, 2020
const author = getUser( getEditedPostAttribute( 'author' ) );
const author =
postAuthor ||
__unstableGetAuthor( getEditedPostAttribute( 'author' ) )[ 0 ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, why do we need [ 0 ] here. getAuthor only returns one author right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it is returned as an array with a single element. I can change this so __unstableGetAuthor returns a single element which makes more sense based on the naming, does that sound good?

@youknowriad
Copy link
Contributor

Seems like there's some valid test failures.

@adamsilverstein
Copy link
Member Author

Seems like there's some valid test failures.

I'll take a look.

@adamsilverstein
Copy link
Member Author

adamsilverstein commented Nov 4, 2020

Seems like there's some valid test failures.

@youknowriad One issue was the selector was showing even when the site only had one author.

I had removed the check for authors.length <= 1 when switching to getAuthors because the authors in check.js uses the same data store as the selector itself, so when you started searching and got zero results, the authors in the check would also be empty and the selector would disappear!

To alleviate this I added originalAuthors behind a memo call in cead69d (#26554) so it is only set once. What do you think?

*/
export function __unstableGetAuthor( state ) {
const authors = getUserQueryResults( state, 'author' );
return authors ? authors[ 0 ] : [];
Copy link
Contributor

Choose a reason for hiding this comment

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

It should probably return null if the author was not found.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, will change.

@youknowriad
Copy link
Contributor

the same data store as the selector itself, so when you started searching and got zero results, the authors in the check would also be empty and the selector would disappear!

I think that's the issue, both selectors use different queries, so they shouldn't share the same "query key".

@adamsilverstein
Copy link
Member Author

I think that's the issue, both selectors use different queries, so they shouldn't share the same "query key".

Right - that is why I changed the single author query key to 'author' vs 'authors'

@youknowriad
Copy link
Contributor

This is looking good for me, but it could use some testing @adamsilverstein

@skorasaurus
Copy link
Member

skorasaurus commented Nov 16, 2020

Hi,

I tested this out (last commit - f507edb ) on a test site [wordpress 5.5.3] with no other plugins installed except this one and the select author dropdown does not appear, even as an admin user. I created another user with the 'editor' role just to be sure that the author drop down didn't display if there was only one registered user and still had the same result.

@tellthemachines
Copy link
Contributor

Tested this branch on top of WP 5.6-beta4 and can confirm the post author dropdown isn't showing for either editor or admin users. The PostAuthor component doesn't seem to be rendering at all now 😕

@youknowriad
Copy link
Contributor

Yep, i just realized I forgot to fix the query key in the selector.😅

@skorasaurus
Copy link
Member

Thanks for the additional fix; with that last commit, I now confirm that the postauthor component now displays as intended and is keyboard accessible as well.

@adamsilverstein
Copy link
Member Author

@youknowriad - Great! Works well in my testing.

Copy link
Member Author

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

👍 Looks good!

@youknowriad youknowriad merged commit 1db8243 into WordPress:master Nov 19, 2020
@github-actions github-actions bot added this to the Gutenberg 9.5 milestone Nov 19, 2020
tellthemachines pushed a commit that referenced this pull request Nov 30, 2020
@tellthemachines tellthemachines mentioned this pull request Nov 30, 2020
6 tasks
tellthemachines added a commit that referenced this pull request Dec 1, 2020
* Provide a minimum of code wrapping for the code block. (#26623)

* Block Support: Fix font size style when applying block support (#26762)

* Fix Separator editor styles (#27071)

* Fix the Post author selector for contributors (#26554)

Co-authored-by: Riad Benguella <[email protected]>

* Align single half width column to left (#27142)

* remove the auto margin for individual column blocks

* update margin values for blocks in blocks to zero insted of auto

* Add backward compatibility support for lightBlockWrapper in getSaveElement (#27189)

* Code block: paste plain text (#27236)

* paste plain text option

* Add e2e test

* Fix crash when null date passed to TimePicker (#27316)

* Fix crash when null date passed.

* Update test

* Fix GH actions "cancel" step (#27025)

* use new syntax for setting env var

* Update package-lock

* Update package-lock again

* Remove the button only option from the UI until it can be wired up to something that works in the front end. (#27379)

* Fix combobox csuggestion list closure when clicking scrollbar (#27367)

Co-authored-by: Joen A <[email protected]>
Co-authored-by: Aaron Robertshaw <[email protected]>
Co-authored-by: Nik Tsekouras <[email protected]>
Co-authored-by: Adam Silverstein <[email protected]>
Co-authored-by: Riad Benguella <[email protected]>
Co-authored-by: andrei draganescu <[email protected]>
Co-authored-by: Daniel Richards <[email protected]>
Co-authored-by: Ella van Durpe <[email protected]>
Co-authored-by: Noah Allen <[email protected]>
Co-authored-by: Andy Peatling <[email protected]>
@tellthemachines tellthemachines removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New authors dropdown breaks author selection for editors
4 participants