Transactions: Use server based export for both sync download on browser and async email export #10049
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
server export instead of using CSV export.
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +95 B (0%) Total Size: 1.36 MB
ℹ️ View Unchanged
|
jessy-p
changed the title
|
I did a manual test on the transactions page with less than 25 transactions filtered.
|
6 tasks
6 tasks
Jinksi
reviewed
Jan 3, 2025
There was a problem hiding this comment.
🌟 Very nice @jessy-p! I prefer this approach to removing client-side download completely.
We can implement server-side logic to significantly increase the client-side download row-count limit higher than 25-100 (10,000? 1,000,000?). We can test server performance to find where this limit should be → when async export is required.
I think this would offer a better UX for most flows, with the existing email-based flow remaining in place for very large and slow exports.
Definitely! But I think we can focus on matching it with pagesize (25-100) currently, so that it adheres to the existing and Core UX principles, which seems to be:
|
|
The PR here achieves good parity with the existing UX flow and in my opinion, can be merged as it is. We can bump up the limit to 100 safely. We can create a separate issue / spike to explore what is the safe highest number for the sync, and incrementally deliver it later. WDYT? |
- Parameter is set from component - Pass parameter to endpoint.
jessy-p
changed the title
For fixing issue with download on Safari Co-authored-by: Eric Jinks <[email protected]>
Jinksi
reviewed
Jan 10, 2025
There was a problem hiding this comment.
As an interim review, while we make sure https://github.com/Automattic/transact-platform-server/pull/7077 will work (see that PR discussion), this is working well when running against local server on trunk or sandbox on trunk.
I've left some minor code comments and updated some falling JS tests in 899f809.
Comment on lines
+92
to
+95
| const path = addQueryArgs( `${ NAMESPACE }/transactions/download`, { | ||
| ...formatQueryFilters( query ), | ||
| download_type: query.downloadType, | ||
| } ); |
There was a problem hiding this comment.
For consistency, download_type: query.downloadType can be placed within formatQueryFilters which is conducting the same type of operation on other filter params.
Comment on lines
738
to
742
| recordEvent( 'wcpay_transactions_download_csv_click', { | ||
| location: props.depositId ? 'deposit_details' : 'transactions', | ||
| download_type: downloadType, | ||
| exported_transactions: rows.length, | ||
| total_transactions: transactionsSummary.count, | ||
| } ); |
There was a problem hiding this comment.
💭 Rather than remove, should we expand the event prop download_type with sync and async for tracking purposes?
| @@ -432,12 +432,13 @@ public function get_fraud_outcome_transactions_export( $request ) { | |||
| * @param string $user_email The email to search for. | |||
| * @param string $deposit_id The deposit to filter on. | |||
| * @param string $locale Site locale. | |||
| * @param string $download_type The type of download to perform. | |||
| * | |||
There was a problem hiding this comment.
💭 It may be worth listing the possible values to clarify what this param is for.
Suggested change
| * | |
| * @param string $download_type The type of download to perform: 'sync' or 'async'. |
added 2 commits
January 14, 2025 20:09
# Conflicts: # client/transactions/list/index.tsx
# Conflicts: # client/transactions/list/test/index.tsx
|
As part of testing this PR, let's check to see if the columns currently exported via client are also available via server export. As reported in #8183, there are discrepancies between columns exported via server/client-based CSV export, (the Update – this is being handled in a separate PR https://github.com/Automattic/transact-platform-server/pull/7142 ✅ |
|
Closing this PR as we are going with #10211 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative approach for #9969
Changes proposed in this Pull Request
In this PR, the UX for browser download remains the same, but the backend implementation is changed so that the server export is used for generating the CSV which is made available immediately. This approach does NOT change the UX for merchant (i.e. download is available immediately for single page export), but it solves the overhead and issues of maintaining multiple implementations that do not match.
Single page downloads are available immediately, multiple page downloads (i.e. number of rows to be exported > Page size ) the download is emailed.
The corresponding server changes are on https://github.com/Automattic/transact-platform-server/pull/7077.
NOTE:
Testing instructions
Test 1:
Test 2:
Test 3:
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge