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: return empty when filtering menuItems by a location with no assigned items #3043

Merged

Conversation

justlevine
Copy link
Collaborator

@justlevine justlevine commented Feb 11, 2024

What does this implement/fix? Explain your changes.

This PR fixes an issue in the MenuItemConnectionResolver logic, where querying a menuItem by a location with unset terms would return all menu items, instead of none.

Logic has been changed so the tax_query args are given a 'term_id' => [].

Tests have been added to cover:

  • A location with no menu items returns null.
  • A location with no menu items returns null as an admin.
  • A location with menu items, doesn't return items from a different location (either public or as an admin).

Does this close any currently open issues?

Fixes #3029

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

Any other comments?

Where has this been tested?

Operating System: Ubuntu 20.04 (wsl2 + devilbox + php 8.1.14)

WordPress Version: 6.4.3

Copy link

codeclimate bot commented Feb 11, 2024

Code Climate has analyzed commit 670b1bf and detected 0 issues on this pull request.

View more on Code Climate.

@justlevine justlevine added Type: Bug Something isn't working Component: Connections Issues related to connections ObjectType: Menu Status: In Review Needs to be reviewed by a project maintainer before merge labels Feb 11, 2024
@coveralls
Copy link

Coverage Status

coverage: 84.275%. remained the same
when pulling 670b1bf on justlevine:fix/menu-items-location-filter
into afe6a52 on wp-graphql:develop.

@jasonbahl
Copy link
Collaborator

@justlevine I pulled this branch and querying for a list of menuItems when no menu has been assigned to a location still returns random menu items 🤔

CleanShot 2024-02-13 at 16 51 04

@jasonbahl
Copy link
Collaborator

@justlevine ah my bad. I was executing as an authenticated user who should see all menu items because they're not considered private. 🤦🏻‍♂️

@jasonbahl jasonbahl merged commit 387b289 into wp-graphql:develop Feb 13, 2024
24 checks passed
@justlevine
Copy link
Collaborator Author

@justlevine ah my bad. I was executing as an authenticated user who should see all menu items because they're not considered private. 🤦🏻‍♂️

If my fix worked correctly then even if the user is authenticated they should only receive the items they're actually filtering for, so it should still come back []. 🤔

@jasonbahl
Copy link
Collaborator

ok, ya, authenticated users still get the incorrect behavior then.

@jasonbahl
Copy link
Collaborator

@justlevine let me confirm that I didn't have anything else going on. . .like WPGraphQL Smart Cache object cache active or anything 👀

@jasonbahl
Copy link
Collaborator

@justlevine oooh. . .so this was related to WPGraphQL Smart Cache, but not really related to caching. . .

WPGraphQL Smart Cache declares WPGraphQL as a dev dependency and my local copy of WPGraphQL Smart Cache was overriding the checked out version of WPGraphQL I was testing 🤔

Interesting.

My hunch is that we don't actually need WPGraphQL defined as a dev dependency for WPGraphQL Smart Cache. That's wild though that the dev dependency was taking precedent over the actual active WPGraphQL plugin 🤔

Anyway, by de-activating WPGraphQL Smart Cache I was able to experience the intended behavior of no nodes resolving when filtering menus by location when that location has no menu assigned yet (even for admin user)

@justlevine justlevine deleted the fix/menu-items-location-filter branch February 14, 2024 00:29
@jasonbahl jasonbahl mentioned this pull request Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Connections Issues related to connections ObjectType: Menu Status: In Review Needs to be reviewed by a project maintainer before merge Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Menu returning wrong items
3 participants