Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@
flex-direction: column;
width: 600px;
}

.fieldError {
margin-top: 12px;
}
}
}

Expand Down
49 changes: 47 additions & 2 deletions src/components/ChallengeEditor/ChallengeReviewer-Field/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,11 @@ class ChallengeReviewerField extends Component {
newReviewer.memberReviewerCount = (defaultReviewer && defaultReviewer.memberReviewerCount) || 1
}

// Clear any prior transient error when add succeeds
if (this.state.error) {
this.setState({ error: null })
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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 updatedReviewers = currentReviewers.concat([newReviewer])
onUpdateReviewers({ field: 'reviewers', value: updatedReviewers })
}
Expand Down Expand Up @@ -513,6 +518,21 @@ class ChallengeReviewerField extends Component {

// Special handling for phase and count changes
if (field === 'phaseId') {
// Before changing phase, ensure we're not creating a duplicate manual reviewer for the target phase
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))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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 (conflict) {
const phase = (challenge.phases || []).find(p => (p.id === targetPhaseId) || (p.phaseId === targetPhaseId))
const phaseName = phase ? (phase.name || targetPhaseId) : targetPhaseId
this.setState({
error: `Cannot move manual reviewer to phase '${phaseName}' because a manual reviewer configuration already exists for that phase.`
})
return
}
}

this.handlePhaseChangeWithReassign(index, value)

