fix: hold vendor downloadable file changes until admin approval - #3372
fix: hold vendor downloadable file changes until admin approval#3372akzmoudud wants to merge 8 commits into
Conversation
When a vendor edits a downloadable product on a marketplace that requires admin approval, the replacement files were delivered to existing customers immediately at save time - the permission swap and the _downloadable_files meta write both ran before any admin review, while the product itself sat in pending review. Existing customers now keep the last approved files while the edit is pending. The submitted file set is staged in the _dokan_pending_downloadable_files meta and applied - together with the usual permission swap - only when an admin publishes the product. Staging happens only when all three are true: the save lands in pending status, the file set actually changed, and at least one customer already holds a download permission. Trusted vendors, marketplaces without approval, admin edits, and unsold products keep the existing behavior. The applier hooks save_post at priority 999 so it also wins when the product is approved from the WooCommerce edit screen, whose own meta box save re-writes the previously approved files at priority 1. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Restructure dokan_downloadable_files_require_approval() so the final filter wraps the whole decision, letting extensions opt in rather than only veto, and make the held statuses filterable via the new dokan_downloadable_files_approval_statuses filter (default: pending). Marketplaces using draft as their review status can opt in. - Discard staged files when a vendor unchecks Downloadable on a later pending save; previously the stale staging survived and would have been applied (and made downloadable) on approval, since WooCommerce's download handler never checks is_downloadable(). - Clear the staging meta after the permission swap and meta write instead of before, so a fatal in a third-party permission hook cannot silently drop the vendor's submission. Nothing in the applier re-fires save_post, so idempotency is unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MdAsifHossainNadim
left a comment
There was a problem hiding this comment.
Verdict: 🔴 Request changes — one blocker defeats the stated purpose
The mechanism itself is well built, and I verified the parts most likely to be wrong — they're all correct:
- Status ordering holds. The gate reads
get_post_status(), which only works if the status is alreadypending. It is:wp_update_post()sets it atProducts.php:542, anddokan_process_product_meta()runs at:573. - The ID-only comparison really does detect file swaps, because download ids are
md5( $file_urls[$i] )(wc-functions.php:346) — a changed file necessarily changes its id. - Priority 999 does beat WooCommerce, whose
save_meta_boxesis priority 1 (class-wc-admin-meta-boxes.php:49). wp_publish_post()works too — it assigns$post->post_status = 'publish'before firingsave_post, so the applier's guard passes on that route. This was the easiest thing here to get wrong.
The second commit's fixes (discard staging when Downloadable is unchecked; clear staging after the swap) are both right.
🔴 Blocker: the admin never sees the file they are approving
The wp-admin product metabox renders $product_object->get_downloads( 'edit' ) (html-product-data-general.php:146), which reads _downloadable_files — the old approved files. Nothing in wp-admin reads _dokan_pending_downloadable_files.
So the flow becomes: admin opens the pending product, sees file A, clicks Publish believing they approved A — and file B, which they have never seen, ships to every existing customer.
#6045 asks for this specifically: "give the marketplace owner a checkpoint before vendor content reaches buyers." Deferring delivery without surfacing the staged file doesn't create that checkpoint — it turns an immediate-delivery bug into a delayed-delivery bug plus false assurance. Before, the owner had no gate and knew it. After, they have a gate that appears to have been passed. Admin surfacing is listed as an intentional follow-up, but it's the half that makes this a review gate rather than a delay, so it belongs in this PR.
Related, and worse in combination: if the admin edits the download rows while approving, their edit is silently discarded — WC's metabox save writes the admin's version at priority 1, then the applier overwrites it with the vendor's staged set at 999 and deletes the staging. No notice, no record. Consider skipping the apply when a wp-admin product save submitted downloads, or reconciling rather than blanket-overwriting.
🟠 The vendor REST path leaves the same hole open
dokan/v1/products is the vendor namespace and handles downloads → save_downloadable_files() → set_downloads() at ProductController.php:1422; Product\Manager::update() does the same at :582. Neither routes through dokan_process_product_meta(), so nothing is staged and the files go live immediately.
A vendor can therefore reproduce #6045 exactly as written through a supported vendor API. Sequencing this separately is defensible, but it needs a tracked follow-up issue, and the changelog should scope the claim to the legacy product form rather than implying the behaviour is fixed.
🟠 No tests on logic with this many branches
Two files changed, zero tests. The gate has ~6 decision branches (pending + changed + has-permission; trusted vendor; approval disabled; unsold product; unchanged files; Downloadable unchecked) and the applier claims 5 entry routes. Lite has no PHPUnit CI job, so tests are the only regression guard — and there is direct precedent to copy in tests/php/src/REST/OrderDownloadsGrantOwnershipTest.php / OrderDownloadsRevokeOwnershipTest.php.
Minimum worth covering: staging fires only when all three conditions hold; staging is discarded on each negative case; the staged set applies exactly once on publish.
Minor
_download_limit,_download_expiryand_download_typestill write live while the files are held, so a vendor can drop an existing customer's download limit to zero on a pending edit and have it apply immediately. Inconsistent with the staging intent — worth a deliberate decision either way.- The vendor template now shows staged files (good) but with no "awaiting approval" notice, so a vendor may conclude their new file is already live.
- The gate's correctness is coupled to ids being
md5( url ). Worth a one-line comment, since a future move to stable ids would silently stop it firing. dokan_downloadable_files_approval_statuseswithdraftopt-in is a nice touch. The direct$wpdbquery is prepared withLIMIT 1and carries the right phpcs ignores — fine as-is.
Scope note
Splitting A from B per #6045's recommendation is the right call. Two things from that issue will be lost when this closes and should be tracked separately: the documentation gap (two published pages still describe the removed "Edited Product Status" setting) and option B (staging the whole pending edit, not just files).
|
@akzmoudud bhai 🔴 1. Regression — approving while editing the file orphans the customer's permissionPriority 999 does not win in the WooCommerce CRUD path. Admin sets File C and publishes in one save: Paid customer loses all download access; staging meta already deleted → unrecoverable without manual DB repair. On Fix direction: anchor the apply step to 🔴 2. "Draft" status bypasses staging completely
Related, and pre-existing: Fix direction: default 🔴 3. New React product editor / REST paths get no staging at allThe fix lives in Pre-existing (same on Fix direction: enforce staging at the CRUD layer ( Lower priority
Suggested testsCRUD publish-with-file-change, draft save, forced |
Extends the downloadable-file approval hold to every product save path and gives the admin something to review, addressing the review blockers. - Gate WooCommerce CRUD saves via woocommerce_before_product_object_save, covering the React product editor, dokan/v1 and dokan/v3 REST endpoints, Product\Manager::update(), the wp-admin product screen and WP-CLI. The submitted files are staged and the approved files are kept on the object so WooCommerce persists them unchanged. Variations are held by their parent's status and released when the parent is published. - Hold files in any status that is not published, rather than only 'pending'; draft is a one-click vendor option and previously bypassed the gate entirely. The released statuses are filterable through dokan_downloadable_files_released_statuses (default: publish). - Release staged files from save_post at priority 999 only, so the release is the last write of the request. Releasing from the CRUD save hook was unsafe: WooCommerce writes product meta after that hook, and a nested save could leave a customer's permission pointing at a file no longer on the product. When the releasing save also changes the files, the saved files win and permissions are re-synced to them; the sync is idempotent and flushed on shutdown. - Compare file sets by URL rather than download id. The classic form keys files by md5( url ) while CRUD saves generate UUIDs, so ids differ across save paths even when the files are identical. - Show the staged files and a warning notice on the wp-admin product edit screen, so an admin no longer publishes a replacement file sight unseen, and tell the vendor their files are awaiting approval. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MdAsifHossainNadim
left a comment
There was a problem hiding this comment.
Verdict: 🔴 Request changes — every previously reported blocker is closed, but two new ones landed with them
I patched dbff4136f onto develop (the branch itself is 25 commits behind) and drove twelve scenarios through a live WooCommerce 11.0.1 / Dokan Pro 5.0.15 install, comparing each end state against unpatched develop. Everything below was reproduced, not reasoned about.
What this commit genuinely fixed — all verified live
| Previous finding | Status |
|---|---|
| Draft bypasses staging (@dev-shahed #2) | ✅ Draft, and every non-publish status, now stages |
| REST / CRUD / React editor get no staging (@dev-shahed #3) | ✅ woocommerce_before_product_object_save catches all of them — variations included, which is more than the description claims |
| Admin sets file C and publishes → orphaned permission (@dev-shahed #1) | ✅ Admin's file wins, permissions re-synced to it |
| Admin approves blind (my blocker) | ✅ Notice + "Replacement files awaiting your approval" panel both render |
| Admin's own row edits silently discarded (my 🟠) | ✅ Fixed |
Two things I checked specifically because they were the easiest to get wrong, and both are right:
dokan_process_product_meta()does not defeat itself. It ends with$product->save(), firing the new before-hook in the same request as the staging write. The freshly-read object has nodownloadschange, so the hook returns early and the staging survives. Clean.- URL-based comparison is the correct call. The classic form keys files by
md5( url )while CRUD saves generate UUIDs; comparing ids would have produced false "changed" verdicts across save paths. Verified both directions.
wp_publish_post() still releases correctly, variations release when the parent is published, and PHPCS is clean on the new code (the three hits in wc-functions.php are pre-existing, at lines 307, 482 and 554).
The admin review surface renders correctly in wp-admin: the Downloadable files rows show the approved file A, and "Replacement files awaiting your approval" lists the staged file B underneath, with the warning notice at the top of the screen. One cosmetic nit on it inline below.
🔴 Blocker 1 — approving over REST permanently destroys the customer's download
Vendor stages file B through the classic form. Admin then approves over REST the way any REST client does it: GET the product, flip status to publish, PUT the payload back with downloads unchanged.
after vendor save: live=[file-A] staged=[file-B] perms=[md5(A)] customer sees: file-A ✔
after admin approve: live=[file-A] staged=(gone) perms=[md5(B)] customer sees: NOTHING ✘
On unpatched develop that same flow ends with the customer holding a working file B, so this is a strict regression. And because the staging meta is already deleted, nothing is left to re-apply — it needs a manual database repair.
The cause is the ordering this commit reasons about, one case short of complete. save_post at priority 999 fires from the wp_update_post() inside WC_Product_Data_Store_CPT::update(); WooCommerce writes _downloadable_files afterwards, in update_post_meta() → update_downloads(). That writer fires whenever downloads is merely present in get_changes(), and WC_REST_Products_Controller::save_downloadable_files() builds fresh WC_Product_Download objects — so an unchanged file list still lands in changes. The applier grants file B's permission, then WooCommerce overwrites the meta back to file A.
Fix direction inline below.
🔴 Blocker 2 — the new product editor silently throws away the vendor's replacement file
The before-hook puts the approved files back on the product object so WooCommerce persists them unchanged. That also means the REST response hands the vendor file A, not the file B they just uploaded.
vendor saves B: live=[file-A] staged=[file-B]
what the REST response shows them: file-A
vendor saves again, posting back what they were shown:
live=[file-A] staged=(gone)
The vendor's submission is destroyed with no notice and no record. The "vendor resubmitting the approved files cancels their pending replacement" branch is a good idea, but it is only safe when the vendor is actually looking at the staged set — which is exactly why templates/products/downloadable.php was taught to read the staged meta. The REST path never got that treatment, so extending staging to CRUD without extending the display turned "the vendor's file goes live too early" into "the vendor's file disappears".
This is reachable through dokan/v1/products and Product\Manager::update() too, so it is not limited to stores running vendor_product_editor = latest.
🟠 Worth a deliberate decision before merge
Unchecking "Downloadable" still cuts existing customers off instantly. _downloadable is written live while the product is pending, and WC_Product::has_file() gates on is_downloadable(), so wc_get_customer_available_downloads() drops the row entirely. Verified: the paying customer sees nothing, before any admin review. Same class of problem as #6045, one click for the vendor, and arguably worse than the file swap since it revokes access rather than substituting it.
Correction to my previous review while we're here: _download_limit and _download_expiry do not affect existing customers — those live per-row on the permission as downloads_remaining and access_expires. Please disregard that half of my earlier "minor"; only the _downloadable flag matters, and it matters more than I gave it credit for.
The gate fires on marketplaces that have no approval step. It keys purely off "status is not publish", so a trusted vendor with approval disabled — or the store admin editing their own product — who saves as Draft now has their file change staged and is shown "These files are awaiting admin approval" on a store where nothing is awaiting approval. Consider also requiring that the save is genuinely subject to review: ! dokan_is_seller_trusted( $author ), or 'publish' !== dokan_get_default_product_status( $author ).
On @dev-shahed's "vendor can force publish from DevTools": that one is closed when Pro is active — dokan_update_product_post_data runs Pro's Products::change_product_status(), which downgrades publish to pending for untrusted vendors. Confirmed live. On Lite-only there is no such filter, so handle_product_update() still passes $_POST['post_status'] to wp_update_post() unvalidated and a vendor can self-approve their own swap. That deserves its own Lite issue rather than expanding this PR.
🟡 Minor
- Staged files survive trash; untrash then publish applies the swap silently. Low, but a trash round-trip is a plausible "reject" gesture.
- The admin cannot approve the product while rejecting the file. Leaving the rendered rows untouched and pressing Publish delivers the staged file; only changing a row overrides it. The notice is honest about this, but there's no way to act on it — a "discard pending files" link would close the loop.
- Still zero tests, on logic that is now ~8 branches and 3 release routes, in a repo with no PHPUnit CI job. The two cases most worth encoding are the two blockers above.
tests/php/src/REST/OrderDownloadsGrantOwnershipTest.phpis the precedent to copy.
Scope note
Two things from #6045 will be lost when this closes and need their own issues: the documentation gap (two published pages still describe the removed "Edited Product Status" setting) and option B (staging the whole pending edit, not just files).
Reviewed against dbff4136f. Verification harness: 12 scenarios via wp eval-file on a live install, each compared to unpatched develop.
| if ( $released ) { | ||
| // publishing while also changing the files: the submitted files win. Drop any | ||
| // staged replacement and re-sync permissions once the new files are written. | ||
| if ( $changed && null !== dokan_get_staged_downloadable_files( $product_id ) ) { | ||
| delete_post_meta( $product_id, '_dokan_pending_downloadable_files' ); | ||
| dokan_downloadable_files_permission_sync( $product_id, 'flag' ); | ||
| } | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
🔴 Blocker 1 — this branch misses the case that costs the customer their download
$released && ! $changed && staged !== null falls straight through here: no delete_post_meta, no sync flag, staging left in place. That combination is what an admin approving over REST produces, and it ends with the permission row and _downloadable_files pointing at different files.
Sequence, reproduced live:
save()→ this hook. Released,$changedfalse (the client echoed the files the API gave it), so nothing happens.WC_Product_Data_Store_CPT::update()→wp_update_post()→ nestedsave_post→dokan_apply_staged_downloadable_files()at 999 grants the staged file's permission and writes_downloadable_files, then deletes the staging.- Back in the same
update(),update_post_meta()→update_downloads()overwrites_downloadable_fileswith the object's files.
update_downloads() writes whenever downloads is merely present in get_changes() — and WC_REST_Products_Controller::save_downloadable_files() builds fresh WC_Product_Download objects, so an unchanged list still lands there. Net result: live files = the approved set, permission = the staged set's id, customer's My Account → Downloads is empty, and the staging is already gone so it can't be recovered.
develop leaves a working download in that same flow, so this is a strict regression.
The reliable fix is to make the release happen on the object, so WooCommerce's own write is the one that lands rather than racing the applier:
if ( $released ) {
$staged = dokan_get_staged_downloadable_files( $product_id );
if ( null !== $staged ) {
if ( ! $changed ) {
// this save is the release: hand WooCommerce the staged files so its
// own write is the last word, instead of racing the save_post applier
$product->set_downloads( dokan_files_array_to_downloads( $staged ) );
}
// when $changed, the admin edited the rows themselves and their set wins
delete_post_meta( $product_id, '_dokan_pending_downloadable_files' );
dokan_downloadable_files_permission_sync( $product_id, 'flag' );
}
return;
}Worth a regression test for exactly this: stage through the classic form, then publish through WC_Product::save() with downloads set to the current files.
There was a problem hiding this comment.
Fixed in 730f328b. The release is recorded for the request, so the meta write that follows is recognised as an echo and swallowed — live files and permission both end on the staged set.
Covered by test_approving_through_a_crud_save_delivers_the_staged_file.
| if ( ! $changed ) { | ||
| // the vendor resubmitting the approved files cancels their pending replacement | ||
| if ( dokan_is_product_author( $owner_product_id ) ) { | ||
| delete_post_meta( $product_id, '_dokan_pending_downloadable_files' ); | ||
| } | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
🔴 Blocker 2 — this cancels a staged swap the vendor was never shown
"Vendor resubmitting the approved files cancels their pending replacement" is the right idea, but it assumes the vendor is looking at the staged set. On the classic form they are, because templates/products/downloadable.php was taught to render _dokan_pending_downloadable_files. On every REST path they are not.
Reproduced live on dokan/v3/products:
vendor saves file B: live=[file-A] staged=[file-B]
REST response hands them back: file-A <- line 1408 put the approved set on the object
vendor saves again (form posts A): live=[file-A] staged=(gone)
Their upload is destroyed with no notice and no record. On develop file B at least went live; here it evaporates. Also reachable through dokan/v1/products and Product\Manager::update(), so it isn't limited to stores on vendor_product_editor = latest.
Two ways out, either is fine:
- Surface the staged set on the vendor's REST read — swap
downloadsfor the staged files indokan_rest_prepare_product_objectwhen the reader owns the product, plus a boolean the editor can hang an "awaiting approval" notice off. That makes the cancel-by-resubmit rule true again on every path. - Or narrow the cancel: only treat a save as a cancellation when the submitted set matches the live files and the client demonstrably saw the staged set (classic form only).
The first is the better shape — it also fixes the vendor being told nothing at all about the hold on the new editor.
There was a problem hiding this comment.
Fixed. The staged set is now surfaced on dokan/v1, dokan/v2 and the product editor, so the vendor always sees their own submission and the withdraw rule holds on every path.
|
|
||
| try { | ||
| // keep the approved files on the object so WooCommerce persists them unchanged | ||
| $product->set_downloads( dokan_files_array_to_downloads( $approved ) ); |
There was a problem hiding this comment.
This line is correct for keeping the approved files in the database, but it's also what makes the REST response lie to the vendor — see the Blocker 2 note above. Whatever surfaces the staged files to the vendor needs to account for the fact that the object deliberately no longer carries what they submitted.
Minor, separate: the catch below falls back to "let the regular save through", which quietly delivers the unreviewed file to existing customers — the exact outcome this PR exists to prevent. set_downloads() throws when a file fails check_is_valid(), so an approved file that has since been deleted from disk would trip it. Failing closed (leave the staging, skip the download write) or at least dokan_log()-ing it would be safer than silently reverting to the old behaviour.
There was a problem hiding this comment.
Both resolved. The staged set is surfaced to the vendor on every read path, and the try/catch no longer exists — the refactor moved the hold to the meta write, so nothing falls back to the old behaviour.
| update_post_meta( $post_id, '_download_type', wc_clean( $data['_download_type'] ) ); | ||
| } | ||
| } else { | ||
| // the product is no longer downloadable; a staged file set must not apply on approval |
There was a problem hiding this comment.
🟠 Correct as far as it goes, but the _downloadable flag itself is still written live a few hundred lines up, and that alone cuts existing customers off before any review.
WC_Product::has_file() gates on is_downloadable(), so once _downloadable is no the row disappears from wc_get_customer_available_downloads() entirely. Verified live: vendor unticks Downloadable on a pending edit, paying customer's Downloads page goes empty immediately, admin has approved nothing.
That's the same class of problem as #6045 — one click, no gate, existing buyers affected — and it revokes access rather than substituting it, so it's arguably worse than the file swap. Either stage _downloadable alongside the files, or block the untick while a product is held. Worth a deliberate decision either way rather than leaving it implicit.
Also a correction to my earlier review: _download_limit and _download_expiry do not affect existing customers, because those live per-row on the permission as downloads_remaining / access_expires. Please disregard that part of my previous "minor" — only the _downloadable flag matters here.
There was a problem hiding this comment.
Fixed. _downloadable is now held like the files and applied on approval, so an existing customer keeps their download until an admin publishes.
Covered by test_unticking_downloadable_is_held_then_applied_on_approval.
| $new_files = dokan_downloads_to_files_array( $product->get_downloads() ); | ||
| $changed = dokan_downloadable_file_sets_differ( $approved, $new_files ); | ||
| $owner_product_id = $is_variation ? $product->get_parent_id() : $product_id; | ||
| $released = in_array( $status, dokan_get_downloadable_files_released_statuses( $product_id ), true ); |
There was a problem hiding this comment.
🟠 The gate is purely "status is not published", which also catches marketplaces that have no approval step at all.
Verified: with Product Status set to Publish and a trusted vendor, saving as Draft still stages the file change and shows the vendor "These files are awaiting admin approval" — on a store where nothing is awaiting approval. The same applies to a store admin drafting their own product.
Consider anding in "this save is actually subject to review", e.g. ! dokan_is_seller_trusted( $author_id ) or 'publish' !== dokan_get_default_product_status( $author_id ), so the hold only exists where an approval gate exists.
There was a problem hiding this comment.
Fixed. The hold now also requires ! user_can( $author, 'manage_woocommerce' ) and 'publish' !== dokan_get_default_product_status( $author ), so it only exists where a review step exists.
| * | ||
| * @return bool | ||
| */ | ||
| function dokan_downloadable_files_permission_sync( $product_id = null, $action = 'peek' ) { |
There was a problem hiding this comment.
🟡 Style, non-blocking: one function carrying flag / peek / flush plus a function-static registry is doing three jobs, and a static inside a global function can't be reset between tests. A small class registered through CommonServiceProvider is the house pattern here and would make the two blocker scenarios above testable.
Also worth one line saying the peek branch deliberately does not clear the flag, so the reconcile intentionally runs a second time on shutdown. As written it reads like an oversight rather than the belt-and-braces it is.
| <?php if ( empty( $staged ) ) : ?> | ||
| <p class="form-field"><?php esc_html_e( 'The vendor removed all downloadable files. Publishing will revoke the downloads of existing customers.', 'dokan-lite' ); ?></p> | ||
| <?php else : ?> | ||
| <ul class="form-field"> |
There was a problem hiding this comment.
🟡 Cosmetic: form-field on a <ul> doesn't do what it does on a <p>. In the rendered panel this list breaks out of the WooCommerce label/value grid and sits flush against the left edge of the metabox, outside the field column and past where the "Downloadable files" label starts.
Wrapping the list inside the value column, or following WC's own <p class="form-field"><label>…</label><span>…</span></p> shape, keeps it aligned with the rows it's describing.
The content itself is right, and the panel plus the admin_notices warning together do close the "admin approves blind" blocker from my last review — thank you for pulling that into this PR rather than leaving it as a follow-up.
There was a problem hiding this comment.
Fixed — the panel now uses WooCommerce's <p class="form-field"><label>…</label><span class="description">…</span></p> shape.
| <?php | ||
| $downloadable_files = get_post_meta( $post_id, '_downloadable_files', true ); | ||
| // while an edit awaits admin approval, show the staged files, not the approved ones | ||
| $staged_files = get_post_meta( $post_id, '_dokan_pending_downloadable_files', true ); |
There was a problem hiding this comment.
🟡 This reads the meta directly while the PR ships dokan_get_staged_downloadable_files() for exactly this. Using the helper keeps the "empty array means the vendor removed every file" convention in one place — as written, a staged empty array falls through to is_array() true and renders an empty table, which happens to be right, but only by coincidence.
$staged_files = dokan_get_staged_downloadable_files( $post_id );
$downloadable_files = null !== $staged_files ? $staged_files : get_post_meta( $post_id, '_downloadable_files', true );There was a problem hiding this comment.
Fixed — the template uses dokan_get_staged_downloadable_files() now.
Replaces the previous hold implementation. It grew across three review rounds into five hooks that manipulated the product object mid-save, and the resulting ordering and state-sync bugs were the source of most review findings: the release raced WooCommerce's own meta write, a second staged meta key could desync from the first, and a validation helper meant to protect customers failed open. Every save path writes _downloadable_files through update_post_meta() — the classic vendor form writes it directly, and WooCommerce CRUD (the REST endpoints, the product editor, the wp-admin screen and WP-CLI) writes it from update_downloads(). Filtering that write covers all of them from one place, so nothing needs to touch the product object: - update_post_metadata / delete_post_metadata stage a submission and leave the live files alone, so existing customers keep what they already have. - The two woocommerce_process_product_file_download_paths_* filters block permission changes while a submission is held. - save_post at priority 999 releases on publish or private. A save that sets the files itself wins over the staged submission; one that merely echoes back what it was handed is an approval, not an edit. Only the vendor can withdraw their own submission. An administrator opening a pending product to review it re-submits the rendered approved files, and that must not discard what the vendor sent. Scope is downloadable files only. Holding the Downloadable checkbox itself is a separate problem and is tracked separately; it accounted for most of the defects found in the previous implementation. Vendors see their pending files in the classic form, in REST product responses and in the product editor, each with an awaiting-approval flag. Administrators get a notice and a read-only list of the pending files, including per-variation ones. Adds DownloadableApprovalHoldTest covering the hold, the release on publish and private, draft, full removal, resubmission, CRUD approval, admin edits during approval, trashing, and the cases that are deliberately not held. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MdAsifHossainNadim
left a comment
There was a problem hiding this comment.
Verdict: 🔴 Request changes — the seam refactor is the right shape and closed both round‑2 blockers, but one of them survives on a second path, and the gate still has no reject
Reviewed 730f328b5 on a live WP / WooCommerce 11.0.1 install with Dokan Pro present. Everything below was reproduced with probe tests, not reasoned about. Baseline facts first:
- The PR's own
DownloadableApprovalHoldTestis 14/14 green. - Full Lite suite: 281 tests, 2 failures — both
OrderStatusChangeTest, which is already red ondevelop. No regressions introduced. composer phpcsis clean on the new code; the three hits inwc-functions.php(307 / 473 / 545) are present ondeveloptoo.
Collapsing five hooks into the single update_post_metadata seam was the right call. It is a much smaller surface and it is why the CRUD ordering bugs are gone.
What this commit genuinely fixed — all verified live
| Previous finding | Status |
|---|---|
| Approving over REST orphans the customer's permission (my R2 🔴1) | ✅ the $released && ! $changed echo detection handles it; live files and permission both land on the staged set |
| New editor discards the vendor's replacement — v3 path (my R2 🔴2) | ✅ dokan_rest_show_staged_downloadable_files surfaces the staged set on dokan/v3 |
| Draft / non‑publish bypass (@dev-shahed #2) | ✅ |
| CRUD / REST / product editor get no staging (@dev-shahed #3) | ✅ |
| Admin edits rows while approving → their set wins | ✅ |
| Gate fires on marketplaces with no approval step (my R2 🟠) | ✅ dokan_get_default_product_status() + manage_woocommerce now gate it |
form-field on a <ul> (my R2 🟡) |
✅ now uses WC's <p class="form-field"><label>…</label><span class="description">…</span></p> shape |
I also re-verified, end to end: the classic vendor form hold and release; variation staging held by the parent's status and released on parent publish (the permission row does move to the new download id); trashing discarding the submission.
🔴 Blocker 1 — dokan/v1/products still destroys the vendor's held submission
This is round 2's Blocker 2. It was fixed on dokan/v3 and left open on dokan/v1, because dokan_rest_show_staged_downloadable_files is hooked on woocommerce_rest_prepare_product_object, while dokan/v1's ProductController extends DokanRESTController and applies dokan_rest_prepare_product_object instead (includes/REST/ProductController.php:1136).
Reproduced:
vendor stages file B: live=[file-A] staged=[file-B]
GET /dokan/v1/products/<id>: downloads=[file-A] dokan_downloads_awaiting_approval: ABSENT
PUT the same payload back: staged meta = '' (deleted) live=[file-A]
The vendor's upload is gone — no notice, no record, nothing left to re-apply. Product\Manager::update() (:582) is worse still: it has no read side at all, so nothing there can ever show the vendor what they submitted.
On develop file B at least reached the customer. Here it evaporates, so this is a strict regression on that path — the same shape as the v3 bug we just fixed.
Fix direction inline below.
🔴 Blocker 2 — the gate has no reject; a rejected file ships on the next approval
Pro's ProductStatusService::STATUS_REJECTED = 'reject' is not in dokan_get_downloadable_files_released_statuses(), so a rejection leaves the submission staged:
vendor stages B → admin rejects (status 'reject') → staged=[file-B] still present
vendor fixes the title, admin publishes → live=[file-B] perms=[file-B]
The administrator receives exactly the file they explicitly rejected. Trashing is handled (trashed_post); the feature that actually exists for rejecting is not. There is also still no way to keep the product and drop the pending files — #3390 lists a Discard button as part of this PR's Phase 1, and it isn't here.
Not a regression against develop (there the file was already live), but it is the half that turns a delay into a review gate, which is the whole point of dokan-pro#6045.
🟠 Worth a deliberate decision before merge
The new React editor never tells the vendor their file is held. dokan_downloads_awaiting_approval is emitted in the REST response and in dokan_product_editor_args — and nothing consumes it. There are zero hits for it (or for any awaiting.?[Aa]pproval) under src/, and the diff touches no .tsx. The classic form got its warning paragraph in templates/products/downloadable.php; the new editor got a flag nobody renders. Given that "resubmitting the approved files withdraws the submission" is the rule, telling the vendor nothing is precisely what makes an accidental withdrawal possible.
An administrator editing files on a product that stays pending has their edit staged, not applied. Reproduced:
vendor stages B → admin (not publishing) sets file C
result: staged=[file-C] live=[file-A] perms=[file-A]
Two problems in one. The reviewer's own edit silently overwrites the submission they were reviewing (B is unrecoverable), and the wp-admin panel then renders C under "The vendor submitted replacement files with this pending update" — it attributes the admin's own file to the vendor. Either an admin's edit should apply immediately (they are the authority), or the panel needs to say who staged what.
Unchecking "Downloadable" still cuts existing customers off instantly. Re-verified: wc_get_customer_available_downloads() goes 1 → 0 the moment a vendor unticks it on a pending product, _downloadable is written no, and no administrator has reviewed anything. Same class as #6045, one click, and it revokes access rather than substituting it.
The commit message says this is "tracked separately". I searched getdokan/dokan, getdokan/dokan-pro and plugin-internal-tasks and could not find an issue for it — #3390 is file versioning and does not cover the flag. Please file it before merge so the deferral is real rather than implicit.
🟡 Minor
- The extra
wp_unslash()corrupts staged values.update_metadata()unslashes before firingupdate_post_metadata, so the value the handler receives is already unslashed and the second one is lossy. Detail and repro inline. save_postat 999 loads a product on every product save.dokan_apply_staged_downloadable_files()callswc_get_product()for every save that lands in a released status, when almost always nothing is staged. Similarlydokan_downloadable_hold_applies()runs an uncachedwc_get_product()+user_can()+$wpdbquery per_downloadable_fileswrite — that is once per row on a CSV import.- Docblocks don't match signatures.
dokan_downloadable_files_released_this_request()is documented@param bool $set/@return boolbut takes an array and returnsarray|null;dokan_staged_downloads_for_rest()'s second parameter$as_attachment_idsis undocumented. - Function‑static state, again (repeat of my round‑2 note).
dokan_releasing_staged_downloadable_files(),dokan_downloadable_files_released_this_request()and the$heldcache indokan_block_held_download_permission_change()are request‑scoped mutable state inside global functions — not resettable between tests, and not the container pattern. The two subtlest mechanisms in this PR are exactly the ones the new tests cannot reach because of it. - Undeclared REST field.
dokan_downloads_awaiting_approvallands on every product response includingwc/v3/products/<id>(verified), but is in no schema, so it is invisible to_fieldsand to generated clients.register_rest_field(), or the Dokan controllers' own schema, would be cleaner — and it probably should not be on the WooCommerce namespace at all. - The PR description is stale. It still describes the first implementation and lists "the REST/new product form path, variable products, and surfacing the staged file to admins" as intentional out‑of‑scope follow‑ups — all three are now in. The How to test steps exercise only the classic form, so QA will not touch any of the CRUD / REST / variation / admin‑panel behaviour this PR actually added. Worth rewriting both before this goes to testing.
- The branch is 33 commits behind
develop.
Things I checked that turned out to be fine
attachment_url_to_postid()returning"0"for every external file URL indokan_staged_downloads_for_rest()looked wrong, but it mirrorsFormSchema.php:1189exactly — pre‑existing, not this PR's.- Backslashes in a file URL are stripped with or without the hold (upstream sanitisation), so only the file name corruption noted inline is attributable here.
use WP_Post;is present inAdmin/Hooks.php, so the notice and panel guards really do pass.- The WooCommerce General tab is only
hide_if_grouped, so the pending‑files panel does render for variable products.
Scope note
Unchanged from both earlier rounds, and still unfiled: the documentation gap from #6045 (two published pages still describe the removed "Edited Product Status" setting) and option B (staging the whole pending edit). Plus the Lite‑only forced‑publish self‑approval — handle_product_update() passes $_POST['post_status'] to wp_update_post() unvalidated, and Pro's change_product_status() only covers it when Pro is active.
Reviewed against 730f328b5. Verification: the PR's suite plus the full Lite suite on a live WP / WC 11.0.1 install, and targeted probe tests for each finding above (dokan/v1 round trip, Pro reject status, admin edit while pending, slashed meta write, variation hold/release, wc/v3 response shape).
| delete_post_meta( $meta_id, '_dokan_pending_downloadable_files' ); | ||
| } | ||
| } | ||
| add_action( 'save_post', 'dokan_apply_staged_downloadable_files', 999, 2 ); |
There was a problem hiding this comment.
🔴 Blocker 2 lives here, and a 🟡 performance note with it.
Reject. Release is keyed off "status is in the released list", and Pro's rejection status is not — ProductStatusService::STATUS_REJECTED = 'reject'. So rejecting leaves the submission staged, and the next publish delivers it:
vendor stages B → admin rejects (status 'reject') → staged=[file-B] still present
vendor fixes the title, admin publishes → live=[file-B] perms=[file-B]
trashed_post covers trashing, but rejection is the gesture the product actually ships for saying no. It needs the same discard — and, separately, the admin needs a way to reject the files while keeping the product (the Discard button #3390 assigns to this PR's Phase 1).
Cost. This runs on every save_post for a product in a released status and immediately calls wc_get_product(), even though in almost every save nothing is staged. A cheap early-out first —
if ( null === dokan_get_staged_downloadable_files( $post_id ) && 'product' === get_post_type( $post_id ) ) {
// only variable parents can carry staging on a child
}— or a parent-level marker meta written whenever a variation is staged, would skip the product load for the overwhelmingly common case. The same applies to dokan_downloadable_hold_applies(), which does an uncached wc_get_product() + user_can() + $wpdb query per _downloadable_files write; that is once per row on a CSV import.
There was a problem hiding this comment.
Reject is fixed. reject and trash now discard the submission, so a later approval cannot deliver a rejected file. There is also a Discard pending files button so an admin can drop the files while keeping the product.
Leaving this open for the performance half: the release handler early-outs before loading the product now, but dokan_downloadable_hold_applies() is still uncached per write.
| return $check; | ||
| } | ||
|
|
||
| update_post_meta( $object_id, '_dokan_pending_downloadable_files', wp_slash( $new_files ) ); |
There was a problem hiding this comment.
🟠 An administrator's own edit lands here too, and it should probably not.
Reproduced: with [file-B] staged, an admin who opens the pending product, sets file-C and saves without publishing ends at
staged=[file-C] live=[file-A] perms=[file-A]
Two consequences. The vendor's submission is destroyed by the person reviewing it, silently and unrecoverably. And Admin\Hooks::render_pending_downloadable_files() then presents file-C under "The vendor submitted replacement files with this pending update", attributing the reviewer's own file to the vendor.
Worth a deliberate call: either a reviewer's edit applies immediately (they are the approving authority — the same reasoning the released-status branch above already uses), or the staging records who wrote it so the admin panel can say so.
There was a problem hiding this comment.
Half done. The misattribution is fixed — the staging records who wrote it and the panel says so.
The behaviour is unchanged: a reviewer's edit is still staged rather than applied. Leaving this open because it is your call — should an admin's edit apply immediately, or stay staged?
Addresses the round-3 review on 730f328, plus three bugs found by manual testing against a live WooCommerce 11.1 / Dokan Pro install. Blockers from the review: * `dokan/v1` and `dokan/v2` destroyed a held submission. They build their own payload and never run `woocommerce_rest_prepare_product_object`, so a vendor was handed the approved files while their own upload was held; saving that back read as a withdrawal and deleted it. The staged set is now surfaced on `dokan_rest_prepare_product_object` too. * Rejecting a product left the submission staged, so the next approval delivered the very file the administrator turned down. Pro's `reject` status (and `trash`) now discard it, via a filterable list. * `private` was a released status and a vendor can reach it: the dashboard passed `$_POST['post_status']` to `wp_update_post()` unvalidated and Pro's `change_product_status()` only downgrades `publish`. Only `publish` releases now, and a submitted status is validated against `dokan_get_available_post_status()`. Also from the review, and agreed with product: * Unticking "Downloadable" revoked a paying customer's access instantly. The flag is now held like the files and applied on approval. * An administrator can reject the files while keeping the product, via a "Discard pending files" button on the product screen. * The new product editor tells the vendor their files are held. * The staging records who wrote it, so a reviewer's own edit is no longer attributed to the vendor in the admin panel. * Dropped a second `wp_unslash()` that corrupted names containing backslashes; fixed two docblocks that did not match their signatures; added a cheap early-out so the release handler stops before loading the product when nothing is staged. Regression introduced by this PR, found in manual testing: * Saving a held product through a different path re-keys its download ids (`md5( url )` vs UUIDs vs attachment ids) with the file unchanged. The hold blocked the permission update and nothing was staged, so the permission orphaned with no later step to repair it. Permissions now follow a re-key. Pre-existing bugs, both verified against `develop` with the PR's hooks removed: * A CRUD/REST file change never updated download permissions. WooCommerce fires `woocommerce_process_product_file_download_paths` and nothing listened — not core, not Dokan — so any swap through REST, the new editor, wp-admin or WP-CLI left the customer's Downloads page empty. A/B: `develop` 1 -> 0 downloads, with this change 1 -> 1. * A vendor's REST save skipped review entirely. `dokan_update_product_post_data` is where Pro downgrades an untrusted vendor's `publish` to pending, and no REST controller applied it, so the approval setting did not exist for the new product editor. Verified live: without this, a vendor's file swap stayed published and went straight to customers. The REST status change is the one piece here that reaches beyond downloadable files — it affects every product saved through the new editor. It is scoped to a vendor saving their own product, skips anyone who can manage WooCommerce, leaves `auto-draft` alone, and is filterable via `dokan_rest_apply_vendor_product_status`. Kept in one commit because the hunks interleave in `wc-functions.php`; happy to split it into its own PR if the team would rather judge it separately. Tests: 24 in DownloadableApprovalHoldTest, covering every case above plus variations, which had no coverage before. Full Lite suite 291 tests, 1 failure (OrderStatusChangeTest, already red on `develop`). PHPCS clean on new code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`dokan_rest_apply_vendor_product_status()` handed `dokan_update_product_post_data` an `ID` of 0 when a product was being created. The filter's consumers are written against the vendor product form, which only ever fires on a product that already exists, and Pro's subscription module calls `wc_get_product( $data['ID'] )->get_status()` with no guard — so creating a product over REST while that module is active was a fatal error. Creation is now skipped. This leaves a separate, pre-existing hole documented inline: `POST /dokan/v3/products` with `status: publish` publishes without review. Verified live (HTTP 201, status `publish`). That deserves its own fix rather than being folded into this PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follows the review on 5f63879. * Moved `dokan_rest_apply_vendor_product_status()` out of this PR. It was the one change whose blast radius exceeded downloadable files — it affects every product saved through the new product editor — and it passed a partial payload (`ID` + `post_status` only) to `dokan_update_product_post_data`, whose existing producer hands consumers the full array. It moves to its own branch where the payload shape can be fixed and judged independently. * `dokan_staged_downloads_for_rest()` no longer collapses every external file to attachment id "0". `attachment_url_to_postid()` returns 0 for anything that is not a WP attachment, and downloadable files are commonly external URLs, so two of them collided on the same row id in the product editor payload. Falls back to the download id, which is unique. * Documented the global surface of the `update_post_metadata` / `delete_post_metadata` filters and why the two guards are ordered as they are. Confirmed `get_post_type()` returns false for a non-post id, so a caller passing something that is not a post never reaches the logic below. * Blank line between `save_custom_bulk_edit_field()` and the method following it. The sniff is not in the project ruleset so PHPCS passed, but the formatting was wrong. On the scheduled-product note: the guard added to `handle_product_update()` falls back to the *current* status rather than a default, so a product genuinely in `future` keeps it. Verified on a live install — offered statuses are draft/pending, a submitted `future` yields `future`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The product editor has two data endpoints applying two different filters. `/dokan/v3/products/init/fields` fires `dokan_product_editor_args`, which was hooked; `/dokan/v3/products/<id>/fields` — the one used to open an existing product, and therefore the only one that can ever have a held submission — fires `dokan_rest_prepare_product_editor_fields`, which was not. Found by a 28-case test matrix across legacy/new editor and trusted/untrusted vendor. Before this, opening a held product in the new editor showed the vendor the approved file, no "awaiting approval" notice, and no flag — so saving it back would have read as a withdrawal and destroyed their upload. Same failure the `dokan/v1` blocker described, on the path the new editor actually uses. Verified: the endpoint now returns the staged file, the notice, and the flag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All Submissions:
Changes proposed in this Pull Request:
On a marketplace where vendor products require admin approval, a vendor could replace the file of an already-approved digital product and the new, unreviewed file reached existing paying customers immediately at save time. Customers simultaneously lost access to the approved file they paid for, and their download count and access date were reset.
This PR holds file delivery until approval, and makes the approval an actual review rather than a delay.
The hold —
dokan_hold_downloadable_files_meta_write()filters the_downloadable_filesmeta write itself. Every save path writes that meta, so one seam covers the classic vendor form,dokan/v1,dokan/v2,dokan/v3,wc/v3, the new product editor, the wp-admin screen, WP-CLI and variations. While a product is awaiting review the submitted set is stored in_dokan_pending_downloadable_filesand the live value is left untouched. The permission handler is blocked for the same period, so existing customers keep both their file and their download count.The hold applies only where a review actually exists: the product is not published, the vendor's saves are genuinely subject to approval (not a trusted vendor, not someone who can manage WooCommerce), and at least one customer already holds a download permission.
Unticking "Downloadable" is held too.
WC_Product::has_file()gates onis_downloadable(), so writingnodropped the product out ofwc_get_customer_available_downloads()entirely — one click, no gate, and it revoked access rather than substituting it. The flag is now staged alongside the files and applied together on approval.Release —
dokan_apply_staged_downloadable_files()runs onsave_postat priority 999. Publishing through any route swaps the permissions and applies the staged set. If the approving save set the files itself, the approver's set wins.Reject — Pro's
rejectstatus and trashing both discard the submission, so a later approval cannot deliver a file the administrator turned down. An administrator can also reject the files while keeping the product, via a Discard pending files button on the product screen.Review surface — the wp-admin product screen shows the pending files with an
admin_noticeswarning, listing the real filename first (the vendor's display label is a free-text field the media picker never rewrites, so after a swap it still reads as the previous file). The staging records who submitted it, so a reviewer's own edit is not attributed to the vendor.Vendor surface — the classic form and the REST/product-editor payloads all show the vendor their own held submission plus an "awaiting approval" notice, so the "resubmitting the approved files withdraws the submission" rule is true on every path.
Download permissions
Three permission bugs surfaced while testing this end to end. Each is called out with its origin:
md5( url )on the classic form, UUIDs or attachment ids on CRUD) with the file unchanged. The hold blocked the permission update and nothing was staged because the URLs matched, so the permission orphaned with no later step to repair it. Permissions now follow a re-key.woocommerce_process_product_file_download_pathsand nothing listened — not WooCommerce core, not Dokan — so any swap through REST, the new editor, wp-admin or WP-CLI left the customer's My Account → Downloads empty. Verified A/B againstdevelop: 1 → 0 downloads there, 1 → 1 with this change.Known follow-ups (deliberately not in this PR)
status: publishpublishes without review — verified (HTTP 201, statuspublish). Pre-existing and unrelated to file delivery.wp_woocommerce_downloadable_product_permissions. An order that is paid but not yet granted its permission — processing on a grant-on-completion setup, or an order in flight during the swap — has no row yet, so a file change goes live and that customer later receives the unreviewed file. Narrow, but real.handle_product_update()now validates the submitted status againstdokan_get_available_post_status(), which closes vendor self-approval on Lite as well.Closes
How to test the changes in this Pull Request:
Setup: Dokan → Settings → Selling Options → Product Status = Pending Review, and an untrusted vendor (one without publish-directly). The hold only engages when a customer already owns the product, so every scenario needs a completed purchase first.
1. Classic vendor form — the core case
wp_woocommerce_downloadable_product_permissionsis untouched.2. New product editor / REST — Dokan → Settings → Appearance → Vendor Product Editor = New UI. Requires
fix/vendor-product-status-on-rest-saveas well, otherwise the product never drops to Pending Review and there is nothing to hold. With both: repeat steps 3–6. The vendor must be shown their own pending file with an "awaiting approval" notice, and the customer must keep file A.3. Approving over REST —
GETthe product, flipstatustopublish,PUTthe payload back unchanged. The customer must end up on file B with a working download, not an empty Downloads page.4. Admin edits while approving — with file B staged, set file C on the wp-admin screen and publish. C wins, and existing customers follow C.
5. Admin saves without publishing — with file B staged, save the pending product without publishing. B must survive.
6. Reject — with file B staged, reject the product. The submission is discarded; approving later delivers file A, never B. Trashing behaves the same.
7. Discard button — with file B staged, press Discard pending files. The submission is dropped and customers keep file A.
8. Unticking Downloadable — with the product pending, untick Downloadable and save. The customer must keep their download until an admin publishes; on publish, access is correctly revoked.
9. Variations — stage a file on a variation of a variable product. It is held by the parent's status and released when the parent is published.
10. Regression checks — trusted vendors and marketplaces with approval disabled still apply file changes instantly; unsold pending products still write files directly; a vendor re-submitting the approved files cancels their staged swap.
Changelog entry
fix: Unreviewed replacement files were delivered to existing customers before admin approval
Previously, when a vendor replaced the file of a downloadable product on a marketplace requiring admin approval, existing customers received the unreviewed file immediately at vendor-save time and lost access to the approved file, while the product sat in Pending Review. Now customers keep the last approved file until an administrator reviews and publishes the update, and administrators can see and discard a pending file before it ships.
Before Changes
A vendor saving a file swap instantly switched every existing customer to the unreviewed file and reset their download count and access date, while the product sat in Pending Review. Approval status was never consulted. Unticking "Downloadable" revoked access outright. File changes made through REST, the new product editor, wp-admin or WP-CLI left customers' permissions pointing at a file that no longer existed, so their Downloads page went empty. Edits made through the new product editor skipped review altogether.
After Changes
Customers keep the approved file, with their original download count, until an administrator publishes the update. The administrator can see what they are approving, override it, or discard it. Rejecting or trashing drops the submission. Download permissions follow the files on every save path, and edits made through the new product editor go through the same review as the vendor product form.
Tests
tests/php/src/Product/DownloadableApprovalHoldTest.php— 24 tests covering the hold, every negative case, release throughwp_update_postand CRUD, admin override, reject, discard, the id re-key, CRUD permission alignment, the vendor REST round trip, and variations.Full Lite suite: 291 tests, 1 failure (
OrderStatusChangeTest, already failing ondevelop). PHPCS clean on the new code.