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

Use the filter param in server-side disputes CSV export #9988

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
+14 −15
Open

Use the filter param in server-side disputes CSV export #9988

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
09d7c8d
Fix awaiting response filtering in disputes CSV export
Dec 18, 2024
a89f434
Add changelog
Dec 18, 2024
dc09a57
Move 'awaiting_response' filtering to formatQueryFilters for
Dec 18, 2024
0a6185a
removed unnecessary import
Dec 18, 2024
0239dcf
Removed code from getDisputesCSV
Dec 18, 2024
cc5bf3b
update the changelog
jessy-p Dec 18, 2024
a13aa52
Merge branch 'develop' into fix/9987-filter-csv-disputes
Dec 18, 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/fix-9987-filter-csv-disputes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fix

Fix filtering on server-side Disputes filtering
12 changes: 10 additions & 2 deletions client/data/disputes/resolvers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
updateDisputesSummary,
updateErrorForDispute,
} from './actions';
import { disputeAwaitingResponseStatuses } from 'wcpay/disputes/filters/config';

const formatQueryFilters = ( query ) => ( {
user_email: query.userEmail,
Expand All @@ -38,11 +39,18 @@ const formatQueryFilters = ( query ) => ( {
} );

export function getDisputesCSV( query ) {
const queryWithSearch = {
...query,
search:
query.filter === 'awaiting_response'
? disputeAwaitingResponseStatuses
: query.search,
};

Copy link
Member

@Jinksi Jinksi Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

💭 it might be better for consistency to apply this in a single place for all /disputes/,/disputes/download,/disputes/summary API requests – I think formatQueryFilters in this file would be suitable since it is also where we are modifying the query in other ways.

const formatQueryFilters = ( query ) => ( {
	user_email: query.userEmail,
	match: query.match,
	store_currency_is: query.storeCurrencyIs,
	date_before: formatDateValue( query.dateBefore, true ),
	date_after: formatDateValue( query.dateAfter ),
	date_between: query.dateBetween && [
		formatDateValue( query.dateBetween[ 0 ] ),
		formatDateValue( query.dateBetween[ 1 ], true ),
	],
-	search: query.search,
+ // If `awaiting_response` filter is present, use the `search` query to select only those disputes awaiting a response.
+ search:
+		query.filter === 'awaiting_response'
+			? disputeAwaitingResponseStatuses
+			: query.search,
	status_is: query.statusIs,
	status_is_not: query.statusIsNot,
	locale: query.locale,
} );

This would then mean we can apply the same filtering to all requests, reducing the chance of inconsistency bugs like the one we're solving in this PR.

We implemented (or really, I did, if looking at git blame) the following awaiting_response filtering in the useDisputes hook, which can be removed if we apply this logic across to all "dispute list" requests. We'd have to do some extra testing to make sure we don't break those endpoints, but it would be worth it IMO.

woocommerce-payments/client/data/disputes/hooks.ts

Lines 101 to 104 in a89f434

const search =
filter === 'awaiting_response'
? disputeAwaitingResponseStatuses
: undefined;

Copy link
Member

Choose a reason for hiding this comment

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

Not to say your solution doesn't work, by the way! But it will mean we are applying this logic in different places for different endpoints (/disputes/, /disputes/download, /disputes/summary).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @Jinksi, I've consolidated the code into formatQueryFilters

const path = addQueryArgs(
`${ NAMESPACE }/disputes/download`,
formatQueryFilters( query )
formatQueryFilters( queryWithSearch )
);

return path;
}

Expand Down
2 changes: 2 additions & 0 deletions client/disputes/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,7 @@ export const DisputesList = (): JSX.Element => {
date_after: dateAfter,
date_between: dateBetween,
match,
filter,
status_is: statusIs,
status_is_not: statusIsNot,
} = getQuery();
Expand Down Expand Up @@ -407,6 +408,7 @@ export const DisputesList = (): JSX.Element => {
dateBefore,
dateBetween,
match,
filter,
statusIs,
statusIsNot,
} ),
Expand Down
Loading