Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove browser based CSV export, default to async server-side export #9986

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
+45 −406
Open

Remove browser based CSV export, default to async server-side export #9986

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
30ad58b
Refactor: Simplify CSV export to always use endpoint download
Dec 17, 2024
9f7787b
added changelog
Dec 18, 2024
829aa3e
Merge branch 'develop' into update/9764-remove-browser-export
jessy-p Dec 18, 2024
6f75ab6
Fixed the tests, removed tests for sync browser based CSV exports.
Dec 18, 2024
0c8a7a4
Merge branch 'update/9764-remove-browser-export' of https://github.co…
Dec 18, 2024
5e9916f
Fixed the tests to test server side download
Dec 18, 2024
fc798ce
Merge branch 'develop' into update/9764-remove-browser-export
Dec 18, 2024
3b0a867
Remove unused vars
Dec 18, 2024
1f054fd
Remove more unused var and imports
Dec 18, 2024
248f47e
Remove unused vars
Dec 18, 2024
f974606
Merge branch 'develop' into update/9764-remove-browser-export
Dec 18, 2024
0ba8731
Merge branch 'develop' into update/9764-remove-browser-export
haszari Dec 18, 2024
87ad001
Merge branch 'develop' into update/9764-remove-browser-export
Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/update-9764-remove-browser-export
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: update

Remove browser based CSV export, use async CSV export for Disputes, DPayouts and Transactions.
Copy link
Contributor

@haszari haszari Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

