Topcoder Admin App - Misc Update 0610 #1105
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -7,6 +7,7 @@ import { | |||
| useMemo, | |||
| useState, | |||
| } from 'react' | |||
| import _ from 'lodash' | |||
There was a problem hiding this comment.
The lodash library is imported but not used in the current code. Consider removing the import statement if lodash functions are not utilized in this file to keep the code clean and avoid unnecessary dependencies.
| @@ -75,7 +76,7 @@ export const ChallengeManagementContextProvider: FC<PropsWithChildren> = props = | |||
| setResourceRolesLoading(true) | |||
| getResourceRoles() | |||
| .then(roles => { | |||
| setResourceRoles(roles) | |||
| setResourceRoles(_.orderBy(roles, ['name'])) | |||
There was a problem hiding this comment.
Consider specifying the order direction explicitly in the _.orderBy function to avoid any ambiguity. For example, use _.orderBy(roles, ['name'], ['asc']) to ensure the roles are sorted in ascending order by name.
| @@ -87,6 +90,7 @@ const reducer = ( | |||
| case GroupsActionType.ADD_GROUPS_DONE: { | |||
| return { | |||
| ...previousState, | |||
| groups: [action.payload, ...previousState.groups], | |||
There was a problem hiding this comment.
Consider checking if action.payload is not null or undefined before adding it to the groups array to prevent potential runtime errors.
| function handleSuccess(group: UserGroup): void { | ||
| if (group.createdBy && !group.createdByHandle) { | ||
| loadUser(group.createdBy) | ||
| } else if (!group.createdByHandle) { |
There was a problem hiding this comment.
Consider handling the case where loadUser fails to load the user. This could lead to createdByHandle remaining empty if the user data is not loaded successfully.
| group.createdByHandle = '' | ||
| } | ||
|
|
||
| if (group.updatedBy && !group.updatedByHandle) { |
There was a problem hiding this comment.
Similarly, consider handling the case where loadUser fails to load the user for updatedBy. This could lead to updatedByHandle remaining empty if the user data is not loaded successfully.
| doFetchGroups() | ||
| }, 1000) // sometimes the backend does not return the new data | ||
| // so I added a 1 second timeout for this | ||
| .then((group: UserGroup) => { |
There was a problem hiding this comment.
The previous implementation included a setTimeout to delay the execution of doFetchGroups() to ensure the backend data was updated. Ensure that the removal of this delay does not cause issues with stale data being fetched if the backend update is not immediate.
| @@ -194,7 +207,7 @@ export function useManagePermissionGroups( | |||
| handleError(e) | |||
| }) | |||
| }, | |||
| [dispatch, doFetchGroups], | |||
| [dispatch, loadUser], | |||
There was a problem hiding this comment.
The dependency array for the useCallback hook has been changed from [dispatch, doFetchGroups] to [dispatch, loadUser]. Ensure that loadUser is the correct dependency here and that doFetchGroups is no longer needed. If doFetchGroups is still required for the callback logic, it should be included in the dependency array.
| @@ -23,7 +23,7 @@ import { useOnComponentDidMount } from './useOnComponentDidMount' | |||
|
|
|||
| // used to get all groups | |||
| const PAGE = 1 | |||
| const PER_PAGE = 1000 | |||
| const PER_PAGE = 10000 | |||
There was a problem hiding this comment.
Increasing PER_PAGE from 1000 to 10000 could lead to performance issues or API rate limits if the data set is large. Consider whether this change is necessary and if there are any potential impacts on the system's performance or API usage.
| @@ -53,15 +53,20 @@ export interface Submission { | |||
| export function recalculateSubmissionRank( | |||
| memberSubmissions: MemberSubmission[], | |||
| ): MemberSubmission[] { | |||
| _.each(memberSubmissions, memberSubmission => { | |||
| const validMemberSubmissions: MemberSubmission[] = _.filter( | |||
There was a problem hiding this comment.
Consider using Array.prototype.filter instead of _.filter for better readability and to reduce dependency on lodash.
| memberSubmission => !!memberSubmission.submissions | ||
| && !!memberSubmission.submissions.length, | ||
| ) | ||
| _.each(validMemberSubmissions, memberSubmission => { |
There was a problem hiding this comment.
Consider using Array.prototype.forEach instead of _.each for better readability and to reduce dependency on lodash.
| memberSubmission.submissions = memberSubmission.submissions.map(adjustSubmissionResponse) | ||
| }) | ||
|
|
||
| const { submissions: finalSubmissions, maxFinalScore }: { | ||
| maxFinalScore: number; | ||
| submissions: MemberSubmission[]; | ||
| } | ||
| = processRanks(memberSubmissions) | ||
| = processRanks(validMemberSubmissions) |
There was a problem hiding this comment.
The variable validMemberSubmissions is used here, but there is no context in the diff about its definition or initialization. Ensure that validMemberSubmissions is correctly defined and initialized before being passed to processRanks. If this is a new variable introduced in this PR, verify its correctness and that it replaces memberSubmissions appropriately.
| @@ -1,6 +1,7 @@ | |||
| /** | |||
| * Groups service | |||
| */ | |||
| import _ from 'lodash' | |||
There was a problem hiding this comment.
The lodash library is imported but not used in this file. Consider removing the import statement if lodash is not needed to avoid unnecessary dependencies.
| @@ -80,7 +81,7 @@ export const fetchGroups = async (params: { | |||
| const result = await xhrGetAsync<UserGroup[]>( | |||
| `${EnvironmentConfig.API.V5}/groups?${qs.stringify(params)}`, | |||
| ) | |||
| return result.map(adjustUserGroupResponse) | |||
| return _.orderBy(result.map(adjustUserGroupResponse), ['createdAt'], ['desc']) | |||
There was a problem hiding this comment.
Consider importing orderBy directly from 'lodash' instead of using the _ namespace to improve tree-shaking and reduce bundle size. For example: import orderBy from 'lodash/orderBy';
| } | ||
|
|
||
| if (!result.updatedBy) { | ||
| result.updatedBy = '00000000' |
There was a problem hiding this comment.
The hardcoded value '00000000' for 'updatedBy' might not be meaningful or appropriate. Consider using a more descriptive placeholder or handling this case differently to avoid confusion.
|
|
||
| if (!result.updatedBy) { | ||
| result.updatedBy = '00000000' | ||
| result.updatedByHandle = '00000000 (not found)' |
There was a problem hiding this comment.
Similarly, the hardcoded value '00000000 (not found)' for 'updatedByHandle' may not be informative. It might be better to use a more descriptive placeholder or handle this scenario in a way that provides more context.
https://www.topcoder.com/challenges/9fa61255-3a49-4301-8077-31ef7364031a