PM-959 tc finance integration #1254
Conversation
fix: wallet app fixes and improvements (core-96)
RELEASE: TSJR-57 skills manager
fix: show time left to release a payment when release date <= 24 hours
fix: label pluralization
feat: add wallet-admin app
fix: filter payment listing by member handle
feat: add wallet admin ui
feat(wallet-admin): improved management of payments, tax forms, and linked payment provider
…ce-subscriptions Mp 392 update preference subscriptions
PROD - Minor wallet admin updates (CORE-635)
PROD DEPLOY - Core 635
PROD - Remove "Hire Topcoder Talent" button
Redirect payment settings to wallet app, and Lint cleanup
PROD - Remove hard-coded tokens
…-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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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.
The setShowResendButton state setter is defined but never used. Consider removing it if it's not needed.
| } | ||
| } | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Consider adding a dependency array to the useEffect hook to ensure it only runs when necessary. Currently, it will run on every render, which might not be the intended behavior.
| {error && <p className={styles.error}>{error}</p>} | ||
| <p> | ||
| For added security we’ve sent a 6-digit code to your | ||
| For added security we've sent a 6-digit code to your |
There was a problem hiding this comment.
Consider using double quotes for consistency with the rest of the JSX code, e.g., "we've" instead of '.
| For added security we’ve sent a 6-digit code to your | ||
| For added security we've sent a 6-digit code to your | ||
| {' '} | ||
| <strong className='body-main-bold'>{props.userEmail ?? '***@gmail.com'}</strong> |
There was a problem hiding this comment.
The default email placeholder ***@gmail.com might not be suitable for all users. Consider using a more generic placeholder like ***@example.com.
| <strong className='body-main-bold'>{props.userEmail ?? '***@gmail.com'}</strong> | ||
| {' '} | ||
| email. The code | ||
| email. The code |
There was a problem hiding this comment.
Using for spacing might not be necessary. Consider using CSS for consistent styling and spacing.
| <p>Can't find the code? Check your spam folder.</p> | ||
| {loading && <LoadingCircles />} | ||
| {!loading && showResendButton && ( | ||
| {/* {!loading && showResendButton && ( |
There was a problem hiding this comment.
The resend button code is commented out. If this is intentional for the integration, ensure that there is a plan to handle cases where users need to resend the OTP. If not, consider uncommenting it or providing an alternative solution.
| ] => { | ||
| 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.
Consider providing a default value for setResolvePromise to ensure it is always initialized properly. Using noop is fine, but it might be clearer to initialize it with a more descriptive function that indicates its purpose.
|
|
||
| const modal = isVisible ? ( | ||
| <OtpModal | ||
| isOpen |
There was a problem hiding this comment.
The isOpen prop is being passed without a value, which implicitly sets it to true. Consider explicitly setting isOpen={true} for clarity.
| } | ||
|
|
||
| .paymeBtn { | ||
| pointer-events: initial; |
There was a problem hiding this comment.
Consider using pointer-events: auto; instead of pointer-events: initial; for better browser compatibility and to ensure the element behaves as expected.
| import React, { useEffect, useState } from 'react' | ||
|
|
||
| import { Button, IconOutline } from '~/libs/ui' | ||
| import { Button, IconOutline, Tooltip } from '~/libs/ui' |
There was a problem hiding this comment.
The Tooltip component is imported but not used in the code. Consider removing it if it's not needed.
| import styles from './PaymentTable.module.scss' | ||
|
|
||
| interface PaymentTableProps { | ||
| minWithdrawAmount: number; |
There was a problem hiding this comment.
The minWithdrawAmount prop is added to the PaymentTableProps interface, but ensure that it is utilized within the component logic. If it's not used, consider removing it.
|
|
||
| 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.
The calculation now uses grossPayment instead of netPayment. Ensure this change is intentional and aligns with the requirements of the finance integration.
| <td className={`body-small-bold ${styles.capitalize}`}>{payment.type}</td> | ||
| <td className='body-small-bold'>{payment.createDate}</td> | ||
| <td className='body-small-bold'>{payment.netPayment}</td> | ||
| <td className='body-small-bold'>{payment.grossPayment}</td> |
There was a problem hiding this comment.
The change from netPayment to grossPayment should be verified to ensure it aligns with the intended logic of the application. If the change is intentional, consider updating any related documentation or tests to reflect this modification.
| <Tooltip | ||
| content={( | ||
| <> | ||
| Minimum withdrawal amounti is $ |
There was a problem hiding this comment.
Typo in the tooltip content: 'amounti' should be 'amount'.
| Please select more payments. | ||
| </> | ||
| )} | ||
| disableTooltip={!isPaymeDisabled} |
There was a problem hiding this comment.
The variable isPaymeDisabled is used here but not defined in the provided diff. Ensure that isPaymeDisabled is correctly defined and initialized in the component.
|
|
||
| export const useCanViewPayout = (profile?: UserProfile): boolean => useMemo(() => ( | ||
| !!profile | ||
| && !profile.email.toLowerCase() |
There was a problem hiding this comment.
Consider handling the case where profile.email might be undefined or null to prevent potential runtime errors.
| @@ -1,4 +1,5 @@ | |||
| export default interface ApiResponse<T> { | |||
| status: 'success' | 'error' | |||
| error?: { code: string; message: string } | |||
There was a problem hiding this comment.
Consider providing more specific types for the code and message properties within the error object to ensure consistency and prevent potential errors. For example, if code is expected to be a specific set of strings, you could define a union type for it.
| account: AccountDetails | ||
| withdrawalMethod: { | ||
| isSetupComplete: boolean | ||
| type: 'paypal' | 'bank' |
There was a problem hiding this comment.
Consider using an enum for the type field instead of a union of string literals to improve type safety and maintainability.
| identityVerification: { | ||
| isSetupComplete: boolean | ||
| } | ||
| primaryCurrency?: string | null; |
There was a problem hiding this comment.
The primaryCurrency field is optional and can be null. Ensure that any logic using this field handles these cases appropriately.
| isSetupComplete: boolean | ||
| } | ||
| primaryCurrency?: string | null; | ||
| estimatedFees?: string | null; |
There was a problem hiding this comment.
The estimatedFees field is optional and can be null. Ensure that any logic using this field handles these cases appropriately.
| } | ||
| primaryCurrency?: string | null; | ||
| estimatedFees?: string | null; | ||
| taxWithholdingPercentage?: string | null; |
There was a problem hiding this comment.
The taxWithholdingPercentage field is optional and can be null. Ensure that any logic using this field handles these cases appropriately.
| export interface PaymentDetail { | ||
| id: string | ||
| netAmount: string | ||
| grossAmount: string |
There was a problem hiding this comment.
The netAmount property has been removed from the PaymentDetail interface. Ensure that this change is intentional and that all parts of the codebase that rely on netAmount are updated accordingly.
| @@ -1,21 +1,20 @@ | |||
| /* eslint-disable camelcase */ | |||
There was a problem hiding this comment.
The ESLint rule camelcase is disabled here. Consider enabling it to maintain consistent naming conventions across the codebase.
| /* eslint-disable camelcase */ | ||
| import { AxiosError } from 'axios' | ||
|
|
||
| import { EnvironmentConfig } from '~/config' |
There was a problem hiding this comment.
The import for xhrDeleteAsync has been removed. Ensure that this function is not needed elsewhere in the code, or if it is, consider re-importing it.
| import { xhrDeleteAsync, xhrGetAsync, xhrPostAsync, xhrPostAsyncWithBlobHandling } from '~/libs/core' | ||
| import { xhrGetAsync, xhrPostAsync, xhrPostAsyncWithBlobHandling } from '~/libs/core' | ||
|
|
||
| import { WalletDetails } from '../models/WalletDetails' |
There was a problem hiding this comment.
The import for PaymentProvider has been removed. Verify that this model is not used elsewhere in the code, or if it is, consider re-importing it.
| import { PaymentProvider } from '../models/PaymentProvider' | ||
| import { WinningDetail } from '../models/WinningDetail' | ||
| import { TaxForm } from '../models/TaxForm' | ||
| import { OtpVerificationResponse } from '../models/OtpVerificationResponse' |
There was a problem hiding this comment.
The import for TaxForm has been removed. Ensure that this model is not used elsewhere in the code, or if it is, consider re-importing it.
| otpCode, | ||
| winningsIds, | ||
| }) | ||
| const url = `${WALLET_API_BASE_URL}/withdraw` |
There was a problem hiding this comment.
Consider using const instead of let for url since its value is not reassigned.
|
|
||
| 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.
The error handling logic here is specific to OTP errors. Consider adding a more general error handling mechanism for other types of errors that might occur.
| }) | ||
|
|
||
| const url = `${baseUrl}/otp/verify` | ||
| const url = `${WALLET_API_BASE_URL}/otp/verify` |
There was a problem hiding this comment.
WALLET_API_BASE_URL should be verified to ensure it is defined and correctly configured in the environment settings. If it is not defined, it could lead to runtime errors.
| * @throws {Error} If the response does not contain a valid link. | ||
| */ | ||
| export async function getTrolleyPortalLink(): Promise<string> { | ||
| const url = `${WALLET_API_BASE_URL}/trolley/portal-link` |
There was a problem hiding this comment.
Consider adding error handling for the xhrGetAsync call to manage potential network or server errors more gracefully.
| const response = await xhrGetAsync<{ link: string }>(url) | ||
|
|
||
| if (!response.link) { | ||
| throw new Error('Error fetching Trolley portal link') |
There was a problem hiding this comment.
The error message could be more descriptive to help with debugging. Consider including the URL or additional context in the error message.
| * @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.
The current implementation does not handle cases where the input value is an empty string. Consider updating the function to return '0' for empty strings as well, if that is the intended behavior.
| 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.
Consider adding validation to ensure that the URL provided in TROLLEY_WIDGET_ORIGIN is a valid URL format. This can help prevent potential issues if the environment variable is incorrectly set.
|
|
||
| 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.
Consider adding error handling or a fallback mechanism in handleConfirm to manage potential issues during the confirmation process.
| /> | ||
| <Button | ||
| disabled={props.canSave === false || props.isLoading} | ||
| disabled={props.canSave === false || props.isLoading || props.isProcessing} |
There was a problem hiding this comment.
Consider renaming props.isProcessing to something more descriptive if it represents a specific state or action, to improve code readability.
| primary | ||
| label={props.action || 'Confirm'} | ||
| onClick={props.onConfirm} | ||
| onClick={handleConfirm} |
There was a problem hiding this comment.
Ensure that handleConfirm is defined and properly handles the confirm action. If handleConfirm is a new function, verify its implementation to ensure it aligns with the intended behavior.
| import { | ||
| Dispatch, | ||
| FC, | ||
| MutableRefObject, |
There was a problem hiding this comment.
The FC (Function Component) type has been removed from the imports. If this was intentional and the component is no longer using FC, ensure that all related type definitions and usages are updated accordingly. If FC is still needed, consider re-adding it.
| 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.
The query.get('tab') as T cast assumes that the URL parameter will always be of type T. Consider adding validation to ensure the type safety of this cast.
| const tabRefs: MutableRefObject<Array<HTMLElement>> = useRef([] as Array<HTMLElement>) | ||
| const [offset, setOffset]: [number, Dispatch<SetStateAction<number>>] = useState<number>(0) | ||
| const [menuIsVisible, setMenuIsVisible]: [boolean, Dispatch<SetStateAction<boolean>>] = useState(false) | ||
| const triggerRef: MutableRefObject<any> = useRef(undefined) |
There was a problem hiding this comment.
The triggerRef is typed as MutableRefObject<any>. Consider providing a more specific type to improve type safety and maintainability.
| ]) | ||
|
|
||
| const handleActivateTab: (tabId: string) => () => void = useCallback((tabId: string) => () => { | ||
| const handleActivateTab: (tabId: T) => () => void = useCallback((tabId: T) => () => { |
There was a problem hiding this comment.
The type of tabId has been changed from string to T. Ensure that T is correctly defined and used throughout the component to avoid type inconsistencies.
| setTabOpened(tabId) | ||
| props.onChildChange?.call(undefined, tabId, childTabId) | ||
| updateOffset(tabId) | ||
| setTabOpened(tabId as T) |
There was a problem hiding this comment.
Suggestion
Consider verifying that the type assertion as T is necessary. If tabId is already of type T, the assertion might be redundant. Type assertions should be used cautiously as they can bypass type checking and potentially lead to runtime errors if the assumption is incorrect.
| ) { | ||
| handleActivateTab(initialTab.current)() | ||
| initialTab.current = '' | ||
| initialTab.current = undefined |
There was a problem hiding this comment.
Consider using null instead of undefined for initialTab.current if the intention is to explicitly indicate the absence of a value. This can help avoid potential issues with type checking or unintended behavior in some cases.
| {props.tabs.map((tab, i) => ( | ||
| <TabsNavbarItem | ||
| key={tab.id} | ||
| key={tab.id as string} |
There was a problem hiding this comment.
Casting tab.id to string using as string may hide potential issues if tab.id is not always a string. Consider ensuring that tab.id is always a string type in the data model or use a more robust type-checking method.
| const isMobile = useMemo(() => screenWidth < 745, [screenWidth]) | ||
| export const TabsNavbarItem: FC<TabsNavbarItemProps<any> & RefAttributes<HTMLElement>> | ||
| = forwardRef<HTMLElement, TabsNavbarItemProps<any>>( | ||
| <T, >(props: TabsNavbarItemProps<T>, ref: ForwardedRef<HTMLElement>, |
There was a problem hiding this comment.
The use of any in TabsNavbarItemProps<any> can lead to potential type safety issues. Consider using a more specific type or a generic type parameter to ensure type safety.
| </> | ||
| ) | ||
|
|
||
| if (props.tab.url) { |
There was a problem hiding this comment.
The key prop on the <a> element is redundant since it is already being set on the parent component. Consider removing it to avoid unnecessary duplication.
| <DropdownMenu | ||
| open={openDropdown} | ||
| setOpen={setOpenDropdown} | ||
| key={props.tab.id as string} |
There was a problem hiding this comment.
The key prop on the DropdownMenu component is redundant since it is already being set on the parent component. Consider removing it to avoid unnecessary duplication.
| props.activeTabId === props.tab.id && 'active', | ||
| props.className, | ||
| )} | ||
| key={props.tab.id as string} |
There was a problem hiding this comment.
The key prop on the <div> element is redundant since it is already being set on the parent component. Consider removing it to avoid unnecessary duplication.
| href={props.tab.url} | ||
| rel='noopener noreferrer' | ||
| target='_blank' | ||
| key={props.tab.id as string} |
There was a problem hiding this comment.
The key prop on the <div> element is redundant since it is already being set on the parent component. Consider removing it to avoid unnecessary duplication.
| > | ||
| {props.content} | ||
| </ReactTooltip> | ||
| {!props.disableTooltip && ( |
There was a problem hiding this comment.
Consider renaming the disableTooltip prop to something more intuitive like isTooltipDisabled to clearly indicate that it is a boolean flag.
5 participants
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-959
What's in this PR?