[PROD RELEASE] #1653
[PROD RELEASE] #1653
Conversation
fix(PM-1214): pagination in submissions tab
fix(PM-1169): send source to accept or decline invite API
PM-1178 Update copilot requests form link
fix(PM-1317): show request copilot for PMs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -19,6 +19,7 @@ module.exports = { | |||
| CHALLENGE_TRACKS_URL: `${DEV_API_HOSTNAME}/v5/challenge-tracks`, | |||
| CHALLENGE_PHASES_URL: `${DEV_API_HOSTNAME}/v5/challenge-phases`, | |||
| CHALLENGE_TIMELINES_URL: `${DEV_API_HOSTNAME}/v5/challenge-timelines`, | |||
| COPILOTS_URL: `https://copilots.${DOMAIN}`, | |||
There was a problem hiding this comment.
Consider using the DEV_API_HOSTNAME variable for consistency with other URLs, unless DOMAIN is intended to be different for this specific endpoint.
| @@ -18,6 +18,7 @@ module.exports = { | |||
| CHALLENGE_TRACKS_URL: `${PROD_API_HOSTNAME}/v5/challenge-tracks`, | |||
| CHALLENGE_PHASES_URL: `${PROD_API_HOSTNAME}/v5/challenge-phases`, | |||
| CHALLENGE_TIMELINES_URL: `${PROD_API_HOSTNAME}/v5/challenge-timelines`, | |||
| COPILOTS_URL: `https://copilots.${DOMAIN}`, | |||
There was a problem hiding this comment.
The COPILOTS_URL is using DOMAIN directly, but other URLs are using PROD_API_HOSTNAME. Consider using PROD_API_HOSTNAME for consistency, or ensure DOMAIN is correctly defined and intended for use here.
| @@ -2,12 +2,12 @@ import { fetchSubmissions } from '../services/challenges' | |||
|
|
|||
| import { LOAD_CHALLENGE_SUBMISSIONS } from '../config/constants' | |||
|
|
|||
| export function loadSubmissions (challengeId) { | |||
| return dispatch => { | |||
| export function loadSubmissions (challengeId, page) { | |||
There was a problem hiding this comment.
The function loadSubmissions now takes an additional parameter page, but there is no default value provided for it. Consider setting a default value to ensure backward compatibility if page is not provided.
| @@ -298,7 +302,7 @@ const ChallengeViewTabs = ({ | |||
| > | |||
| RESOURCES | |||
| </a> | |||
| {challengeSubmissions.length ? ( | |||
| {totalSubmissions ? ( | |||
There was a problem hiding this comment.
Consider renaming totalSubmissions to hasSubmissions or submissionsExist to make it clear that this is a boolean check rather than a count of submissions.
| @@ -311,7 +315,7 @@ const ChallengeViewTabs = ({ | |||
| }} | |||
| className={getSelectorStyle(selectedTab, 2)} | |||
| > | |||
| SUBMISSIONS ({submissions.length}) | |||
| SUBMISSIONS ({totalSubmissions}) | |||
There was a problem hiding this comment.
Ensure that totalSubmissions is correctly defined and updated elsewhere in the code to reflect the accurate number of submissions. This change assumes that totalSubmissions is a reliable source of truth for the submission count.
| @@ -396,7 +404,11 @@ ChallengeViewTabs.propTypes = { | |||
| deleteResource: PropTypes.func.isRequired, | |||
| showRejectChallengeModal: PropTypes.func.isRequired, | |||
| loggedInUser: PropTypes.object.isRequired, | |||
| onApproveChallenge: PropTypes.func | |||
| onApproveChallenge: PropTypes.func, | |||
| loadSubmissions: PropTypes.func, | |||
There was a problem hiding this comment.
The loadSubmissions prop is added but not marked as required. If this function is essential for the component's functionality, consider marking it as required using PropTypes.func.isRequired.
| onApproveChallenge: PropTypes.func | ||
| onApproveChallenge: PropTypes.func, | ||
| loadSubmissions: PropTypes.func, | ||
| totalSubmissions: PropTypes.number, |
There was a problem hiding this comment.
The totalSubmissions prop is added but not marked as required. If this value is crucial for the component's logic, consider marking it as required using PropTypes.number.isRequired.
| onApproveChallenge: PropTypes.func, | ||
| loadSubmissions: PropTypes.func, | ||
| totalSubmissions: PropTypes.number, | ||
| submissionsPerPage: PropTypes.number, |
There was a problem hiding this comment.
The submissionsPerPage prop is added but not marked as required. If this value is necessary for pagination or other logic, consider marking it as required using PropTypes.number.isRequired.
| loadSubmissions: PropTypes.func, | ||
| totalSubmissions: PropTypes.number, | ||
| submissionsPerPage: PropTypes.number, | ||
| page: PropTypes.number |
There was a problem hiding this comment.
The page prop is added but not marked as required. If this value is important for pagination or other functionality, consider marking it as required using PropTypes.number.isRequired.
| @@ -7,16 +7,13 @@ import React from 'react' | |||
| import PT from 'prop-types' | |||
| import moment from 'moment' | |||
| import _ from 'lodash' | |||
| import { STUDIO_URL, SUBMISSION_REVIEW_APP_URL, getTCMemberURL } from '../../../config/constants' | |||
| import { PAGINATION_PER_PAGE_OPTIONS, STUDIO_URL, SUBMISSION_REVIEW_APP_URL, getTCMemberURL } from '../../../config/constants' | |||
There was a problem hiding this comment.
Consider alphabetically sorting the imported constants for better readability and maintainability.
| import { PrimaryButton } from '../../Buttons' | ||
| import AlertModal from '../../Modal/AlertModal' | ||
| import cn from 'classnames' | ||
| import ReactSVG from 'react-svg' | ||
| import { | ||
| getRatingLevel, | ||
| sortList, | ||
| getProvisionalScore, | ||
| getFinalScore, | ||
| checkDownloadSubmissionRoles, |
There was a problem hiding this comment.
The functions sortList, getProvisionalScore, and getFinalScore have been removed from the import statement. Ensure that these functions are no longer needed in the codebase, or if they are used elsewhere, update the imports accordingly.
| @@ -32,8 +29,9 @@ import modalStyles from '../../../styles/modal.module.scss' | |||
| import { ArtifactsListModal } from '../ArtifactsListModal' | |||
| import Tooltip from '../../Tooltip' | |||
| import { RatingsListModal } from '../RatingsListModal' | |||
| import Select from '../../Select' | |||
There was a problem hiding this comment.
Consider checking if the Select component is being used in the file. If not, it might be unnecessary to import it.
| @@ -32,8 +29,9 @@ import modalStyles from '../../../styles/modal.module.scss' | |||
| import { ArtifactsListModal } from '../ArtifactsListModal' | |||
| import Tooltip from '../../Tooltip' | |||
| import { RatingsListModal } from '../RatingsListModal' | |||
| import Select from '../../Select' | |||
| import Pagination from 'react-js-pagination' | |||
There was a problem hiding this comment.
Ensure that the Pagination component from 'react-js-pagination' is being utilized in the code. If it's not used, the import can be removed to keep the code clean.
| @@ -53,161 +51,21 @@ class SubmissionsComponent extends React.Component { | |||
| }, | |||
| isShowInformation: false, | |||
| memberOfModal: '', | |||
| sortedSubmissions: [], | |||
| submissions: [], | |||
There was a problem hiding this comment.
The sortedSubmissions state has been removed, but ensure that any functionality relying on sorted submissions is still correctly handled with the new submissions state. Verify that sorting logic is not needed elsewhere in the component.
| this.checkIsReviewPhaseComplete = this.checkIsReviewPhaseComplete.bind( | ||
| this | ||
| ) | ||
| this.downloadSubmission = this.downloadSubmission.bind(this) | ||
| this.handlePageChange = this.handlePageChange.bind(this) | ||
| this.handlePerPageChange = this.handlePerPageChange.bind(this) | ||
| } | ||
|
|
There was a problem hiding this comment.
The componentDidMount lifecycle method has been removed. Ensure that any initialization logic previously handled in componentDidMount is now correctly managed elsewhere, especially if it involved fetching or sorting data.
| @@ -267,17 +125,58 @@ class SubmissionsComponent extends React.Component { | |||
| }) | |||
| } | |||
|
|
|||
| /** | |||
| * Update filter for getting project by pagination | |||
There was a problem hiding this comment.
The comment for the handlePerPageChange method mentions updating the filter for getting project by pagination, but the method seems to be related to submissions rather than projects. Consider updating the comment to accurately reflect the functionality of the method.
| } | ||
|
|
||
| /** | ||
| * Update filter for getting project by pagination |
There was a problem hiding this comment.
Similar to the comment for handlePerPageChange, the comment for handlePageChange mentions updating the filter for getting project by pagination. It would be more accurate to describe the method as updating the pagination for submissions.
| render () { | ||
| const { challenge, token, loggedInUserResource } = this.props | ||
| const { challenge, token, loggedInUserResource, page, submissionsPerPage, totalSubmissions, submissions } = this.props |
There was a problem hiding this comment.
The render method now includes additional props such as page, submissionsPerPage, totalSubmissions, and submissions. Ensure that these props are being used effectively within the method to avoid unnecessary prop declarations.
| @@ -319,7 +218,7 @@ class SubmissionsComponent extends React.Component { | |||
| const isBugHunt = _.includes(tags, 'Bug Hunt') | |||
|
|
|||
| // copy colorStyle from registrants to submissions | |||
| _.forEach(sortedSubmissions, s => { | |||
| _.forEach(submissions, s => { | |||
There was a problem hiding this comment.
The change from sortedSubmissions to submissions might affect the order in which submissions are processed. Ensure that the order of processing is not critical, or consider sorting submissions if necessary.
| @@ -344,7 +243,7 @@ class SubmissionsComponent extends React.Component { | |||
| <div className={cn(styles.container, styles.view)}> | |||
| <div className={styles['title']}>ROUND 2 (FINAL) SUBMISSIONS</div> | |||
| <div className={styles['content']}> | |||
| {sortedSubmissions.map(renderSubmission)} | |||
| {submissions.map(renderSubmission)} | |||
There was a problem hiding this comment.
The change from sortedSubmissions to submissions may affect the order in which submissions are displayed. If the order is important for the functionality or user experience, consider ensuring that submissions is sorted appropriately before rendering.
| @@ -388,117 +287,20 @@ class SubmissionsComponent extends React.Component { | |||
| <tr> | |||
There was a problem hiding this comment.
The removal of the sorting functionality for the 'Rating' column might affect the user experience if sorting is a required feature. Consider whether this change is intentional and aligns with the overall functionality requirements.
| <ReactSVG path={assets(`${ArrowDown}`)} /> | ||
| </div> | ||
| </button> | ||
| <span>Rating</span> | ||
| </th> | ||
| )} | ||
| <th> |
There was a problem hiding this comment.
The removal of the sorting functionality for the 'Username' column might impact the ability to organize data efficiently. Ensure that this change is in line with the intended design and user needs.
| <ReactSVG path={assets(`${ArrowDown}`)} /> | ||
| </div> | ||
| </button> | ||
| <span>Username</span> | ||
| </th> | ||
| <th> |
There was a problem hiding this comment.
The sorting functionality for the 'Email' column has been removed. Verify if this change is intentional and if it meets the requirements for data management and user interaction.
| <ReactSVG path={assets(`${ArrowDown}`)} /> | ||
| </div> | ||
| </button> | ||
| <span>Email</span> | ||
| </th> | ||
| <th> |
There was a problem hiding this comment.
The 'Submission Date' column no longer supports sorting. Confirm if this change is deliberate and if it aligns with the expected behavior of the application.
| <ReactSVG path={assets(`${ArrowDown}`)} /> | ||
| </div> | ||
| </button> | ||
| <span>Submission Date</span> | ||
| </th> | ||
| <th> |
There was a problem hiding this comment.
The removal of sorting for the 'Initial / Final Score' column could affect data analysis capabilities. Ensure this change is intentional and consistent with user requirements.
| @@ -518,7 +320,7 @@ class SubmissionsComponent extends React.Component { | |||
| </tr> | |||
| </thead> | |||
| <tbody> | |||
| {sortedSubmissions.map(s => { | |||
| {submissions.map(s => { | |||
There was a problem hiding this comment.
The change from sortedSubmissions to submissions might affect the order in which submissions are displayed. If the order is important, consider ensuring that submissions is sorted appropriately before mapping over it.
| @@ -679,7 +481,7 @@ class SubmissionsComponent extends React.Component { | |||
| const allFiles = [] | |||
| let downloadedFile = 0 | |||
| const checkToCompressFiles = () => { | |||
| if (downloadedFile === sortedSubmissions.length) { | |||
| if (downloadedFile === submissions.length) { | |||
There was a problem hiding this comment.
Ensure that submissions is defined and correctly initialized before this line. The change from sortedSubmissions to submissions suggests a variable name change or a different data structure, which should be verified for correctness.
| @@ -694,7 +496,7 @@ class SubmissionsComponent extends React.Component { | |||
| } | |||
| } | |||
| checkToCompressFiles() | |||
| _.forEach(sortedSubmissions, (submission) => { | |||
| _.forEach(submissions, (submission) => { | |||
There was a problem hiding this comment.
The change from sortedSubmissions to submissions may affect the order of processing submissions. Ensure that the order of submissions is not critical for subsequent operations, or consider sorting submissions if necessary.
| <div className={styles.paginationWrapper}> | ||
| <div className={styles.perPageContainer}> | ||
| <Select | ||
| styles={styles} |
There was a problem hiding this comment.
The styles prop passed to the Select component is likely intended to customize the appearance of the dropdown. Ensure that the styles object contains the correct CSS-in-JS style definitions for the Select component from the library being used (e.g., react-select).
| <Select | ||
| styles={styles} | ||
| name='perPage' | ||
| value={{ label: submissionsPerPage, value: submissionsPerPage }} |
There was a problem hiding this comment.
The value prop for the Select component is set to an object with label and value. Verify that this structure is compatible with the Select component being used, as some libraries may expect a different format or additional properties.
| itemsCountPerPage={submissionsPerPage} | ||
| totalItemsCount={totalSubmissions} | ||
| pageRangeDisplayed={5} | ||
| onChange={this.handlePageChange} |
There was a problem hiding this comment.
Consider making pageRangeDisplayed a configurable prop or constant if it might need to change in the future, rather than hardcoding the value 5.
| @@ -759,7 +585,11 @@ SubmissionsComponent.propTypes = { | |||
| }).isRequired, | |||
| submissions: PT.arrayOf(PT.shape()), | |||
There was a problem hiding this comment.
The submissions prop type is defined as an array of shapes, but the shape is not specified. Consider defining the shape of the objects within the array for better type checking.
| @@ -759,7 +585,11 @@ SubmissionsComponent.propTypes = { | |||
| }).isRequired, | |||
| submissions: PT.arrayOf(PT.shape()), | |||
| token: PT.string, | |||
| loggedInUserResource: PT.any | |||
| loggedInUserResource: PT.any, | |||
There was a problem hiding this comment.
The loggedInUserResource prop type is defined as PT.any, which is not specific. Consider defining a more specific prop type to improve type safety.
| @@ -94,11 +94,11 @@ const ChallengesComponent = ({ | |||
| className={styles.btnOutline} | |||
| /> | |||
| )} | |||
| {checkAdmin(auth.token) && ( | |||
| {(checkAdmin(auth.token) || checkManager(auth.token)) && ( | |||
There was a problem hiding this comment.
Consider checking if checkManager function is defined and imported correctly to avoid potential runtime errors.
| <OutlineButton | ||
| text='Request Copilot' | ||
| type={'info'} | ||
| url={`${TYPEFORM_URL}#handle=${auth.user.handle}&projectid=${activeProjectId}`} | ||
| url={`${COPILOTS_URL}/requests/new`} |
There was a problem hiding this comment.
Ensure that COPILOTS_URL is defined and correctly configured to point to the intended endpoint. This change modifies the URL structure, so verify that the new URL meets the requirements.
| ) { | ||
| 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 Promise.all to handle the array of promises for loadResources and loadSubmissions to ensure both are awaited correctly.
| 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.
| @@ -42,9 +44,9 @@ const ProjectInvitations = ({ match, auth, isProjectLoading, history, projectDet | |||
| } | |||
| }, [projectId, auth, projectDetail, isProjectLoading, history]) | |||
|
|
|||
| const updateInvite = useCallback(async (status) => { | |||
| const updateInvite = useCallback(async (status, source) => { | |||
| setIsUpdating(status) | |||
There was a problem hiding this comment.
The updateInvite function now takes an additional source parameter, but it's not clear how this parameter is being used within the function. Ensure that the source parameter is necessary and is being utilized appropriately in the updateProjectMemberInvite function.
| @@ -56,8 +58,8 @@ const ProjectInvitations = ({ match, auth, isProjectLoading, history, projectDet | |||
| history.push(status === PROJECT_MEMBER_INVITE_STATUS_ACCEPTED ? `/projects/${projectId}/challenges` : '/projects') | |||
| }, [projectId, invitation, loadProjectInvites, history]) | |||
|
|
|||
| const acceptInvite = useCallback(() => updateInvite(PROJECT_MEMBER_INVITE_STATUS_ACCEPTED), [updateInvite]) | |||
| const declineInvite = useCallback(() => updateInvite(PROJECT_MEMBER_INVITE_STATUS_REFUSED), [updateInvite]) | |||
| const acceptInvite = useCallback(() => updateInvite(PROJECT_MEMBER_INVITE_STATUS_ACCEPTED, source), [updateInvite, source]) | |||
There was a problem hiding this comment.
The acceptInvite function now includes a source parameter. Ensure that source is defined and passed correctly to avoid potential runtime errors.
| const acceptInvite = useCallback(() => updateInvite(PROJECT_MEMBER_INVITE_STATUS_ACCEPTED), [updateInvite]) | ||
| const declineInvite = useCallback(() => updateInvite(PROJECT_MEMBER_INVITE_STATUS_REFUSED), [updateInvite]) | ||
| const acceptInvite = useCallback(() => updateInvite(PROJECT_MEMBER_INVITE_STATUS_ACCEPTED, source), [updateInvite, source]) | ||
| const declineInvite = useCallback(() => updateInvite(PROJECT_MEMBER_INVITE_STATUS_REFUSED, source), [updateInvite, source]) |
There was a problem hiding this comment.
The declineInvite function now includes a source parameter. Ensure that source is defined and passed correctly to avoid potential runtime errors.
| } | ||
|
|
||
| export default function (state = initialState, action) { | ||
| switch (action.type) { | ||
| case LOAD_CHALLENGE_SUBMISSIONS_SUCCESS: | ||
| return { | ||
| ...state, | ||
| challengeSubmissions: action.payload, | ||
| challengeSubmissions: action.payload.data, |
There was a problem hiding this comment.
Ensure that action.payload.data is always defined and structured as expected. Consider adding validation or error handling for cases where action.payload.data might be undefined or malformed.
| isLoading: false, | ||
| loadingId: null, | ||
| challengeId: state.loadingId | ||
| challengeId: state.loadingId, | ||
| totalSubmissions: action.payload.headers['x-total'], |
There was a problem hiding this comment.
Accessing headers with action.payload.headers['x-total'] assumes that the headers object and the specific header exist. Consider adding error handling or default values in case the header is missing.
| challengeId: state.loadingId | ||
| challengeId: state.loadingId, | ||
| totalSubmissions: action.payload.headers['x-total'], | ||
| page: action.payload.page, |
There was a problem hiding this comment.
Ensure that action.payload.page is always defined and a valid number. Consider adding validation or default values to handle unexpected cases.
| challengeId: state.loadingId, | ||
| totalSubmissions: action.payload.headers['x-total'], | ||
| page: action.payload.page, | ||
| submissionsPerPage: action.payload.perPage |
There was a problem hiding this comment.
Ensure that action.payload.perPage is always defined and a valid number. Consider adding validation or default values to handle unexpected cases.
| export async function fetchSubmissions (challengeId) { | ||
| const response = await axiosInstance.get(`${SUBMISSIONS_API_URL}?challengeId=${challengeId}&perPage=100`) | ||
| return _.get(response, 'data', []) | ||
| export async function fetchSubmissions (challengeId, pageObj) { |
There was a problem hiding this comment.
The function fetchSubmissions now takes a pageObj parameter, but there is no validation to ensure that pageObj contains valid page and perPage properties. Consider adding validation to handle cases where pageObj might be undefined or missing these properties.
| return _.get(response, 'data', []) | ||
| export async function fetchSubmissions (challengeId, pageObj) { | ||
| const { page, perPage } = pageObj | ||
| const response = await axiosInstance.get(`${SUBMISSIONS_API_URL}?challengeId=${challengeId}&perPage=${perPage}&page=${page}`) |
There was a problem hiding this comment.
The URL construction for the API request now includes page and perPage parameters. Ensure that these parameters are correctly handled by the API and that they do not exceed any limits imposed by the API.
| @@ -8,9 +8,9 @@ import { PROJECTS_API_URL } from '../config/constants' | |||
| * @param {string} status the new status for invitation | |||
| * @return {object} project member invite returned by api | |||
| */ | |||
| export function updateProjectMemberInvite (projectId, inviteId, status) { | |||
| export function updateProjectMemberInvite (projectId, inviteId, status, source) { | |||
There was a problem hiding this comment.
The function updateProjectMemberInvite now includes an additional parameter source. Ensure that all calls to this function throughout the codebase are updated to pass this new parameter, or provide a default value if it's optional.
No description provided.