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: [WIP] unstable term cursor identical names #2930

Conversation

mohjak
Copy link

@mohjak mohjak commented Sep 14, 2023

What does this implement/fix? Explain your changes.

This is mainly stabilizing the term cursor when there are more than one term have the same term name.

Does this close any currently open issues?

relates to #2926 but it doesn't close it because there are 2 failed tests after applying the changes:

---------
1) MenuConnectionQueriesTest: Query with first and last
 Test  tests/wpunit/MenuConnectionQueriesTest.php:testQueryWithFirstAndLast
Failed asserting that two arrays are identical.
- Expected | + Actual
@@ @@
Array &0 (
-    'databaseId' => 153
+    'databaseId' => 155
)
#1  /var/www/html/wp-content/plugins/wp-graphql/tests/wpunit/MenuConnectionQueriesTest.php:354
#2  /var/www/html/wp-content/plugins/wp-graphql/vendor/bin/codecept:120

---------
2) TermObjectConnectionQueriesTest: Query with first and last
 Test  tests/wpunit/TermObjectConnectionQueriesTest.php:testQueryWithFirstAndLast
Failed asserting that two arrays are identical.
- Expected | + Actual
@@ @@
Array &0 (
-    'databaseId' => 299
+    'databaseId' => 301
)
#1  /var/www/html/wp-content/plugins/wp-graphql/tests/wpunit/TermObjectConnectionQueriesTest.php:332
#2  /var/www/html/wp-content/plugins/wp-graphql/vendor/bin/codecept:120

EDIT:

closes: #2926

The above tests have been fixed. see: #2930 (comment)

mohjak added 2 commits September 14, 2023 10:20
wp-graphql#2926

Terms cursor now handles identical names accurately with asc/desc and after/before.
@codeclimate
Copy link

codeclimate bot commented Sep 14, 2023

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

View more on Code Climate.

@mohjak mohjak changed the title Bugfix 2926/fix unstable term cursor identical names fix: [WIP] unstable term cursor identical names Sep 14, 2023
@jasonbahl jasonbahl added the Needs: Reviewer Response This needs the attention of a codeowner or maintainer. label Sep 19, 2023
@jasonbahl
Copy link
Collaborator

@mohjak I've confirmed that the changes work when testing locally.

I'll take a look at the menu tests 👀 and also ensure we have proper tests for term queries to account for these changes.

…s to fail

- @todo: add tests to cover the original issue scenario: paginating terms that have the same name and the query orders by name
@jasonbahl
Copy link
Collaborator

@mohjak I pulled down your branch to see if I could diagnose why the 2 tests are failing.

It seems that this change is causing the tests to fail.

I added it back and the tests pass.


Below is an overview of the behavior I'm experiencing now.

Similar to your scenario, I've created a category with several child categories, and I've given all the children the same name "Child 1"

CleanShot 2023-10-25 at 10 08 32

BEFORE:

Query for the first 3 categories that are children of category:70 (works roughly as expected. . .I get 3 of the child categories of category:70)

CleanShot 2023-10-25 at 10 13 51

Use the cursor for category:77 as the value for the after cursor.

Expectation: see 3 more categories that are children of category:70

Actual: No results:

CleanShot 2023-10-25 at 10 17 12

AFTER:

Now, with this PR (your changes + my change to add back the previously removed code)...

Query for the first 3 categories that are children of category:70 (I get 3 of the child categories of category:70)

CleanShot 2023-10-25 at 10 18 26

Now, we can take the cursor for category:80 and use it as the after cursor:

Expectation: See categories 79, 78, 77

Actual: see categories 79, 78, 77

CleanShot 2023-10-25 at 10 20 29

@jasonbahl
Copy link
Collaborator

@mohjak I pushed some commits to fix a broken test unrelated to this feature (the akismet plugin had a name change that was causing some plugin query tests to fail)

If you could confirm that this fixes the issue(s) you were running into, that would be great 🙏🏻

@jasonbahl jasonbahl merged commit ef67e99 into wp-graphql:develop Oct 27, 2023
30 checks passed
@jasonbahl jasonbahl mentioned this pull request Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Reviewer Response This needs the attention of a codeowner or maintainer.
Projects
None yet
2 participants