Skip to content

Removed reviewer config for every required phase and Set Reviewer con… #1686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Removed reviewer config for every required phase and Set Reviewer con… #1686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
56 changes: 0 additions & 56 deletions src/components/ChallengeEditor/ChallengeReviewer-Field/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ class ChallengeReviewerField extends Component {
this.renderReviewerForm = this.renderReviewerForm.bind(this)
this.handleApplyDefault = this.handleApplyDefault.bind(this)
this.isAIReviewer = this.isAIReviewer.bind(this)
this.getMissingRequiredPhases = this.getMissingRequiredPhases.bind(this)
}

isAIReviewer (reviewer) {
Expand All @@ -31,49 +30,6 @@ class ChallengeReviewerField extends Component {
)
}

getMissingRequiredPhases () {
const { challenge } = this.props
const reviewers = challenge.reviewers || []
const phases = Array.isArray(challenge.phases) ? challenge.phases : []

const requiredPhaseNames = [
'Screening',
'Review',
'Post-mortem',
'Approval',
'Checkpoint Screening',
'Iterative Review'
]

const normalize = (name) => (name || '')
.toString()
.toLowerCase()
.replace(/[-\s]/g, '')

const requiredNormalized = new Set(requiredPhaseNames.map(normalize))

// Map challenge phases by normalized name to phase ids (only those we care about)
const requiredPhaseEntries = phases
.filter(p => requiredNormalized.has(normalize(p.name)))
.map(p => ({ name: p.name, id: p.phaseId || p.id }))

const missing = []
for (const entry of requiredPhaseEntries) {
const hasReviewerWithScorecard = reviewers.some(r => {
const rPhaseId = r.phaseId
const hasScorecard = !!r.scorecardId
return rPhaseId === entry.id && hasScorecard
})
if (!hasReviewerWithScorecard) {
// Use the canonical capitalization from requiredPhaseNames when possible
const canonical = requiredPhaseNames.find(n => normalize(n) === normalize(entry.name)) || entry.name
missing.push(canonical)
}
}

return missing
}

componentDidMount () {
if (this.props.challenge.track || this.props.challenge.type) {
this.loadScorecards()
Expand Down Expand Up @@ -130,7 +86,6 @@ class ChallengeReviewerField extends Component {

// only load default reviewers if we have typeId and trackId
if (!challenge.typeId || !challenge.trackId) {
console.log('Cannot load default reviewers: missing typeId or trackId')
return
}

Expand Down Expand Up @@ -625,17 +580,6 @@ class ChallengeReviewerField extends Component {
<label>Review Configuration :</label>
</div>
<div className={cn(styles.field, styles.col2)}>
{(!readOnly && challenge.submitTriggered) && (() => {
const missing = this.getMissingRequiredPhases()
if (missing.length > 0) {
return (
<div className={styles.error}>
{`Please configure a scorecard for: ${missing.join(', ')}`}
</div>
)
}
return null
})()}
Comment on lines -628 to -638
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need to remove this?

{!readOnly && (
<div className={styles.description}>
Configure how this challenge will be reviewed. You can add multiple reviewers including AI and member reviewers.
Expand Down
41 changes: 0 additions & 41 deletions src/components/ChallengeEditor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,32 +772,6 @@ class ChallengeEditor extends Component {
isValidReviewers () {
const { challenge } = this.state
const reviewers = challenge.reviewers || []
const requiredPhaseNames = [
'Screening',
'Review',
'Post-mortem',
'Approval',
'Checkpoint Screening',
'Iterative Review'
]
const normalize = (name) => (name || '')
.toString()
.toLowerCase()
.replace(/[-\s]/g, '')
const requiredNormalized = new Set(requiredPhaseNames.map(normalize))
const challengePhases = Array.isArray(challenge.phases) ? challenge.phases : []
const requiredPhaseIds = []
for (const phase of challengePhases) {
const phaseNameNorm = normalize(phase.name)
if (requiredNormalized.has(phaseNameNorm)) {
requiredPhaseIds.push(phase.phaseId || phase.id)
}
}

// If any required phase exists and no reviewers configured, it's invalid
if (reviewers.length === 0 && requiredPhaseIds.length > 0) {
return false
}
Comment on lines -797 to -800
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we want at least one reviewer for a challenge?


Copy link

Choose a reason for hiding this comment

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

The logic for checking required phases has been removed. Ensure that the removal aligns with the new requirement of making reviewer configuration optional. If the intention is to allow challenges without reviewers even when certain phases are present, this change is appropriate. Otherwise, consider implementing a new validation logic that reflects the updated requirements.

// Validate each reviewer
for (const reviewer of reviewers) {
Expand Down Expand Up @@ -827,21 +801,6 @@ class ChallengeEditor extends Component {
return false
}
}

// Enforce coverage: if the challenge has any of the required phases,
// then there must be at least one reviewer with a scorecard for that phase.
// If any required phase exists, ensure at least one reviewer with scorecard configured for it
for (const phaseId of requiredPhaseIds) {
const hasReviewerForPhase = reviewers.some(r => {
const rPhaseId = r.phaseId
const hasScorecard = !!r.scorecardId
return rPhaseId === phaseId && hasScorecard
})
if (!hasReviewerForPhase) {
return false
}
}

return true
}
Comment on lines -831 to 805
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm. as far as I can see, based on the description provided in slack, this is the only piece that makes sense to be removed. why the rest of the cleanup?

Here's the provided description:

Root Cause Analysis (RCA):
Previously, the system enforced reviewer configuration checks across all required phases (i.e., both Screening and Review) for design challenges. This meant that if reviewer configuration was missing in any of these phases, the system would trigger a validation error, even in cases where reviewer configuration was not necessary for a particular phase.
Solution:
To address this, I refactored the logic to make reviewer configuration optional for each phase. Now, if a user chooses not to set reviewer configuration in the Screening phase, but does so in the Review phase, the system will not display a validation error. This provides greater flexibility and aligns with the actual requirements for design challenges.
Additionally, following confirmation from Kiril, reviewer configuration is now set as optional. This change ensures that users are not forced to configure reviewers in phases where it is not needed


Expand Down