fix(vendor): treat a missing selling flag as pending when listing vendors - #3400
Conversation
…dors Three code paths defined "pending" three different ways: dokan_get_seller_status_count() all - active, so a missing key is pending UserList::filter_pending_vendors dokan_enable_selling = 'no' OR NOT EXISTS Vendor\Manager::get_vendors() dokan_enable_selling = 'no' A vendor that never had the meta written -- created before the flag existed, or through a path that does not set it -- is therefore counted as pending but cannot be listed. The Vendors screen shows a non-zero Pending tab that opens on "No data found", which is what #3321 reports. Measured on a site with 33 vendors carrying dokan_enable_selling = 'yes', none carrying 'no', and 9 sellers with no key at all: before GET /dokan/v1/stores?status=pending -> 0 rows, X-Status-Pending: 9 after GET /dokan/v1/stores?status=pending -> 9 rows, X-Status-Pending: 9 Approved is unchanged and still matches on 'yes' exactly. Closes #3321 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TVJsU3yzAAzntUfuyX1Zsk
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TVJsU3yzAAzntUfuyX1Zsk
…ta group The previous commit expressed pending as `dokan_enable_selling = 'no' OR NOT EXISTS`. Semantically right, but WP_Meta_Query compiles that OR group into three LEFT JOINs on wp_usermeta, two of them with no meta_key in the ON clause -- the same shape the reporter of #3398 measured at ~5.5M rows and ~3s per query. That would hand back a large part of what #3399 recovers, on the badge query that runs on every admin load. Pending is simply everyone who is not approved, so it is now one keyed subquery: AND wp_users.ID NOT IN ( SELECT user_id FROM wp_usermeta WHERE meta_key = 'dokan_enable_selling' AND meta_value = 'yes' ) Measured on the badge query, 50 users: develop (= 'no' only) joins=0 est. rows 1,485 returns 0 (wrong) OR group (= 'no' OR NOT EXISTS) joins=3 est. rows 83,160 returns 9 this (NOT IN subquery) joins=0 est. rows 1,848 returns 9 Approved is untouched. Asking for approved and pending together now means everyone, which is what it means under a consistent definition. Closes #3321 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TVJsU3yzAAzntUfuyX1Zsk
Collapse the three status booleans into a single resolve_status() step so get_vendors() reads as: approved adds one meta clause, pending appends the keyed NOT IN subquery, anything else is unfiltered. The approved clause is appended the same way the featured clause already is, dropping the single-clause OR wrapper. The badge helper's docblock described the old 'no'-only listing as the reason it existed; that reason is gone, so it now states the shared rule. Adds VendorStatusFilterTest: every status form, get_total(), the caller's own meta_query surviving the pending filter, and the pre_user_query hook being detached after the query. Fails 6/12 on develop, passes here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LjTc7UnPfeFU3Dj6PZLoGL
phpcs now scans every changed file whole on pull requests, and this file carried three auto-fixable errors from before this branch: a loose comparison on the featured flag (every caller passes a string) and the parentheses on a multi-line apply_filters() call. phpcbf output, unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LjTc7UnPfeFU3Dj6PZLoGL
The pending listing registers `exclude_approved_vendors()` on the global `pre_user_query` hook, so anything else building a `WP_User_Query` inside that hook inherited the clause, and a throw anywhere in the query left the callback registered for the rest of the request. The query now carries a `dokan_pending_only` var that the callback checks, and the registration is unwound in a `finally`. The clause itself becomes a correlated `NOT EXISTS`, which rides the usermeta index and stops at the first match per row instead of materialising every approved vendor before the first page of results. Tests move onto `DokanTestCase` and cover what #3321 actually reported — the counter and the list disagreeing — plus the two leaks above and `featured` combined with `pending`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…round the query The previous commit unhooked exclude_approved_vendors() in a finally, unconditionally. A get_vendors() call made from inside pre_get_users -- for any status -- therefore unregistered the outer pending listing's callback before its own pre_user_query fired, and the outer query silently returned approved vendors too. The callback is already a no-op without the dokan_pending_only query var, so it is now hooked once and left in place; the try/finally goes with it. dokan_get_seller_count() reads only ['count'] from its pending listing but ran it with number => -1 and the default fields => 'all', building a Vendor object per row. That was cheap while pending meant "explicitly disabled"; now that it means "everyone not approved" the listing is capped at one row. Restores the administrator note on dokan_get_pending_vendor_count(): a flagless admin counts as pending here, in dokan_get_seller_status_count() and in the Users-screen "Pending Vendors" filter alike, since dokan_admin_user_register() only writes the flag for the seller role. Replaces the detach-on-throw test, which described the removed mechanism, with the nested-listing case; it fails against the previous commit and passes here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M8JtirSrFT3FRumwhACL38
MdAsifHossainNadim
left a comment
There was a problem hiding this comment.
Posted as a comment rather than a formal approval: GitHub does not allow the author of a pull request to stamp Approve on it. Treat this as an approving review — it needs a second pair of eyes for the green check.
Reviewed at 75df70e93. No blockers in the code — this is good to merge. One change I would make before it goes in is in the description rather than the diff: the issue link should be Refs #3321, not Closes #3321.
What the change gets right
Three code paths defined "pending" three different ways and the vendor listing was the odd one out. Reading a missing dokan_enable_selling flag as pending brings get_vendors() in line with dokan_get_seller_status_count(), UserList::filter_pending_vendors() and dokan_is_seller_enabled(). That is the right direction to reconcile in — widening the listing to match the counters, rather than narrowing three counters to match one listing.
I re-verified the mechanics against the current tree:
- Core builds the user-query cache key and the
FOUND_ROWS()total from the final SQL, afterpre_user_queryhas run. The injectedNOT EXISTSis therefore honoured by the rows, the total and the object cache alike — no risk of a count that disagrees with the list it is counting. Vendor\Manageronly ever exists as thedokan()->vendorsingleton, so thehas_action()hook-once guard cannot double-append the clause.- The correlated
NOT EXISTSis the right shape here. It rides theusermetaindex and stops at the first match per row, where the previous OR / meta-query form forced a fullwp_usersscan with three joins underDISTINCT. - Keeping the callback registered and gating it on the
dokan_pending_onlyquery var is the correct fix for the nested-query problem. Unhooking around the query let a nested listing disarm the outer one, andtest_nested_listing_does_not_disarm_the_pending_filterpins exactly that.
Branch is even with develop and mergeable. PHPCS and PHPUnit 8.3 are green. The 14 tests cover the status matrix, counter-versus-list agreement, both hook-leak directions, caller-supplied meta queries and the featured filter.
The one thing to change: Closes #3321 → Refs #3321
#3321 reports All (0), Approved (0), Pending (0) with "No data found", while a direct REST call returns two stores. This PR fixes a non-zero Pending counter that opens onto an empty list. Those are different failures, and merging this will not close the reported one.
The tab counters come from the X-Status-* response headers, i.e. dokan_get_seller_status_count() over role__in = [ seller, administrator ]. All can never be 0 while an administrator is looking at the page — the viewing admin is themselves in the result set. So the reported screen cannot be produced by any server-side status filter.
It matches the front end instead. In src/admin/dashboard/pages/vendors.tsx, counts initialises to { all: 0, approved: 0, pending: 0 } and the catch in fetchVendors resets data and totalItems but never touches counts. A failed or unauthorised first fetch therefore renders exactly the reported screen: three zero tabs and "No data found".
That also explains why the reporter's manual REST check looked healthy — an unauthenticated GET /dokan/v1/stores returns 200 with rows but without the X-Status-* headers.
Suggest keeping #3321 open and asking the reporter for the Vendors screen's own Network entry (status code plus response headers), since the console does log Failed to fetch vendors on that path.
Follow-ups, none blocking this PR
- Flagless administrators now list as pending.
dokan_admin_user_register()only writes the flag for thesellerrole, so a fresh install will show aVendors (1)badge until the admin toggles their own store. The behaviour is consistent with the other two paths, so it belongs here — but the principled fix is for the Installer to writeyesfor administrators alongside thedokandarcapability. Worth its own issue. - The stores
statuscollection param has noenum. Any unrecognised string now resolves to the wider pending set instead of the old narrower one. See the inline note. UserList::filter_pending_vendors()still hand-builds its own OR /NOT EXISTSgroup and could delegate to the new query var. That file is in flight in #3399, so leave it alone until that lands.- The reformat of the
dokan_vendor_create_datafilter call reads as churn but is load-bearing — CI runs PHPCS over the whole changed file anddevelop's copy carries pre-existing errors.
CI
The PHPUnit (PHP 7.4) job is red for infrastructure reasons, not for this PR. It fails after ~2m in Start WordPress environment, at RUN apt-get -qy install sudo in the wp-env image build, against a Debian mirror that no longer serves those paths. It never reaches a test. The same job fails identically on other PRs from the same day.
🤖 Generated with Claude Code
kzamanbd
left a comment
There was a problem hiding this comment.
Fix is correct and the mechanism checks out — pre_user_query lands before the request is built, the cache key hashes the full SQL, the has_action guard is safe because vendor is addShared, and number => 1 still yields a real count. A few things to resolve first.
1. includes/Vendor/Manager.php:135 — resolve_status() fails open.
Any mix resolves to all, so a typo like [ 'approved', 'rejected' ] returns every vendor on a listing that feeds the public /dokan/v1/stores. Please use an explicit whitelist and default the ambiguous case to the narrowest set.
2. includes/Vendor/Manager.php:92 — Pro fallout.
pending now includes administrators (role__in default is [ seller, administrator ]). dokan-pro includes/Announcement/Manager.php:382 passes status => [ 'pending' ] for disabled_seller, so that announcement will now reach admin accounts. Needs a Pro-side role__in => [ 'seller' ] or an explicit accept. Please add the Dependency With Pro label.
3. tests/php/src/Vendor/VendorStatusFilterTest.php:118 — name contradicts the design.
75df70e deliberately leaves the callback hooked; the test asserts it is a no-op for other queries. Rename to something like test_hooked_filter_is_a_noop_for_other_queries.
4. PR description — one row of the behaviour table is wrong.
[ 'approved', 'pending' ] did not collapse to approved-only; the old OR meta group returned the union. [ 'all', 'approved' ] is the pair that collapsed.
Notes (not blocking):
PHPUnit (PHP 7.4)is red at "Start WordPress environment" —Run PHPUnitwas skipped. Infra, please re-run job101633730661.- Follow-up worth filing:
GET /dokan/v1/storesispermission_callback => '__return_true'with a publicstatusparam.Abilities\Definitions\VendorsQuery::resolve_status()already forces non-Store-Admins toapproved; the REST controller should do the same. - Needs a changelog entry.
kzamanbd
left a comment
There was a problem hiding this comment.
Anchoring the three items from my review to the lines they apply to.
resolve_status() mapped anything that was not 'all' or 'approved' onto 'pending', so a mix such as [ 'approved', 'rejected' ] resolved to two statuses and fell through to 'all' -- every vendor on a listing that also feeds the public /dokan/v1/stores. A single unknown value was no better: it used to mean dokan_enable_selling = 'no', and this branch had widened that to "everyone not approved". The three statuses the listing understands are now an explicit whitelist. Unknown values are dropped, and input that names none of them falls back to the listing's own default, 'approved', rather than to the widest set. Asking for both halves, or for 'all' outright, still means everyone. Abilities\Definitions\VendorsQuery::resolve_status() already coerces an unknown status to 'approved' the same way. Reachable through REST only as a single bogus string -- the collection param is declared as a string, so status[] is rejected with a 400 before it gets here -- but the mix is reachable from PHP and through the dokan_seller_listing_args filter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
75df70e deliberately leaves exclude_approved_vendors() hooked after the listing, so the filter does outlive the query; what the test pins is that it is inert for a query that did not ask for pending. Renamed accordingly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — three of the four are fixed, the fourth is verified and accepted. Head is now 1. One correction on the reachability, in the thread: the array form never reaches 2. Pro fallout — real, measured, accepted; label added. With the admin's flag deleted on a site carrying 9 flagless sellers, 3. Test name — fixed ( 4. Behaviour table — fixed in the description. Changelog entry — added as its own section in the description. CI — re-run on the new head. PHPCS and PHPUnit (PHP 8.3) are green. PHPUnit (PHP 7.4) is not worth re-running: it dies in Debian bullseye-security stopped being refreshed, so the release file is permanently expired and every re-run hits it. It is not specific to this PR: every Locally, against 🤖 Generated with Claude Code |
Changes proposed in this Pull Request
Three code paths defined "pending" three different ways:
dokan_get_seller_status_count()all − active, so a missing key counts as pendingUserList::filter_pending_vendors()dokan_enable_selling = 'no'ORNOT EXISTSVendor\Manager::get_vendors()dokan_enable_selling = 'no'onlyA vendor that never had the meta written — created before the flag existed, or through a path that doesn't set it — is therefore counted as pending but can never be listed. The Vendors screen shows a non-zero Pending tab that opens on "No data found".
get_vendors()is the odd one out, so it now agrees with the other two. Approved is untouched and still matches'yes'exactly.The
statusargument also gained an explicit whitelist while the two were being reconciled. Onlyall,approvedandpendingare understood; asking for both halves (or foralloutright) means everyone; anything else names no status this listing can filter on and falls back to the listing's own default,approved, rather than widening the result. That is the coercionAbilities\Definitions\VendorsQuery::resolve_status()already applies to the same three statuses.Related Pull Request(s)
UserList::filter_pending_vendors()touches the same meta onpre_get_usersand is fixed separately in fix(admin): stop stacking the pending-vendor meta query onto every user query #3399.Refs
Not
Closes: #3321 reports All (0), Approved (0), Pending (0) with "No data found" while a direct REST call returns two stores. This PR fixes a non-zero Pending counter that opens onto an empty list — a different failure. The tab counters come from theX-Status-*response headers, i.e.dokan_get_seller_status_count()overrole__in = [ seller, administrator ], so All can never be 0 while an administrator is looking at the page — the viewing admin is in the result set themselves. The reported screen matchessrc/admin/dashboard/pages/vendors.tsxinstead, wherecountsinitialises to{ all: 0, approved: 0, pending: 0 }and thecatchinfetchVendorsresetsdataandtotalItemsbut nevercounts: a failed or unauthorised first fetch renders exactly three zero tabs and "No data found". That also explains the reporter's healthy-looking manual REST check — an unauthenticatedGET /dokan/v1/storesreturns200with rows but without theX-Status-*headers. Worth asking the reporter for the Vendors screen's own Network entry (status code plus response headers) before closing that issue.How to test the changes in this Pull Request
dokan_enable_sellinguser meta at all:Expected: the tab lists exactly as many vendors as its counter claims. Check the Approved and All tabs are unchanged.
Worth testing on both admin surfaces — the legacy screen (
admin.php?page=dokan#/vendors) and the new dashboard (admin.php?page=dokan-dashboard#/vendors). Both consume the same REST endpoint and both were affected.For the whitelist,
GET /dokan/v1/stores?status=<anything else>should now return the approved directory instead of the pending set.Changelog entry
Fixed the Vendors screen's Pending tab counting vendors it could not list
The Pending tab counted every vendor that was not approved, but listed only the ones explicitly disabled. A vendor carrying no selling flag at all — created before the flag existed, or through a path that never wrote it — was counted but never shown, so the tab opened on "No data found" with a non-zero counter next to it. The listing now reads a missing flag as pending, the same way the counters, the Users screen's "Pending Vendors" filter and
dokan_is_seller_enabled()already did, so the tab and its counter always agree. An unrecognisedstatuson the store listing now returns the approved directory instead of a wider set.Test results
On a site with 33 vendors carrying
dokan_enable_selling = 'yes', none carrying'no', and 9 sellers with no key at all:Verified in the browser on both surfaces: the Pending tab went from "No data found" / "No vendors found" (0 items) to listing all 9, with All and Approved unchanged. Re-verified after the whitelist change — All (43) / Approved (34) / Pending (9) on both screens, Pending listing 9 rows.
PHPCS on the changed files: 0 errors.
Manager.phpkeeps one pre-existingslow_db_query_meta_querywarning;functions.phpis at itsdevelopwarning count.VendorStatusFilterTest: 16 tests, 60 assertions. The nested-listing test fails against9a5229238, and the three whitelist cases fail against75df70e93.Behaviour notes
role__indefaults to[ seller, administrator ]anddokan_admin_user_register()only writesdokan_enable_sellingfor thesellerrole, so an administrator who never touched the seller fields has no flag.dokan_get_seller_status_count()has counted them as inactive since 2.9.23 and the Users-screen "Pending Vendors" filter (UserList::filter_pending_vendors()) already lists them;get_vendors()was the one place that did not. This PR makes the Vendors screen agree with the other two rather than carving administrators out of all three — that is a separate decision if anyone wants it.Announcement\Managersends thedisabled_sellerannouncement tostatus => [ 'pending' ]with norole__inoverride, so a flagless administrator now receives it. Measured on a site with 9 flagless sellers:disabled_sellergoes from 0 recipients to 10. The same account is already in theall_selleraudience today (that type asks forstatus => [ 'all' ], which has always included administrators), so this makes the announcement audiences consistent rather than introducing admins to them. Scoping the vendor announcement types torole__in => [ 'seller' ]is a Pro-side change and belongs to all four types, not just this one, so it is left to a Pro follow-up. LabelledDependency With Pro.approvedorallmeantdokan_enable_selling = 'no'; the first cut of this PR carried that forward onto the much wider "everyone not approved" set, and a mix such as[ 'approved', 'rejected' ]collapsed toall. Both now resolve toapproved. A genuine mix of the two halves —[ 'approved', 'pending' ]— still means everyone, as it did before this PR through the OR meta group.[ 'all', 'approved' ]used to collapse to approved-only and now means everyone, which is what it asks for. No caller in Lite or Pro passes a mix or an unknown status.dokan_get_seller_count()agrees too. The admin dashboard's vendor widget takes its inactive figure fromstatus => 'pending', so it now matches the badge as well. That listing is capped at one row, since only the total is read and the pending set is larger than it used to be.pre_user_querycallback stays registered after the first pending listing. It is a no-op for any query without thedokan_pending_onlyvar; unhooking it around the query let a nestedget_vendors()(from insidepre_get_users) disarm the outer one. Covered bytest_nested_listing_does_not_disarm_the_pending_filter.Notes for the reviewer
Two adjacent things found while investigating, both left out of this PR:
src/stores/vendors/resolvers.ts— thegetVendorsresolver fetches/dokan/v1/storesand then never dispatchessetVendors()or clearssetLoading( true ). This matches the "Source-code observation" in Dokan 5.0.9 admin Vendors list shows zero vendors while REST API returns valid stores #3321, but that store is not what drives either vendors list (pages/vendors.tsxdoes its ownapiFetch), so it is dead-code rot rather than the cause. Worth its own issue.statuscollection param onGET /dokan/v1/storesstill declares noenum. With the whitelist in place an unknown value is coerced toapprovedrather than silently widening, so this is now cosmetic — anenumwould turn it into a400instead. Note the endpoint ispermission_callback => '__return_true'whileAbilities\Definitions\VendorsQuery::resolve_status()forces non-Store-Admins toapproved; the REST controller arguably should do the same. Worth its own issue.🤖 Generated with Claude Code