enhance: change the withdraw exporter to woocommerce one and add the proper filtering#3054
enhance: change the withdraw exporter to woocommerce one and add the proper filtering#3054imehedi357 wants to merge 8 commits intodevelopfrom
Conversation
imehedi357
added
Needs: Testing
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces frontend withdraw export flow with client-side CSV download when full data is available and a backend export + polling flow otherwise; maps frontend filter params to backend equivalents (status → status_code, after → start_date, before → end_date); adds CSV utilities and type declarations. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Withdraw Page UI
participant Controller as WithdrawExportController
participant ExportSvc as Export Service / Storage
participant CSVUtil as Browser CSV Utility
User->>UI: Click Export (with filters)
activate UI
UI->>UI: Set isExporting = true, show spinner/toast
rect rgba(200,220,255,0.5)
Note over UI,Controller: Send current filters (filterArgs)
UI->>Controller: POST /dokan/v1/reports/withdraws/export (filterArgs)
end
activate Controller
Controller->>Controller: Map status → status_code\nMap after → start_date\nMap before → end_date
Controller->>ExportSvc: Enqueue export job / generate CSV
ExportSvc-->>Controller: Return export_id or download_url
Controller-->>UI: Return export_id / status endpoint
deactivate Controller
rect rgba(220,255,220,0.5)
loop poll until ready/error
UI->>ExportSvc: GET export status(export_id)
ExportSvc-->>UI: Status (pending/ready/error) or download_url
end
end
alt ready & URL returned
UI->>UI: isExporting = false
UI->>CSVUtil: Trigger direct download or fetch download_url
UI->>User: File downloaded
else error/timeout
UI->>UI: isExporting = false
UI->>User: Show error/toast
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/admin/dashboard/pages/withdraw/index.tsx (1)
578-582: Consider using a toast notification instead ofalert().While the error handling logic is correct and the
finallyblock properly resets the state, usingalert()for error messages provides a suboptimal user experience. Consider replacing it with a toast or notification component for more modern and user-friendly error feedback.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/REST/WithdrawExportController.phpsrc/admin/dashboard/pages/withdraw/index.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
includes/REST/WithdrawExportController.php (2)
dokan.php (1)
dokan(90-92)includes/Withdraw/Manager.php (1)
get_status_code(285-299)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (1, 3)
🔇 Additional comments (6)
includes/REST/WithdrawExportController.php (2)
98-100: LGTM!The mapping of
aftertostart_datecorrectly transforms the REST API parameter to the internal query argument format expected bydokan()->withdraw->all().
102-104: LGTM!The mapping of
beforetoend_datecorrectly transforms the REST API parameter to the internal query argument format expected bydokan()->withdraw->all().src/admin/dashboard/pages/withdraw/index.tsx (4)
24-24: LGTM!The
Loader2import is correctly added and used for the export loading indicator.
91-91: LGTM!The
isExportingstate correctly tracks the export operation status and controls the button's disabled state and loading indicator.
585-595: LGTM!The conditional rendering provides clear visual feedback during the export process. The loading spinner with "Exporting..." text and the disabled button state create a good user experience.
554-571: Clarify whetherper_page: 100limits the export to 100 records or serves as a batch size for internal pagination.The export implementation uses WooCommerce's
GenericController+ExportableInterfacepattern with background job processing (indicated by the polling mechanism). However, theWithdrawExportController::get_items()method directly appliesper_pageas a databaseLIMITwithout visible pagination looping.To confirm this fulfills the PR requirement ("export should include all matching rows across pagination"), verify:
- Whether WooCommerce's
GenericControllerinternally loops through pages during export processing- Or if
per_page: 100acts as a hard limit on total exported recordsIf the latter, the export should either remove the per_page limit or loop through all pages to fetch complete datasets.
| if ( ! empty( $request['status'] ) ) { | ||
| $args['status'] = $request['status']; | ||
| $args['status'] = dokan()->withdraw->get_status_code( $request['status'] ); | ||
| } |
There was a problem hiding this comment.
Consider validating the status conversion result.
If an invalid status somehow bypasses the enum validation or if get_status_code returns null, assigning null to $args['status'] might produce unexpected query behavior. Consider only setting the status if the conversion succeeds:
🔎 Suggested defensive check
if ( ! empty( $request['status'] ) ) {
- $args['status'] = dokan()->withdraw->get_status_code( $request['status'] );
+ $status_code = dokan()->withdraw->get_status_code( $request['status'] );
+ if ( null !== $status_code ) {
+ $args['status'] = $status_code;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( ! empty( $request['status'] ) ) { | |
| $args['status'] = $request['status']; | |
| $args['status'] = dokan()->withdraw->get_status_code( $request['status'] ); | |
| } | |
| if ( ! empty( $request['status'] ) ) { | |
| $status_code = dokan()->withdraw->get_status_code( $request['status'] ); | |
| if ( null !== $status_code ) { | |
| $args['status'] = $status_code; | |
| } | |
| } |
🤖 Prompt for AI Agents
In includes/REST/WithdrawExportController.php around lines 90 to 92, the code
assigns $args['status'] directly from
dokan()->withdraw->get_status_code($request['status']) which may return null;
update this to capture the conversion result in a variable, check that it is not
null (or not false) before setting $args['status'], and if the conversion fails
either skip setting the status filter or handle the invalid value (e.g., return
a validation error or use a safe default) to avoid passing null into the query.
|
@mrabbani, @imtiaz-pranto vai, the feature of the dynamic name of the filtered data is not added. The file name is currently the default name provided from WooCommerce. |
|
@mrabbani This pr not working for export.. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/admin/dashboard/pages/withdraw/index.tsx (1)
557-576:⚠️ Potential issue | 🔴 CriticalExport request likely broken: endpoint, date params, and pagination concerns.
Several issues with this export flow:
Endpoint mismatch with PR objective: The PR states the export controller was switched to v1 using WooCommerce Admin's export (
/dokan/v1/reports/withdraws/exportvia POST), but this handler sends a GET todokan/v2/withdraw. Which endpoint actually handles the export? The existinghandleExportWithdraws(Line 668) already uses the v1 endpoint with POST — is this button supposed to use the same flow?Date filter params won't reach the backend:
filterArgsstores dates asstart_date/end_date(set at Line 1004–1005), but per the PR description the backend REST API expectsafter/before(which it then maps internally). The export request will silently drop date filters.Spreading
...viewsends irrelevant UI state:viewcontainstype,titleField,layout,fields(an array of column IDs), etc. These are meaningless to the API and could cause unexpected behavior if any key collides with a query param.Pagination not removed: PR changelog says "pagination removed during export so all matching records are included," but
perPageandpagefromvieware still sent.Suggested approach (if using the v1 export endpoint)
- const path = addQueryArgs( 'dokan/v2/withdraw', { - ...view, - ...filterArgs, - is_export: true, - } ); - const res = await apiFetch( { path } ); - if ( res && res.url ) { - window.location.assign( res.url as string ); - } + // Map filterArgs date keys to what the REST API expects + const exportArgs = { + status: view.status === 'all' ? '' : view.status, + search: view.search || '', + ...(filterArgs.user_id && { user_id: filterArgs.user_id }), + ...(filterArgs.payment_method && { payment_method: filterArgs.payment_method }), + ...(filterArgs.start_date && { after: filterArgs.start_date }), + ...(filterArgs.end_date && { before: filterArgs.end_date }), + }; + const res = await apiFetch( { + path: '/dokan/v1/reports/withdraws/export', + method: 'POST', + data: { report_args: exportArgs, email: false }, + } ); + // Handle response per the v1 export flow...
🧹 Nitpick comments (2)
src/admin/dashboard/pages/withdraw/index.tsx (2)
572-573: Consider replacingalert()with a toast/snackbar notification.Native
alert()blocks the UI thread and feels out of place in a modern React admin dashboard. A non-blocking toast (e.g., via@wordpress/noticesor a similar pattern already used in this codebase) would be more consistent.
668-706: Dead or underutilized code:handleExportWithdrawsis only used for PayPal mass payment.This function implements the v1 export endpoint flow (
/dokan/v1/reports/withdraws/exportwith POST + polling), which aligns with the PR's stated backend changes. However, the new Export button (Line 557) doesn't use it — it sends a GET todokan/v2/withdrawinstead. If the main export is supposed to use the v1 WooCommerce Admin export flow, consider wiring the Export button to callhandleExportWithdraws(or a refactored version of it) rather than duplicating a separate, potentially broken flow.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/admin/dashboard/pages/withdraw/index.tsx (1)
753-809:⚠️ Potential issue | 🟠 MajorPolling implementation has structural issues.
resolveisundefinedon first call (Line 808):checkStatusis called without arguments, soresolveisundefined. Theif (resolve)guard prevents a crash, but means the outerawaiton Line 808 resolves immediately (sincecheckStatusreturns after the first iteration without awaiting the recursive chain). The polling effectively becomes fire-and-forget.Recursive
setTimeout+async/awaitmix is fragile: WrappingsetTimeoutin anew Promiseand recursively callingcheckStatusinside it creates a hard-to-follow control flow. A simple loop withawait new Promise(r => setTimeout(r, 5000))would be clearer and correctly awaitable.Suggested rewrite
- const pollExportStatus = async ( exportId ) => { - const maxAttempts = 60; - let attempts = 0; - - const checkStatus = async ( resolve ) => { - try { - const statusResponse = await apiFetch( { - path: `/dokan/v1/reports/withdraws/export/${ exportId }/status`, - method: 'GET', - } ); - - console.log( 'Export status:', statusResponse ); - - if ( statusResponse.percent_complete === 100 ) { - if ( statusResponse.download_url ) { - const link = document.createElement( 'a' ); - link.href = statusResponse.download_url; - link.download = ''; - document.body.appendChild( link ); - link.click(); - document.body.removeChild( link ); - console.log( 'Export completed and downloaded' ); - } else { - throw new Error( 'Download URL not available' ); - } - } else { - attempts++; - if ( attempts < maxAttempts ) { - await new Promise( - ( res ) => - setTimeout( - async () => await checkStatus( res ), - 5000 - ) - ); - } else { - throw new Error( 'Export timeout - please try again' ); - } - } - } catch ( error ) { - console.error( 'Error checking export status:', error ); - alert( __( 'Export failed. Please try again.', 'dokan-lite' ) ); - } - - if ( resolve ) { - resolve(); - } - }; - - await checkStatus(); - }; + const pollExportStatus = async ( exportId ) => { + const maxAttempts = 60; + + for ( let attempt = 0; attempt < maxAttempts; attempt++ ) { + const statusResponse = await apiFetch( { + path: `/dokan/v1/reports/withdraws/export/${ exportId }/status`, + method: 'GET', + } ); + + if ( statusResponse.percent_complete === 100 ) { + if ( ! statusResponse.download_url ) { + throw new Error( 'Download URL not available' ); + } + const link = document.createElement( 'a' ); + link.href = statusResponse.download_url; + link.download = ''; + document.body.appendChild( link ); + link.click(); + document.body.removeChild( link ); + return; + } + + await new Promise( ( r ) => setTimeout( r, 5000 ) ); + } + + throw new Error( 'Export timeout - please try again' ); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/admin/dashboard/pages/withdraw/index.tsx` around lines 753 - 809, The polling routine in pollExportStatus is broken because checkStatus is called without a resolver so the outer await returns immediately and the recursive setTimeout/Promise mix is fragile; replace the recursive checkStatus pattern with a clear loop inside pollExportStatus (use maxAttempts and attempts already present) that awaits a simple sleep (await new Promise(r => setTimeout(r, 5000))) between iterations, perform the apiFetch each loop, handle percent_complete === 100 by downloading via the same DOM-link logic in your diff, and on errors either throw or return a rejected Promise so callers awaiting pollExportStatus actually wait for completion or failure; update/remove checkStatus and ensure pollExportStatus is an async function that only resolves after either successful download or a terminal error/timeout.
🧹 Nitpick comments (3)
src/utils/download-csv.ts (1)
64-64: Remove commented-out debug statement.Leftover debug artifact.
Proposed fix
- //console.log(rows); -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/download-csv.ts` at line 64, Remove the leftover commented debug statement "//console.log(rows);" from the download-csv utility; locate the commented line in the downloadCsv/downloadCSV function or wherever "rows" is handled and delete the commented console.log to clean up the code and prevent clutter in production.src/admin/dashboard/pages/withdraw/index.tsx (2)
712-751:handleExportWithdrawsusesconsole.logandalert— prefer toast for consistency.The rest of the new export code uses
toast()for user feedback, but this function still usesconsole.log(Line 716, 732) andalert(Line 744). Consider aligning to the toast pattern for consistent UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/admin/dashboard/pages/withdraw/index.tsx` around lines 712 - 751, In handleExportWithdraws replace the console.log and alert usages with the app's toast pattern: use toast.info or toast.loading when starting the export (instead of console.log 'Starting withdraw export...'), use toast.success when export is initiated (instead of console.log for exportResponse.export_id), and use toast.error in the catch block with the same message used in the alert (instead of alert and console.error); keep polling via pollExportStatus and apiFetch unchanged so only the user-facing messages switch to toast for consistency.
578-589:shouldEmailheuristic is fragile — it compares a single page to the total.
dataholds only the current page (e.g. 10 items) whiletotalItemsis the full count. Sodata.length < totalItemswill almost always betruewhenever there's more than one page, forcing the email path even for modest result sets. If the intent is to offer direct download when all data is available client-side, you'd need to fetch all pages first or always use the backend export for multi-page data sets (which seems to be the case). Consider adding a comment clarifying this intent, asshouldEmailreads like a heuristic rather than a deliberate design choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/admin/dashboard/pages/withdraw/index.tsx` around lines 578 - 589, The current heuristic using shouldEmail (const shouldEmail = data && data.length < totalItems) is fragile because data is a single page; replace it with an explicit check for whether all results are loaded (e.g., introduce an isAllDataLoaded boolean or use totalItems === data.length) and use that to decide directDownloadCSV vs backend email; if you can't load all pages client-side, always use the backend export for multi-page results and add a comment above the check clarifying the intent (referencing shouldEmail, data, totalItems, directDownloadCSV, WITHDRAW_CSV_HEADERS, and filterArgs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/admin/dashboard/pages/withdraw/index.tsx`:
- Around line 574-621: The export onClick handler mishandles errors and misses
the backend-polling path: update the async handler so the catch block shows a
user-facing toast (or other UI error) instead of re-throwing; when checking
exportResponse ensure you safely read the error message (e.g., fallback to
exportResponse?.data?.message || exportResponse?.message || a default) before
throwing; and after a successful POST that returns exportResponse.export_id (and
when shouldEmail is false and directDownloadCSV wasn't used) call the existing
pollExportStatus helper (or reuse handleExportWithdraws flow) to poll/download
the CSV, ensuring setIsExporting(false) still runs in finally and toasts are
used for success/failure notifications.
In `@types/externals.d.ts`:
- Around line 59-62: Update the generateCSVDataFromTable declaration to use the
correct row element shape: change the rows parameter type from
Array<Array<string>> to Array<Array<{ display: string; value: string | number
}>> so it matches `@woocommerce/csv-export` expectations; locate the function
signature for generateCSVDataFromTable in types/externals.d.ts and replace the
rows type accordingly, keeping headers as Array<{ key: string; label: string }>
and preserving the return type string.
---
Outside diff comments:
In `@src/admin/dashboard/pages/withdraw/index.tsx`:
- Around line 753-809: The polling routine in pollExportStatus is broken because
checkStatus is called without a resolver so the outer await returns immediately
and the recursive setTimeout/Promise mix is fragile; replace the recursive
checkStatus pattern with a clear loop inside pollExportStatus (use maxAttempts
and attempts already present) that awaits a simple sleep (await new Promise(r =>
setTimeout(r, 5000))) between iterations, perform the apiFetch each loop, handle
percent_complete === 100 by downloading via the same DOM-link logic in your
diff, and on errors either throw or return a rejected Promise so callers
awaiting pollExportStatus actually wait for completion or failure; update/remove
checkStatus and ensure pollExportStatus is an async function that only resolves
after either successful download or a terminal error/timeout.
---
Nitpick comments:
In `@src/admin/dashboard/pages/withdraw/index.tsx`:
- Around line 712-751: In handleExportWithdraws replace the console.log and
alert usages with the app's toast pattern: use toast.info or toast.loading when
starting the export (instead of console.log 'Starting withdraw export...'), use
toast.success when export is initiated (instead of console.log for
exportResponse.export_id), and use toast.error in the catch block with the same
message used in the alert (instead of alert and console.error); keep polling via
pollExportStatus and apiFetch unchanged so only the user-facing messages switch
to toast for consistency.
- Around line 578-589: The current heuristic using shouldEmail (const
shouldEmail = data && data.length < totalItems) is fragile because data is a
single page; replace it with an explicit check for whether all results are
loaded (e.g., introduce an isAllDataLoaded boolean or use totalItems ===
data.length) and use that to decide directDownloadCSV vs backend email; if you
can't load all pages client-side, always use the backend export for multi-page
results and add a comment above the check clarifying the intent (referencing
shouldEmail, data, totalItems, directDownloadCSV, WITHDRAW_CSV_HEADERS, and
filterArgs).
In `@src/utils/download-csv.ts`:
- Line 64: Remove the leftover commented debug statement "//console.log(rows);"
from the download-csv utility; locate the commented line in the
downloadCsv/downloadCSV function or wherever "rows" is handled and delete the
commented console.log to clean up the code and prevent clutter in production.
| onClick={ async () => { | ||
| setIsExporting(true); | ||
|
|
||
| try { | ||
| // Minimal placeholder; backend export flow may vary. | ||
| // Attempt to hit export endpoint via same query params. | ||
| const path = addQueryArgs( 'dokan/v2/withdraw', { | ||
| const shouldEmail = data && data.length < totalItems; | ||
|
|
||
| // If all data is already loaded, download directly | ||
| if ( ! shouldEmail && data && data.length > 0 ) { | ||
| const csvData = data.map( ( item ) => ( { | ||
| ...item, | ||
| user_id: item.user?.id, | ||
| vendor_name: item.user?.store_name, | ||
| } ) ); | ||
| directDownloadCSV( 'withdraws', WITHDRAW_CSV_HEADERS, csvData, filterArgs ); | ||
| return; | ||
| } | ||
|
|
||
| if ( shouldEmail ) { | ||
| toast( { | ||
| type: 'info', | ||
| title: __( 'Your withdraw Report will be emailed to you.', 'dokan-lite' ), | ||
| } ); | ||
| } | ||
|
|
||
| // Otherwise, use email delivery via API | ||
| const reportArgs = { | ||
| ...view, | ||
| ...filterArgs, | ||
| is_export: true, | ||
| } ); | ||
| const res = await apiFetch( { path } ); | ||
| if ( res && res.url ) { | ||
| window.location.assign( res.url as string ); | ||
| }; | ||
|
|
||
| const exportResponse = await apiFetch({ | ||
| path: '/dokan/v1/reports/withdraws/export', | ||
| method: 'POST', | ||
| data: { | ||
| report_args: reportArgs, | ||
| email: shouldEmail, | ||
| }, | ||
| }); | ||
|
|
||
| if ( ! exportResponse.export_id ) { | ||
| throw new Error(exportResponse.data.message); | ||
| } | ||
| } catch ( e ) { | ||
| // eslint-disable-next-line no-console | ||
| console.error( 'Export failed or not supported yet', e ); | ||
|
|
||
| } catch (e) { | ||
| throw new Error( __('Export failed. Please try again.', 'dokan-lite') ); | ||
| } finally { | ||
| setIsExporting(false); | ||
| } |
There was a problem hiding this comment.
Export handler has multiple error-handling issues and a dead-end code path.
-
Unhandled re-throw (Line 618): The
catchblock re-throws a newError, but since this is anasync onClickhandler, the re-thrown error becomes an unhandled promise rejection. The user sees no feedback. Either show a toast/alert here instead of re-throwing, or remove the re-throw. -
Potential TypeError (Line 614): If the API response lacks
export_id, accessingexportResponse.data.messagemay throw aTypeErrorifexportResponse.datais also undefined. -
No polling after backend export (Lines 604–615): When
shouldEmailisfalsebut the direct-download path at Line 581 is skipped (e.g., emptydata), the code obtains anexport_idfrom the API but never callspollExportStatusto retrieve the file. ThehandleExportWithdrawsfunction (Line 713) already implements this pattern — consider reusing it or adding polling here.
Proposed fix for error handling
- } catch (e) {
- throw new Error( __('Export failed. Please try again.', 'dokan-lite') );
+ } catch (e) {
+ toast( {
+ type: 'error',
+ title: __( 'Export failed. Please try again.', 'dokan-lite' ),
+ } );
} finally {
setIsExporting(false);
} if ( ! exportResponse.export_id ) {
- throw new Error(exportResponse.data.message);
+ throw new Error(exportResponse?.data?.message || __('Export failed.', 'dokan-lite'));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin/dashboard/pages/withdraw/index.tsx` around lines 574 - 621, The
export onClick handler mishandles errors and misses the backend-polling path:
update the async handler so the catch block shows a user-facing toast (or other
UI error) instead of re-throwing; when checking exportResponse ensure you safely
read the error message (e.g., fallback to exportResponse?.data?.message ||
exportResponse?.message || a default) before throwing; and after a successful
POST that returns exportResponse.export_id (and when shouldEmail is false and
directDownloadCSV wasn't used) call the existing pollExportStatus helper (or
reuse handleExportWithdraws flow) to poll/download the CSV, ensuring
setIsExporting(false) still runs in finally and toasts are used for
success/failure notifications.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/admin/dashboard/pages/withdraw/index.tsx (1)
787-789: Inconsistent error notification:alert()vstoast().
pollExportStatususesalert()for error feedback (line 789), while the rest of the new export flow usestoast(). Consider usingtoasthere as well for a consistent UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/admin/dashboard/pages/withdraw/index.tsx` around lines 787 - 789, The catch block in the pollExportStatus function uses alert(...) which is inconsistent with the rest of the export flow that uses toast; replace the alert call with the same toast-based notification used elsewhere (e.g., toast.error or toast(...)) and ensure the toast utility/import used by the module is referenced (match the existing toast usage pattern in this file) so the error message and UX remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/admin/dashboard/pages/withdraw/index.tsx`:
- Around line 28-34: WITHDRAW_CSV_HEADERS is computed at module load using
Object.entries(dokanAdminDashboardSettings.withdraw.columns) which will throw if
withdraw or columns is missing; change this to safely access the path and
provide a fallback empty object, e.g. replace the top-level computation with a
guarded expression using optional chaining and a default:
Object.entries(dokanAdminDashboardSettings?.withdraw?.columns ?? {}) and map
that result, and to avoid module-load crashes compute it lazily where used (e.g.
inside the component or a useMemo) rather than at top-level; update references
to WITHDRAW_CSV_HEADERS accordingly or generate headers via a
getWithdrawCsvHeaders() helper that performs the same guarded access.
- Around line 527-529: The current mapping of fields produces an empty string
for the vendor field (fields: fields.map(... field.id !== 'vendor' ? field.id :
'')) which leaves a bogus entry; replace this with a filter-first (or
filter-after) approach so only real ids are included—for example use
fields.filter(f => f.id !== 'vendor').map(f => f.id) (or map then
.filter(Boolean)) so the fields array passed to DataViews contains no empty
strings; update the code that builds fields (the fields variable/mapping)
accordingly.
- Around line 556-578: The onClick handler in tabsAdditionalContents can fall
through and call the backend export with email: false when there are zero rows;
update the handler to short-circuit early when totalItems === 0 (or when data &&
data.length === 0 and totalItems === 0) to avoid kicking off a backend export
for zero rows: if no items, reset isExporting (call setIsExporting(false)),
optionally notify the user, and return before building csvData or calling the
export API; keep the existing directDownloadCSV path (data.length > 0) and the
backend POST path for real exports unchanged.
- Around line 569-577: The CSV export is including the raw nested details object
so the details column becomes "[object Object]"; update the csvData mapping used
before calling directDownloadCSV so that instead of spreading item.details as-is
you normalize/flatten it the same way the UI does (use the existing
processDetails(item.details) helper or its returned flattened fields) and assign
those flattened/serializable values (or a JSON/string representation) to the
details-related CSV fields; keep WITHDRAW_CSV_HEADERS in sync with those
flattened keys so objectToCSVRows produces readable CSV rows.
---
Nitpick comments:
In `@src/admin/dashboard/pages/withdraw/index.tsx`:
- Around line 787-789: The catch block in the pollExportStatus function uses
alert(...) which is inconsistent with the rest of the export flow that uses
toast; replace the alert call with the same toast-based notification used
elsewhere (e.g., toast.error or toast(...)) and ensure the toast utility/import
used by the module is referenced (match the existing toast usage pattern in this
file) so the error message and UX remain consistent.
| // Define withdraw CSV headers | ||
| const WITHDRAW_CSV_HEADERS = Object.entries( | ||
| dokanAdminDashboardSettings.withdraw.columns | ||
| ).map( ( [ key, value ] ) => ( { | ||
| key, | ||
| label: value, | ||
| } ) ); |
There was a problem hiding this comment.
No defensive check on dokanAdminDashboardSettings.withdraw.columns.
This runs at module-load time. If withdraw or columns is missing from the global settings object (e.g., a different page context, a race, or a plugin conflict), Object.entries(...) will throw and the entire page will crash.
Proposed fix
// Define withdraw CSV headers
const WITHDRAW_CSV_HEADERS = Object.entries(
- dokanAdminDashboardSettings.withdraw.columns
+ dokanAdminDashboardSettings?.withdraw?.columns ?? {}
).map( ( [ key, value ] ) => ( {
key,
label: value,
} ) );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Define withdraw CSV headers | |
| const WITHDRAW_CSV_HEADERS = Object.entries( | |
| dokanAdminDashboardSettings.withdraw.columns | |
| ).map( ( [ key, value ] ) => ( { | |
| key, | |
| label: value, | |
| } ) ); | |
| // Define withdraw CSV headers | |
| const WITHDRAW_CSV_HEADERS = Object.entries( | |
| dokanAdminDashboardSettings?.withdraw?.columns ?? {} | |
| ).map( ( [ key, value ] ) => ( { | |
| key, | |
| label: value, | |
| } ) ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin/dashboard/pages/withdraw/index.tsx` around lines 28 - 34,
WITHDRAW_CSV_HEADERS is computed at module load using
Object.entries(dokanAdminDashboardSettings.withdraw.columns) which will throw if
withdraw or columns is missing; change this to safely access the path and
provide a fallback empty object, e.g. replace the top-level computation with a
guarded expression using optional chaining and a default:
Object.entries(dokanAdminDashboardSettings?.withdraw?.columns ?? {}) and map
that result, and to avoid module-load crashes compute it lazily where used (e.g.
inside the component or a useMemo) rather than at top-level; update references
to WITHDRAW_CSV_HEADERS accordingly or generate headers via a
getWithdrawCsvHeaders() helper that performs the same guarded access.
| fields: fields.map( ( field ) => | ||
| field.id !== 'vendor' ? field.id : '' | ||
| ), |
There was a problem hiding this comment.
Empty string in fields array — use filter instead of mapping to ''.
Mapping the vendor field to an empty string leaves a bogus entry in the array. If the DataViews component iterates over fields to render columns, it may produce an empty/broken column or a console warning.
Proposed fix
- fields: fields.map( ( field ) =>
- field.id !== 'vendor' ? field.id : ''
- ),
+ fields: fields
+ .map( ( field ) => field.id )
+ .filter( ( id ) => id !== 'vendor' ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fields: fields.map( ( field ) => | |
| field.id !== 'vendor' ? field.id : '' | |
| ), | |
| fields: fields | |
| .map( ( field ) => field.id ) | |
| .filter( ( id ) => id !== 'vendor' ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin/dashboard/pages/withdraw/index.tsx` around lines 527 - 529, The
current mapping of fields produces an empty string for the vendor field (fields:
fields.map(... field.id !== 'vendor' ? field.id : '')) which leaves a bogus
entry; replace this with a filter-first (or filter-after) approach so only real
ids are included—for example use fields.filter(f => f.id !== 'vendor').map(f =>
f.id) (or map then .filter(Boolean)) so the fields array passed to DataViews
contains no empty strings; update the code that builds fields (the fields
variable/mapping) accordingly.
| const tabsAdditionalContents = [ | ||
| <button | ||
| type="button" | ||
| className="inline-flex items-center gap-2 rounded-md border border-gray-300 bg-white px-3 py-1.5 text-sm text-[#575757] hover:bg-[#7047EB] hover:text-white" | ||
| disabled={ isExporting } | ||
| className="inline-flex items-center gap-2 rounded-md border border-gray-300 bg-white px-3 py-1.5 text-sm text-[#575757] hover:bg-[#7047EB] hover:text-white disabled:opacity-50 disabled:cursor-not-allowed" | ||
| onClick={ async () => { | ||
| setIsExporting(true); | ||
|
|
||
| try { | ||
| // Minimal placeholder; backend export flow may vary. | ||
| // Attempt to hit export endpoint via same query params. | ||
| const path = addQueryArgs( 'dokan/v2/withdraw', { | ||
| const shouldEmail = data && data.length < totalItems; | ||
|
|
||
| // If all data is already loaded, download directly | ||
| if ( ! shouldEmail && data && data.length > 0 ) { | ||
| const csvData = data.map( ( item ) => ( { | ||
| ...item, | ||
| user_id: item.user?.id, | ||
| vendor_name: item.user?.store_name, | ||
| payment_method: item.method_title, | ||
| payable: item.receivable, | ||
| date: item.created, | ||
| } ) ); | ||
| directDownloadCSV( 'withdraws', WITHDRAW_CSV_HEADERS, csvData, filterArgs ); | ||
| return; |
There was a problem hiding this comment.
Direct-download path doesn't account for the !shouldEmail && data.length === 0 edge case.
When data is loaded but empty (data.length === 0 and totalItems === 0), shouldEmail is false and the direct-download guard (data.length > 0) is also false, so execution falls through to the API POST with email: false. This silently kicks off a backend export for zero rows. Consider short-circuiting early:
Proposed fix
try {
+ if ( ! data || data.length === 0 && totalItems === 0 ) {
+ toast( {
+ type: 'info',
+ title: __( 'No data to export.', 'dokan-lite' ),
+ } );
+ return;
+ }
+
const shouldEmail = data && data.length < totalItems;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const tabsAdditionalContents = [ | |
| <button | |
| type="button" | |
| className="inline-flex items-center gap-2 rounded-md border border-gray-300 bg-white px-3 py-1.5 text-sm text-[#575757] hover:bg-[#7047EB] hover:text-white" | |
| disabled={ isExporting } | |
| className="inline-flex items-center gap-2 rounded-md border border-gray-300 bg-white px-3 py-1.5 text-sm text-[#575757] hover:bg-[#7047EB] hover:text-white disabled:opacity-50 disabled:cursor-not-allowed" | |
| onClick={ async () => { | |
| setIsExporting(true); | |
| try { | |
| // Minimal placeholder; backend export flow may vary. | |
| // Attempt to hit export endpoint via same query params. | |
| const path = addQueryArgs( 'dokan/v2/withdraw', { | |
| const shouldEmail = data && data.length < totalItems; | |
| // If all data is already loaded, download directly | |
| if ( ! shouldEmail && data && data.length > 0 ) { | |
| const csvData = data.map( ( item ) => ( { | |
| ...item, | |
| user_id: item.user?.id, | |
| vendor_name: item.user?.store_name, | |
| payment_method: item.method_title, | |
| payable: item.receivable, | |
| date: item.created, | |
| } ) ); | |
| directDownloadCSV( 'withdraws', WITHDRAW_CSV_HEADERS, csvData, filterArgs ); | |
| return; | |
| const tabsAdditionalContents = [ | |
| <button | |
| type="button" | |
| disabled={ isExporting } | |
| className="inline-flex items-center gap-2 rounded-md border border-gray-300 bg-white px-3 py-1.5 text-sm text-[`#575757`] hover:bg-[`#7047EB`] hover:text-white disabled:opacity-50 disabled:cursor-not-allowed" | |
| onClick={ async () => { | |
| setIsExporting(true); | |
| try { | |
| if ( ! data || data.length === 0 && totalItems === 0 ) { | |
| toast( { | |
| type: 'info', | |
| title: __( 'No data to export.', 'dokan-lite' ), | |
| } ); | |
| return; | |
| } | |
| const shouldEmail = data && data.length < totalItems; | |
| // If all data is already loaded, download directly | |
| if ( ! shouldEmail && data && data.length > 0 ) { | |
| const csvData = data.map( ( item ) => ( { | |
| ...item, | |
| user_id: item.user?.id, | |
| vendor_name: item.user?.store_name, | |
| payment_method: item.method_title, | |
| payable: item.receivable, | |
| date: item.created, | |
| } ) ); | |
| directDownloadCSV( 'withdraws', WITHDRAW_CSV_HEADERS, csvData, filterArgs ); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin/dashboard/pages/withdraw/index.tsx` around lines 556 - 578, The
onClick handler in tabsAdditionalContents can fall through and call the backend
export with email: false when there are zero rows; update the handler to
short-circuit early when totalItems === 0 (or when data && data.length === 0 and
totalItems === 0) to avoid kicking off a backend export for zero rows: if no
items, reset isExporting (call setIsExporting(false)), optionally notify the
user, and return before building csvData or calling the export API; keep the
existing directDownloadCSV path (data.length > 0) and the backend POST path for
real exports unchanged.
| const csvData = data.map( ( item ) => ( { | ||
| ...item, | ||
| user_id: item.user?.id, | ||
| vendor_name: item.user?.store_name, | ||
| payment_method: item.method_title, | ||
| payable: item.receivable, | ||
| date: item.created, | ||
| } ) ); | ||
| directDownloadCSV( 'withdraws', WITHDRAW_CSV_HEADERS, csvData, filterArgs ); |
There was a problem hiding this comment.
details column will export as [object Object] in the CSV.
item.details is a nested object (see processDetails() usage throughout this file). When spread into csvData, the details key retains the raw object, so objectToCSVRows will serialize it as "[object Object]" in the CSV output. You need to flatten it the same way the UI does.
Proposed fix
const csvData = data.map( ( item ) => ( {
...item,
user_id: item.user?.id,
vendor_name: item.user?.store_name,
payment_method: item.method_title,
payable: item.receivable,
date: item.created,
+ details: processDetails( item.details, item.method ),
} ) );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const csvData = data.map( ( item ) => ( { | |
| ...item, | |
| user_id: item.user?.id, | |
| vendor_name: item.user?.store_name, | |
| payment_method: item.method_title, | |
| payable: item.receivable, | |
| date: item.created, | |
| } ) ); | |
| directDownloadCSV( 'withdraws', WITHDRAW_CSV_HEADERS, csvData, filterArgs ); | |
| const csvData = data.map( ( item ) => ( { | |
| ...item, | |
| user_id: item.user?.id, | |
| vendor_name: item.user?.store_name, | |
| payment_method: item.method_title, | |
| payable: item.receivable, | |
| date: item.created, | |
| details: processDetails( item.details, item.method ), | |
| } ) ); | |
| directDownloadCSV( 'withdraws', WITHDRAW_CSV_HEADERS, csvData, filterArgs ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin/dashboard/pages/withdraw/index.tsx` around lines 569 - 577, The CSV
export is including the raw nested details object so the details column becomes
"[object Object]"; update the csvData mapping used before calling
directDownloadCSV so that instead of spreading item.details as-is you
normalize/flatten it the same way the UI does (use the existing
processDetails(item.details) helper or its returned flattened fields) and assign
those flattened/serializable values (or a JSON/string representation) to the
details-related CSV fields; keep WITHDRAW_CSV_HEADERS in sync with those
flattened keys so objectToCSVRows produces readable CSV rows.
3 participants
Fix withdraw export ignoring filters, and disable the export button while processing using the new exporter.
All Submissions:
Changes proposed in this Pull Request:
WithdrawExportControllerto convert the status string (e.g.,'approved') received from the API into its corresponding integer status code (e.g.,1) before querying the database. Previously, passing the string directly caused the query to fail or default to the wrong status.WithdrawExportControllerto map standard REST API date parameters (after,before) to the internal query arguments (start_date,end_date) required bydokan()->withdraw->all().index.tsx) to passafterandbeforeparameters for date filtering instead ofstart_dateandend_date, adhering to the API expectations.Note on Filename: The issue description mentioned that the exported filename should reflect the applied filters (e.g.,
statement_vendorA_...). This change was not included in this PR. The export functionality relies onAutomattic\WooCommerce\Admin\API\Reports\GenericControllerfor generating the file, and modifying the filename generation logic would require overriding significant portions of the core WooCommerce Admin export handling, which was deemed too intrusive and risky for this fix.Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
fix filtered export and swap to new controller
Before Changes
The export function ignored the active status and date filters, downloading an internal default set or all records regardless of what was shown in the table.
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
New Features
Improvements