Merge in develop #1656
Merge in develop #1656
Conversation
Standardised skills and minor fixes
Fix bug where copilots can't assign tasks to themselves, even before activation
Update the URL for saving skills
PROD HOTFIX - Handle strings or booleans for `show_data_dashboard` flag
PROD - Support dashboard checkbox for code challenges
Switch how we save the challenge (PS-263)
PROD Release - Work Manager Security Issues (5442)
…update PM-683 - send jobid on update for taas projects
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| showSelectUserError: false, | ||
| isAdding: false, | ||
| addUserError: false, | ||
| showInviteUserModal: false, // Add state for invite user modal |
There was a problem hiding this comment.
Consider renaming showInviteUserModal to isInviteUserModalVisible for consistency with other boolean state variables like showRemoveConfirmationModal.
| @@ -36,10 +34,9 @@ class Users extends Component { | |||
| } | |||
| this.setProjectOption = this.setProjectOption.bind(this) | |||
| this.onAddUserClick = this.onAddUserClick.bind(this) | |||
| this.onInviteUserClick = this.onInviteUserClick.bind(this) // Bind the new method | |||
There was a problem hiding this comment.
Consider removing the inline comment as it is not necessary to explain the binding of methods.
| this.onUpdateUserToAdd = this.onUpdateUserToAdd.bind(this) | ||
| this.onAddUserConfirmClick = this.onAddUserConfirmClick.bind(this) | ||
| this.updatePermission = this.updatePermission.bind(this) | ||
| this.resetInviteUserState = this.resetInviteUserState.bind(this) // Bind reset method |
There was a problem hiding this comment.
Consider removing the inline comment as it is not necessary to explain the binding of methods.
| showSelectUserError: !userToAdd | ||
| }) | ||
| resetAddUserState () { | ||
| this.setState({ showAddUserModal: false }) |
There was a problem hiding this comment.
Consider resetting other related states such as userToAdd, showSelectUserError, isAdding, userPermissionToAdd, and addUserError when resetting the add user state to ensure consistency and avoid potential issues.
| this.setState({ isAdding: false, addUserError: error }) | ||
| } | ||
| resetInviteUserState () { | ||
| this.setState({ showInviteUserModal: false }) |
There was a problem hiding this comment.
Consider resetting other related states such as userToAdd, showSelectUserError, isAdding, userPermissionToAdd, and addUserError when resetting the invite user state to ensure consistency and avoid potential issues.
| @@ -167,12 +110,15 @@ class Users extends Component { | |||
| async onRemoveConfirmClick () { | |||
| if (this.state.isRemoving) { return } | |||
|
|
|||
| const { removeProjectNember } = this.props | |||
| const { removeProjectMember, invitedMembers } = this.props | |||
There was a problem hiding this comment.
Typo in the variable name removeProjectNember. It should be removeProjectMember to match the destructured prop.
| const userToRemove = this.state.userToRemove | ||
| const isInvite = !!_.find(invitedMembers, { email: userToRemove.email }) |
There was a problem hiding this comment.
Consider using a more descriptive variable name than isInvite to improve code readability, such as isUserInvited.
| await removeUserFromProject(userToRemove.projectId, userToRemove.id) | ||
| removeProjectNember(userToRemove) | ||
| await ( | ||
| isInvite ? deleteProjectMemberInvite(userToRemove.projectId, userToRemove.id) : removeUserFromProject(userToRemove.projectId, userToRemove.id) |
There was a problem hiding this comment.
The ternary operation here is quite complex. Consider breaking it into a separate function or using an if statement for better readability.
| @@ -210,10 +156,13 @@ class Users extends Component { | |||
| const { | |||
| projects, | |||
| projectMembers, | |||
| updateProjectNember, | |||
| invitedMembers, | |||
There was a problem hiding this comment.
Typo fix: The function updateProjectNember was renamed to updateProjectMember. Ensure that all references to this function in the codebase are updated accordingly to prevent any runtime errors.
| @@ -225,10 +174,11 @@ class Users extends Component { | |||
| } | |||
| }) | |||
| const loggedInHandle = this.getHandle() | |||
| const membersExist = projectMembers && projectMembers.length > 0 | |||
| const membersExist = (projectMembers && projectMembers.length > 0) || (invitedMembers && invitedMembers.length > 0) | |||
There was a problem hiding this comment.
Consider simplifying the condition by extracting it into a separate function for better readability and maintainability.
| const isCopilotOrManager = this.checkIsCopilotOrManager(projectMembers, loggedInHandle) | ||
| const isAdmin = checkAdmin(this.props.auth.token) | ||
| const showAddUser = isEditable && this.state.projectOption && (isCopilotOrManager || isAdmin) | ||
| const isManager = checkManager(this.props.auth.token) | ||
| const showAddUser = isEditable && this.state.projectOption && (isCopilotOrManager || isAdmin || isManager) |
There was a problem hiding this comment.
Ensure that checkManager function is defined and properly handles all possible cases for the token validation.
| @@ -246,6 +196,7 @@ class Users extends Component { | |||
| onChange={(e) => { this.setProjectOption(e) }} | |||
| onInputChange={this.debouncedOnInputChange} | |||
| isLoading={isSearchingUserProjects} | |||
| onMenuScrollBottom={loadNextProjects} | |||
There was a problem hiding this comment.
Consider adding error handling for the loadNextProjects function to ensure that any issues during the loading process are managed gracefully.
| ) | ||
| } | ||
| { | ||
| this.state.showRemoveConfirmationModal && ( | ||
| <ConfirmationModal | ||
| title='Confirm Removal' | ||
| message={`Are you sure you want to remove ${this.state.userToRemove.handle} from this project?`} | ||
| message={`Are you sure you want to remove ${this.state.userToRemove.handle || this.state.userToRemove.email} from this project?`} |
There was a problem hiding this comment.
Consider handling cases where both this.state.userToRemove.handle and this.state.userToRemove.email might be undefined or null to avoid displaying 'undefined' in the message.
| @@ -403,7 +253,7 @@ class Users extends Component { | |||
| ) | |||
| } | |||
| { | |||
| membersExist && ( | |||
| !isLoadingProject && membersExist && ( | |||
There was a problem hiding this comment.
Consider checking the order of conditions in the expression !isLoadingProject && membersExist. If isLoadingProject is a more frequently changing state than membersExist, it might be beneficial to check membersExist first for potential performance optimization.
| @@ -430,7 +280,23 @@ class Users extends Component { | |||
| <UserCard | |||
| user={member} | |||
| onRemoveClick={this.onRemoveClick} | |||
| updateProjectNember={updateProjectNember} | |||
| updateProjectMember={updateProjectMember} | |||
There was a problem hiding this comment.
The prop name updateProjectNember was corrected to updateProjectMember. Ensure that this change is consistent throughout the codebase and that the function updateProjectMember is defined and used correctly.
| return ( | ||
| <li className={styles.userItem} key={`user-card-${member.id}`}> | ||
| <UserCard | ||
| isInvite |
There was a problem hiding this comment.
The isInvite prop is added to the UserCard component. Verify that the UserCard component handles this new prop appropriately and that it does not break existing functionality.
| </div> | ||
| {showEmailError && ( | ||
| <div className={styles.row}> | ||
| <div className={styles.errorMesssage}>Please enter a valid email address.</div> |
There was a problem hiding this comment.
Typo in class name: styles.errorMesssage should be styles.errorMessage.
| </div> | ||
| {showSelectUserError && ( | ||
| <div className={styles.row}> | ||
| <div className={styles.errorMesssage}>Please select a member.</div> |
There was a problem hiding this comment.
Typo in class name: styles.errorMesssage should be styles.errorMessage.
| </div> | ||
| </div> | ||
| {addUserError && ( | ||
| <div className={styles.errorMesssage}>{addUserError}</div> |
There was a problem hiding this comment.
Typo in class name: styles.errorMesssage should be styles.errorMessage.
| export const CREATE_FORUM_TYPE_IDS = typeof process.env.CREATE_FORUM_TYPE_IDS === 'string' ? process.env.CREATE_FORUM_TYPE_IDS.split(',') : process.env.CREATE_FORUM_TYPE_IDS | ||
| export const PROJECTS_API_URL = process.env.PROJECTS_API_URL || process.env.PROJECT_API_URL |
There was a problem hiding this comment.
Consider renaming PROJECTS_API_URL to PROJECT_API_URL for consistency with the environment variable PROJECT_API_URL, unless there is a specific reason for the plural form.
| export const FILE_PICKER_CNAME = process.env.FILE_PICKER_CNAME || 'fs.topcoder.com' | ||
| export const FILE_PICKER_FROM_SOURCES = ['local_file_system', 'googledrive', 'dropbox'] | ||
| export const ASSETS_FILE_PICKER_FROM_SOURCES = ['local_file_system'] |
There was a problem hiding this comment.
Consider consolidating FILE_PICKER_FROM_SOURCES and ASSETS_FILE_PICKER_FROM_SOURCES if they are intended to serve similar purposes, or clarify their distinct roles if they are meant to be different.
| export const FILE_PICKER_CNAME = process.env.FILE_PICKER_CNAME || 'fs.topcoder.com' | ||
| export const FILE_PICKER_FROM_SOURCES = ['local_file_system', 'googledrive', 'dropbox'] | ||
| export const ASSETS_FILE_PICKER_FROM_SOURCES = ['local_file_system'] | ||
| export const ASSETS_FILE_PICKER_MAX_FILES = 4 | ||
| export const FILE_PICKER_ACCEPT = ['.bmp', '.gif', '.jpg', '.tex', '.xls', '.xlsx', '.doc', '.docx', '.zip', '.txt', '.pdf', '.png', '.ppt', '.pptx', '.rtf', '.csv'] | ||
| export const FILE_PICKER_MAX_FILES = 10 |
There was a problem hiding this comment.
Ensure that the FILE_PICKER_MAX_FILES and ASSETS_FILE_PICKER_MAX_FILES values are intentionally different and document the reason for this difference elsewhere if necessary, as it might cause confusion.
| export const FILE_PICKER_ACCEPT = ['.bmp', '.gif', '.jpg', '.tex', '.xls', '.xlsx', '.doc', '.docx', '.zip', '.txt', '.pdf', '.png', '.ppt', '.pptx', '.rtf', '.csv'] | ||
| export const FILE_PICKER_MAX_FILES = 10 | ||
| export const FILE_PICKER_MAX_SIZE = 500 * 1024 * 1024 // 500Mb | ||
| export const FILE_PICKER_PROGRESS_INTERVAL = 100 | ||
| export const FILE_PICKER_UPLOAD_RETRY = 2 | ||
| export const FILE_PICKER_UPLOAD_TIMEOUT = 30 * 60 * 1000 // 30 minutes | ||
| export const SPECIFICATION_ATTACHMENTS_FOLDER = 'SPECIFICATION_ATTACHMENTS' | ||
| export const MEMBERS_API_URL = process.env.MEMBERS_API_URL |
There was a problem hiding this comment.
Ensure that process.env.MEMBERS_API_URL is defined and has a fallback value if it's not set in the environment. This will prevent potential runtime errors if the environment variable is missing.
| @@ -232,6 +258,7 @@ export const MARATHON_MATCH_SUBTRACKS = [ | |||
|
|
|||
| export const PROJECT_ROLES = { | |||
| READ: 'observer', | |||
| CUSTOMER: 'customer', | |||
| WRITE: 'customer', | |||
There was a problem hiding this comment.
The role 'CUSTOMER' is being added with the same value as the existing 'WRITE' role. Consider if this duplication is intentional or if a different value is needed for 'CUSTOMER'.
| @@ -298,11 +325,17 @@ export const COPILOT_ROLES = [ | |||
| 'copilot' | |||
| ] | |||
|
|
|||
| export const MANAGER_ROLES = [ | |||
| 'project manager' | |||
There was a problem hiding this comment.
Consider expanding the MANAGER_ROLES array to include other potential manager roles if applicable, to ensure future scalability.
| @@ -396,7 +436,8 @@ export const SPECIAL_CHALLENGE_TAGS = [ | |||
| /** | |||
| * Possible statuses of projects | |||
| */ | |||
| export const PROJECT_STATUS = [ | |||
| export const PROJECT_STATUSES = [ | |||
There was a problem hiding this comment.
Consider renaming the constant to PROJECT_STATUS_OPTIONS to better reflect that it is an array of objects with labels and values, rather than just statuses.
| @@ -405,6 +446,15 @@ export const PROJECT_STATUS = [ | |||
| { label: 'Paused', value: 'paused' } | |||
| ] | |||
|
|
|||
| export const PROJECT_STATUS = { | |||
There was a problem hiding this comment.
The constant PROJECT_STATUS is defined twice in the file, once as an array and once as an object. Consider renaming one of them to avoid confusion and potential conflicts.
| @@ -418,3 +468,17 @@ export const JOB_WORKLOAD_OPTIONS = [ | |||
| { value: 'fulltime', label: 'Full-Time' }, | |||
| { value: 'fractional', label: 'Fractional' } | |||
| ] | |||
|
|
|||
| /* | |||
There was a problem hiding this comment.
Consider removing the block comment style for consistency, as the rest of the file uses single-line comments.
| export const ATTACHMENT_TYPE_FILE = 'file' | ||
| export const ATTACHMENT_TYPE_LINK = 'link' | ||
|
|
||
| /** |
There was a problem hiding this comment.
Consider removing the block comment style for consistency, as the rest of the file uses single-line comments.
| ) { | ||
| let projectId = _.get(newMatch.params, 'projectId', null) | ||
| projectId = projectId ? parseInt(projectId) : null | ||
| const challengeId = _.get(newMatch.params, 'challengeId', null) | ||
| await [loadResources(challengeId), loadSubmissions(challengeId)] | ||
| await [loadResources(challengeId), loadSubmissions(challengeId, { |
There was a problem hiding this comment.
Consider using a more descriptive variable name for submissionsPerPage to clarify its purpose and ensure consistency across the codebase.
| showRejectChallengeModal: PropTypes.func, | ||
| totalSubmissions: PropTypes.number, | ||
| submissionsPerPage: PropTypes.number, | ||
| page: PropTypes.number | ||
| // members: PropTypes.arrayOf(PropTypes.shape()) |
There was a problem hiding this comment.
The members prop type is commented out. If it's not needed, consider removing it entirely to keep the code clean.
| @@ -6,25 +6,22 @@ import React, { Component, Fragment } from 'react' | |||
| // import { Redirect } from 'react-router-dom' | |||
| import PropTypes from 'prop-types' | |||
| import { connect } from 'react-redux' | |||
| import { Link } from 'react-router-dom' | |||
| import ChallengesComponent from '../../components/ChallengesComponent' | |||
There was a problem hiding this comment.
The Link import from 'react-router-dom' has been removed. If this component relies on routing or navigation, ensure that the removal of Link does not affect the functionality.
| import ChallengesComponent from '../../components/ChallengesComponent' | ||
| import ProjectCard from '../../components/ProjectCard' | ||
| // import Loader from '../../components/Loader' | ||
| import { |
There was a problem hiding this comment.
The ProjectCard import has been removed. Verify that this component is no longer needed in the current implementation, or if it has been replaced by another component.
| // import Loader from '../../components/Loader' | ||
| import { | ||
| loadChallengesByPage, | ||
| partiallyUpdateChallengeDetails, | ||
| deleteChallenge, | ||
| loadChallengeTypes | ||
| } from '../../actions/challenges' | ||
| import { loadProject, updateProject } from '../../actions/projects' | ||
| import { loadProject, loadProjects, updateProject } from '../../actions/projects' |
There was a problem hiding this comment.
The import statement for loadProjects has been moved from the sidebar actions to the projects actions. Ensure that this change is intentional and that loadProjects is correctly defined in the projects actions.
| import { | ||
| loadProjects, | ||
| loadNextProjects, |
There was a problem hiding this comment.
The function loadNextProjects is newly imported from ../../actions/sidebar. Verify that this function is implemented and used correctly in the code.
| import styles from './Challenges.module.scss' | ||
| import { checkAdmin, checkAdminOrCopilot } from '../../util/tc' | ||
| import { PrimaryButton } from '../../components/Buttons' | ||
| import { checkAdmin, checkIsUserInvitedToProject } from '../../util/tc' |
There was a problem hiding this comment.
The function checkIsUserInvitedToProject is now imported instead of checkAdminOrCopilot. Ensure that this change is intended and that the logic in the code using this function is updated accordingly.
| import { checkAdmin, checkAdminOrCopilot } from '../../util/tc' | ||
| import { PrimaryButton } from '../../components/Buttons' | ||
| import { checkAdmin, checkIsUserInvitedToProject } from '../../util/tc' | ||
| import { withRouter } from 'react-router-dom' |
There was a problem hiding this comment.
The withRouter import from react-router-dom is added. Make sure that the Challenges component is wrapped with withRouter if routing props are needed.
| @@ -46,6 +43,7 @@ class Challenges extends Component { | |||
| } = this.props | |||
| loadChallengeTypes() | |||
| if (dashboard) { | |||
| this.props.loadProjects('', {}) | |||
There was a problem hiding this comment.
Consider passing specific parameters to loadProjects instead of empty string and empty object to ensure clarity and avoid potential issues with default values.
| @@ -59,6 +57,14 @@ class Challenges extends Component { | |||
| } | |||
| } | |||
|
|
|||
| componentDidUpdate () { | |||
There was a problem hiding this comment.
Consider adding a prevProps parameter to componentDidUpdate to compare previous and current props, which can help avoid unnecessary operations.
| componentDidUpdate () { | ||
| const { auth } = this.props | ||
|
|
||
| if (checkIsUserInvitedToProject(auth.token, this.props.projectDetail)) { |
There was a problem hiding this comment.
Ensure that checkIsUserInvitedToProject is a pure function and does not cause side effects. If it involves asynchronous operations or side effects, consider handling them appropriately.
| const { auth } = this.props | ||
|
|
||
| if (checkIsUserInvitedToProject(auth.token, this.props.projectDetail)) { | ||
| this.props.history.push(`/projects/${this.props.projectId}/invitation`) |
There was a problem hiding this comment.
Using this.props.history.push directly in componentDidUpdate can lead to multiple re-renders if the condition is frequently met. Consider adding a condition to prevent repeated navigation.
| @@ -140,49 +146,20 @@ class Challenges extends Component { | |||
| dashboard, | |||
| selfService, | |||
| auth, | |||
| metadata | |||
| metadata, | |||
| fetchNextProjects | |||
There was a problem hiding this comment.
The fetchNextProjects prop is added to the ChallengesComponent. Ensure that ChallengesComponent is updated to handle this new prop appropriately.
| {(dashboard || activeProjectId !== -1 || selfService) && ( | ||
| <ChallengesComponent | ||
| activeProject={{ | ||
| ...projectInfo, | ||
| ...(reduxProjectInfo && reduxProjectInfo.id === activeProjectId |
There was a problem hiding this comment.
The spread operator is used to merge reduxProjectInfo into activeProject. Ensure that reduxProjectInfo is defined and has the expected structure to avoid potential runtime errors.
| @@ -227,6 +204,7 @@ Challenges.defaultProps = { | |||
| } | |||
|
|
|||
| Challenges.propTypes = { | |||
| history: PropTypes.object, | |||
| projects: PropTypes.arrayOf(PropTypes.shape()), | |||
There was a problem hiding this comment.
Consider specifying the shape of the objects within the projects array for better type checking and clarity. For example, PropTypes.arrayOf(PropTypes.shape({ id: PropTypes.number, name: PropTypes.string })).
| @@ -227,6 +204,7 @@ Challenges.defaultProps = { | |||
| } | |||
|
|
|||
| Challenges.propTypes = { | |||
| history: PropTypes.object, | |||
| projects: PropTypes.arrayOf(PropTypes.shape()), | |||
| menu: PropTypes.string, | |||
| challenges: PropTypes.arrayOf(PropTypes.object), | |||
There was a problem hiding this comment.
Consider specifying the shape of the objects within the challenges array for better type checking and clarity. For example, PropTypes.arrayOf(PropTypes.shape({ id: PropTypes.number, title: PropTypes.string })).
| @@ -292,12 +272,15 @@ const mapDispatchToProps = { | |||
| loadChallengesByPage, | |||
| resetSidebarActiveParams, | |||
| loadProject, | |||
| loadProjects, | |||
| fetchNextProjects: loadNextProjects, | |||
There was a problem hiding this comment.
Consider renaming fetchNextProjects to loadNextProjects for consistency with the existing naming convention.
| ]} | ||
| selectedIndex={selectedTab} | ||
| onSelect={setSelectedTab} | ||
| classsName={styles.blockTabs} |
There was a problem hiding this comment.
Typo in prop name: classsName should be className.
| } | ||
| }, [files, links, selectedTab]) | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
The useEffect dependency array should include loadOnlyProjectInfo to ensure it is not stale if the function changes.
|
|
||
| picker.open() | ||
| } | ||
| }, [fileUploadClient, projectId]) |
There was a problem hiding this comment.
Consider adding setPendingUploadFiles and setShowAttachmentOptions to the dependency array of useCallback to ensure they are not stale.
No description provided.