Conversation
PM-3731 Show roles for design ch in resources tab
What was broken - The Reviewer control was missing in the challenge editor, so V6 task reviewers could not be selected in WM. - Internal review submissions no longer enforced selecting a reviewer. Root cause - The ReviewType field component was reduced to a null stub, and related wiring/validation in ChallengeEditor was removed. What was changed - Restored the ReviewType field UI with reviewer radio/select behavior and required inline error state. - Reconnected reviewer options and selection callback wiring from ChallengeEditor to ReviewTypeField. - Restored validation that blocks saving when internal review has no selected reviewer. Any added/updated tests - Added ReviewTypeField tests to verify task reviewer control rendering and required validation messaging.
This reverts commit 1bd68aa.
…scorecard phase change and selection.
PM-3879 Fix typo
Projects api v6
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| CHALLENGE_TIMELINES_URL: `${DEV_API_HOSTNAME}/v6/challenge-timelines`, | ||
| COPILOTS_URL: 'https://copilots.topcoder-dev.com', | ||
| PROJECT_API_URL: `${DEV_API_HOSTNAME}/v5/projects`, | ||
| PROJECT_API_URL: `${DEV_API_HOSTNAME}/v6/projects`, |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that all dependencies and integrations using the PROJECT_API_URL are compatible with the new API version v6. This change may affect other parts of the system relying on the previous v5 endpoint.
| const LOCAL_RESOURCE_API = 'http://localhost:3004/v6' | ||
| const LOCAL_REVIEW_API = 'http://localhost:3005/v6' | ||
| const LOCAL_SKILLS_API_V5 = 'http://localhost:3006/v5/standardized-skills' | ||
| const LOCAL_PROJECTS_API = 'http://localhost:3008/v6/projects' |
There was a problem hiding this comment.
[security]
Consider using HTTPS instead of HTTP for LOCAL_PROJECTS_API to ensure secure communication, even in local development environments. This can help catch potential issues early and align with production security standards.
| CHALLENGE_TIMELINES_URL: `${PROD_API_HOSTNAME}/v6/challenge-timelines`, | ||
| COPILOTS_URL: `https://copilots.${DOMAIN}`, | ||
| PROJECT_API_URL: `${PROD_API_HOSTNAME}/v5/projects`, | ||
| PROJECT_API_URL: `${PROD_API_HOSTNAME}/v6/projects`, |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that all dependent services and clients are updated to use the new v6 endpoint for projects. This change could break integrations if they are still expecting the v5 endpoint.
| */ | ||
| export function loadEngagements (projectId, status = 'all', filterName = '', includePrivate = false) { | ||
| export function loadEngagements (projectId, status = 'all', filterName = '', includePrivate = false, projectIds = []) { | ||
| const hasProjectIdsArg = arguments.length >= 5 |
There was a problem hiding this comment.
[maintainability]
The projectIds parameter is added to the loadEngagements function, but its usage is conditional on arguments.length >= 5. This check is redundant since projectIds has a default value of an empty array. Consider removing the hasProjectIdsArg check and directly using projectIds to simplify the logic.
| export function loadEngagements (projectId, status = 'all', filterName = '', includePrivate = false, projectIds = []) { | ||
| const hasProjectIdsArg = arguments.length >= 5 | ||
| return async (dispatch) => { | ||
| if (hasProjectIdsArg && Array.isArray(projectIds) && !projectIds.length) { |
There was a problem hiding this comment.
[💡 readability]
The check for Array.isArray(projectIds) && !projectIds.length is redundant because projectIds is initialized as an empty array by default. Consider simplifying this condition to just !projectIds.length.
| if (application.status === 'SELECTED') { | ||
| return | ||
| } | ||
| const applicationUserId = application.userId || application.user_id || application.memberId || application.member_id |
There was a problem hiding this comment.
[correctness]
The applicationUserId is determined using multiple properties (userId, user_id, memberId, member_id). Ensure that these properties are consistently available and correctly populated in the application objects to avoid potential issues with member identification.
| const isExistingAssignedMember = applicationUserId != null && countableAssignmentMemberIds.has(String(applicationUserId)) | ||
| const isAtCapacity = hasRequiredMemberCount && assignedMemberCount >= requiredMemberCountValue | ||
|
|
||
| if (isAtCapacity && !isExistingAssignedMember) { |
There was a problem hiding this comment.
[correctness]
The condition isAtCapacity && !isExistingAssignedMember could lead to unexpected behavior if isAtCapacity is true but isExistingAssignedMember is not correctly evaluated. Consider verifying the logic to ensure it aligns with the intended behavior when handling capacity errors.
| } | ||
| onUpdateStatus(application.id, option.value) | ||
| try { | ||
| await onUpdateStatus(application.id, option.value) |
There was a problem hiding this comment.
[maintainability]
The onUpdateStatus function is called within a try-catch block, but the catch block only contains a comment. Consider logging the error or handling it appropriately to ensure any issues during status updates are not silently ignored.
| @@ -26,6 +26,7 @@ const ModalAddLink = ({ | |||
| classsName, | |||
There was a problem hiding this comment.
[❗❗ correctness]
Typo in classsName. It should be className to avoid potential issues with class assignment.
| @@ -26,12 +26,12 @@ const ModalAttachmentOptions = ({ | |||
| classsName, | |||
There was a problem hiding this comment.
[❗❗ correctness]
The prop classsName seems to have a typo. It should likely be className. This could lead to confusion or bugs if the incorrect prop is used elsewhere.
| toastr.success('Success', 'Updated file successfully.') | ||
| setIsProcessing(false) | ||
| updateAttachment(result) | ||
| onSaved() |
There was a problem hiding this comment.
[correctness]
Consider checking if onSaved is a function before calling it to prevent potential runtime errors if onSaved is not passed or is not a function.
| if (count === 0) { | ||
| setIsProcessing(false) | ||
| if (hasSuccessfulUpload) { | ||
| onSaved() |
There was a problem hiding this comment.
[correctness]
Consider checking if onSaved is a function before calling it to prevent potential runtime errors if onSaved is not passed or is not a function.
| allowedUsers | ||
| }) | ||
| .then(result => { | ||
| hasSuccessfulUpload = true |
There was a problem hiding this comment.
[correctness]
Consider checking if onSaved is a function before calling it to prevent potential runtime errors if onSaved is not passed or is not a function.
| import cn from 'classnames' | ||
| import { getProjectMemberByUserId } from '../../../util/tc' | ||
|
|
||
| const ProjectMembers = ({ classsName, members, allowedUsers, maxShownNum }) => { |
There was a problem hiding this comment.
[correctness]
The prop name classsName seems to have a typo with an extra 's'. Consider renaming it to className for consistency and to avoid potential confusion.
| allowedUsers: PropTypes.arrayOf( | ||
| PropTypes.oneOfType([PropTypes.string, PropTypes.number]) | ||
| ), | ||
| members: PropTypes.arrayOf(PropTypes.shape()) |
There was a problem hiding this comment.
[maintainability]
The members prop type is defined as PropTypes.arrayOf(PropTypes.shape()), which does not specify the shape of the objects within the array. Consider defining the expected shape to improve type checking and maintainability.
|
|
||
| jest.mock('../../../util/tc', () => ({ | ||
| getProjectMemberByUserId: (projectMembers, userId) => ( | ||
| (projectMembers || []).find(member => `${member.userId}` === `${userId}`) || null |
There was a problem hiding this comment.
[correctness]
Using template literals for comparison may introduce subtle bugs if userId or member.userId are not strings. Consider ensuring both are strings before comparison to avoid unexpected behavior.
| @@ -0,0 +1,160 @@ | |||
| /* eslint-disable react/prop-types */ | |||
There was a problem hiding this comment.
[maintainability]
Disabling react/prop-types globally for the entire file may hide potential issues with prop validation. Consider enabling it and explicitly disabling it only where necessary.
| checkAdminOrCopilotOrManager: jest.fn(), | ||
| checkCanViewProjectAssets: jest.fn(), | ||
| checkManager: jest.fn(), | ||
| getProjectMemberByUserId: jest.fn((project, userId) => { |
There was a problem hiding this comment.
[💡 correctness]
The getProjectMemberByUserId function uses string interpolation for comparison, which might be unnecessary if userId is always a string. Consider ensuring userId is consistently typed to avoid potential issues.
|
|
||
| const renderComponent = (props = {}) => { | ||
| act(() => { | ||
| ReactDOM.render( |
There was a problem hiding this comment.
[correctness]
The act function should wrap the entire rendering process, including any state updates or asynchronous operations that might affect the component. Ensure all state changes are wrapped in act to avoid warnings in tests.
| value: engagement.status | ||
| } : null} | ||
| options={['Open', 'Active', 'Cancelled', 'Closed'].map(status => ({ | ||
| options={['Open', 'Active', 'On Hold', 'Cancelled', 'Closed'].map(status => ({ |
There was a problem hiding this comment.
[correctness]
Consider adding a default case for the status options to handle unexpected values gracefully. This can prevent potential issues if the status is set to a value not included in the predefined options.
| loadOptions={loadParentProjectOptions} | ||
| placeholder='Type at least 2 characters to search projects...' | ||
| noOptionsMessage={({ inputValue }) => { | ||
| return inputValue && inputValue.trim().length >= 2 |
There was a problem hiding this comment.
[💡 readability]
The noOptionsMessage function could be simplified by using a ternary operator directly in the return statement. This would improve readability by reducing the number of lines and making the logic more concise.
| onUpdateInput({ | ||
| target: { | ||
| name: 'projectId', | ||
| value: option ? String(option.value) : null |
There was a problem hiding this comment.
[correctness]
Ensure that option.value is always a valid string before calling String(option.value). Consider adding a check or default value to handle cases where option might be null or undefined.
| onUpdateInput({ | ||
| target: { | ||
| name: 'projectName', | ||
| value: option ? option.label : null |
There was a problem hiding this comment.
[correctness]
Ensure that option.label is always a valid string before assigning it to projectName. Consider adding a check or default value to handle cases where option might be null or undefined.
| align-items: center; | ||
| gap: 8px; | ||
| flex-wrap: wrap; | ||
| flex: 0 0 auto; |
There was a problem hiding this comment.
[design]
The addition of flex: 0 0 auto; to .memberActions ensures that the flex item does not grow or shrink, which might be intended for layout stability. However, consider if this could lead to layout issues on smaller screens where space is constrained.
| padding: 6px 12px; | ||
| font-size: 12px; | ||
| font-weight: 600; | ||
| white-space: nowrap; |
There was a problem hiding this comment.
[design]
Adding white-space: nowrap; to .toggleButton prevents text from wrapping, which is useful for maintaining button integrity. Ensure this does not cause overflow issues on smaller screens.
| display: flex; | ||
| flex-direction: column; | ||
| gap: 4px; | ||
| flex: 1; |
There was a problem hiding this comment.
[design]
The addition of flex: 1; to .memberInfo allows it to grow and fill available space, which is generally good for responsive design. Ensure this does not conflict with other flex items in the same container.
| flex-direction: column; | ||
| gap: 4px; | ||
| flex: 1; | ||
| min-width: 0; |
There was a problem hiding this comment.
[💡 design]
Adding min-width: 0; to .memberInfo is a common practice to prevent flex items from overflowing their container. This is a good addition for maintaining layout integrity.
|
|
||
| .memberMetaValue { | ||
| line-height: 1.4; | ||
| word-break: break-word; |
There was a problem hiding this comment.
[💡 design]
The use of word-break: break-word; in .memberMetaValue ensures long words break and do not overflow their container. This is important for maintaining layout consistency, especially with dynamic content.
| const normalizedAssignmentStatus = normalizeAssignmentStatus(assignmentStatusRaw) | ||
| const assignmentStatusLabel = normalizedAssignmentStatus || 'Unknown' | ||
| const isAssignedStatus = normalizedAssignmentStatus.toLowerCase() === 'assigned' | ||
| const assignmentStatusLower = normalizedAssignmentStatus.toLowerCase() |
There was a problem hiding this comment.
[maintainability]
The variable assignmentStatusLower is used multiple times in the following lines. Consider using a function to encapsulate the logic of converting normalizedAssignmentStatus to lowercase to avoid repetition and potential errors if the logic needs to change.
| <span className={styles.memberMetaValue}>{endDate}</span> | ||
| </div> | ||
| </div> | ||
| {assignmentStatusLower === 'terminated' && member.terminationReason && ( |
There was a problem hiding this comment.
[maintainability]
The condition assignmentStatusLower === 'terminated' is repeated in multiple places. Consider extracting this logic into a function like isTerminatedStatus() to improve readability and maintainability.
| @@ -23,7 +25,6 @@ const VISIBILITY_OPTIONS = [ | |||
| { label: 'Private', value: 'private' } | |||
| ] | |||
There was a problem hiding this comment.
[correctness]
The removal of DEFAULT_STATUS_OPTION may lead to unexpected behavior if any part of the code relies on it being set to 'Open' by default. Ensure that this change is intentional and that all dependent logic is updated accordingly.
| } | ||
|
|
||
| const handleDeleteConfirm = async () => { | ||
| if (!deleteTarget || !deleteTarget.id) { |
There was a problem hiding this comment.
[usability]
Consider adding a loading indicator or disabling the delete button while isDeleting is true to prevent multiple delete requests being sent if the user clicks the confirm button multiple times.
| setDeleteError('') | ||
|
|
||
| try { | ||
| await deleteEngagement(deleteTarget.id, deleteTarget.projectId) |
There was a problem hiding this comment.
[maintainability]
Ensure that deleteEngagement handles errors gracefully and that any potential side effects are managed. Consider logging errors for monitoring purposes.
| errorMessage = error.message.trim() | ||
| } | ||
|
|
||
| const normalizedMessage = typeof errorMessage === 'string' ? errorMessage.toLowerCase() : '' |
There was a problem hiding this comment.
[💡 readability]
The error message normalization logic could be extracted into a utility function to improve readability and reusability.
| return () => React.createElement('div', null, 'Loading') | ||
| }) | ||
|
|
||
| jest.mock('../Modal/ConfirmationModal', () => null) |
There was a problem hiding this comment.
[correctness]
Mocking ConfirmationModal with null might cause issues if the component is expected to be rendered or interacted with in the tests. Consider providing a minimal mock implementation instead.
| 'data-testid': inputId, | ||
| value: value && value.value ? value.value : '', | ||
| onChange: (event) => { | ||
| const selectedOption = options.find((option) => option.value === event.target.value) |
There was a problem hiding this comment.
[💡 correctness]
The find method could return undefined if no matching option is found, which is handled by || null. Ensure that onChange can handle null values appropriately.
| import { getProjectMemberByUserId } from '../../util/tc' | ||
|
|
||
| function getMemberOptionLabel (member, fallbackUserId) { | ||
| const fallbackValue = _.isNil(fallbackUserId) ? '' : `${fallbackUserId}`.trim() |
There was a problem hiding this comment.
[💡 maintainability]
Using _.isNil to check for null or undefined is appropriate, but consider using String(fallbackUserId).trim() directly for simplicity and to avoid unnecessary lodash dependency.
| return _.uniqBy( | ||
| (projectMembers || []) | ||
| .map(member => { | ||
| if (_.isNil(member.userId) || `${member.userId}`.trim().length === 0) { |
There was a problem hiding this comment.
[💡 readability]
The check _.isNil(member.userId) || ${member.userId}.trim().length === 0 is correct, but could be simplified by using !String(member.userId).trim() to improve readability and reduce lodash dependency.
|
|
||
| jest.mock('../../util/tc', () => ({ | ||
| getProjectMemberByUserId: (projectMembers, userId) => ( | ||
| (projectMembers || []).find(member => `${member.userId}` === `${userId}`) || null |
There was a problem hiding this comment.
[correctness]
Using template literals for comparison in getProjectMemberByUserId can lead to unexpected behavior if userId or member.userId are not strings. Consider ensuring both are strings before comparison to avoid potential type coercion issues.
|
|
||
| const defaultProps = { | ||
| value: [], | ||
| onChangeValue: () => {}, |
There was a problem hiding this comment.
[💡 maintainability]
The onChangeValue function is defined as a no-op. If this is intentional for the test, consider explicitly stating its purpose or mocking it to ensure it behaves as expected during tests.
| <div className={styles.container}> | ||
| <Link | ||
| to={`/projects/${projectId}/${isInvited ? 'invitation' : 'challenges'}`} | ||
| to={`/projects/${projectId}`} |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of the isInvited parameter from the URL path changes the routing logic. Ensure that this change is intentional and that the new URL structure is compatible with the rest of the application, as it may affect navigation and access control.
| projectId: PT.number.isRequired, | ||
| projectName: PT.string.isRequired, | ||
| isInvited: PT.bool.isRequired, | ||
| selected: PT.bool |
There was a problem hiding this comment.
[maintainability]
The isInvited prop has been removed from propTypes, which is consistent with its removal from the component props. Verify that no other parts of the application rely on this prop, as its removal could lead to unexpected behavior elsewhere.
| container: styles.modalContainer | ||
| } | ||
|
|
||
| function normalizeDisplayValue (value) { |
There was a problem hiding this comment.
[💡 performance]
The function normalizeDisplayValue uses _.isNil to check for null or undefined, which is appropriate. However, consider adding a check for empty strings before trimming, as trimming an empty string is unnecessary and could be avoided for performance reasons, albeit minor.
| render () { | ||
| const { isInvite, user, onRemoveClick, isEditable } = this.props | ||
| const showRadioButtons = _.includes(_.values(PROJECT_ROLES), user.role) | ||
| const userDisplayName = normalizeDisplayValue(user.handle) || |
There was a problem hiding this comment.
[maintainability]
The logic for determining userDisplayName and inviteDisplayName is repeated. Consider refactoring this logic into a separate utility function to improve maintainability and reduce duplication.
| return '(unknown user)' | ||
| } | ||
|
|
||
| const displayName = [user.handle, user.email, user.userId] |
There was a problem hiding this comment.
[correctness]
Consider using ?? (nullish coalescing operator) instead of || to avoid treating falsy values like 0 as false. This ensures that only null or undefined are replaced with an empty string.
| export const ADMIN_ROLES = [ | ||
| 'administrator', | ||
| 'connect admin' | ||
| 'administrator' |
There was a problem hiding this comment.
[❗❗ security]
The removal of 'connect admin' from ADMIN_ROLES could have implications on role-based access control. Ensure that this change aligns with the intended access permissions and that any dependent logic or documentation is updated accordingly.
| await this.props.loadApplications(engagementId) | ||
| await this.props.loadEngagementDetails(projectId, engagementId) | ||
| } catch (error) { | ||
| throw error |
There was a problem hiding this comment.
[maintainability]
Consider logging the error before re-throwing it. This can help with debugging and monitoring, especially if the error is not caught higher up in the call stack.
| loadSubmissions, | ||
| projectDetail, | ||
| loggedInUser, | ||
| submissionsPerPage |
There was a problem hiding this comment.
[correctness]
The submissionsPerPage parameter is added to the fetchChallengeDetails method call. Ensure that this parameter is properly handled within the fetchChallengeDetails method to avoid potential issues with pagination or data retrieval.
| } = this.props | ||
| const { challengeTypes = [] } = metadata | ||
| const isActiveProjectLoaded = | ||
| reduxProjectInfo && `${reduxProjectInfo.id}` === `${activeProjectId}` |
There was a problem hiding this comment.
[correctness]
The use of template literals to compare reduxProjectInfo.id and activeProjectId is unnecessary and could potentially lead to unexpected behavior if either value is null or undefined. Consider using a strict equality check (===) after ensuring both values are of the same type.
|
|
||
| componentDidMount () { | ||
| async componentDidMount () { | ||
| const { match, loadEngagementDetails, loadProject } = this.props |
There was a problem hiding this comment.
[correctness]
The componentDidMount method is now asynchronous, which means it returns a promise. Ensure that any lifecycle methods or components relying on this method handle the promise appropriately to avoid potential issues with unhandled promise rejections.
| } | ||
|
|
||
| try { | ||
| const response = await fetchMemberProjects({ |
There was a problem hiding this comment.
[maintainability]
Consider adding error handling for the fetchMemberProjects call to provide feedback to the user in case of a failure, rather than silently returning an empty array.
|
|
||
| if ( | ||
| engagement.projectId && | ||
| `${engagement.projectId}` !== `${currentProjectId}` |
There was a problem hiding this comment.
[💡 performance]
The comparison of engagement.projectId and currentProjectId using string interpolation is unnecessary and could be simplified to a direct comparison, which would be more efficient.
| */ | ||
| canEditParentProject () { | ||
| const { auth } = this.props | ||
| return checkAdmin(auth.token) || checkTalentManager(auth.token) |
There was a problem hiding this comment.
[❗❗ security]
The canEditParentProject method relies on auth.token to determine permissions. Ensure that auth.token is always available and valid to prevent potential security issues.
| .map(projectId => `${projectId}`) | ||
| ) | ||
| } catch (error) { | ||
| return [] |
There was a problem hiding this comment.
[maintainability]
Catching all errors and returning an empty array might hide issues that should be logged or handled differently. Consider logging the error or handling specific error cases to avoid silent failures.
|
|
||
| while (hasMore) { | ||
| const response = await fetchMemberProjects({ memberOnly: true, page, perPage }) | ||
| const pageProjects = _.get(response, 'projects', []) |
There was a problem hiding this comment.
[correctness]
Using _.get to access deeply nested properties is safe, but consider checking if response is valid before accessing its properties to avoid potential runtime errors if fetchMemberProjects returns an unexpected result.
|
|
||
| useEffect(() => { | ||
| if (projectId) { | ||
| if (projectId && `${projectDetailId || ''}` !== `${projectId}`) { |
There was a problem hiding this comment.
[💡 readability]
The condition projectId && ${projectDetailId || ''}!==${projectId}`` could be simplified by directly comparing projectDetailId and `projectId` without converting them to strings. This would improve readability and avoid unnecessary string operations.
| loadOnlyProjectInfo(projectId) | ||
| } | ||
| }, [projectId]) | ||
| }, [loadOnlyProjectInfo, projectId, projectDetailId]) |
There was a problem hiding this comment.
[performance]
The dependency array for useEffect now includes loadOnlyProjectInfo, projectId, and projectDetailId. Ensure that loadOnlyProjectInfo is stable (i.e., does not change on every render) to prevent unnecessary re-renders.
| } from '../../actions/projects' | ||
| import { setActiveProject } from '../../actions/sidebar' | ||
| import { checkAdminOrCopilot, checkAdmin, checkIsUserInvitedToProject } from '../../util/tc' | ||
| import { checkAdminOrCopilotOrManager, checkAdmin, checkIsUserInvitedToProject, checkManager } from '../../util/tc' |
There was a problem hiding this comment.
[correctness]
The function checkAdminOrCopilotOrManager is being used in two different contexts: one with projectDetail and one without. Ensure that the function can handle both scenarios correctly, as the logic might differ when projectDetail is not provided.
| this.props.history.push('/projects') | ||
| // For create flow there is no project context yet, so only evaluate global JWT roles. | ||
| const canManageProject = isEdit | ||
| ? checkAdminOrCopilotOrManager(auth.token, projectDetail) |
There was a problem hiding this comment.
[correctness]
The checkAdminOrCopilotOrManager function is called with different parameters based on the isEdit flag. Ensure that this function is designed to handle both cases correctly, as the absence of projectDetail might lead to unexpected behavior.
| projectDetail, | ||
| token | ||
| }) => { | ||
| const projectId = _.get(match, 'params.projectId') |
There was a problem hiding this comment.
[💡 readability]
Using _.get for accessing match.params.projectId is unnecessary here since match is a required prop and should always have params. Consider using direct access: match.params.projectId.
| if ( | ||
| !resolvedProjectId || | ||
| isProjectLoading || | ||
| `${_.get(projectDetail, 'id', '')}` !== `${resolvedProjectId}` |
There was a problem hiding this comment.
[💡 readability]
The comparison ${_.get(projectDetail, 'id', '')} !== ${resolvedProjectId} uses template literals unnecessarily. Consider simplifying to _.get(projectDetail, 'id', '') !== resolvedProjectId.
| return | ||
| } | ||
|
|
||
| const destination = checkIsUserInvitedToProject(token, projectDetail) |
There was a problem hiding this comment.
[maintainability]
The function checkIsUserInvitedToProject is called with token and projectDetail, but it's unclear if token is necessary for this check. Ensure that token is required for the logic inside checkIsUserInvitedToProject to avoid passing unnecessary props.
| import PropTypes from 'prop-types' | ||
| import Loader from '../../components/Loader' | ||
| import { checkAdminOrCopilot, checkIsUserInvitedToProject, checkManager } from '../../util/tc' | ||
| import { checkAdminOrCopilotOrManager, checkManager } from '../../util/tc' |
There was a problem hiding this comment.
[❗❗ correctness]
The function checkAdminOrCopilotOrManager is used here, but it's unclear if it correctly replaces the functionality of checkAdminOrCopilot. Ensure that the new function includes all necessary checks and does not inadvertently change the intended behavior.
| <div className={styles.headerLine}> | ||
| <h2>Projects</h2> | ||
| {checkAdminOrCopilot(auth.token) && ( | ||
| {checkAdminOrCopilotOrManager(auth.token) && ( |
There was a problem hiding this comment.
[❗❗ correctness]
The change from checkAdminOrCopilot to checkAdminOrCopilotOrManager should be verified to ensure that managers are intended to have access to create new projects. Confirm that this change aligns with the business requirements.
| <li key={p.id}> | ||
| <ProjectCard | ||
| isInvited={!!checkIsUserInvitedToProject(auth.token, p)} | ||
| projectStatus={p.status} |
There was a problem hiding this comment.
[❗❗ correctness]
The isInvited prop was removed from ProjectCard. Ensure that this prop is not required for the component's functionality or visual representation, as its removal could lead to unintended side effects.
| const resolvedToken = token || currentToken | ||
| return !!resolvedToken && (checkAdmin(resolvedToken) || checkCopilot(resolvedToken)) | ||
| const resolvedProjectDetail = projectDetail || this.props.projectDetail | ||
| return checkCanViewProjectAssets(resolvedToken, resolvedProjectDetail) |
There was a problem hiding this comment.
[❗❗ correctness]
The function checkCanViewProjectAssets now relies on projectDetail. Ensure that projectDetail is always correctly populated and available in the component's props to avoid potential runtime errors.
| } | ||
| if (pathname === '/engagements') { | ||
| return isAdmin ? 3 : 0 | ||
| return canViewEngagements ? 3 : 0 |
There was a problem hiding this comment.
[❗❗ security]
The logic for determining the tab for /engagements has changed from checking isAdmin to canViewEngagements. Ensure that canViewEngagements correctly reflects the intended permissions, as this change could impact access control.
| this.props.unloadProjects() | ||
| this.setState({ currentTab: 2 }) | ||
| } else if (tab === 3 && isAdmin) { | ||
| } else if (tab === 3 && canViewEngagements) { |
There was a problem hiding this comment.
[❗❗ security]
The condition for navigating to the /engagements tab has changed from isAdmin to canViewEngagements. Verify that canViewEngagements accurately represents the intended user roles to prevent unauthorized access.
| fetchProjectById(projectId), | ||
| fetchProjectMembers(projectId), | ||
| getProjectMemberInvites(projectId) | ||
| ]).then(([project, projectMembers, invitedMembers]) => { |
There was a problem hiding this comment.
[correctness]
Consider adding error handling for the Promise.all call to manage potential rejections from any of the promises. This will improve the robustness of the loadProject method by preventing unhandled promise rejections.
| return { | ||
| ...state, | ||
| projectDetail: {}, | ||
| hasProjectAccess: initialState.hasProjectAccess |
There was a problem hiding this comment.
[💡 maintainability]
Resetting hasProjectAccess to initialState.hasProjectAccess might be misleading if the initial state changes in the future. Consider explicitly setting it to false to make the intention clear.
| } | ||
| } | ||
|
|
||
| componentWillMount () { |
There was a problem hiding this comment.
[❗❗ maintainability]
The use of componentWillMount is deprecated and can lead to unexpected behavior. Consider using componentDidMount or the constructor for initializing data.
| }) | ||
| } | ||
|
|
||
| componentDidUpdate (prevProps) { |
There was a problem hiding this comment.
[performance]
The componentDidUpdate method should include a condition to check if the relevant props have changed before calling resolveAssetsRouteAccess. This can prevent unnecessary calls and improve performance.
| } | ||
|
|
||
| this.setAssetsAccessStatus(projectId, 'loading') | ||
| this.props.loadOnlyProjectInfo(projectId) |
There was a problem hiding this comment.
[correctness]
Ensure that loadOnlyProjectInfo handles all potential errors gracefully, especially network-related issues, to avoid leaving the application in an inconsistent state.
| path='/projects/:projectId/assets' | ||
| render={({ match }) => { | ||
| const routeProjectId = _.get(match.params, 'projectId') | ||
| const isProjectDetailForRequestedProject = `${_.get(projectDetail, 'id', '')}` === `${routeProjectId}` |
There was a problem hiding this comment.
[💡 readability]
The logic for determining canViewRequestedProjectAssets is complex and could benefit from being extracted into a separate method for clarity and maintainability.
| */ | ||
| export async function fetchSubmissions (challengeId, pageObj) { | ||
| const { page, perPage } = pageObj | ||
| export async function fetchSubmissions (challengeId, pageObj = {}) { |
There was a problem hiding this comment.
[correctness]
Consider providing a default value for pageObj in the function signature to ensure that the destructuring of page and perPage does not throw an error if pageObj is null or undefined. While _.get handles undefined properties, the function signature should reflect the expected input type.
| export async function fetchSubmissions (challengeId, pageObj = {}) { | ||
| const page = _.get(pageObj, 'page', 1) | ||
| const perPage = _.get(pageObj, 'perPage', 10) | ||
| const response = await axiosInstance.get(`${SUBMISSIONS_API_URL}?challengeId=${challengeId}&perPage=${perPage}&page=${page}`) |
There was a problem hiding this comment.
[security]
The URL construction for the GET request does not encode the query parameters. Consider using qs.stringify to ensure that special characters in challengeId, page, or perPage are properly encoded, preventing potential issues with the API request.
| }) | ||
| } | ||
| } catch (error) { | ||
| return project |
There was a problem hiding this comment.
[maintainability]
The catch block at line 130 silently swallows errors without logging them. Consider logging the error to aid in debugging and monitoring.
| } | ||
|
|
||
| try { | ||
| const membersByUserId = await fetchInviteMembers(missingUserIds) |
There was a problem hiding this comment.
[performance]
The function fetchInviteMembers is called with missingUserIds, which can be an empty array if all members have handles and all attachment creators are members. Consider adding a check to avoid making an unnecessary API call when missingUserIds is empty.
| * @returns {Promise<Array>} Project members. | ||
| */ | ||
| export async function fetchProjectMembers (projectId) { | ||
| const response = await axiosInstance.get(`${PROJECTS_API_URL}/${projectId}/members`, { |
There was a problem hiding this comment.
[💡 design]
The fetchProjectMembers function uses a hardcoded query parameter fields: 'handle'. Ensure that this is the intended behavior and that no additional fields are required for future use cases.
| * @returns {boolean} True when the error is the capacity-limit message. | ||
| */ | ||
| export const isCapacityLimitError = (message, status) => { | ||
| if (status !== undefined && status !== null && Number(status) !== 400) { |
There was a problem hiding this comment.
[💡 maintainability]
The check for status being undefined or null is redundant since Number(status) !== 400 will already handle these cases correctly by returning false. Consider simplifying the condition to Number(status) !== 400.
| return false | ||
| } | ||
|
|
||
| const normalizedMessage = rawMessage.trim().replace(/\s+/g, ' ').toLowerCase() |
There was a problem hiding this comment.
[💡 performance]
The use of replace(/\s+/g, ' ') to normalize whitespace is effective but could be optimized by using \s+ directly in the trim() method if supported, or by ensuring this normalization is necessary for all inputs. Consider evaluating if this step is always required.
| return true | ||
| } | ||
|
|
||
| return normalizedMessage.includes('required members') && |
There was a problem hiding this comment.
[correctness]
The logic for checking if the normalizedMessage includes certain substrings is somewhat fragile and could lead to false positives if the message structure changes slightly. Consider using a more robust pattern matching or regular expression approach to improve reliability.
| describe('engagement status normalization', () => { | ||
| it('converts ON_HOLD API values to On Hold labels', () => { | ||
| expect(fromEngagementStatusApi('ON_HOLD')).toBe('On Hold') | ||
| expect(fromEngagementStatusApi(' on_hold ')).toBe('On Hold') |
There was a problem hiding this comment.
[maintainability]
Consider trimming and normalizing the input within the fromEngagementStatusApi function itself to handle variations in input consistently. This would improve maintainability by centralizing input handling logic.
|
|
||
| it('converts On Hold labels to ON_HOLD API values', () => { | ||
| expect(toEngagementStatusApi('On Hold')).toBe('ON_HOLD') | ||
| expect(toEngagementStatusApi('on hold')).toBe('ON_HOLD') |
There was a problem hiding this comment.
[maintainability]
Consider normalizing the input within the toEngagementStatusApi function to handle variations in input consistently. This would improve maintainability by ensuring that input handling logic is centralized.
| * @param {String|Number} userId Authenticated user id to match. | ||
| * @returns {Object|null} Matching member record or `null`. | ||
| */ | ||
| export const getProjectMemberByUserId = (projectDetail, userId) => { |
There was a problem hiding this comment.
[correctness]
The getProjectMemberByUserId function uses _.get(projectDetail, 'members', []) to retrieve members, which is appropriate. However, consider adding a type check to ensure projectDetail is an object when it's not an array to prevent potential runtime errors.
| ? projectDetail | ||
| : _.get(projectDetail, 'members', []) | ||
|
|
||
| if (!normalizedUserId || !Array.isArray(members)) { |
There was a problem hiding this comment.
[💡 maintainability]
The check !Array.isArray(members) is redundant because members is always an array due to the default value provided by _.get. Consider removing this check.
| const tokenData = decodeToken(token) | ||
| return project && !_.isEmpty(project) && (_.find(project.invites, d => d.userId === tokenData.userId || d.email === tokenData.email)) | ||
| return project && !_.isEmpty(project) && (_.find(project.invites, d => ( | ||
| d.status === PROJECT_MEMBER_INVITE_STATUS_PENDING && |
There was a problem hiding this comment.
[correctness]
The condition d.status === PROJECT_MEMBER_INVITE_STATUS_PENDING is a good addition for filtering invites. Ensure that PROJECT_MEMBER_INVITE_STATUS_PENDING is defined and used consistently across the codebase.
3 participants
Use new projects-api-v6
https://topcoder.atlassian.net/browse/PM-3765