-
Notifications
You must be signed in to change notification settings - Fork 10
Pm 959 tc finance integration #1075
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
Conversation
…x-and-fees PM-1222 - show taxes & fees applied to payments
@@ -4,10 +4,11 @@ import { FC, useEffect, useState } from 'react' | |||
import { UserProfile } from '~/libs/core' | |||
import { IconOutline, LinkButton, LoadingCircles } from '~/libs/ui' | |||
|
|||
import { Balance, WalletDetails } from '../../../lib/models/WalletDetails' | |||
import { getWalletDetails } from '../../../lib/services/wallet' | |||
import { Balance } from '../../../lib/models/WalletDetails' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statement for WalletDetails
has been removed, but it is unclear if this is intentional or if WalletDetails
is still needed elsewhere in the code. Please verify if WalletDetails
is used in other parts of the file or project.
import { InfoRow } from '../../../lib' | ||
import { BannerImage, BannerText } from '../../../lib/assets/home' | ||
import { nullToZero } from '../../../lib/util' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider checking if nullToZero
is used in the file. If not, ensure that its import is necessary or remove it to keep the code clean.
@@ -17,27 +18,8 @@ interface HomeTabProps { | |||
} | |||
|
|||
const HomeTab: FC<HomeTabProps> = () => { | |||
const [walletDetails, setWalletDetails] = useState<WalletDetails | undefined>(undefined) | |||
const [isLoading, setIsLoading] = useState(false) | |||
const { data: walletDetails, isLoading, error }: WalletDetailsResponse = useWalletDetails() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider handling the error
state more explicitly. Currently, the error
variable is being destructured from useWalletDetails()
, but there is no logic to handle or display this error within the component. Implementing error handling will improve the user experience by providing feedback when the wallet details cannot be fetched.
@@ -17,27 +18,8 @@ interface HomeTabProps { | |||
} | |||
|
|||
const HomeTab: FC<HomeTabProps> = () => { | |||
const [walletDetails, setWalletDetails] = useState<WalletDetails | undefined>(undefined) | |||
const [isLoading, setIsLoading] = useState(false) | |||
const { data: walletDetails, isLoading, error }: WalletDetailsResponse = useWalletDetails() | |||
const [balanceSum, setBalanceSum] = useState(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The balanceSum
state is initialized but not used in the provided code snippet. Ensure that this state is necessary, and if it is, implement logic to update and utilize it. If not, consider removing it to clean up the code.
@@ -58,7 +40,7 @@ const HomeTab: FC<HomeTabProps> = () => { | |||
<BannerImage /> | |||
</div> | |||
{isLoading && <LoadingCircles />} | |||
{!isLoading && ( | |||
{!isLoading && walletDetails && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a null check for walletDetails
to ensure it is not null or undefined before rendering the component. This will prevent potential runtime errors if walletDetails
is not properly initialized.
@@ -75,6 +57,24 @@ const HomeTab: FC<HomeTabProps> = () => { | |||
} | |||
/> | |||
|
|||
{walletDetails.withdrawalMethod.isSetupComplete && walletDetails.taxForm.isSetupComplete && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a check for walletDetails
before accessing its properties to prevent potential runtime errors if walletDetails
is undefined.
<InfoRow | ||
title='Est. Payment Fees and Tax Withholding %' | ||
// eslint-disable-next-line max-len | ||
value={`Fee: ${nullToZero(walletDetails.estimatedFees)} USD / Tax Withholding: ${nullToZero(walletDetails.taxWithholdingPercentage)}%`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment // eslint-disable-next-line max-len
suggests that the line exceeds the maximum length limit. Consider refactoring the line to adhere to the length limit for better readability and maintainability.
icon={IconOutline.ArrowRightIcon} | ||
size='md' | ||
link | ||
to='#payout' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The to
prop value '#payout'
seems to be a hash link. Ensure that the corresponding element with the id payout
exists in the DOM to avoid broken links.
line-height: 20px; | ||
font-size: 14px; | ||
|
||
+ .taxes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more descriptive class name instead of .mt
to improve readability and maintainability of the code.
@@ -2,6 +2,7 @@ | |||
/* eslint-disable react/jsx-no-bind */ | |||
import { toast } from 'react-toastify' | |||
import { AxiosError } from 'axios' | |||
import { Link } from 'react-router-dom' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Link
import from react-router-dom
is added but not used in the code. Consider removing it if it's not needed to avoid unnecessary imports.
@@ -12,6 +13,8 @@ import { Winning, WinningDetail } from '../../../lib/models/WinningDetail' | |||
import { FilterBar } from '../../../lib' | |||
import { ConfirmFlowData } from '../../../lib/models/ConfirmFlowData' | |||
import { PaginationInfo } from '../../../lib/models/PaginationInfo' | |||
import { useWalletDetails, WalletDetailsResponse } from '../../../lib/hooks/use-wallet-details' | |||
import { nullToZero } from '../../../lib/util' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function nullToZero
is imported but not used in this file. Consider removing the import if it's not needed.
@@ -180,6 +185,58 @@ const ListView: FC<ListViewProps> = (props: ListViewProps) => { | |||
fetchWinnings() | |||
} | |||
|
|||
function handlePayMeClick( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming handlePayMeClick
to a more descriptive name like handlePaymentConfirmationClick
to better convey the function's purpose.
@@ -180,6 +185,58 @@ const ListView: FC<ListViewProps> = (props: ListViewProps) => { | |||
fetchWinnings() | |||
} | |||
|
|||
function handlePayMeClick( | |||
paymentIds: { [paymentId: string]: Winning }, | |||
totalAmount: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The totalAmount
parameter is typed as a string
. Consider using a numeric type if this value is intended to represent a monetary amount to avoid potential issues with numeric operations.
<div className={`${styles.taxes} ${styles.mt}`}> | ||
You can adjust your payout settings to customize your estimated payment fee and tax withholding percentage in | ||
{' '} | ||
<Link to='#payout'>Payout</Link> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Link
component's to
prop is set to '#payout'
. Ensure that this hash link correctly corresponds to an existing element ID on the page to avoid broken links.
title: 'Are you sure?', | ||
}) | ||
}} | ||
onPayMeClick={handlePayMeClick} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function handlePayMeClick
is used here, but its definition and implementation are not visible in the diff. Ensure that handlePayMeClick
correctly replicates the functionality of the previous inline function, including setting the confirmFlow
state and calling processPayouts
with the correct parameters.
export interface Response<T> { | ||
data?: Readonly<T> | ||
error?: Readonly<string> | ||
mutate: KeyedMutator<any> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider specifying a more precise type for KeyedMutator<any>
. Using any
can lead to potential type safety issues. If possible, replace any
with a more specific type that matches the expected data structure.
@@ -17,4 +17,7 @@ export interface WalletDetails { | |||
taxForm: { | |||
isSetupComplete: boolean | |||
} | |||
primaryCurrency?: string | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more specific type for primaryCurrency
if possible, such as an enum or a union of string literals representing valid currency codes, to ensure type safety.
@@ -17,4 +17,7 @@ export interface WalletDetails { | |||
taxForm: { | |||
isSetupComplete: boolean | |||
} | |||
primaryCurrency?: string | null; | |||
estimatedFees?: string | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The estimatedFees
field is currently typed as string | null
. If this field represents a numeric value, consider using a numeric type instead to prevent potential issues with string manipulation.
@@ -17,4 +17,7 @@ export interface WalletDetails { | |||
taxForm: { | |||
isSetupComplete: boolean | |||
} | |||
primaryCurrency?: string | null; | |||
estimatedFees?: string | null; | |||
taxWithholdingPercentage?: string | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The taxWithholdingPercentage
field is typed as string | null
. If this field is meant to represent a percentage value, consider using a numeric type to facilitate calculations and prevent errors related to string handling.
@@ -64,7 +64,7 @@ export async function processWinningsPayments(winningsIds: string[]): Promise<{ | |||
const body = JSON.stringify({ | |||
winningsIds, | |||
}) | |||
const url = `${baseUrl}/withdraw` | |||
const url = `${WALLET_API_BASE_URL}/withdraw` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider verifying that WALLET_API_BASE_URL
is defined and has the correct value before using it to construct the URL. This will help prevent potential runtime errors if the environment variable is not set or is incorrect.
@@ -107,7 +107,7 @@ export async function resendOtp(transactionId: string): Promise<TransactionRespo | |||
transactionId, | |||
}) | |||
|
|||
const url = `${baseUrl}/otp/resend` | |||
const url = `${WALLET_API_BASE_URL}/otp/resend` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from baseUrl
to WALLET_API_BASE_URL
should be verified to ensure that WALLET_API_BASE_URL
is correctly defined and imported in the module. This change could potentially affect the endpoint being called.
* @param value - The input value which can be a string, `null`, or `undefined`. | ||
* @returns The original value if it is a valid string (and not `'null'`), otherwise returns `'0'`. | ||
*/ | ||
export const nullToZero = (value: string | null | undefined): string => (value === 'null' ? '0' : value ?? '0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using strict equality checks for null
and undefined
to ensure type safety. The current implementation uses the nullish coalescing operator (??
) which is appropriate, but strict checks can be more explicit and clear.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/
What's in this PR?