Skip to content

Topcoder Admin App - Misc Update 0505 #1070

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 8, 2025
Merged

Topcoder Admin App - Misc Update 0505 #1070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 23 additions & 27 deletions src/apps/admin/src/challenge-management/ChallengeManagementPage/ChallengeManagementPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
useState,
} from 'react'
import { useSearchParams } from 'react-router-dom'
import _ from 'lodash'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider importing only the specific lodash functions you need instead of the entire library to reduce bundle size and improve performance.


import { LoadingSpinner, PageDivider, PageTitle } from '~/libs/ui'
import { PaginatedResponse } from '~/libs/core'
Expand All @@ -36,6 +37,17 @@ import { useEventCallback } from '../../lib/hooks'

import styles from './ChallengeManagementPage.module.scss'

const defaultFilter: ChallengeFilterCriteria = {
challengeId: '',
legacyId: 0,
name: '',
page: 1,
perPage: 25,
status: ChallengeStatus.Active,
track: null!, // eslint-disable-line @typescript-eslint/no-non-null-assertion, unicorn/no-null
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using null! for track may lead to runtime errors if the value is accessed before being properly initialized. Consider using a safer default value or handling the case where track is null.

type: null!, // eslint-disable-line @typescript-eslint/no-non-null-assertion, unicorn/no-null
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using null! for type may lead to runtime errors if the value is accessed before being properly initialized. Consider using a safer default value or handling the case where type is null.

}

/**
* Challenge Management page.
*/
Expand All @@ -45,15 +57,9 @@ export const ChallengeManagementPage: FC = () => {
ChallengeFilterCriteria,
Dispatch<SetStateAction<ChallengeFilterCriteria>>,
] = useState<ChallengeFilterCriteria>({
challengeId: '',
legacyId: 0,
name: '',
page: 1,
perPage: 25,
status: ChallengeStatus.Active,
track: null!, // eslint-disable-line @typescript-eslint/no-non-null-assertion, unicorn/no-null
type: null!, // eslint-disable-line @typescript-eslint/no-non-null-assertion, unicorn/no-null
...defaultFilter,
})
const disableReset = useMemo(() => _.isEqual(filterCriteria, defaultFilter), [filterCriteria])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a dependency on defaultFilter to the useMemo hook to ensure that changes to defaultFilter are accounted for in the memoization process.

const [challenges, setChallenges]: [
Array<Challenge>,
Dispatch<SetStateAction<Array<Challenge>>>,
Expand Down Expand Up @@ -119,19 +125,6 @@ export const ChallengeManagementPage: FC = () => {
}
}, [pageChangeEvent]) // eslint-disable-line react-hooks/exhaustive-deps -- missing dependency: search

// Reset
const [resetEvent, setResetEvent] = useState(false)
useEffect(() => {
if (resetEvent) {
search()
setResetEvent(false)
}
}, [resetEvent]) // eslint-disable-line react-hooks/exhaustive-deps -- missing dependency: search

