Prod - August 25th deploy #1210
Conversation
jmgasper
commented
Aug 25, 2025
jmgasper
commented
- Add Terms Management to System Admin app
- Move gamification admin UI into System Admin App
- Add additional features for submission management (non-MMs) to System Admin app
- Copilot portal fixes:
- https://topcoder.atlassian.net/browse/PM-1616
- https://topcoder.atlassian.net/browse/PM-1614
- https://topcoder.atlassian.net/browse/PM-1650
(cherry picked from commit 2f1995b)
…7360 Topcoder Admin App - Add Terms Management Final fix
Topcoder admin app: move src/apps/gamification-admin -> src/apps/admin
PM-1612 Update visible columns on copilot applications
feat(PM-1650): paginated copilot requests page
Limit other payment type field to 8 char
…7611 Topcoder Admin App - Add Non-MM Submission Management
PM-1616 Prevent undefined error
Add max length to other payment types
fix(PM-1650): sort from server side
fix(PM-1614): add white space below copilot request table
Feat/system admin release for terms, moving gamification admin, and additional features for submissions
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * @param units units | ||
| * @returns file size | ||
| */ | ||
| export function humanFileSize(inputBytes: number, units: string[]): string { |
There was a problem hiding this comment.
The function humanFileSize does not handle cases where the units array is empty or does not contain enough elements to match the size of the input bytes. Consider adding a check to ensure that the units array has sufficient elements to prevent potential out-of-bounds errors.
| */ | ||
| export function humanFileSize(inputBytes: number, units: string[]): string { | ||
| let bytes = inputBytes | ||
| if (Math.abs(bytes) < 1024) { |
There was a problem hiding this comment.
The condition Math.abs(bytes) < 1024 assumes that the smallest unit is always less than 1024 bytes. Ensure that the units array is structured such that this assumption holds true, or handle cases where the smallest unit might differ.
| u += 1 | ||
| } while (Math.abs(bytes) >= 1024 && u < units.length) | ||
|
|
||
| return `${bytes.toFixed(1)}${units[u]}` |
There was a problem hiding this comment.
The toFixed(1) method is used to format the bytes value, which may lead to rounding issues. Consider whether this level of precision is appropriate for all use cases, or if additional formatting options should be provided.
| isValid: true, | ||
| key: key ?? undefined, | ||
| } | ||
| } catch (error) {} |
There was a problem hiding this comment.
The catch block is currently empty, which means any errors thrown by AmazonS3URI(fileURL) will be silently ignored. Consider logging the error or handling it appropriately to avoid potential debugging issues.
| FormUsersFilters, | ||
| } from '../models' | ||
| import { FormEditUserStatus } from '../models/FormEditUserStatus.model' | ||
| import { FormAddRoleMembers } from '../models/FormAddRoleMembers.type' | ||
| import { FormAddSSOLoginData } from '../models/FormAddSSOLoginData.model' | ||
| import { FormAddTermUser } from '../models/FormAddTermUser.model' | ||
|
|
||
| const docusignTypeId |
There was a problem hiding this comment.
The variable docusignTypeId is declared but not initialized. Ensure that it is assigned a value or remove it if not needed.
| docusignTemplateId: Yup.string() | ||
| .trim() | ||
| .when('agreeabilityTypeId', (agreeabilityTypeId, schema) => { | ||
| if (agreeabilityTypeId[0] === docusignTypeId) { |
There was a problem hiding this comment.
Consider checking if agreeabilityTypeId is not undefined or empty before accessing agreeabilityTypeId[0] to prevent potential runtime errors.
| /** | ||
| * Terms List Page. | ||
| */ | ||
| import { FC, useState } from 'react' |
There was a problem hiding this comment.
The import statement for react should include React to ensure compatibility with older versions of React that require React to be in scope for JSX.
| import classNames from 'classnames' | ||
|
|
||
| import { colWidthType, LinkButton, PageDivider } from '~/libs/ui' | ||
| import { PlusIcon } from '@heroicons/react/solid' |
There was a problem hiding this comment.
Consider using a more specific import path for PlusIcon to reduce bundle size if tree-shaking is not effectively removing unused exports.
| TableNoRecord, | ||
| TermsFilters, | ||
| TermsTable, | ||
| } from '../../../lib' |
There was a problem hiding this comment.
The import path for components from ../../../lib could be more specific to avoid importing unnecessary modules, which can increase the bundle size.
| page, | ||
| setPage, | ||
| setFilterCriteria, | ||
| }: useManageTermsProps = useManageTerms() |
There was a problem hiding this comment.
The type useManageTermsProps is used here, but it might be more appropriate to define the type inline or ensure it is imported from the correct module if it is used elsewhere.
| <LinkButton | ||
| primary | ||
| size='lg' | ||
| to='add' |
There was a problem hiding this comment.
The to prop value 'add' in LinkButton should be verified to ensure it correctly routes to the intended path. Consider using a constant or a route helper for better maintainability.
| } | ||
|
|
||
| .removeSelectionButtonContainer { | ||
| padding: 20px 0 30px $sp-8; |
There was a problem hiding this comment.
The padding value $sp-8 is used here, but it might be beneficial to ensure consistency with other spacing units used in the file or project. Consider verifying if $sp-8 is the intended spacing unit.
| justify-content: center; | ||
| bottom: 50px; | ||
| height: 64px; | ||
| left: $sp-8; |
There was a problem hiding this comment.
The use of $sp-8 for the left property might need verification for consistency with other spacing units. Ensure that $sp-8 is the correct spacing unit intended here.
| } | ||
|
|
||
| export const TermsUsersPage: FC<Props> = (props: Props) => { | ||
| const [showDialogAddUser, setShowDialogAddUser] = useState<boolean>() |
There was a problem hiding this comment.
Consider initializing showDialogAddUser with a default value, such as false, to ensure consistent behavior and avoid potential undefined state issues.
| export const TermsUsersPage: FC<Props> = (props: Props) => { | ||
| const [showDialogAddUser, setShowDialogAddUser] = useState<boolean>() | ||
| useAutoScrollTopWhenInit() | ||
| const { id = '' }: { id?: string } = useParams<{ |
There was a problem hiding this comment.
The destructuring assignment for useParams could be simplified by directly using const { id = '' } = useParams(); without specifying the type again, as TypeScript can infer it.
| icon={PlusIcon} | ||
| iconToLeft | ||
| label='Add User' | ||
| onClick={function onClick() { |
There was a problem hiding this comment.
The onClick function could be defined outside of the JSX to improve readability and maintainability, especially if it becomes more complex in the future.
| variant='danger' | ||
| disabled={!hasSelected || isRemovingBool} | ||
| size='lg' | ||
| onClick={function onClick() { |
There was a problem hiding this comment.
Similar to the previous suggestion, consider defining the onClick function outside of the JSX to enhance readability and maintainability.
| {showDialogAddUser && termInfo && ( | ||
| <DialogAddTermUser | ||
| open | ||
| setOpen={function setOpen() { |
There was a problem hiding this comment.
The setOpen function could be defined outside of the JSX to improve readability and maintainability, especially if it becomes more complex in the future.
| @@ -291,12 +291,13 @@ const CopilotOpportunityDetails: FC<{}> = () => { | |||
| ) | |||
| } | |||
| {activeTab === CopilotDetailsTabViews.details && <OpportunityDetails opportunity={opportunity} />} | |||
| {activeTab === CopilotDetailsTabViews.applications && isAdminOrPM && opportunity && ( | |||
| {activeTab === CopilotDetailsTabViews.applications && opportunity && ( | |||
There was a problem hiding this comment.
The isAdminOrPM check has been removed from the condition. If this was intentional, ensure that the CopilotApplications component handles cases where isAdminOrPM might not be true. If it was not intentional, consider re-adding the check to maintain the previous logic.
| @@ -96,12 +97,18 @@ const CopilotApplications: FC<{ | |||
|
|
|||
| const tableData = useMemo(getData, [props.copilotApplications, props.members]) | |||
|
|
|||
| const visibleColumns = props.isAdminOrPM | |||
There was a problem hiding this comment.
Consider renaming visibleColumns to filteredColumns for clarity, as it better describes the action being performed on the columns.
| @@ -96,12 +97,18 @@ const CopilotApplications: FC<{ | |||
|
|
|||
| const tableData = useMemo(getData, [props.copilotApplications, props.members]) | |||
|
|
|||
| const visibleColumns = props.isAdminOrPM | |||
| ? tableColumns | |||
| : tableColumns.filter(col => ![ | |||
There was a problem hiding this comment.
The filter logic for visibleColumns could be extracted into a separate function for better readability and potential reuse. This would also make the component cleaner.
| {copilotOpportunity.paymentType === 'standard' | ||
| ? copilotOpportunity.paymentType : copilotOpportunity.otherPaymentType} | ||
| ? copilotOpportunity.paymentType : copilotOpportunity.otherPaymentType.slice(0, 8)} |
There was a problem hiding this comment.
The use of slice(0, 8) to truncate otherPaymentType might not be clear to future developers. Consider if this truncation is necessary and if it aligns with the intended functionality. If truncation is required, ensure that it does not lead to loss of important information.
| @@ -276,6 +276,11 @@ const CopilotRequestForm: FC<{}> = () => { | |||
| key: 'otherPaymentType', | |||
| message: 'Field cannot be left empty', | |||
| }, | |||
| { | |||
| condition: formValues.otherPaymentType && formValues.otherPaymentType.trim().length > 8, | |||
There was a problem hiding this comment.
Consider using a constant for the maximum length value (8) to improve readability and maintainability of the code. This will make it easier to update the value in the future if needed.
| @@ -600,6 +605,7 @@ const CopilotRequestForm: FC<{}> = () => { | |||
| onChange={bind(handleFormValueChange, this, 'otherPaymentType')} | |||
| error={formErrors.otherPaymentType} | |||
| tabIndex={0} | |||
| maxLength={8} | |||
There was a problem hiding this comment.
The maxLength attribute is set to 8. Ensure this is the intended limit for the input field, as it might restrict user input more than necessary. Consider whether this constraint aligns with the expected data format and user requirements.
| .actionButtons { | ||
| display: flex; | ||
| gap: $sp-1; | ||
| align-items: center; | ||
| padding: $sp-2 0; | ||
| } | ||
|
|
||
| .title { | ||
| max-width: 200px; |
There was a problem hiding this comment.
The .title class has a max-width of 200px, which may conflict with the min-width of 200px set in the media query below. Consider revising these styles to ensure consistent behavior across different screen sizes.
| @@ -9,7 +9,7 @@ import { | |||
| ContentLayout, | |||
| IconCheck, | |||
| IconSolid, | |||
| LoadingSpinner, | |||
| LoadingCircles, | |||
There was a problem hiding this comment.
The component LoadingSpinner has been replaced with LoadingCircles. Ensure that LoadingCircles is correctly imported and used throughout the application, and verify that it provides the intended functionality and appearance as LoadingSpinner did.
| @@ -18,11 +18,11 @@ import { | |||
| } from '~/libs/ui' | |||
| import { profileContext, ProfileContextData, UserRole } from '~/libs/core' | |||
| import { EnvironmentConfig } from '~/config' | |||
| import { Sort } from '~/apps/admin/src/platform/gamification-admin/src/game-lib' | |||
There was a problem hiding this comment.
The import statement for Sort from ~/apps/admin/src/platform/gamification-admin/src/game-lib is added, but it is not used in this file. Consider removing it if it's unnecessary to avoid unused imports.
| @@ -18,11 +18,11 @@ import { | |||
| } from '~/libs/ui' | |||
| import { profileContext, ProfileContextData, UserRole } from '~/libs/core' | |||
| import { EnvironmentConfig } from '~/config' | |||
| import { Sort } from '~/apps/admin/src/platform/gamification-admin/src/game-lib' | |||
|
|
|||
| import { ProjectTypeLabels } from '../../constants' | |||
| import { approveCopilotRequest, CopilotRequestsResponse, useCopilotRequests } from '../../services/copilot-requests' | |||
| import { CopilotRequest } from '../../models/CopilotRequest' | |||
There was a problem hiding this comment.
The import statement for ProjectsResponse has been removed, but ensure that it is not used elsewhere in this file. If it is still needed, it should be re-added.
| setSize, | ||
| size, | ||
| page, | ||
| }: CopilotRequestsResponse = useCopilotRequests(sort) |
There was a problem hiding this comment.
The variable page is introduced in the destructuring assignment from useCopilotRequests, but it is not used anywhere in the code. Consider removing it if it is not needed.
| @@ -178,7 +174,6 @@ const CopilotRequestsPage: FC = () => { | |||
| label: 'Project', | |||
| propertyName: 'projectName', | |||
| renderer: (copilotRequest: CopilotRequest) => { | |||
| const projectName = projectsMap[copilotRequest.projectId]?.name | |||
| const projectLink = ` | |||
There was a problem hiding this comment.
The variable projectName is declared but not used in the code. Consider removing it to clean up the code and avoid unnecessary declarations.
| @@ -190,7 +185,7 @@ const CopilotRequestsPage: FC = () => { | |||
| rel='noreferrer' | |||
| onClick={handleLinkClick} | |||
| > | |||
| {projectName} | |||
| {copilotRequest.project?.name} | |||
There was a problem hiding this comment.
Consider adding a null check for copilotRequest.project before accessing name to prevent potential runtime errors if project is undefined. For example: copilotRequest.project?.name ?? 'Default Project Name'.
| @@ -207,7 +202,7 @@ const CopilotRequestsPage: FC = () => { | |||
| }, | |||
| { | |||
| label: 'Type', | |||
| propertyName: 'type', | |||
| propertyName: 'projectType', | |||
There was a problem hiding this comment.
The property name has been changed from type to projectType. Ensure that all references to this property in the codebase are updated accordingly to prevent any runtime errors or unexpected behavior.
| projectName: projectsMap[request.projectId]?.name, | ||
| type: ProjectTypeLabels[request.projectType] ?? '', | ||
| })), [projectsMap, requests]) | ||
| projectName: request.project?.name, |
There was a problem hiding this comment.
The projectName property is now directly accessing request.project?.name. Ensure that request.project is always defined or handle cases where it might be undefined to avoid potential runtime errors.
| projectType: ProjectTypeLabels[request.projectType] ?? '', | ||
| })), [requests]) | ||
|
|
||
| function loadMore(): void { |
There was a problem hiding this comment.
Consider adding a debounce or throttle mechanism to the loadMore function if it is triggered by a user action like scrolling or clicking, to prevent excessive calls that might lead to performance issues.
| setSize(size + 1) | ||
| } | ||
|
|
||
| function onToggleSort(s: Sort): void { |
There was a problem hiding this comment.
The onToggleSort function directly sets the sort state. Ensure that this function is only called with valid Sort objects to prevent unexpected behavior.
| /> | ||
| )} | ||
| <Table | ||
| className={(page === 1 && tableData.length < 10) ? styles.shortPage : styles.tableWrapper} |
There was a problem hiding this comment.
Consider renaming the styles.shortPage and styles.tableWrapper to more descriptive names that clearly indicate their purpose or the conditions they represent.
| className={(page === 1 && tableData.length < 10) ? styles.shortPage : styles.tableWrapper} | ||
| columns={tableColumns} | ||
| data={tableData} | ||
| moreToLoad={hasMoreCopilotRequests} |
There was a problem hiding this comment.
The moreToLoad prop is used here, but ensure that its logic is correctly implemented and tested to handle cases where there are no more items to load.
| onLoadMoreClick={loadMore} | ||
| onToggleSort={onToggleSort} | ||
| /> | ||
| {requestsLoading && <LoadingCircles /> } |
There was a problem hiding this comment.
The requestsLoading condition is used to display LoadingCircles. Verify that this condition accurately reflects the loading state and that LoadingCircles is the appropriate component for this scenario.
| {viewRequestDetails && ( | ||
| <CopilotRequestModal | ||
| request={viewRequestDetails} | ||
| project={projectsMap[viewRequestDetails.projectId]} | ||
| project={viewRequestDetails.project as Project} |
There was a problem hiding this comment.
Ensure that the type casting viewRequestDetails.project as Project is safe and that viewRequestDetails.project is always of type Project. Consider adding type checks or validation if necessary.
|
|
||
| import { EnvironmentConfig } from '~/config' | ||
| import { xhrGetAsync, xhrPatchAsync, xhrPostAsync } from '~/libs/core' | ||
| import { buildUrl } from '~/libs/shared/lib/utils/url' | ||
| import { Sort } from '~/apps/admin/src/platform/gamification-admin/src/game-lib' |
There was a problem hiding this comment.
The import path for Sort seems to be quite long and specific. Consider checking if this import path can be simplified or if the module can be restructured to make it more intuitive.
|
|
||
| import { EnvironmentConfig } from '~/config' | ||
| import { xhrGetAsync, xhrPatchAsync, xhrPostAsync } from '~/libs/core' | ||
| import { buildUrl } from '~/libs/shared/lib/utils/url' | ||
| import { Sort } from '~/apps/admin/src/platform/gamification-admin/src/game-lib' | ||
| import { getPaginatedAsync, PaginatedResponse } from '~/libs/core/lib/xhr/xhr-functions/xhr.functions' |
There was a problem hiding this comment.
The import path for getPaginatedAsync and PaginatedResponse is lengthy and deeply nested. It might be beneficial to review the module structure to see if it can be simplified for better maintainability.
| @@ -27,24 +31,61 @@ function copilotRequestFactory(data: any): CopilotRequest { | |||
| } | |||
| } | |||
|
|
|||
| export type CopilotRequestsResponse = SWRResponse<CopilotRequest[], CopilotRequest[]> | |||
| export type CopilotRequestsResponse = { | |||
There was a problem hiding this comment.
The type CopilotRequestsResponse has been changed from SWRResponse<CopilotRequest[], CopilotRequest[]> to a custom object. Ensure that all usages of CopilotRequestsResponse throughout the codebase are updated to accommodate this new structure, as it may affect how the response is handled.
| if (previousPageData && previousPageData.length < PAGE_SIZE) return undefined | ||
| const url = buildUrl(`${baseUrl}${projectId ? `/${projectId}` : ''}/copilots/requests`) | ||
| return ` | ||
| ${url}?page=${pageIndex + 1}&pageSize=${PAGE_SIZE}&sort=${sort.fieldName} ${sort.direction} |
There was a problem hiding this comment.
The URL template string includes a newline character, which might lead to unexpected formatting issues. Consider removing the newline to ensure the URL is constructed correctly.
| revalidateOnFocus: false, | ||
| }) | ||
| const latestPage = data[data.length - 1] || {} | ||
| const copilotRequests = data.flatMap(page => page.data) | ||
| const hasMoreCopilotRequests = latestPage.page + 1 < latestPage.totalPages |
There was a problem hiding this comment.
The hasMoreCopilotRequests logic assumes that latestPage.page and latestPage.totalPages are always defined. Consider adding a check to ensure these properties exist to prevent potential runtime errors.
| @@ -29,6 +29,7 @@ const PaymentProviderTable: React.FC<PaymentMethodTableProps> = (props: PaymentM | |||
| <th className='body-ultra-small-bold'>CONNECTED PROVIDER</th> | |||
| <th className='body-ultra-small-bold'>PROVIDER ID</th> | |||
| <th className='body-ultra-small-bold'>STATUS</th> | |||
| {/* eslint-disable-next-line jsx-a11y/control-has-associated-label */} | |||
There was a problem hiding this comment.
Disabling ESLint rule jsx-a11y/control-has-associated-label without providing a clear reason might lead to accessibility issues. Consider ensuring that the table header has an associated label or provide a justification for disabling the rule.
| @@ -30,6 +30,7 @@ const TaxFormTable: React.FC<TaxFormTableProps> = (props: TaxFormTableProps) => | |||
| <th className='body-ultra-small-bold'>FORM</th> | |||
| <th className='body-ultra-small-bold'>DATE FILED</th> | |||
| <th className='body-ultra-small-bold'>STATUS</th> | |||
| {/* eslint-disable-next-line jsx-a11y/control-has-associated-label */} | |||
There was a problem hiding this comment.
Disabling the eslint rule jsx-a11y/control-has-associated-label may lead to accessibility issues. Consider providing an appropriate label for the control to ensure it is accessible to all users.
| AV_SCAN_SCORER_REVIEW_TYPE_ID: '68c5a381-c8ab-48af-92a7-7a869a4ee6c3', | ||
| AVSCAN_TOPIC: 'avscan.action.scan', | ||
| AWS_CLEAN_BUCKET: '', |
There was a problem hiding this comment.
The AWS_CLEAN_BUCKET variable is currently set to an empty string. Ensure that this is intentional and that the correct bucket name will be provided in the production environment.
| AVSCAN_TOPIC: 'avscan.action.scan', | ||
| AWS_CLEAN_BUCKET: '', | ||
| AWS_DMZ_BUCKET: 'topcoder-dev-submissions', | ||
| AWS_QUARANTINE_BUCKET: '', |
There was a problem hiding this comment.
The AWS_QUARANTINE_BUCKET variable is currently set to an empty string. Verify if this is intentional and that the appropriate bucket name will be configured in the production environment.
| AV_SCAN_SCORER_REVIEW_TYPE_ID: '55bbb17d-aac2-45a6-89c3-a8d102863d05', | ||
| AVSCAN_TOPIC: 'avscan.action.scan', | ||
| AWS_CLEAN_BUCKET: '', |
There was a problem hiding this comment.
The AWS_CLEAN_BUCKET variable is initialized with an empty string. If this is intentional, consider adding validation logic elsewhere to ensure it is set correctly before use.
| AVSCAN_TOPIC: 'avscan.action.scan', | ||
| AWS_CLEAN_BUCKET: '', | ||
| AWS_DMZ_BUCKET: 'topcoder-submissions', | ||
| AWS_QUARANTINE_BUCKET: '', |
There was a problem hiding this comment.
The AWS_QUARANTINE_BUCKET variable is initialized with an empty string. If this is intentional, consider adding validation logic elsewhere to ensure it is set correctly before use.
| onBlur={() => setStateHasFocus(false)} | ||
| onBlur={() => { | ||
| setStateHasFocus(false) | ||
| props.onBlur?.() |
There was a problem hiding this comment.
Consider checking if props.onBlur is a function before calling it to avoid potential runtime errors. You can use typeof props.onBlur === 'function' for this check.
| @@ -168,6 +169,7 @@ const InputSelectReact: FC<InputSelectReactProps> = props => { | |||
| backspaceRemovesValue | |||
| isDisabled={props.disabled} | |||
| filterOption={props.filterOption} | |||
| isLoading={props.isLoading} | |||
There was a problem hiding this comment.
Ensure that the isLoading prop is being passed correctly from the parent component and that it is a boolean. If this prop is optional, consider providing a default value to avoid potential issues.
