Prod deploy for recent changes #1712
Conversation
fix: user should be able to add only one manual reviewer config per phase
fix(PM-2372): show the duplicate reviewer config error on saving
…d editable if it's missed during challenge creation
|
|
||
| // Clear any prior transient error when add succeeds | ||
| if (this.state.error) { | ||
| this.setState({ error: null }) |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a single setState call to clear the error state after successful operations, as this pattern is repeated multiple times in the code. This can improve maintainability by reducing redundancy.
| const targetPhaseId = value | ||
| const isCurrentMember = (updatedReviewers[index] && (updatedReviewers[index].isMemberReview !== false)) | ||
| if (isCurrentMember) { | ||
| const conflict = (currentReviewers || []).some((r, i) => i !== index && (r.isMemberReview !== false) && (r.phaseId === targetPhaseId)) |
There was a problem hiding this comment.
[correctness]
The conflict check uses some to find a duplicate manual reviewer for the target phase. Ensure that the logic correctly handles cases where phaseId might be undefined or null, as this could lead to unexpected behavior.
| if (!isAI) { | ||
| const existingReviewer = currentReviewers[index] || {} | ||
| const phaseId = existingReviewer.phaseId | ||
| const conflict = currentReviewers.some((r, i) => i !== index && (r.isMemberReview !== false) && (r.phaseId === phaseId)) |
There was a problem hiding this comment.
[correctness]
The conflict check here is similar to the one on line 525. Ensure that phaseId is always defined and valid to avoid potential issues with undefined values causing incorrect conflict detection.
| const phase = (challenge.phases || []).find(p => (p.id === phaseId) || (p.phaseId === phaseId)) | ||
| const phaseName = phase ? (phase.name || phaseId) : phaseId | ||
| this.setState({ | ||
| error: `Cannot switch to Member Reviewer: a manual reviewer configuration already exists for phase '${phaseName}'. Increase "Number of Reviewers" on the existing configuration instead.` |
There was a problem hiding this comment.
[💡 maintainability]
Consider centralizing error message handling to avoid duplication and potential inconsistencies. This can help with maintainability and ensure consistent messaging across the application.
| const isSupportedTrack = challenge.trackId === DS_TRACK_ID || challenge.trackId === DEV_TRACK_ID | ||
| const isSupportedType = challenge.typeId === MARATHON_TYPE_ID || challenge.typeId === CHALLENGE_TYPE_ID | ||
|
|
||
| return (isSupportedTrack && isSupportedType) || Boolean(useDashboardData) |
There was a problem hiding this comment.
[correctness]
The logic for showDashBoard might be overly permissive. It currently allows the dashboard to be shown if useDashboardData exists, regardless of its value. Consider checking the value of useDashboardData to ensure it aligns with the intended behavior.
| id='isDashboardEnabled' | ||
| checked={useDashboard} | ||
| readOnly | ||
| disabled |
There was a problem hiding this comment.
[design]
The disabled attribute on the checkbox input prevents user interaction. If the intention is to allow users to toggle this setting, consider removing the disabled attribute.
| * | ||
| * @param {Object} challenge the challenge data to evaluate | ||
| */ | ||
| shouldShowDashboardSetting (challenge = {}) { |
There was a problem hiding this comment.
[💡 readability]
The shouldShowDashboardSetting function uses _.get to access properties of the challenge object. Consider using optional chaining (e.g., challenge?.typeId) for improved readability and performance.
|
|
||
| const counts = {} | ||
| reviewers.forEach(r => { | ||
| if (r && (r.isMemberReview !== false) && r.phaseId) { |
There was a problem hiding this comment.
[💡 readability]
In getDuplicateManualReviewerPhases, the condition r && (r.isMemberReview !== false) && r.phaseId could be simplified to r?.isMemberReview !== false && r.phaseId for better readability.
| const isDevChallenge = trackId === DEV_TRACK_ID | ||
| const isMM = typeId === MARATHON_TYPE_ID | ||
| const showDashBoard = (isDataScience && isChallengeType) || (isDevChallenge && isMM) || (isDevChallenge && isChallengeType) | ||
| const showDashBoard = this.shouldShowDashboardSetting({ trackId, typeId, metadata: challengeMetadata }) |
There was a problem hiding this comment.
[correctness]
The showDashBoard variable is assigned the result of shouldShowDashboardSetting, which is called with an object containing trackId, typeId, and metadata. Ensure that these properties are always available and correctly typed to avoid runtime errors.
| const showCheckpointPrizes = challenge.timelineTemplateId === MULTI_ROUND_CHALLENGE_TEMPLATE_ID | ||
| const showDashBoard = (challenge.trackId === DS_TRACK_ID && isChallengeType) || (isDevChallenge && isMM) || (isDevChallenge && isChallengeType) | ||
| const useDashboardData = _.find(challenge.metadata, { name: 'show_data_dashboard' }) | ||
| const showDashBoard = this.shouldShowDashboardSetting(challenge) |
There was a problem hiding this comment.
[maintainability]
The showDashBoard variable is reassigned using the shouldShowDashboardSetting method. Consider consolidating this logic to avoid redundant calls and ensure consistency.
| type='checkbox' | ||
| id='isDashboardEnabled' | ||
| checked={useDashboard} | ||
| onChange={(e) => this.onUpdateMetadata('show_data_dashboard', e.target.checked)} |
There was a problem hiding this comment.
[performance]
The onChange handler for the dashboard toggle checkbox directly updates metadata. Consider debouncing this update to prevent excessive state updates and potential performance issues.
https://topcoder.atlassian.net/browse/PM-2372
https://topcoder.atlassian.net/browse/PM-3170