const handleReset = useEventCallback(() => {
previousPageChangeEvent.current = false
setResetEvent(true)
})
const handlePageChange = useEventCallback((page: number) => {
setFilterCriteria({ ...filterCriteria, page })
setPageChangeEvent(true)
Expand All @@ -149,12 +142,15 @@ export const ChallengeManagementPage: FC = () => {
onFilterCriteriaChange={setFilterCriteria}
onSearch={search}
disabled={searching || !filtersInited}
showResetButton={
previousPageChangeEvent.current
&& searched
&& challenges.length === 0
}
onReset={handleReset}
onReset={function onReset() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using an arrow function for onReset to maintain consistency with other inline functions in the component.

setFilterCriteria({
...defaultFilter,
})
setTimeout(() => {
search()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setTimeout function is used here without a delay argument. If the intention is to execute search() asynchronously, consider adding a comment explaining the reasoning or adjust the logic if a delay is needed.

})
}}
disableReset={disableReset}
/>
<PageDivider />
{searching && (
Expand Down
18 changes: 0 additions & 18 deletions ...ps/admin/src/lib/components/BillingAccountResourcesTable/BillingAccountResourcesTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,6 @@ export const BillingAccountResourcesTable: FC<Props> = (props: Props) => {
propertyName: 'name',
type: 'text',
},
{
label: 'Status',
propertyName: 'status',
type: 'text',
},
{
className: styles.blockColumnAction,
label: '',
Expand Down Expand Up @@ -81,21 +76,8 @@ export const BillingAccountResourcesTable: FC<Props> = (props: Props) => {
},
],
[
{
label: 'Status label',
mobileType: 'label',
propertyName: 'status',
renderer: () => <div>Status:</div>,
type: 'element',
},
{
...columns[1],
mobileType: 'last-value',
},
],
[
{
...columns[2],
colSpan: 2,
mobileType: 'last-value',
},
Expand Down
2 changes: 1 addition & 1 deletion src/apps/admin/src/lib/components/BillingAccountsFilter/BillingAccountsFilter.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
display: flex;
justify-content: flex-end;
align-items: flex-start;
gap: 30px;
gap: 15px;
flex-wrap: wrap;

@include ltemd {
Expand Down
29 changes: 25 additions & 4 deletions src/apps/admin/src/lib/components/BillingAccountsFilter/BillingAccountsFilter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,22 +27,30 @@ interface Props {
onSubmitForm?: (data: FormBillingAccountsFilter) => void
}

const defaultValues: FormBillingAccountsFilter = {
endDate: undefined,
name: '',
startDate: undefined,
status: '1',
user: '',
}

export const BillingAccountsFilter: FC<Props> = (props: Props) => {
const maxDate = useMemo(() => moment()
.add(20, 'y')
.toDate(), [])
const {
register,
reset,
handleSubmit,
control,
formState: { isValid },
formState: { isValid, isDirty },
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addition of isDirty to formState is a good change for tracking form modifications, but ensure that its usage is properly integrated into the component logic to handle scenarios where the form's dirty state needs to be checked or acted upon.

}: UseFormReturn<FormBillingAccountsFilter> = useForm({
defaultValues: {
status: '1',
},
defaultValues,
mode: 'all',
resolver: yupResolver(formBillingAccountsFilterSchema),
})

const onSubmit = useCallback(
(data: FormBillingAccountsFilter) => {
props.onSubmitForm?.(data)
Expand Down Expand Up @@ -162,6 +170,19 @@ export const BillingAccountsFilter: FC<Props> = (props: Props) => {
>
Filter
</Button>
<Button
secondary
onClick={function onClick() {
reset(defaultValues)
setTimeout(() => {
onSubmit(defaultValues)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a delay duration to setTimeout for clarity, even if it's 0, to explicitly indicate the intended behavior.

})
}}
size='lg'
disabled={!isDirty}
>
Reset
</Button>
</div>
</form>
)
Expand Down
11 changes: 7 additions & 4 deletions src/apps/admin/src/lib/components/ChallengeFilters/ChallengeFilters.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,6 @@
gap: 15px 30px;
}

.searchButton {
margin-left: auto;
}

@media (max-width: #{$lg-max}) {
.filters {
grid-template-columns: 1fr 1fr;
Expand All @@ -32,3 +28,10 @@
}
}
}

.blockBtns {
margin-left: auto;
display: flex;
gap: 15px;
justify-content: flex-end;
}
12 changes: 4 additions & 8 deletions src/apps/admin/src/lib/components/ChallengeFilters/ChallengeFilters.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ import styles from './ChallengeFilters.module.scss'
interface ChallengeFiltersProps {
filterCriteria: ChallengeFilterCriteria
disabled: boolean
showResetButton: boolean
onFilterCriteriaChange: (newFilterCriteria: ChallengeFilterCriteria) => void
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The property showResetButton has been removed, but there is no indication of how the reset button visibility is now controlled. Ensure that the new disableReset property is correctly managing the reset button's functionality and visibility.

onSearch: () => void
onReset: () => void
disableReset: boolean
}

const ChallengeFilters: FC<ChallengeFiltersProps> = props => {
Expand Down Expand Up @@ -188,28 +188,24 @@ const ChallengeFilters: FC<ChallengeFiltersProps> = props => {
disabled={props.disabled}
/>
</div>
{!props.showResetButton && (
<div className={styles.blockBtns}>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional rendering logic for the showResetButton prop has been removed, which may affect the intended functionality. Consider reviewing whether this change is necessary and if it aligns with the desired behavior.

<Button
primary
className={styles.searchButton}
onClick={props.onSearch}
disabled={props.disabled}
size='lg'
>
Search
</Button>
)}
{props.showResetButton && (
<Button
secondary
className={styles.searchButton}
onClick={handleReset}
disabled={props.disabled}
disabled={props.disabled || props.disableReset}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The disabled prop for the Reset button now includes props.disableReset. Ensure that props.disableReset is correctly set and intended to be used in conjunction with props.disabled to control the button's disabled state.

size='lg'
>
Reset
</Button>
)}
</div>
</div>
)
}
Expand Down
8 changes: 7 additions & 1 deletion src/apps/admin/src/lib/components/ChallengeList/ChallengeList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,13 @@ const ChallengeList: FC<ChallengeListProps> = props => {
propertyName: 'name',
renderer: (challenge: Challenge) => (
// eslint-disable-next-line jsx-a11y/anchor-is-valid
<a href='#' className={styles.challengeTitle}>
<a
href={`${EnvironmentConfig.ADMIN.CHALLENGE_URL}/${challenge.id}`}
className={styles.challengeTitle}
onClick={function onClick() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The onClick handler is redundant because the href attribute already navigates to the desired URL. Consider removing the onClick function to avoid unnecessary duplication.

window.location.href = `${EnvironmentConfig.ADMIN.CHALLENGE_URL}/${challenge.id}`
}}
>
{challenge.name}
</a>
),
Expand Down
2 changes: 1 addition & 1 deletion src/apps/admin/src/lib/components/ClientsFilter/ClientsFilter.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
display: flex;
justify-content: flex-end;
align-items: flex-start;
gap: 30px;
gap: 15px;
flex-wrap: wrap;

@include ltemd {
Expand Down
27 changes: 23 additions & 4 deletions src/apps/admin/src/lib/components/ClientsFilter/ClientsFilter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,19 +27,25 @@ interface Props {
onSubmitForm?: (data: FormClientsFilter) => void
}

const defaultValues: FormClientsFilter = {
endDate: undefined,
name: '',
startDate: undefined,
status: '1',
}

export const ClientsFilter: FC<Props> = (props: Props) => {
const maxDate = useMemo(() => moment()
.add(20, 'y')
.toDate(), [])
const {
register,
reset,
handleSubmit,
control,
formState: { isValid },
formState: { isValid, isDirty },
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addition of isDirty to formState is a good practice for tracking form changes, but ensure that its usage is consistent throughout the component to handle form reset or submission logic based on dirty state.

}: UseFormReturn<FormClientsFilter> = useForm({
defaultValues: {
status: '1',
},
defaultValues,
mode: 'all',
resolver: yupResolver(formClientsFilterSchema),
})
Expand Down Expand Up @@ -149,6 +155,19 @@ export const ClientsFilter: FC<Props> = (props: Props) => {
>
Filter
</Button>
<Button
secondary
onClick={function onClick() {
reset(defaultValues)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider providing a delay duration for setTimeout to ensure consistent behavior across different environments.

setTimeout(() => {
onSubmit(defaultValues)
})
}}
size='lg'
disabled={!isDirty}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that isDirty is correctly defined and updated to avoid potential issues with the disabled state of the button.

>
Reset
</Button>
</div>
</form>
)
Expand Down
5 changes: 4 additions & 1 deletion src/apps/admin/src/lib/components/ReviewSummaryList/ReviewSummaryList.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,18 @@
}
}

.challengeTitle {
.challengeTitleText,
.challengeTitleLink {
min-width: 200px;
padding: 0;
justify-content: flex-start;
border-radius: 0;
color: $body-color;
line-height: 16px;
white-space: break-spaces;
}

.challengeTitleLink {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new class .challengeTitleLink is defined but lacks any base styles. Consider adding base styles to ensure consistent styling across different browsers and contexts.

&:hover {
color: $blue-110;
}
Expand Down
11 changes: 8 additions & 3 deletions src/apps/admin/src/lib/components/ReviewSummaryList/ReviewSummaryList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { FC, useMemo } from 'react'
import { useNavigate } from 'react-router-dom'

import { EnvironmentConfig } from '~/config'
import { useWindowSize, WindowSize } from '~/libs/shared'
import { Button, LinkButton, Table, type TableColumn } from '~/libs/ui'
import { Sort } from '~/apps/gamification-admin/src/game-lib/pagination'
Expand Down Expand Up @@ -45,13 +46,17 @@ const ChallengeTitle: FC<{
review: ReviewSummary
}> = props => {
const goToChallenge = useEventCallback(() => {
window.location.href = `https://www.topcoder.com/challenges/${props.review.legacyChallengeId}`
window.location.href = `${EnvironmentConfig.ADMIN.CHALLENGE_URL}/${props.review.legacyChallengeId}`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a null or undefined check for EnvironmentConfig.ADMIN.CHALLENGE_URL to ensure it is defined before using it in the URL construction.

})

return (
<LinkButton onClick={goToChallenge} className={styles.challengeTitle}>
return props.review.legacyChallengeId ? (
<LinkButton onClick={goToChallenge} className={styles.challengeTitleLink}>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class name challengeTitleLink should be verified to ensure it is defined in the styles object. If it is a new class, make sure it is added to the corresponding CSS module.

{props.review.challengeName}
</LinkButton>
) : (
<span className={styles.challengeTitleText}>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class name challengeTitleText should be verified to ensure it is defined in the styles object. If it is a new class, make sure it is added to the corresponding CSS module.

{props.review.challengeName}
</span>
)
}

Expand Down
6 changes: 6 additions & 0 deletions src/apps/admin/src/lib/components/UsersFilters/UsersFilters.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,9 @@
font-size: 14px;
color: $black-60;
}

.blockBtns {
display: flex;
gap: 15px;
justify-content: flex-end;
}
Loading