// update payment based on default reviewer
Expand Down Expand Up @@ -632,6 +652,21 @@ class ChallengeReviewerField extends Component {
const currentReviewers = challenge.reviewers || []
const updatedReviewers = currentReviewers.slice()

// Block switching an AI reviewer to a member reviewer if another manual reviewer exists for same phase
if (!isAI) {
const existingReviewer = currentReviewers[index] || {}
const phaseId = existingReviewer.phaseId
const conflict = currentReviewers.some((r, i) => i !== index && (r.isMemberReview !== false) && (r.phaseId === phaseId))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

if (conflict) {
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.`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 maintainability]
Consider centralizing error message handling to avoid duplication and potential inconsistencies. This can help with maintainability and ensure consistent messaging across the application.

})
return
}
}

// Update reviewer type by setting/clearing aiWorkflowId
const currentReviewer = updatedReviewers[index]

Expand Down Expand Up @@ -674,6 +709,11 @@ class ChallengeReviewerField extends Component {
this.handleToggleShouldOpen(index, true)
}

// Clear any transient error when successful change is applied
if (this.state.error) {
this.setState({ error: null })
}

onUpdateReviewers({ field: 'reviewers', value: updatedReviewers })
}}
>
Expand Down Expand Up @@ -772,10 +812,10 @@ class ChallengeReviewerField extends Component {
const isPostMortemPhase = norm === 'postmortem'
const isCurrentlySelected = reviewer.phaseId && ((phase.id === reviewer.phaseId) || (phase.phaseId === reviewer.phaseId)) && !isSubmissionPhase

// Collect phases already assigned to other reviewers (excluding current reviewer)
// Collect phases already assigned to other manual (member) reviewers (excluding current reviewer)
const assignedPhaseIds = new Set(
(challenge.reviewers || [])
.filter((r, i) => i !== index)
.filter((r, i) => i !== index && (r.isMemberReview !== false))
.map(r => r.phaseId)
.filter(id => id !== undefined && id !== null)
)
Expand Down Expand Up @@ -1051,6 +1091,11 @@ class ChallengeReviewerField extends Component {
/>
</div>
)}
{error && !isLoading && (
<div className={cn(styles.fieldError, styles.error)}>
{error}
</div>
)}
</div>
</div>
</>
Expand Down
35 changes: 27 additions & 8 deletions src/components/ChallengeEditor/ChallengeView/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ import {
PHASE_PRODUCT_CHALLENGE_ID_FIELD,
MULTI_ROUND_CHALLENGE_TEMPLATE_ID,
DS_TRACK_ID,
DEV_TRACK_ID,
MARATHON_TYPE_ID,
CHALLENGE_TYPE_ID,
COMMUNITY_APP_URL
} from '../../../config/constants'
import PhaseInput from '../../PhaseInput'
Expand Down Expand Up @@ -94,11 +97,33 @@ const ChallengeView = ({
const isTask = _.get(challenge, 'task.isTask', false)
const phases = _.get(challenge, 'phases', [])
const showCheckpointPrizes = _.get(challenge, 'timelineTemplateId') === MULTI_ROUND_CHALLENGE_TEMPLATE_ID
const isDataScience = challenge.trackId === DS_TRACK_ID
const useDashboardData = _.find(challenge.metadata, { name: 'show_data_dashboard' })
const useDashboard = useDashboardData
? (_.isString(useDashboardData.value) && useDashboardData.value === 'true') ||
(_.isBoolean(useDashboardData.value) && useDashboardData.value) : false
const showDashBoard = (() => {
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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

})()
const dashboardToggle = showDashBoard && (
<div className={styles.row}>
<div className={styles.col}>
<label className={styles.fieldTitle} htmlFor='isDashboardEnabled'>Use data dashboard :</label>
</div>
<div className={styles.col}>
<input
name='isDashboardEnabled'
type='checkbox'
id='isDashboardEnabled'
checked={useDashboard}
readOnly
disabled
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

/>
</div>
</div>
)

return (
<div className={styles.wrapper}>
Expand Down Expand Up @@ -138,13 +163,6 @@ const ChallengeView = ({
</a></span>
</div>
</div>
{isDataScience && (
<div className={cn(styles.row, styles.topRow)}>
<div className={styles.col}>
<span><span className={styles.fieldTitle}>Show data dashboard:</span> {useDashboard ? 'Yes' : 'No'}</span>
</div>
</div>
)}
{isTask &&
<AssignedMemberField challenge={challenge} assignedMemberDetails={assignedMemberDetails} readOnly />}
<CopilotField challenge={{
Expand Down Expand Up @@ -175,6 +193,7 @@ const ChallengeView = ({
</div>
{openAdvanceSettings && (
<>
{dashboardToggle}
<NDAField beta challenge={challenge} readOnly />
<div className={cn(styles.row, styles.topRow)}>
<div className={styles.col}>
Expand Down
116 changes: 73 additions & 43 deletions src/components/ChallengeEditor/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
MILESTONE_STATUS,
PHASE_PRODUCT_CHALLENGE_ID_FIELD,
QA_TRACK_ID, DESIGN_CHALLENGE_TYPES, ROUND_TYPES,
MULTI_ROUND_CHALLENGE_TEMPLATE_ID, DS_TRACK_ID,
MULTI_ROUND_CHALLENGE_TEMPLATE_ID,
CHALLENGE_STATUS,
SKILLS_OPTIONAL_BILLING_ACCOUNT_IDS
} from '../../config/constants'
Expand Down Expand Up @@ -148,6 +148,7 @@ class ChallengeEditor extends Component {
this.onSaveChallenge = this.onSaveChallenge.bind(this)
this.getCurrentTemplate = this.getCurrentTemplate.bind(this)
this.onUpdateMetadata = this.onUpdateMetadata.bind(this)
this.shouldShowDashboardSetting = this.shouldShowDashboardSetting.bind(this)
this.getTemplatePhases = this.getTemplatePhases.bind(this)
this.getAvailableTimelineTemplates = this.getAvailableTimelineTemplates.bind(this)
this.autoUpdateChallengeThrottled = _.throttle(this.validateAndAutoUpdateChallenge.bind(this), 3000) // 3s
Expand Down Expand Up @@ -669,6 +670,20 @@ class ChallengeEditor extends Component {
this.setState({ challenge: newChallenge })
}

/**
* Determines when the data dashboard toggle should be shown.
*
* @param {Object} challenge the challenge data to evaluate
*/
shouldShowDashboardSetting (challenge = {}) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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 typeId = _.get(challenge, 'typeId')
const metadata = _.get(challenge, 'metadata', [])
const hasDashboardMetadata = _.some(metadata, { name: 'show_data_dashboard' })
const isMarathonMatch = typeId === MARATHON_TYPE_ID

return isMarathonMatch || hasDashboardMetadata
}

/**
* Remove Phase from challenge Phases list
* @param index
Expand Down Expand Up @@ -897,11 +912,47 @@ class ChallengeEditor extends Component {
return !(isRequiredMissing || _.isEmpty(this.state.currentTemplate))
}

// Return array of phase names that have more than one manual (member) reviewer configured.
// If none, returns empty array.
getDuplicateManualReviewerPhases () {
const { challenge } = this.state
const reviewers = (challenge && challenge.reviewers) || []
const phases = (challenge && challenge.phases) || []

const counts = {}
reviewers.forEach(r => {
if (r && (r.isMemberReview !== false) && r.phaseId) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 readability]
In getDuplicateManualReviewerPhases, the condition r && (r.isMemberReview !== false) && r.phaseId could be simplified to r?.isMemberReview !== false && r.phaseId for better readability.

const pid = String(r.phaseId)
counts[pid] = (counts[pid] || 0) + 1
}
})

const duplicatedPhaseIds = Object.keys(counts).filter(pid => counts[pid] > 1)
if (duplicatedPhaseIds.length === 0) return []

return duplicatedPhaseIds.map(pid => {
const p = phases.find(ph => String(ph.phaseId || ph.id) === pid)
return p ? (p.name || pid) : pid
})
}

validateChallenge () {
if (this.isValidChallenge()) {
// Additional validation: block saving draft if there are duplicate manual reviewer configs per phase
const duplicates = this.getDuplicateManualReviewerPhases()
if (duplicates && duplicates.length > 0) {
const message = `Duplicate manual reviewer configuration found for phase(s): ${duplicates.join(', ')}. Only one manual reviewer configuration is allowed per phase.`
this.setState({ hasValidationErrors: true, error: message })
return false
}

if (this.state.error) {
this.setState({ error: null })
}
this.setState({ hasValidationErrors: false })
return true
}

this.setState(prevState => ({
...prevState,
challenge: {
Expand Down Expand Up @@ -1095,11 +1146,7 @@ class ChallengeEditor extends Component {
const { challenge: { name, trackId, typeId, milestoneId, roundType, challengeType, metadata: challengeMetadata } } = this.state
const { timelineTemplates } = metadata
const isDesignChallenge = trackId === DES_TRACK_ID
const isDataScience = trackId === DS_TRACK_ID
const isChallengeType = typeId === CHALLENGE_TYPE_ID
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 })
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.


// indicate that creating process has started
this.setState({ isSaving: true })
Expand Down Expand Up @@ -1754,18 +1801,33 @@ class ChallengeEditor extends Component {
const showTimeline = false // disables the timeline for time being https://github.com/topcoder-platform/challenge-engine-ui/issues/706
const copilotResources = metadata.members || challengeResources
const isDesignChallenge = challenge.trackId === DES_TRACK_ID
const isDevChallenge = challenge.trackId === DEV_TRACK_ID
const isMM = challenge.typeId === MARATHON_TYPE_ID
const isChallengeType = challenge.typeId === CHALLENGE_TYPE_ID
const showRoundType = isDesignChallenge && isChallengeType
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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ maintainability]
The showDashBoard variable is reassigned using the shouldShowDashboardSetting method. Consider consolidating this logic to avoid redundant calls and ensure consistency.


const useDashboard = useDashboardData
? (_.isString(useDashboardData.value) && useDashboardData.value === 'true') ||
(_.isBoolean(useDashboardData.value) && useDashboardData.value) : false

const dashboardToggle = showDashBoard && (
<div className={styles.row}>
<div className={cn(styles.field, styles.col1)}>
<label htmlFor='isDashboardEnabled'>Use data dashboard :</label>
</div>
<div className={cn(styles.field, styles.col2)}>
<input
name='isDashboardEnabled'
type='checkbox'
id='isDashboardEnabled'
checked={useDashboard}
onChange={(e) => this.onUpdateMetadata('show_data_dashboard', e.target.checked)}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

/>
</div>
</div>
)

const workTypes = getDomainTypes(challenge.trackId)
let filteredTypes = metadata.challengeTypes.filter(type => workTypes.includes(type.abbreviation))

Expand Down Expand Up @@ -1794,22 +1856,7 @@ class ChallengeEditor extends Component {
}
<ChallengeNameField challenge={challenge} onUpdateInput={this.onUpdateInput} />
{
showDashBoard && (
<div className={styles.row}>
<div className={cn(styles.field, styles.col1)}>
<label htmlFor='isDashboardEnabled'>Use data dashboard :</label>
</div>
<div className={cn(styles.field, styles.col2)}>
<input
name='isDashboardEnabled'
type='checkbox'
id='isDashboardEnabled'
checked={useDashboard}
onChange={(e) => this.onUpdateMetadata('show_data_dashboard', e.target.checked)}
/>
</div>
</div>
)
dashboardToggle
}
{projectDetail.version === 'v4' && <MilestoneField milestones={activeProjectMilestones} onUpdateSelect={this.onUpdateSelect} projectId={projectDetail.id} selectedMilestoneId={selectedMilestoneId} />}
{useTask && (<DiscussionField hasForum={hasForum} toggleForum={this.toggleForumOnCreate} />)}
Expand Down Expand Up @@ -1841,24 +1888,6 @@ class ChallengeEditor extends Component {
</div>

<ChallengeNameField challenge={challenge} onUpdateInput={this.onUpdateInput} />
{
showDashBoard && (
<div className={styles.row}>
<div className={cn(styles.field, styles.col1)}>
<label htmlFor='isDashboardEnabled'>Use data dashboard :</label>
</div>
<div className={cn(styles.field, styles.col2)}>
<input
name='isDashboardEnabled'
type='checkbox'
id='isDashboardEnabled'
checked={useDashboard}
onChange={(e) => this.onUpdateMetadata('show_data_dashboard', e.target.checked)}
/>
</div>
</div>
)
}
{isTask && (
<AssignedMemberField
challenge={challenge}
Expand Down Expand Up @@ -1890,6 +1919,7 @@ class ChallengeEditor extends Component {
</div>
{isOpenAdvanceSettings && (
<React.Fragment>
{dashboardToggle}
<NDAField challenge={challenge} toggleNdaRequire={this.toggleNdaRequire} />
{/* remove terms field and use default term */}
{false && (<TermsField terms={metadata.challengeTerms} challenge={challenge} onUpdateMultiSelect={this.onUpdateMultiSelect} />)}
Expand Down
Loading