enhance: change the withdraw exporter to woocommerce one and add the proper filtering #3054
Conversation
imehedi357
added
Needs: Testing
📝 WalkthroughWalkthroughThe changes fix the export-ignores-filters bug by mapping filter parameters to their backend equivalents (status codes, date ranges) and implementing a client-side export flow with status polling and loading indicators during the export process. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Withdraw Page UI
participant Controller as WithdrawExportController
participant ExportSvc as Export Service
User->>UI: Click Export with filters applied
activate UI
UI->>UI: Set isExporting = true, show Loader2
rect rgb(200, 220, 255)
Note over UI,Controller: Export Initiation with Filtered Params
UI->>Controller: POST /reports/withdraws/export<br/>(report_args: status, start_date, end_date, ...)
end
activate Controller
Controller->>Controller: Map status to code<br/>(via get_status_code)
Controller->>Controller: Map after → start_date<br/>Map before → end_date
Controller->>ExportSvc: Trigger export with mapped params
deactivate Controller
Controller-->>UI: Return export_id
deactivate UI
rect rgb(220, 255, 220)
Note over UI,ExportSvc: Status Polling Loop
loop Until export ready
UI->>ExportSvc: pollExportStatus(export_id)
ExportSvc-->>UI: Status (pending/ready/error)
end
end
alt Export Ready
UI->>UI: Set isExporting = false
UI->>User: Export file available
else Export Error
UI->>UI: Set isExporting = false<br/>Show alert
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
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. |
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
✏️ Tip: You can customize this high-level summary in your review settings.