Remove browser based CSV export, default to async server-side export #9986
Conversation
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: -557 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
…m/Automattic/woocommerce-payments into update/9764-remove-browser-export
- Also fixed some issues with the test file.
|
The linter was showing some unused vars and imports. I have removed them via - 3b0a867 Manual testsI tested by reducing the list to less than 25 to make sure it does not go via browser dowload.
|
There was a problem hiding this comment.
Did a round of review.
Just a question about whether to reuse existing track events or create new ones. If we decide to use existing track events then we will need to add back the recordEvents
Sorry about the repetitive text. I wish I could select all three occurences and ask the question at one place :D
| recordEvent( 'wcpay_deposits_download', { | ||
| exported_deposits: rows.length, | ||
| total_deposits: depositsSummary.count, | ||
| download_type: 'browser', | ||
| } ); |
There was a problem hiding this comment.
Should we deprecate this track event and create a new one, or should we continue using this with download_type set as endpoint ? The second option will allow continuity with the older data. Good to have some more opinions.
There was a problem hiding this comment.
I think we should keep the existing tracks events, but update the tracks event information noting that the browser download type is deprecated, no longer in use.
There was a problem hiding this comment.
I agree, the user intention is not changing, so we should ideally keep the existing events.
We should have events for the key user actions, and ideally all events will be triggered in front-end code, close to the action happening.
There was a problem hiding this comment.
I'll comment on the specifics of all the tracks events in my review comment.
| recordEvent( 'wcpay_disputes_download', { | ||
| exported_disputes: csvRows.length, | ||
| total_disputes: disputesSummary.count, | ||
| download_type: 'browser', | ||
| } ); |
There was a problem hiding this comment.
Should we deprecate this track event and create a new one, or should we continue using this with download_type set as endpoint ? The second option will allow continuity with the older data. Good to have some more opinions.
| 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.
Should we deprecate this track event and create a new one, or should we continue using this with download_type set as endpoint ? The second option will allow continuity with the older data. Good to have some more opinions.
|
I've marked this PR as a fix for #9303, which will be resolve when we remove JS-based CSV exporting. |
| Significance: minor | ||
| Type: update | ||
|
|
||
| Remove browser based CSV export, use async CSV export for Disputes, DPayouts and Transactions. |
There was a problem hiding this comment.
Might be clearer to word this positively, the negative wording assumes/requires context to understand.
Something like this:
Generate all CSV exports asynchronously via service (includes transactions, disputes, and payouts tables). Previously smaller CSVs were generated immediately via JavaScript code.
There was a problem hiding this comment.
Super excited to see this move ahead, and all the code we get to delete 👩🏻🍳 💋 !
Tracks
The tracks events are a little interesting, but TLDR I think we should keep all the same events and ensure they fire as before to accurately represent the user action.
If the existing events don't accurately capture the user action … then we could consider tweaking them a little while we're here.
If needed, IMO renaming or changing tracks is ok as long as we communicate that it's happening, and we don't upset any current experiments. Good to clarify and refine when we can!
wcpay_deposits_downloadwcpay_disputes_download- The naming of the events is not ideal. User action is clicking a button to initiate/request a CSV. So a new event name might be better fit.
depositsis goneburgers 💣 🍔 ⇒payouts
wcpay_transactions_download_csv_click- This name is more accurate IMO! Also super good to mention
csvin the name – clear.
- This name is more accurate IMO! Also super good to mention
downloadTypecould be deprecated, it's not needed anymore. If we keep it, could consider clearer name –endpointis technical.exported_transactionsexported_disputesexported_deposits- If we use a common property then we can analyse across CSV type. Suggest
row_count, or maybeexported_row_count(depending ontotal_below). total_depositstotal_disputestotal_transactions- When is this different from row count? Do we need it?
- If we need/keep this, would also be good to use consistent name, e.g.
total_row_count.
- If we use a common property then we can analyse across CSV type. Suggest
Summary of my suggestions for tracks events:
- Keep/add CSV export click events. Rename to focus on user action:
wcpay_transactions_export_csv_clickwcpay_payouts_export_csv_clickwcpay_disputes_export_csv_click
- Use consistent names for common properties.
exported_row_counttotal_row_countif needed
- Consider removing
downloadTypeprop – no longer relevant.
Tracks events could be a separate issue. If we do separate issue, ideally we ship in same release to avoid a gap in the data (though maybe not a big deal!).
Flow / testing
- The button label is
Download… but the function is export, and the download is async. We should consider changing button label toExport
- The email and the export filename give no clue what the data is. I always like it when web services name the files for me so my downloads folder isn't a confusing jungle. Potential quick wins:
- Add the download type & maybe row count or date of request to the email (and subject?). So each email notification is a reminder of what & when, is identifiable.
- Include the download type in the filename.
- We are now WooPayments …
- Example:
WooPayments-transactions-acct_1234abcd-2024_12_19_01_04_59-nr7mpt2jy2.csv
Keen for @rogermattic thoughts on the above. (and FYI @mjdeacon ). We could log as follow up issues to make CSV experience slightly nicer.
I tested on my pressable site, using Jetpack beta tester to run this branch. I exported a big list (transactions) and a small list (all disputes) and both worked fine, usual async email flow.
🚢
| @@ -312,48 +305,11 @@ export const DepositsList = (): JSX.Element => { | |||
|
|
|||
| const onDownload = async () => { | |||
| setIsDownloading( true ); | |||
| const downloadType = totalRows > rows.length ? 'endpoint' : 'browser'; | |||
There was a problem hiding this comment.
I like this – the name endpoint was a little weird/arcane IMO!
| recordEvent( 'wcpay_deposits_download', { | ||
| exported_deposits: rows.length, | ||
| total_deposits: depositsSummary.count, | ||
| download_type: 'browser', | ||
| } ); |
There was a problem hiding this comment.
I agree, the user intention is not changing, so we should ideally keep the existing events.
We should have events for the key user actions, and ideally all events will be triggered in front-end code, close to the action happening.
| recordEvent( 'wcpay_deposits_download', { | ||
| exported_deposits: rows.length, | ||
| total_deposits: depositsSummary.count, | ||
| download_type: 'browser', | ||
| } ); |
There was a problem hiding this comment.
I'll comment on the specifics of all the tracks events in my review comment.
Fixes #9969
Fixes #9303 by removing JS-based CSV export
Changes proposed in this Pull Request
When the number of rows are less than 25, browser based CSV export was used. This is removed.
Export defaults to async server-side export for transactions, disputes and payouts.
Testing instructions
update/9764-remove-browser-export, it will use the server side async export. The export will be generated async and sent by email.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