Close the A-Z Filters automated-coverage gap (GVAZ-9/10/11/12) - #66
Merged
Conversation
The widget filters by the `letter` GET parameter and applies its own gravityview/view/query GF_Query condition, but never told GravityView that `letter` is a search. So core's is_search() ignored `?letter=`, and a "Hide entries until search" View stayed hidden when a visitor clicked a letter (ViewRenderer/Widget gate entry loading on is_search()). Downstream consumers missed it too (DataTables' filtered-count total, GVDT-44/GVDT-45). The widget now registers with core: - gk/gravityview/search/request/search-arguments: adds `letter` so the request counts as a search (GravityView 2.55+). - gk/gravityview/search/request/filters: strips `letter` back out of the built filters, since it is not a form field (gf_query_filter applies the filtering). - gravityview/widget/hide_until_searched (@SInCE 1.5.4): reveals the View when a letter is active, covering GravityView older than the search-request pipeline. Adds the extension's first PHPUnit suite (gkunit): reproduces the miss and covers all three hooks. Green on PHP 7.4 and 8.0.
…hed Views Adds the extension's first browser E2E setup (wp-env + Playwright via @gravitykit/e2e-bootstrap/e2e-fixtures), mounting AZ-Filters, GravityView, and Gravity Forms. Seeds a Cities form, entries, a List View, and the az_filter widget through the fixtures REST routes (no editor UI). Two specs: - hideUntilSearchedAZReveal: an active ?letter= reveals only the matching entries on a hide-until-searched View (fails on the pre-fix widget). - azFilterNoHideRegression: plain A-Z filtering still returns the correct entries per letter on a View without hide-until-searched.
Wires the test suites into CI, mirroring the sibling GravityView-extension pattern (DataTables): a `prepare` job provisions Gravity Forms and a GravityView source checkout (git clone + composer install, so its tests/ bootstrap and prefixed Foundation exist), then: - run_unit_tests: npx @gravitykit/phpunit test --parallel (PHP 7.4 + 8.0) - run_e2e_tests: boots wp-env and runs the Playwright specs GravityView + Gravity Forms reach wp-env via WP_ENV_PLUGINS from .env in CI; locally, wp-env.config.js auto-mounts the sibling repos when present. Release build gates on the unit tests, matching the sibling repos. Validated locally: phpunit test --parallel passes on 7.4 and 8.0, the E2E specs pass, and `circleci config validate` passes.
Adds the cases the GVAZ-9 and GVAZ-10/11/12 fixes did not take: - The 0-9 bucket on a normal form field still matches field values, so the Created By branch does not capture it. - A Created By 0-9 bucket that matches no display name returns nothing rather than every entry. - Building a View query leaves the gform_gf_query_sql registry at its baseline, asserted on the callback count rather than on results. - Comparisons spent by one query do not rewrite a later query in the request. - An accented display name groups under its base letter where the collation folds accents. - Swedish a-ring and umlauts stay separate letters under a binary collation.
Takes the A-Z front end from two specs to fifteen. Bug-derived: an active letter leaves another View on the same page untouched, the same View embedded twice returns the same rows, a letter combined with a Search Bar search keeps the search matching its own field, and the Created By 0-9 bucket resolves through the author display name. Unguarded paths: the bar renders the whole configured alphabet, three localized alphabets (Russian, Greek, Swedish) render and filter, an alphabet added through the localization filter replaces the letters, "Show All" clears the filter and appears only while one is active, a letter with no matches shows the empty state, and the letter survives paging. The harness mu-plugin gains routes to publish a page of View shortcodes and to attribute entries to an author with a known display name, plus two opt-in test levers: a custom alphabet, and a binary collation forced by ?e2e_collation=bin. The collation lever is what makes a leaked rewrite observable from the browser.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the automated-coverage gap on the A-Z Entry Filter widget: 19 new tests, taking the suite from 13 to 32 (17 PHPUnit, 15 Playwright).
Important
This branch contains #65 rebased onto
develop. The first four commits are the GVAZ-1 work already under review there, so this diff includes them. The new coverage needs both #65's Playwright harness and the GVAZ-9/10/11/12 fixes ondevelop, and neither base carries both. Merge #65 first, or close it in favour of this branch.Why the harness moved
The Playwright harness lived only on
fix/az-search-request; the fixes lived only ondevelop. The rebase applied with zero conflicts —tests/bootstrap.phpwas already byte-identical on both branches, so the expected conflict never materialised.PHPUnit (6)
test_zero_nine_bucket_on_a_form_field_matches_field_valuestest_zero_nine_bucket_created_by_without_digit_display_names_matches_nothingtest_query_building_leaves_the_sql_filter_registry_at_its_baselinetest_spent_letter_expressions_do_not_rewrite_a_later_querytest_accented_display_name_groups_under_its_base_lettertest_swedish_a_ring_and_umlauts_stay_separate_letters_under_a_binary_collationPlaywright (13)
Bug-derived: an active letter leaves another View on the same page untouched; the same View embedded twice returns the same rows; a letter plus a Search Bar search keeps the search matching its own field; the Created By
0-9bucket resolves through the author display name.Unguarded paths: the bar renders the whole configured alphabet; Russian, Greek and Swedish alphabets render and filter; the localization filter replaces the letters; "Show All" clears the filter and is offered only while one is active; a letter with no matches shows the empty state; the letter survives paging.
Catch-the-bug gate
Each bug-derived test was run against the pre-fix widget (widget file only — a plain revert also deletes the test files it builds on).
Red pre-fix, green post-fix:
..._created_by_without_digit_display_names_matches_nothing(3 rows vs 0) ·..._sql_filter_registry_at_its_baseline(leaked callback) ·..._swedish_a_ring_and_umlauts...(3 vs 1) ·azSearchBarCombination·azMultiViewIsolation→ untouched View ·azCreatedByDigits.Reported honestly rather than counted as coverage:
test_zero_nine_bucket_on_a_form_field_matches_field_values— green both ways. Guards the control-flow restructure; does not catch the original bug.test_accented_display_name_groups_under_its_base_letter— green both ways. Accent folding is database collation behaviour that neither fix changed; characterisation only.test_spent_letter_expressions_do_not_rewrite_a_later_query— red pre-fix, but structurally: the once-registration is new API, so it fails on "not registered" rather than on an observed double-wrap. Weaker signal than the others.azMultiViewIsolation→ same View twice — green both ways. A stackedLOWER(LOWER(x))is functionally idempotent, so rows stay correct pre-fix. Regression guard, not a bug-catcher.Happy-path tests have no fix to revert, so they were proven by mutation: truncating every alphabet to
a,b,cturned all five alphabet tests red; forcingget_filter_letter()tofalseturned all four navigation tests red.Flakiness gate
Three consecutive clean runs each — Playwright 15/15 ×3, PHPUnit 17/17 ×3 on PHP 7.4 and 8.0 — plus one
E2E_WORKERS=1run matching CI's serialised config.One genuine flake was found and fixed at the root, not with a longer timeout:
getByRole('link', { name: 'Show All' })matches accessible names by substring, and the fixture View titles ("AZ Show All Absent") render as title links, so the locator was matching the page title instead of the widget. Replaced with a markup-scopedshowAllLink()helper and verified 20/20 under--repeat-each=5.Harness additions
tests/E2E/mu-plugins/e2e-view-fields.phpgains two routes — publish a page of View shortcodes (the fixtures API exposes one View per call and can't express two Views rendering in one request), and attribute entries to an author with a known display name — plus two opt-in levers: a custom alphabet for the localization-filter test, and a binary collation forced by?e2e_collation=bin. That last one matters: without a forced collation the letter rewrite only addsLOWER(), which is harmless on a case-insensitive column, so a leaked rewrite is invisible from the browser. It is what makes the GVAZ-10 and GVAZ-11 specs able to fail at all.Not covered
A-Z View + DataTables View on one page. DataTables is a separate commercial GravityView extension, absent from this harness and from
package.json, so the scenario is not reproducible here. Substituted the same View embedded twice test, which exercises the same multi-query rewrite path — with the caveat above that it is green both ways.Reviewer notes
importto match the existing specs, not the CommonJS used by the helpers.cp .env.example .envonce (gitignored; CI already does this), otherwise the fixtures API has nobaseUrl.gk-e2e/v1route untilnpm run tests:e2e:setupre-mounts it.💾 Build file (d71c483).