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.

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

add a new page component for redirecting disputes => details: #7310

Merged
merged 32 commits into from
Oct 3, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
86e8a85
add a new page component for redirecting disputes => details:
Sep 27, 2023
1409a10
implement the redirect using history push +
Sep 28, 2023
55c3f5a
Merge branch 'develop' into fix/6891-redirect-dispute-detail-to-trans…
haszari Sep 28, 2023
8d3a244
Merge branch 'develop' into fix/6891-redirect-dispute-detail-to-trans…
haszari Sep 28, 2023
466877d
Merge branch 'develop' into fix/6891-redirect-dispute-detail-to-trans…
haszari Sep 28, 2023
83054c8
TypeScript tweak - allow null return from component:
Sep 28, 2023
bf4eee8
Merge branch 'fix/6891-redirect-dispute-detail-to-transaction' of git…
Sep 28, 2023
83dcc37
use notice and spinner to reassure user during redirect
Sep 28, 2023
73b5a8f
ensure spinner and text are on same line
Sep 28, 2023
416c7ed
use WP Flex components to align redirect notice items
Sep 28, 2023
74267c0
ensure correct behaviour with feature flag off:
Sep 28, 2023
341e7f6
Add a hint and bail early from dispute details page component:
Sep 28, 2023
1520273
remove unused
Sep 28, 2023
d5d7a2e
clarify comment about Dispute.charge cast
Sep 28, 2023
042880f
Merge branch 'develop' into fix/6891-redirect-dispute-detail-to-trans…
haszari Oct 1, 2023
500b176
switch to centred, on-page spinner + "redirecting":
Oct 1, 2023
8986b83
Merge branch 'fix/6891-redirect-dispute-detail-to-transaction' of git…
Oct 1, 2023
9fe73b5
streamline conditional page registration:
Oct 1, 2023
e977712
remove feature flag bailout in legacy dispute details + rename:
Oct 1, 2023
1a2f772
add changelog (placeholder)
Oct 2, 2023
c97fb30
Merge branch 'develop' into fix/6891-redirect-dispute-detail-to-trans…
haszari Oct 2, 2023
2c4dbfc
Define component using `React.FC<>`
Jinksi Oct 3, 2023
ad9a2ee
Remove redundant `let` for `transactionDetailsUrl`
Jinksi Oct 3, 2023
45349c1
Handle redirect in useEffect hook to fix "setState in render" error
Jinksi Oct 3, 2023
9a3df78
Remove Dispute type cast
Jinksi Oct 3, 2023
1a76dce
Rename var to clarify meaning
Jinksi Oct 3, 2023
54ac2ce
Fix PHP linter error (use snake_case)
Jinksi Oct 3, 2023
bec0846
Add dispute fetch error wp.data getter/setter
Jinksi Oct 3, 2023
5e006f8
Fix test for getDispute resolved on error
Jinksi Oct 3, 2023
6111fac
Add error state to useDispute hook return props
Jinksi Oct 3, 2023
6b00887
Handle network error in UI
Jinksi Oct 3, 2023
6216ae0
Merge branch 'develop' into fix/6891-redirect-dispute-detail-to-trans…
haszari Oct 3, 2023
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
10 changes: 9 additions & 1 deletion client/disputes/details/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,19 @@ const DisputeDetails = ( {
query: { id: disputeId },
}: {
query: { id: string };
} ): JSX.Element => {
} ): JSX.Element | null => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm (now) in the habit of declaring React components as Element | null, consistent with classic React. As I understand, returning null is totally fine way to not render anything.

Question for front end experts – is this a good approach? Should we have a standard type for this? What does the TypeScript React community do?

Copy link
Contributor

@shendy-a8c shendy-a8c Sep 29, 2023

Choose a reason for hiding this comment

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

Not a frontend expert but since as of currently, there is a possibility this component will return null, this | null addition is necessary.

However, I have my opinion that checking for feature flag and returning null is not necessary here. Read along =)

Copy link
Member

Choose a reason for hiding this comment

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

Note that when we use React.FC/React.FunctionComponent, the return type is implicit and we don't have to define it (see TS guidelines doc).

I've switched this to React.FC in 2c4dbfc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha! Now I'm a fan of React.FC and will use from here on in. Can we encourage that pattern with a linter or tooling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the doc, we might be able to distil this to a principle:

  • Use React.FC type for React components

const { dispute, isLoading, doAccept } = useDispute( disputeId );
const disputeObject = dispute || ( {} as Dispute );
const disputeIsAvailable = ! isLoading && dispute && disputeObject.id;

// Bail early and return nothing if the feature flag is not enabled.
// Here as a hint/reminder to delete this file when the feature flag is removed.
const isDisputeOnTransactionPageEnabled =
Copy link
Contributor

@shendy-a8c shendy-a8c Sep 29, 2023

Choose a reason for hiding this comment

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

Hmm.. I think this is not necessary because this component does not need to know the business logic of when it will be rendered or not. You already check the feature flag when registering the page, ie when feature flag is on, you register RedirectToTransactionDetails instead of DisputeDetails, so that itself guarantees this page will not be rendered at any case.

I cannot find any reference to this DisputeDetails other than in client/index.js.

If the purpose of this change is as a reminder to remove this legacy dispute details component, I'd say we can just mentioned it in #7289.

