[PROD] -Trolley Release #1266
[PROD] -Trolley Release #1266
Conversation
…-tabs PM-1097 - remove wallet tabs
PM-1098 payout tab
…-tabs Pm 1097 remove wallet tabs
PM-1142 reset tax forms
…t-admin-app PM-1152 - cleanup wallet admin
…for-payout-status PM-1154 - update check for payout status
…latform/platform-ui into PM-959_tc-finance-integration
…dpoint PM-1099 - updated the withdrawal endpoint
…x-and-fees PM-1222 - show taxes & fees applied to payments
minor admin-wallet fixes
fix for wallet: status filter
…able fix spacing in wallet-admin table
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| export interface PaymentDetail { | ||
| id: string | ||
| netAmount: string | ||
| grossAmount: string |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of netAmount from PaymentDetail may impact any existing code that relies on this property. Ensure that all references to netAmount are updated accordingly to prevent runtime errors.
| export interface Winning { | ||
| id: string | ||
| description: string | ||
| externalId: string |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of netPayment and netPaymentNumber from Winning may impact any existing code that relies on these properties. Ensure that all references to these properties are updated accordingly to prevent runtime errors.
| createdAt: string | ||
| releaseDate: string | ||
| datePaid: string | ||
| paymentStatus?: PayoutStatus |
There was a problem hiding this comment.
[correctness]
The addition of paymentStatus as an optional property in WinningDetail should be carefully integrated into any logic that processes WinningDetail objects. Ensure that the code handles cases where paymentStatus might be undefined to prevent potential runtime errors.
| import ApiResponse from '../models/ApiResponse' | ||
|
|
||
| const baseUrl = `${EnvironmentConfig.API.V5}/payments` | ||
| const baseUrl = `${EnvironmentConfig.TC_FINANCE_API}` |
There was a problem hiding this comment.
[❗❗ correctness]
The change from EnvironmentConfig.API.V5 to EnvironmentConfig.TC_FINANCE_API for baseUrl might affect other parts of the application relying on the previous endpoint. Ensure that this change is compatible with all existing API calls and that the new endpoint provides the same or improved functionality.
| = useState<WalletTabViews>(activeTabHash) | ||
|
|
||
| const [activeTab, setActiveTab]: [string, Dispatch<SetStateAction<string>>] = useState<string>(activeTabHash) | ||
| const tabsConfig = useMemo(() => WalletTabsConfig.filter(tab => ( |
There was a problem hiding this comment.
[performance]
The use of useMemo for tabsConfig is appropriate for performance optimization, but ensure that WalletTabsConfig is not mutated elsewhere in the component lifecycle, as this could lead to unexpected behavior.
| {activeTab === WalletTabViews.home && <HomeTab profile={props.profile} />} | ||
|
|
||
| {activeTab === WalletTabViews.taxforms && <TaxFormsTab profile={props.profile} />} | ||
| <PayoutGuard profile={props.profile}> |
There was a problem hiding this comment.
[correctness]
The use of PayoutGuard around the PayoutTab is a good approach for conditional rendering based on permissions. However, ensure that PayoutGuard correctly handles cases where canViewPayout changes dynamically, as this could affect the rendering logic.
| winnings = '1', | ||
| taxforms = '2', | ||
| withdrawalmethods = '3', | ||
| home, |
There was a problem hiding this comment.
[❗❗ correctness]
Changing the enum values from strings to numbers may affect any existing code that relies on the specific string values. Ensure that all references to these enum values are updated accordingly, especially if they are used in external systems or stored in databases.
| ] | ||
|
|
||
| export function getHashFromTabId(tabId: string): string { | ||
| export function getHashFromTabId(tabId: WalletTabViews): string { |
There was a problem hiding this comment.
[design]
The default case in getHashFromTabId returns '#home', which assumes that any unrecognized tabId should default to the home tab. Verify that this behavior is intended and that it doesn't lead to incorrect navigation or user experience.
| } | ||
|
|
||
| export function getTabIdFromHash(hash: string): string { | ||
| export function getTabIdFromHash(hash: string): WalletTabViews { |
There was a problem hiding this comment.
[design]
The default case in getTabIdFromHash returns WalletTabViews.home, which assumes that any unrecognized hash should default to the home tab. Ensure this behavior aligns with the expected user experience and doesn't lead to incorrect tab selection.
|
|
||
| fetchWalletDetails() | ||
| }, []) | ||
| const { data: walletDetails, isLoading, error }: WalletDetailsResponse = useWalletDetails() |
There was a problem hiding this comment.
[maintainability]
Consider handling the error state more robustly. Currently, it only displays the error message as a string. It might be beneficial to provide more context or a user-friendly message, especially if the error could be more complex or require user action.
| </div> | ||
| {isLoading && <LoadingCircles />} | ||
| {!isLoading && ( | ||
| {!isLoading && walletDetails && ( |
There was a problem hiding this comment.
[💡 readability]
The condition !isLoading && walletDetails is repeated multiple times. Consider extracting this condition into a variable for better readability and maintainability.
|
|
||
| .iframe { | ||
| width: 100%; | ||
| height: 100%; |
There was a problem hiding this comment.
[maintainability]
The height property is set twice with different values (100% and 90vh). The second declaration will override the first, which might not be the intended behavior. Consider removing the redundant height: 100%; to avoid confusion.
|
|
||
| const PayoutTab: FC<PayoutTabProps> = props => { | ||
| const loading = useRef<number>() | ||
| const frameRef: MutableRefObject<HTMLElement | any> = useRef() |
There was a problem hiding this comment.
[maintainability]
Using MutableRefObject<HTMLElement | any> for frameRef is too permissive. Consider specifying a more precise type for better type safety, such as HTMLIFrameElement.
| return undefined | ||
| } | ||
|
|
||
| const handleEvent: (event: any) => void = (event: any) => { |
There was a problem hiding this comment.
[maintainability]
Avoid using any for the event parameter. Use a more specific type, such as MessageEvent, to improve type safety and clarity.
| const handleEvent: (event: any) => void = (event: any) => { | ||
| const { data: widgetEvent, origin }: { data: { event: string, data: number }, origin: string } = event | ||
|
|
||
| if (origin.indexOf(EnvironmentConfig.TROLLEY_WIDGET_ORIGIN) === -1) { |
There was a problem hiding this comment.
[❗❗ security]
Using indexOf to check the origin is not secure against potential spoofing. Consider using origin === EnvironmentConfig.TROLLEY_WIDGET_ORIGIN for a stricter comparison.
| const [isProcessing, setIsProcessing] = useState(false) | ||
|
|
||
| const winningIds = useMemo(() => props.payments.map(p => p.id), [props.payments]) | ||
| const totalAmount = useMemo(() => props.payments.reduce((acc, payment) => acc + parseFloat(payment.grossPayment.replace(/[^0-9.-]+/g, '')), 0), [props.payments]) |
There was a problem hiding this comment.
[❗❗ correctness]
The use of parseFloat on payment.grossPayment.replace(/[^0-9.-]+/g, '') assumes that the grossPayment string is always in a format that can be safely converted to a number after removing non-numeric characters. Consider validating the format of grossPayment before parsing to avoid potential runtime errors or incorrect calculations.
| } | ||
|
|
||
| try { | ||
| await processWinningsPayments(winningIds, otpCode) |
There was a problem hiding this comment.
[❗❗ correctness]
The processWinningsPayments function is awaited but does not appear to handle potential errors internally. Ensure that any errors thrown by this function are properly caught and handled to prevent unhandled promise rejections.
| }) | ||
| props.onClose(true) | ||
| } catch (error) { | ||
| if ((error as any)?.code?.startsWith('otp_')) { |
There was a problem hiding this comment.
[correctness]
Casting error to any and accessing properties like code and message directly may lead to runtime errors if the expected structure is not present. Consider using TypeScript type guards or checking for property existence to ensure safe access.
|
|
||
| let message = 'Failed to process payments. Please try again later.' | ||
|
|
||
| if (error instanceof AxiosError) { |
There was a problem hiding this comment.
[correctness]
The error message construction logic assumes that error.response?.data?.error?.message or similar properties are always strings. Consider adding checks to ensure these properties are strings before manipulating them to avoid potential runtime errors.
| </li> | ||
| <li> | ||
| <span> | ||
| Tax Witholding ( |
There was a problem hiding this comment.
[💡 style]
Typo in 'Tax Witholding' should be corrected to 'Tax Withholding' for clarity and professionalism.
| line-height: 20px; | ||
| font-size: 14px; | ||
|
|
||
| + .taxes { |
There was a problem hiding this comment.
[correctness]
The adjacent sibling combinator (+ .taxes) is used here, which means the margin-top will only apply if another .taxes element directly follows this one. Ensure this is the intended behavior, as it might lead to unexpected layout issues if not properly controlled.
| align-items: center; | ||
| color: $black-100; | ||
|
|
||
|
|
There was a problem hiding this comment.
[💡 style]
Consider removing the extra blank line here for consistency and to avoid unnecessary whitespace.
| datePaid: payment.details[0].datePaid ? formatIOSDateString(payment.details[0].datePaid) : '-', | ||
| description: payment.description, | ||
| details: payment.details, | ||
| grossPayment: formatCurrency(payment.details[0].totalAmount, payment.details[0].currency), |
There was a problem hiding this comment.
[❗❗ correctness]
The grossPayment field is calculated using totalAmount, which is also used for netPayment. Ensure that totalAmount represents the correct value for gross payment, as this could lead to incorrect payment calculations.
| <PaymentsTable | ||
| currentPage={pagination.currentPage} | ||
| numPages={pagination.totalPages} | ||
| minWithdrawAmount={walletDetails?.minWithdrawAmount ?? 0} |
There was a problem hiding this comment.
[correctness]
The minWithdrawAmount is set to 0 if walletDetails is undefined. Consider handling the case where walletDetails might be undefined more explicitly to avoid potential issues with zero values.
| const [selectedPayments, setSelectedPayments] = React.useState<{ [paymentId: string]: Winning }>({}) | ||
| const [isLoading, setIsLoading] = React.useState<boolean>(false) | ||
| const [filters, setFilters] = React.useState<Record<string, string[]>>({}) | ||
| const { data: walletDetails }: WalletDetailsResponse = useWalletDetails() |
There was a problem hiding this comment.
[maintainability]
The useWalletDetails hook is used but not checked for errors or loading states. Consider adding error handling or loading state management to ensure robustness.
| const [error, setError] = React.useState('') | ||
| const [showResendButton, setShowResendButton] = React.useState(false) | ||
| const [error, setError] = React.useState<string>() | ||
| const [, setShowResendButton] = React.useState(false) |
There was a problem hiding this comment.
[maintainability]
The setShowResendButton state is being initialized but never used in the component. Consider removing this state if it is not needed, or ensure it is used appropriately.
| setError(err.message) | ||
| }) | ||
| props.onOtpEntered(code) | ||
| } else if (code.length < 6) { |
There was a problem hiding this comment.
[❗❗ correctness]
The handleChange function calls props.onOtpEntered(code) when the OTP length is 6, but does not handle any verification logic. Ensure that the verification logic is handled appropriately elsewhere, as this change removes the previous verifyOtp call.
| } | ||
|
|
||
| useEffect(() => { | ||
| setError(props.error) |
There was a problem hiding this comment.
[correctness]
The useEffect hook sets the error state based on props.error. Ensure that props.error is correctly managed and updated in the parent component to avoid inconsistent error states.
| <p>Can't find the code? Check your spam folder.</p> | ||
| {loading && <LoadingCircles />} | ||
| {!loading && showResendButton && ( | ||
| {/* {!loading && showResendButton && ( |
There was a problem hiding this comment.
[design]
The resend button code is commented out. If the resend functionality is intended to be removed, ensure that this decision is documented and communicated, as it impacts user experience.
| @@ -1 +1,2 @@ | |||
| export { default as OtpModal } from './OtpModal' | |||
| export * from './use-otp-modal' | |||
There was a problem hiding this comment.
[maintainability]
Using export * can lead to unintentional exports if new exports are added to ./use-otp-modal in the future. Consider explicitly listing the exports to improve maintainability and avoid potential conflicts.
| ] => { | ||
| const [isVisible, setIsVisible] = useState(false) | ||
| const [error, setError] = useState<string>() | ||
| const [resolvePromise, setResolvePromise] = useState<(value?: boolean | string) => void |
There was a problem hiding this comment.
[correctness]
Using useState to store a function reference (resolvePromise) can lead to stale closures if the component re-renders before the promise is resolved. Consider using a ref to store the function instead to ensure it always has the latest reference.
| const handleClose = useCallback(() => { | ||
| setIsVisible(false) | ||
| resolvePromise() | ||
| }, [resolvePromise]) |
There was a problem hiding this comment.
[correctness]
Calling resolvePromise() without any arguments in handleClose might lead to unexpected behavior if the consumer of collectOtpCode expects a boolean or string. Ensure that the consumer can handle an undefined value or consider providing a default value.
| color: #333; | ||
| } | ||
|
|
||
| .paymeBtn { |
There was a problem hiding this comment.
[maintainability]
The class name .paymeBtn is inconsistent with .payMeButton used in the media query. Consider using a consistent naming convention to avoid confusion and potential errors.
|
|
||
| const calculateTotal = () => Object.values(selectedPayments) | ||
| .reduce((acc, payment) => acc + parseFloat(payment.netPayment.replace(/[^0-9.-]+/g, '')), 0) | ||
| .reduce((acc, payment) => acc + parseFloat(payment.grossPayment.replace(/[^0-9.-]+/g, '')), 0) |
There was a problem hiding this comment.
[❗❗ correctness]
The change from netPayment to grossPayment in the calculateTotal function could impact the business logic if the total amount should be based on net payments. Please verify if this change aligns with the intended calculation logic.
| disabled={total === 0} | ||
| <Tooltip | ||
| content={( | ||
| <> |
There was a problem hiding this comment.
[💡 style]
There is a typo in the tooltip content: 'amounti' should be 'amount'.
| import { useCanViewPayout } from '../hooks/use-can-view-payout' | ||
|
|
||
| export interface PayoutGuardProps { | ||
| profile?: UserProfile |
There was a problem hiding this comment.
[correctness]
Consider making the profile prop non-optional if useCanViewPayout requires a UserProfile to function correctly. This will ensure that the component is always used with the necessary data, preventing potential runtime errors.
|
|
||
| export const useCanViewPayout = (profile?: UserProfile): boolean => useMemo(() => ( | ||
| !!profile | ||
| && !profile.email.toLowerCase() |
There was a problem hiding this comment.
[❗❗ correctness]
The use of toLowerCase() assumes that profile.email is always defined when profile is truthy. Consider adding a nullish check for profile.email to avoid potential runtime errors.
| export interface Response<T> { | ||
| data?: Readonly<T> | ||
| error?: Readonly<string> | ||
| mutate: KeyedMutator<any> |
There was a problem hiding this comment.
[maintainability]
Consider using a more specific type for KeyedMutator<any> to improve type safety and maintainability. Using any can lead to potential runtime errors that TypeScript is designed to prevent.
| return { | ||
| data, | ||
| error, | ||
| isLoading: isValidating && !data && !error, |
There was a problem hiding this comment.
[correctness]
The isLoading logic could be misleading if isValidating is true but data or error is already present. Consider revising the condition to better reflect the loading state, such as isLoading: !data && !error && isValidating.
| @@ -1,4 +1,5 @@ | |||
| export default interface ApiResponse<T> { | |||
| status: 'success' | 'error' | |||
| error?: { code: string; message: string } | |||
There was a problem hiding this comment.
[maintainability]
Consider using a more structured type for error to ensure consistency and ease of maintenance. For example, defining an ErrorDetail interface could improve readability and maintainability.
| } | ||
| primaryCurrency?: string | null; | ||
| estimatedFees?: string | null; | ||
| taxWithholdingPercentage?: string | null; |
There was a problem hiding this comment.
[correctness]
Consider changing taxWithholdingPercentage to a number type instead of string. This will prevent potential issues with calculations and ensure type consistency, as percentages are typically numerical values.
| isSetupComplete: boolean | ||
| } | ||
| primaryCurrency?: string | null; | ||
| estimatedFees?: string | null; |
There was a problem hiding this comment.
[correctness]
Consider changing estimatedFees to a number type instead of string. This will help avoid potential issues with calculations and ensure type consistency, as fees are typically numerical values.
| identityVerification: { | ||
| isSetupComplete: boolean | ||
| } | ||
| primaryCurrency?: string | null; |
There was a problem hiding this comment.
[💡 maintainability]
Consider changing primaryCurrency to a non-nullable string if possible. This will simplify the handling of this property and reduce the need for null checks.
| export interface PaymentDetail { | ||
| id: string | ||
| netAmount: string | ||
| grossAmount: string |
There was a problem hiding this comment.
[❗❗ correctness]
Changing netAmount to grossAmount in the PaymentDetail interface might affect calculations or logic that depend on net values. Ensure that any logic relying on net amounts is updated accordingly.
| type: string | ||
| createDate: string | ||
| netPayment: string | ||
| grossPayment: string |
There was a problem hiding this comment.
[❗❗ correctness]
The change from netPayment to grossPayment in the Winning interface could impact any logic that differentiates between net and gross payments. Verify that all related logic and calculations are adjusted to accommodate this change.
|
|
||
| if (response.status === 'error') { | ||
| throw new Error('Error removing tax form') | ||
| if (response.status === 'error' && response.error?.code?.startsWith('otp_')) { |
There was a problem hiding this comment.
[❗❗ security]
Throwing response.error directly might expose internal error structures to the caller, which could be a security risk. Consider wrapping the error in a custom error message to avoid leaking sensitive information.
| const url = `${WALLET_API_BASE_URL}/trolley/portal-link` | ||
| const response = await xhrGetAsync<{ link: string }>(url) | ||
|
|
||
| if (!response.link) { |
There was a problem hiding this comment.
[correctness]
The check if (!response.link) assumes that response is always defined and correctly structured. Consider adding a check to ensure response is not null or undefined before accessing its properties to prevent runtime errors.
| * @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.
[maintainability]
The function nullToZero uses the nullish coalescing operator (??) to handle null and undefined values, but it does not explicitly check for the string 'null' in the same condition. Consider using a more explicit condition to ensure clarity and maintainability, such as (value === null || value === undefined || value === 'null') ? '0' : value. This makes the logic more straightforward and avoids potential confusion.
| } | ||
|
|
||
| export const STANDARDIZED_SKILLS_API = `${API.V5}/standardized-skills` | ||
| export const TC_FINANCE_API = `${API.V5}/finance` |
There was a problem hiding this comment.
[❗❗ security]
Consider verifying that the TC_FINANCE_API endpoint is correctly configured and secured, especially if it handles sensitive financial data.
| TALENTSEARCH: getReactEnv<string>('USERFLOW_SURVEY_TALENTSEARCH', 'd1030c93-dd36-4ae0-b5d0-95004b8e9d32'), | ||
| } | ||
|
|
||
| export const TROLLEY_WIDGET_ORIGIN = getReactEnv<string>('TROLLEY_WIDGET_ORIGIN', 'https://widget.trolley.com') |
There was a problem hiding this comment.
[❗❗ security]
Ensure that the TROLLEY_WIDGET_ORIGIN URL is correctly configured and trusted, as it could impact security if it interacts with user data or sensitive operations.
|
|
||
| const ConfirmModal: FC<ConfirmModalProps> = (props: ConfirmModalProps) => { | ||
| const isLoading = props.isLoading | ||
| const handleConfirm = useCallback((): void => props.onConfirm(), [props.onConfirm]) |
There was a problem hiding this comment.
[maintainability]
The useCallback hook is used correctly here to memoize the handleConfirm function. However, ensure that props.onConfirm is stable and does not change between renders, otherwise it could lead to unexpected behavior. If props.onConfirm is a prop passed from a parent component, consider verifying its stability or wrapping it in useCallback in the parent component as well.
| const TabsNavbar = <T, >(props: TabsNavbarProps<T>): JSX.Element => { | ||
| const query: URLSearchParams = new URLSearchParams(window.location.search) | ||
| const initialTab: MutableRefObject<string | null> = useRef<string|null>(query.get('tab')) | ||
| const initialTab: MutableRefObject<T | undefined> = useRef<T|undefined>(query.get('tab') as T) |
There was a problem hiding this comment.
[❗❗ correctness]
Casting query.get('tab') to T might lead to runtime errors if the URL parameter is not of type T. Consider adding a type guard or validation to ensure the value is of the expected type.
| setTabOpened(tabId) | ||
| props.onChildChange?.call(undefined, tabId, childTabId) | ||
| updateOffset(tabId) | ||
| setTabOpened(tabId as T) |
There was a problem hiding this comment.
[❗❗ correctness]
Casting tabId to T without validation could lead to runtime errors if tabId is not of type T. Consider adding a type check or validation to ensure safety.
| {props.tabs.map((tab, i) => ( | ||
| <TabsNavbarItem | ||
| key={tab.id} | ||
| key={tab.id as string} |
There was a problem hiding this comment.
[correctness]
Casting tab.id to string for the key prop assumes that tab.id can always be safely converted to a string. Ensure that tab.id is always a type that can be converted to a string to avoid potential issues.
| handleActivateChildTab: (tabId: string, childTabId: string) => () => void | ||
| activeTabId?: T | ||
| handleActivateTab: (tabId: T) => () => void | ||
| handleActivateChildTab: (tabId: T, childTabId: string) => () => void |
There was a problem hiding this comment.
[maintainability]
The handleActivateChildTab function still uses string for childTabId, while tabId has been generalized to type T. Consider updating childTabId to be of type T for consistency and to support cases where child tab IDs might not be strings.
| > | ||
| <ul> | ||
| {props.tab.children.map(item => ( | ||
| <li key={item.title}> |
There was a problem hiding this comment.
[correctness]
Using item.title as the key for list items may lead to issues if titles are not unique. Consider using a unique identifier for keys to prevent potential rendering issues.
| import { TabsNavItemBadge } from './tab-nav-item-badge.model' | ||
|
|
||
| export interface TabsNavItem { | ||
| export interface TabsNavItem<T = string> { |
There was a problem hiding this comment.
[❗❗ correctness]
The generic type parameter T for TabsNavItem allows flexibility for the id type, but ensure that all usages of TabsNavItem in the codebase are updated to handle this change appropriately. This could impact type safety if not managed correctly.
| children?: ReactNode | ||
| triggerOn?: 'click' | 'hover' | ||
| strategy?: 'absolute' | 'fixed' | ||
| disableTooltip?: boolean |
There was a problem hiding this comment.
[💡 maintainability]
Consider documenting the disableTooltip prop in the interface comments to ensure clarity for future developers regarding its purpose and usage.
| > | ||
| {props.content} | ||
| </ReactTooltip> | ||
| {!props.disableTooltip && ( |
There was a problem hiding this comment.
[correctness]
The conditional rendering of ReactTooltip based on disableTooltip is correct, but ensure that any logic relying on the tooltip being present handles the case where it is disabled. This includes verifying that any event handlers or dependencies on tooltip visibility are appropriately managed.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-23
What's in this PR?