Sync v6 with dev #1269
Sync v6 with dev #1269
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}
| 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 areas of the codebase that rely on this field. 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 could affect calculations or logic that depend on these fields. Verify that all dependent code is updated to use grossAmount and grossAmountNumber if applicable.
| createdAt: string | ||
| releaseDate: string | ||
| datePaid: string | ||
| paymentStatus?: PayoutStatus |
There was a problem hiding this comment.
[maintainability]
The addition of paymentStatus as an optional field in WinningDetail should be checked for null or undefined cases in the code that uses this interface to avoid 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 should be verified to ensure that the new API endpoint provides the same or expected data structure and behavior. This is crucial to avoid breaking changes in the application.
|
|
||
| const WalletTabs: FC<WalletHomeProps> = (props: WalletHomeProps) => { | ||
| const { hash }: { hash: string } = useLocation() | ||
| const canViewPayout = useCanViewPayout(props.profile) |
There was a problem hiding this comment.
[correctness]
Consider verifying the props.profile object before passing it to useCanViewPayout. If props.profile is undefined or malformed, it might lead to unexpected behavior.
|
|
||
| const [activeTab, setActiveTab]: [string, Dispatch<SetStateAction<string>>] = useState<string>(activeTabHash) | ||
| const tabsConfig = useMemo(() => WalletTabsConfig.filter(tab => ( | ||
| tab.id !== WalletTabViews.payout || canViewPayout |
There was a problem hiding this comment.
[design]
The filtering logic for tabsConfig assumes that WalletTabViews.payout is the only tab that requires permission. Ensure this assumption holds true across future changes or consider a more flexible permission system.
| {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]
Ensure that PayoutGuard correctly handles cases where props.profile is invalid or missing. This could prevent potential runtime errors.
| winnings = '1', | ||
| taxforms = '2', | ||
| withdrawalmethods = '3', | ||
| home, |
There was a problem hiding this comment.
[❗❗ correctness]
The WalletTabViews enum now uses auto-incremented values starting from 0. If the numeric values are important for external systems or data storage, this change could lead to unexpected behavior. Ensure that the change in enum values is intentional and does not affect any dependent logic.
| ] | ||
|
|
||
| export function getHashFromTabId(tabId: string): string { | ||
| export function getHashFromTabId(tabId: WalletTabViews): string { |
There was a problem hiding this comment.
[❗❗ correctness]
The function getHashFromTabId now uses WalletTabViews as the parameter type. Ensure that all calls to this function are updated to pass the correct enum type instead of a string, as this could lead to type errors if not handled properly.
| } | ||
|
|
||
| export function getTabIdFromHash(hash: string): string { | ||
| export function getTabIdFromHash(hash: string): WalletTabViews { |
There was a problem hiding this comment.
[❗❗ correctness]
The function getTabIdFromHash now returns a WalletTabViews type. Ensure that all usages of this function handle the enum type correctly, as this change may affect how the returned value is processed.
|
|
||
| fetchWalletDetails() | ||
| }, []) | ||
| const { data: walletDetails, isLoading, error }: WalletDetailsResponse = useWalletDetails() |
There was a problem hiding this comment.
[correctness]
The destructuring of error from useWalletDetails is not utilized effectively. Consider handling the error state more robustly, such as displaying a user-friendly message or logging the error for debugging purposes.
| </div> | ||
| {isLoading && <LoadingCircles />} | ||
| {!isLoading && ( | ||
| {!isLoading && walletDetails && ( |
There was a problem hiding this comment.
[💡 maintainability]
The condition !isLoading && walletDetails is used multiple times. Consider extracting this logic into a variable for improved readability and maintainability.
| {walletDetails.withdrawalMethod.isSetupComplete && ( | ||
| <InfoRow | ||
| title='Est. Payment Fees' | ||
| value={`$${nullToZero(walletDetails.estimatedFees)} USD`} |
There was a problem hiding this comment.
[correctness]
Ensure that nullToZero(walletDetails.estimatedFees) handles all edge cases correctly, such as when estimatedFees is undefined or not a number. This is crucial for preventing unexpected behavior in the UI.
| {walletDetails.taxForm.isSetupComplete && ( | ||
| <InfoRow | ||
| title='Est. Tax Withholding %' | ||
| value={`${nullToZero(walletDetails.taxWithholdingPercentage)}%`} |
There was a problem hiding this comment.
[correctness]
Ensure that nullToZero(walletDetails.taxWithholdingPercentage) handles all edge cases correctly, such as when taxWithholdingPercentage is undefined or not a number. This is crucial for preventing unexpected behavior in the UI.
|
|
||
| .iframe { | ||
| width: 100%; | ||
| height: 100%; |
There was a problem hiding this comment.
[maintainability]
The height property is set twice, first to 100% and then to 90vh. This could lead to confusion or unintended behavior. Consider removing the redundant height: 100%; declaration.
| height: 100%; | ||
| border: none; | ||
| height: 90vh; | ||
| } No newline at end of file |
There was a problem hiding this comment.
[💡 style]
The file does not end with a newline character. While this is not a functional issue, adding a newline at the end of the file is a common convention that can improve compatibility with various tools.
|
|
||
| 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 broad. Consider narrowing the type to HTMLIFrameElement for better type safety and clarity.
| return undefined | ||
| } | ||
|
|
||
| const handleEvent: (event: any) => void = (event: any) => { |
There was a problem hiding this comment.
[maintainability]
Avoid using any for event types. Use MessageEvent instead to provide better type safety.
| 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.
[💡 performance]
Consider using startsWith instead of indexOf for checking the origin to improve readability and performance.
| return (): void => { | ||
| window.removeEventListener('message', handleEvent, false) | ||
| } | ||
| }, [frameRef.current?.src]) |
There was a problem hiding this comment.
[❗❗ correctness]
The dependency array [frameRef.current?.src] might not trigger updates as expected due to the mutable nature of frameRef. Consider using a state or a more reliable method to track changes.
| 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 parseFloat usage on payment.grossPayment.replace(/[^0-9.-]+/g, '') assumes that the grossPayment string will always be 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 NaN results.
| }) | ||
| 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 code and message properties directly can lead to runtime errors if the structure of error changes. Consider using a type guard or refining the type check to ensure these properties exist.
|
|
||
| let message = 'Failed to process payments. Please try again later.' | ||
|
|
||
| if (error instanceof AxiosError) { |
There was a problem hiding this comment.
[correctness]
The error handling for AxiosError assumes that error.response?.data?.error?.message or error.response?.data?.message will always be a string. Consider adding a check to ensure these properties are strings before using them to avoid potential runtime errors.
| line-height: 20px; | ||
| font-size: 14px; | ||
|
|
||
| + .taxes { |
There was a problem hiding this comment.
[maintainability]
The adjacent sibling combinator (+ .taxes) is used to apply a margin-top to subsequent .taxes elements. Ensure that this selector is necessary and correctly applied in the context of your HTML structure, as it may not be intuitive and could lead to unexpected layout issues if the HTML structure changes.
| color: $black-100; | ||
|
|
||
|
|
||
| @include ltelg { |
There was a problem hiding this comment.
[design]
The @include ltelg mixin changes the flex direction to column and aligns items to flex-start. Verify that this mixin is correctly defined and intended for use here, as it significantly alters the layout for larger screens, which might not be the desired behavior.
| canBeReleased: new Date(payment.releaseDate) <= new Date() && payment.details[0].status === 'OWED', | ||
| createDate: formatIOSDateString(payment.createdAt), | ||
| currency: payment.details[0].currency, | ||
| datePaid: payment.details[0].datePaid ? formatIOSDateString(payment.details[0].datePaid) : '-', |
There was a problem hiding this comment.
[❗❗ correctness]
The grossPayment and netPayment seem to be using the same value from payment.details[0].totalAmount. If these are intended to represent different amounts, ensure that the correct values are being used.
| <PaymentsTable | ||
| currentPage={pagination.currentPage} | ||
| numPages={pagination.totalPages} | ||
| minWithdrawAmount={walletDetails?.minWithdrawAmount ?? 0} |
There was a problem hiding this comment.
[correctness]
Ensure that walletDetails?.minWithdrawAmount is correctly initialized and fetched. If walletDetails is undefined, this could lead to unexpected behavior in the PaymentsTable component.
| 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 initialized but never used due to the commented-out code block. Consider removing this state if it's not needed, or ensure the resend functionality is implemented properly.
| } | ||
| } | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
[correctness]
The effect setting the error state from props.error runs on every render where props.error changes. Ensure that props.error is correctly managed and updated to avoid unnecessary renders or stale error messages.
| {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.
[💡 style]
The use of ' for apostrophes in JSX is unnecessary. You can use a regular apostrophe ' directly within JSX.
| ] => { | ||
| 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 (resolvePromise) can lead to unexpected behavior if the component re-renders before the promise is resolved. Consider using useRef instead to store the function, which would maintain the same reference across renders.
|
|
||
| const handleClose = useCallback(() => { | ||
| setIsVisible(false) | ||
| resolvePromise() |
There was a problem hiding this comment.
[correctness]
Calling resolvePromise() without an argument in handleClose might lead to unexpected behavior if the promise expects a boolean or string. Ensure that the promise resolution aligns with expected usage.
| } | ||
|
|
||
| .paymeBtn { | ||
| pointer-events: initial; |
There was a problem hiding this comment.
[💡 maintainability]
The use of pointer-events: initial; on .paymeBtn might not be necessary unless there's a specific reason to override a previous pointer-events setting. Consider removing it if it's not needed for clarity and maintainability.
|
|
||
| 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]
Switching from netPayment to grossPayment may have implications on the calculated total. Ensure that this change aligns with business requirements and that grossPayment is the intended value for total calculations.
| disabled={total === 0} | ||
| <Tooltip | ||
| content={( | ||
| <> |
There was a problem hiding this comment.
[💡 readability]
The tooltip content has a typo: '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 property non-optional if the useCanViewPayout hook requires a UserProfile to function correctly. This would prevent potential runtime errors if undefined is passed.
|
|
||
| export const useCanViewPayout = (profile?: UserProfile): boolean => useMemo(() => ( | ||
| !!profile | ||
| && !profile.email.toLowerCase() |
There was a problem hiding this comment.
[❗❗ correctness]
Consider handling the case where profile.email might be undefined or null to prevent potential runtime errors.
| export type WalletDetailsResponse = Response<WalletDetails> | ||
|
|
||
| export function useWalletDetails(): WalletDetailsResponse { | ||
| const { data, error, mutate, isValidating }: SWRResponse = useSWR('wallet-details', getWalletDetails) |
There was a problem hiding this comment.
[maintainability]
The useSWR hook is called with a static key 'wallet-details'. Consider using a dynamic key if the wallet details can vary based on user or context, to ensure proper caching and revalidation behavior.
| return { | ||
| data, | ||
| error, | ||
| isLoading: isValidating && !data && !error, |
There was a problem hiding this comment.
[correctness]
The isLoading condition isValidating && !data && !error may not correctly represent the loading state if data is initially undefined but later becomes null. Consider explicitly checking for undefined to ensure clarity in loading state.
| @@ -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 help manage error codes and messages more effectively across the application.
| } | ||
| primaryCurrency?: string | null; | ||
| estimatedFees?: string | null; | ||
| taxWithholdingPercentage?: string | null; |
There was a problem hiding this comment.
[correctness]
Consider changing taxWithholdingPercentage from string | null to number | null for better type safety and to avoid potential parsing errors when performing calculations.
| isSetupComplete: boolean | ||
| } | ||
| primaryCurrency?: string | null; | ||
| estimatedFees?: string | null; |
There was a problem hiding this comment.
[correctness]
Consider changing estimatedFees from string | null to number | null for better type safety and to avoid potential parsing errors when performing calculations.
| 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 may affect calculations or logic elsewhere in the codebase that rely on net amounts. Ensure that all dependent code is updated accordingly to prevent incorrect financial calculations.
| 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 may expose internal error details to the client, which could be a security risk. Consider throwing a more generic error message instead.
| 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 the response object will always be defined. Consider adding a check for response itself to avoid potential runtime errors if the response is undefined.
| * @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.
[correctness]
The function nullToZero currently treats the string 'null' as equivalent to null or undefined, converting it to '0'. This behavior might be unexpected for some use cases where 'null' is considered a valid string. Consider documenting this behavior clearly or revisiting the requirement to ensure it aligns with expected use cases.
|
|
||
| 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, but consider adding dependencies that might affect the onConfirm logic in the future. This ensures that the function is updated when necessary.
| /> | ||
| <Button | ||
| disabled={props.canSave === false || props.isLoading} | ||
| disabled={props.canSave === false || props.isLoading || props.isProcessing} |
There was a problem hiding this comment.
[correctness]
Adding props.isProcessing to the disabled condition for the confirm button is a good approach to prevent user interaction during processing. Ensure that isProcessing is correctly managed to avoid blocking user actions unnecessarily.
| 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 type T could lead to runtime errors if the query parameter doesn't match the expected type T. Consider adding validation or default handling to ensure type safety.
| 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 type T without validation may lead to runtime errors if tabId is not of type T. Consider adding type checks or constraints to ensure type 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 of a type that can be reliably cast to string or handle cases where it might not be.
| 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.
[correctness]
The handleActivateChildTab function still uses string for childTabId, while tabId has been changed to generic T. Consider updating childTabId to be of type T for consistency and to support cases where child tab IDs might not be strings.
| 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.
[maintainability]
The use of any for the generic type in TabsNavbarItemProps<any> could lead to type safety issues. Consider using a more specific type or a default generic type to ensure better type safety.
| > | ||
| <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 each item, such as item.id, if available.
| 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 in the type of id, but it could introduce potential issues if not used consistently across the codebase. Ensure that all usages of TabsNavItem are updated to handle this change appropriately to avoid type mismatches.
| children?: ReactNode | ||
| triggerOn?: 'click' | 'hover' | 'click-hover' | ||
| strategy?: 'absolute' | 'fixed' | ||
| disableTooltip?: boolean |
There was a problem hiding this comment.
[💡 maintainability]
Consider adding a default value for disableTooltip in TooltipProps to ensure consistent behavior when the prop is not provided.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/
What's in this PR?
Merged dev into v6
Now wallet & wallet admin will use the v6 finance