50 changes: 3 additions & 47 deletions client/deposits/list/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@ import { __, _n, sprintf } from '@wordpress/i18n';
import moment from 'moment';
import { TableCard, Link } from '@woocommerce/components';
import { onQueryChange, getQuery } from '@woocommerce/navigation';
import {
downloadCSVFile,
generateCSVDataFromTable,
generateCSVFileName,
} from '@woocommerce/csv-export';
import apiFetch from '@wordpress/api-fetch';
import { useDispatch } from '@wordpress/data';
import { parseInt } from 'lodash';
Expand Down Expand Up @@ -221,8 +216,6 @@ export const DepositsList = (): JSX.Element => {
depositsSummary.store_currencies ||
( isCurrencyFiltered ? [ getQuery().store_currency_is ] : [] );

const title = __( 'Payouts', 'woocommerce-payments' );

const downloadable = !! rows.length;

const endpointExport = async ( language: string ) => {
Expand Down Expand Up @@ -312,48 +305,11 @@ export const DepositsList = (): JSX.Element => {

const onDownload = async () => {
setIsDownloading( true );
const downloadType = totalRows > rows.length ? 'endpoint' : 'browser';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this – the name endpoint was a little weird/arcane IMO!


if ( 'endpoint' === downloadType ) {
if ( ! isDefaultSiteLanguage() && ! isExportModalDismissed() ) {
setCSVExportModalOpen( true );
} else {
endpointExport( '' );
}
if ( ! isDefaultSiteLanguage() && ! isExportModalDismissed() ) {
setCSVExportModalOpen( true );
} else {
const params = getQuery();

const csvColumns = [
{
...columns[ 0 ],
label: __( 'Payout Id', 'woocommerce-payments' ),
},
...columns.slice( 1 ),
];

const csvRows = rows.map( ( row ) => [
row[ 0 ],
{
...row[ 1 ],
value: dateI18n(
'Y-m-d',
moment.utc( row[ 1 ].value ).toISOString(),
true
),
},
...row.slice( 2 ),
] );

downloadCSVFile(
generateCSVFileName( title, params ),
generateCSVDataFromTable( csvColumns, csvRows )
);

recordEvent( 'wcpay_deposits_download', {
exported_deposits: rows.length,
total_deposits: depositsSummary.count,
download_type: 'browser',
} );
Comment on lines -346 to -350
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll comment on the specifics of all the tracks events in my review comment.

endpointExport( '' );
}

setIsDownloading( false );
Expand Down
74 changes: 9 additions & 65 deletions client/deposits/list/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,8 @@
*/
import { render, waitFor } from '@testing-library/react';
import { updateQueryString } from '@woocommerce/navigation';
import { downloadCSVFile } from '@woocommerce/csv-export';
import apiFetch from '@wordpress/api-fetch';

import os from 'os';

/**
* Internal dependencies
*/
Expand All @@ -19,7 +16,6 @@ import {
useDepositsSummary,
useReportingExportLanguage,
} from 'wcpay/data';
import { formatDate, getUnformattedAmount } from 'wcpay/utils/test-utils';
import {
CachedDeposit,
CachedDeposits,
Expand Down Expand Up @@ -117,10 +113,6 @@ const mockUseDepositsSummary = useDepositsSummary as jest.MockedFunction<
typeof useDepositsSummary
>;

const mockDownloadCSVFile = downloadCSVFile as jest.MockedFunction<
typeof downloadCSVFile
>;

const mockUseReportingExportLanguage = useReportingExportLanguage as jest.MockedFunction<
typeof useReportingExportLanguage
>;
Expand Down Expand Up @@ -279,66 +271,18 @@ describe( 'Deposits list', () => {
} );
} );

test( 'should render expected columns in CSV when the download button is clicked', () => {
test( 'should fetch export when the download button is clicked', async () => {
const { getByRole } = render( <DepositsList /> );
getByRole( 'button', { name: 'Download' } ).click();

const expected = [
'"Payout Id"',
'Date',
'Type',
'Amount',
'Status',
'"Bank account"',
'"Bank reference ID"',
];

const csvContent = mockDownloadCSVFile.mock.calls[ 0 ][ 1 ];
const csvHeaderRow = csvContent.split( os.EOL )[ 0 ].split( ',' );
expect( csvHeaderRow ).toEqual( expected );
} );

test( 'should match the visible rows', () => {
const { getByRole, getAllByRole } = render( <DepositsList /> );
getByRole( 'button', { name: 'Download' } ).click();

const csvContent = mockDownloadCSVFile.mock.calls[ 0 ][ 1 ];
const csvRows = csvContent.split( os.EOL );
const displayRows = getAllByRole( 'row' );

expect( csvRows.length ).toEqual( displayRows.length );

const csvFirstDeposit = csvRows[ 1 ].split( ',' );
const displayFirstDeposit = Array.from(
displayRows[ 1 ].querySelectorAll( 'td' )
).map( ( td ) => td.textContent );

// Note:
//
// 1. CSV and display indexes are off by 1 because the first field in CSV is deposit id,
// which is missing in display.
//
// 2. The indexOf check in amount's expect is because the amount in CSV may not contain
// trailing zeros as in the display amount.
//
expect( formatDate( csvFirstDeposit[ 1 ], 'M j, Y' ) ).toBe(
displayFirstDeposit[ 0 ]
); // date
expect( csvFirstDeposit[ 2 ] ).toBe( displayFirstDeposit[ 1 ] ); // type
expect(
getUnformattedAmount( displayFirstDeposit[ 2 ] ).indexOf(
csvFirstDeposit[ 3 ]
)
).not.toBe( -1 ); // amount
expect( csvFirstDeposit[ 4 ] ).toBe(
`"${ displayFirstDeposit[ 3 ] }"`
); // status
expect( csvFirstDeposit[ 5 ] ).toBe(
`"${ displayFirstDeposit[ 4 ] }"`
); // bank account
expect( csvFirstDeposit[ 6 ] ).toBe(
`${ displayFirstDeposit[ 5 ] }`
); // bank reference key
await waitFor( () => {
expect( mockApiFetch ).toHaveBeenCalledTimes( 1 );
expect( mockApiFetch ).toHaveBeenCalledWith( {
method: 'POST',
path:
'/wc/v3/payments/deposits/download?user_email=mock%40example.com&locale=en',
} );
} );
} );

test( 'should fetch export after confirmation when download button is selected for unfiltered exports larger than 1000.', async () => {
Expand Down
74 changes: 4 additions & 70 deletions client/disputes/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@ import moment from 'moment';
import { Button } from '@wordpress/components';
import { TableCard, Link } from '@woocommerce/components';
import { onQueryChange, getQuery, getHistory } from '@woocommerce/navigation';
import {
downloadCSVFile,
generateCSVDataFromTable,
generateCSVFileName,
} from '@woocommerce/csv-export';
import classNames from 'classnames';
import apiFetch from '@wordpress/api-fetch';
import { useDispatch } from '@wordpress/data';
Expand Down Expand Up @@ -43,7 +38,6 @@ import {
} from 'multi-currency/interface/functions';
import DisputesFilters from './filters';
import DownloadButton from 'components/download-button';
import disputeStatusMapping from 'components/dispute-status-chip/mappings';
import { CachedDispute, DisputesTableHeader } from 'wcpay/types/disputes';
import { getDisputesCSV } from 'wcpay/data/disputes/resolvers';
import {
Expand Down Expand Up @@ -444,71 +438,11 @@ export const DisputesList = (): JSX.Element => {

const onDownload = async () => {
setIsDownloading( true );
const title = __( 'Disputes', 'woocommerce-payments' );
const downloadType = totalRows > rows.length ? 'endpoint' : 'browser';

if ( 'endpoint' === downloadType ) {
if ( ! isDefaultSiteLanguage() && ! isExportModalDismissed() ) {
setCSVExportModalOpen( true );
} else {
endpointExport( '' );
}

if ( ! isDefaultSiteLanguage() && ! isExportModalDismissed() ) {
setCSVExportModalOpen( true );
} else {
const csvColumns = [
{
...headers[ 0 ],
label: __( 'Dispute Id', 'woocommerce-payments' ),
},
...headers.slice( 1, -1 ), // Remove details (position 0) and action (last position) column headers.
];

const csvRows = rows.map( ( row ) => {
return [
...row.slice( 0, 3 ), // Amount, Currency, Status.
{
// Reason.
...row[ 3 ],
value:
disputeStatusMapping[ row[ 3 ].value ?? '' ]
.message,
},
{
// Source.
...row[ 4 ],
value: formatStringValue(
( row[ 4 ].value ?? '' ).toString()
),
},
...row.slice( 5, 10 ), // Order #, Customer, Email, Country.
{
// Disputed On.
...row[ 10 ],
value: dateI18n(
'Y-m-d',
moment( row[ 10 ].value ).toISOString()
),
},
{
// Respond by.
...row[ 11 ],
value: dateI18n(
'Y-m-d / g:iA',
moment( row[ 11 ].value ).toISOString()
),
},
];
} );

downloadCSVFile(
generateCSVFileName( title, getQuery() ),
generateCSVDataFromTable( csvColumns, csvRows )
);

recordEvent( 'wcpay_disputes_download', {
exported_disputes: csvRows.length,
total_disputes: disputesSummary.count,
download_type: 'browser',
} );
Comment on lines -511 to -515
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

endpointExport( '' );
}

setIsDownloading( false );
Expand Down
Loading
Loading