What do you think?

wcpaySettings.featureFlags.isDisputeOnTransactionPageEnabled;
Jinksi marked this conversation as resolved.
Show resolved Hide resolved
if ( isDisputeOnTransactionPageEnabled ) {
return null;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this so when we're working on #7289 we find this class (search for feature flag) and remember to delete it. 🐘

Copy link
Contributor

@shendy-a8c shendy-a8c Sep 29, 2023

Choose a reason for hiding this comment

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

To be explicit, I just added one check item in #7289:

const actions = disputeIsAvailable && (
<Actions
id={ disputeObject.id }
Expand Down
58 changes: 58 additions & 0 deletions client/disputes/redirect-to-transaction-details.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/**
* External dependencies
*/
import React from 'react';
import { getHistory } from '@woocommerce/navigation';
import { Notice, Spinner, Flex, FlexItem } from '@wordpress/components';

/**
* Internal dependencies.
*/
import Page from 'components/page';
import { useDispute } from 'data/index';
import { Charge } from 'wcpay/types/charges';
import { Dispute } from 'wcpay/types/disputes';
import { getAdminUrl } from 'wcpay/utils';

const RedirectToTransactionDetails = ( {
query: { id: disputeId },
}: {
query: { id: string };
} ): JSX.Element | null => {
const { dispute, isLoading } = useDispute( disputeId );
const disputeObject = dispute || ( {} as Dispute );
const disputeIsAvailable = ! isLoading && dispute && disputeObject.id;
// Dispute type allows charge as nested object or string ID,
// so we have to hint we expect a Charge object here.
const chargeObject = disputeObject.charge as Charge;

let transactionDetailsUrl = '';
if ( disputeIsAvailable ) {
transactionDetailsUrl = getAdminUrl( {
page: 'wc-admin',
path: '/payments/transactions/details',
id: chargeObject.payment_intent,
transaction_id: chargeObject.balance_transaction,
type: 'dispute',
} );
getHistory().replace( transactionDetailsUrl );
return null;
}

return (
<Page>
<Notice status="info" isDismissible={ false }>
<Flex justify="left">
<FlexItem>
<Spinner />
</FlexItem>
<FlexItem>
<span>Redirecting to payment details…</span>
</FlexItem>
</Flex>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy days using core components to get a nice, consistent look out of the box!

</Notice>
</Page>
);
};

export default RedirectToTransactionDetails;
64 changes: 46 additions & 18 deletions client/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import TransactionsPage from 'transactions';
import PaymentDetailsPage from 'payment-details';
import DisputesPage from 'disputes';
import DisputeDetailsPage from 'disputes/details';
import RedirectToTransactionDetails from 'disputes/redirect-to-transaction-details';
import DisputeEvidencePage from 'disputes/evidence';
import AdditionalMethodsPage from 'wcpay/additional-methods-setup';
import MultiCurrencySetupPage from 'wcpay/multi-currency-setup';
Expand Down Expand Up @@ -150,24 +151,51 @@ addFilter(
},
capability: 'manage_woocommerce',
} );
pages.push( {
container: DisputeDetailsPage,
path: '/payments/disputes/details',
wpOpenMenu: menuID,
breadcrumbs: [
rootLink,
[
'/payments/disputes',
__( 'Disputes', 'woocommerce-payments' ),
],
__( 'Dispute details', 'woocommerce-payments' ),
],
navArgs: {
id: 'wc-payments-disputes-details',
parentPath: '/payments/disputes',
},
capability: 'manage_woocommerce',
} );

// If disputes on transaction page feature is enabled, set up a soft
// redirect component; otherwise register the (legacy) dispute details page.
const isDisputeOnTransactionPageEnabled =
window.wcpaySettings.featureFlags.isDisputeOnTransactionPageEnabled;
pages.push(
isDisputeOnTransactionPageEnabled
? {
container: RedirectToTransactionDetails,
path: '/payments/disputes/details',
wpOpenMenu: menuID,
breadcrumbs: [
rootLink,
[
'/payments/disputes',
__( 'Disputes', 'woocommerce-payments' ),
],
__( 'Dispute details', 'woocommerce-payments' ),
],
navArgs: {
id: 'wc-payments-disputes-details-legacy-redirect',
parentPath: '/payments/disputes',
},
capability: 'manage_woocommerce',
}
: {
container: DisputeDetailsPage,
path: '/payments/disputes/details',
wpOpenMenu: menuID,
breadcrumbs: [
rootLink,
[
'/payments/disputes',
__( 'Disputes', 'woocommerce-payments' ),
],
__( 'Dispute details', 'woocommerce-payments' ),
],
navArgs: {
id: 'wc-payments-disputes-details',
parentPath: '/payments/disputes',
},
capability: 'manage_woocommerce',
}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I was expecting only the container that needs to conditionally change between RedirectToTransactionDetails and DisputeDetailsPage based on whether isDisputeOnTransactionPageEnabled is on or not.

Why navArgs.id needs to be different?

Also, not sure what the correlation between this page registration in Javascript and in PHP.

Per the docs, react powered page needs to be registered both in Javascript and PHP.


pages.push( {
container: DisputeEvidencePage,
path: '/payments/disputes/challenge',
Expand Down
Loading