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

fix: user roles should return empty #2865

Merged
merged 3 commits into from Jul 26, 2023
Merged

Conversation

j3ang
Copy link
Contributor

@j3ang j3ang commented Jul 24, 2023

What does this implement/fix? Explain your changes.

When my user does not have any roles assigned. wp-graphql is returning all existing roles instead of null or empty roles.

Does this close any currently open issues?

Any relevant logs, error output, GraphiQL screenshots, etc?

CleanShot 2023-07-24 at 15 43 49

Any other comments?

Where has this been tested?

Operating System: aws lightsail bitnami wordpress

WordPress Version: 6.2

Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

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

Thanks @j3ang !

It would probably be more performant if we bail before the ConnectionResolver, instead of forcing the resolver to run a query that we know will return null. Would you mind updating the resolver logic?

E.g.

if ( empty( $user->roles ) ) {
  return null;
}

$resolver = new UserRoleConnectionResolver( $user, $args, $context, $info );
$resolver->set_query_arg( 'slugIn', $user->roles );

return $resolver->get_connection();

@jasonbahl jasonbahl self-requested a review July 24, 2023 21:07
@jasonbahl
Copy link
Collaborator

@j3ang would you be willing to write a test for this to ensure we don't have regressions on this in the future?

Something similar to the tests here: https://github.com/wp-graphql/wp-graphql/blob/develop/tests/wpunit/UserRoleConnectionQueriesTest.php

we'd want a test that:

  • Creates a user with no rule
  • Executes the query you reported this issue with
  • Asserts that no roles are returned for that user

The test should fail on current latest WPGraphQL and pass with your change in place.

@codeclimate
Copy link

codeclimate bot commented Jul 25, 2023

Code Climate has analyzed commit 57ecab5 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@jasonbahl
Copy link
Collaborator

@j3ang thanks for adding the test 🙌🏻

@jasonbahl
Copy link
Collaborator

I made some modifications locally, but Git isn't letting me push to your branch, so I will merge this and follow-up with the changes

@jasonbahl jasonbahl merged commit 87f4de5 into wp-graphql:develop Jul 26, 2023
12 of 32 checks passed
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.

None yet

3 participants