Skip to content

PM-1833 & PM-1939 - Update Review Configuration UI #1674

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

Merged
merged 2 commits into from
Sep 18, 2025
Merged

PM-1833 & PM-1939 - Update Review Configuration UI #1674

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93d5eff
Fixed minor bugs
rishabhtc Sep 16, 2025
48494ed
PM-1939 Updated Review Configuration
rishabhtc Sep 18, 2025
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
59 changes: 51 additions & 8 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 @@ -28,6 +28,23 @@ class ChallengeReviewerField extends Component {
this.loadDefaultReviewers()
}

componentDidUpdate (prevProps) {
const { challenge } = this.props
const prevChallenge = prevProps.challenge

// Reload scorecards if challenge type or track changed
if (challenge && prevChallenge &&
(challenge.type !== prevChallenge.type || challenge.track !== prevChallenge.track)) {
this.loadScorecards()
}

// Reload default reviewers if challenge type or track changed
if (challenge && prevChallenge &&
(challenge.typeId !== prevChallenge.typeId || challenge.trackId !== prevChallenge.trackId)) {
Copy link

Choose a reason for hiding this comment

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

Consider consolidating the conditions in the if statements on lines 36 and 42, as they both check for changes in challenge.type and challenge.track. This could help reduce redundancy and improve readability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

one is type and other is typeId so not required

this.loadDefaultReviewers()
}
}

loadScorecards () {
const { challenge, loadScorecards } = this.props
// Build query parameters for the scorecard API
Expand All @@ -38,6 +55,11 @@ class ChallengeReviewerField extends Component {
filters.challengeTrack = challenge.track.toUpperCase()
}

// Add challenge type if available
if (challenge.type) {
filters.challengeType = challenge.type
}

loadScorecards(filters)
}

Expand Down Expand Up @@ -69,15 +91,21 @@ class ChallengeReviewerField extends Component {
)
const firstReviewPhase = reviewPhases && reviewPhases.length > 0 ? reviewPhases[0] : null

const isAIReviewer = (defaultReviewer && defaultReviewer.isAIReviewer) || false
Copy link

Choose a reason for hiding this comment

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

The variable isAIReviewer is defined but not used until line 98. Consider moving its declaration closer to its usage to improve readability.


const newReviewer = {
scorecardId: (defaultReviewer && defaultReviewer.scorecardId) || '',
isMemberReview: true,
memberReviewerCount: (defaultReviewer && defaultReviewer.memberReviewerCount) || 1,
isMemberReview: !isAIReviewer,
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 determining isMemberReview has changed from always being true to being dependent on isAIReviewer. Ensure that this change aligns with the intended functionality and does not introduce any unintended behavior.

phaseId: (defaultReviewer && defaultReviewer.phaseId) || (firstReviewPhase ? (firstReviewPhase.id || firstReviewPhase.phaseId) : ''),
basePayment: (defaultReviewer && defaultReviewer.basePayment) || '0',
incrementalPayment: (defaultReviewer && defaultReviewer.incrementalPayment) || 0,
type: (defaultReviewer && defaultReviewer.opportunityType) || REVIEW_OPPORTUNITY_TYPES.REGULAR_REVIEW,
isAIReviewer: (defaultReviewer && defaultReviewer.isAIReviewer) || false
isAIReviewer: isAIReviewer
}

// Only include memberReviewerCount for member reviewers
if (!isAIReviewer) {
newReviewer.memberReviewerCount = (defaultReviewer && defaultReviewer.memberReviewerCount) || 1
Copy link

Choose a reason for hiding this comment

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

The condition for setting memberReviewerCount has changed to only apply when isAIReviewer is false. Verify that this logic is correct and that memberReviewerCount should indeed be excluded for AI reviewers.

}

const updatedReviewers = currentReviewers.concat([newReviewer])
Expand Down Expand Up @@ -157,7 +185,7 @@ class ChallengeReviewerField extends Component {
return (
<div key={`reviewer-${index}`} className={styles.reviewerForm}>
<div className={styles.reviewerHeader}>
{index > 0 && <h4>Reviewer {index + 1}</h4>}
<h4>Reviewer Type {index + 1}</h4>
{!readOnly && (
<OutlineButton
text='Remove'
Expand Down Expand Up @@ -192,17 +220,23 @@ class ChallengeReviewerField extends Component {
// Update both fields atomically to ensure XOR constraint is satisfied
// Maintain correct field order as expected by API schema
const currentReviewer = updatedReviewers[index]
updatedReviewers[index] = {
const updatedReviewer = {
Copy link

Choose a reason for hiding this comment

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

The variable updatedReviewer is declared but not used. Consider using it to update updatedReviewers[index] or remove it if it's unnecessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is used on line 292

scorecardId: currentReviewer.scorecardId,
isMemberReview: !isAI,
memberReviewerCount: currentReviewer.memberReviewerCount || 1,
phaseId: currentReviewer.phaseId,
basePayment: currentReviewer.basePayment || '0',
incrementalPayment: currentReviewer.incrementalPayment || 0,
type: currentReviewer.type,
isAIReviewer: isAI
}

// Only include memberReviewerCount for member reviewers
if (!isAI) {
updatedReviewer.memberReviewerCount = currentReviewer.memberReviewerCount || 1
Copy link

Choose a reason for hiding this comment

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

Consider checking if currentReviewer.memberReviewerCount is a valid number before using it, to prevent potential issues with unexpected values.

}

updatedReviewers[index] = updatedReviewer

onUpdateReviewers({ field: 'reviewers', value: updatedReviewers })
}}
>
Expand Down Expand Up @@ -255,9 +289,18 @@ class ChallengeReviewerField extends Component {
<option value=''>Select Phase</option>
{challenge.phases && challenge.phases
.filter(phase => {
const isReviewPhase = phase.name && phase.name.toLowerCase().includes('review')
const phaseName = phase.name ? phase.name.toLowerCase() : ''
const isReviewPhase = phaseName.includes('review')
Copy link

Choose a reason for hiding this comment

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

Consider using toLowerCase() only once for phase.name to improve performance and readability. You can store the result in a variable and use it for both isReviewPhase and isSubmissionPhase checks.

const isSubmissionPhase = phaseName.includes('submission')
const isCurrentlySelected = reviewer.phaseId && ((phase.id === reviewer.phaseId) || (phase.phaseId === reviewer.phaseId))
return isReviewPhase || isCurrentlySelected

// For AI reviewers, allow both review and submission phases
// For member reviewers, only allow review phases
if (reviewer.isAIReviewer) {
return (isReviewPhase || isSubmissionPhase) || isCurrentlySelected
Copy link

Choose a reason for hiding this comment

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

The condition (isReviewPhase || isSubmissionPhase) || isCurrentlySelected can be simplified to isReviewPhase || isSubmissionPhase || isCurrentlySelected.

} else {
return isReviewPhase || isCurrentlySelected
}
})
.map(phase => (
<option key={phase.id || phase.phaseId} value={phase.phaseId || phase.id}>
Expand Down