PM-1833 & PM-1939 - Update Review Configuration UI #1674
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // Reload default reviewers if challenge type or track changed | ||
| if (challenge && prevChallenge && | ||
| (challenge.typeId !== prevChallenge.typeId || challenge.trackId !== prevChallenge.trackId)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
one is type and other is typeId so not required
| ) | ||
| const firstReviewPhase = reviewPhases && reviewPhases.length > 0 ? reviewPhases[0] : null | ||
|
|
||
| const isAIReviewer = (defaultReviewer && defaultReviewer.isAIReviewer) || false |
There was a problem hiding this comment.
The variable isAIReviewer is defined but not used until line 98. Consider moving its declaration closer to its usage to improve readability.
| scorecardId: (defaultReviewer && defaultReviewer.scorecardId) || '', | ||
| isMemberReview: true, | ||
| memberReviewerCount: (defaultReviewer && defaultReviewer.memberReviewerCount) || 1, | ||
| isMemberReview: !isAIReviewer, |
There was a problem hiding this comment.
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.
|
|
||
| // Only include memberReviewerCount for member reviewers | ||
| if (!isAIReviewer) { | ||
| newReviewer.memberReviewerCount = (defaultReviewer && defaultReviewer.memberReviewerCount) || 1 |
There was a problem hiding this comment.
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.
| // Maintain correct field order as expected by API schema | ||
| const currentReviewer = updatedReviewers[index] | ||
| updatedReviewers[index] = { | ||
| const updatedReviewer = { |
There was a problem hiding this comment.
The variable updatedReviewer is declared but not used. Consider using it to update updatedReviewers[index] or remove it if it's unnecessary.
There was a problem hiding this comment.
it is used on line 292
|
|
||
| // Only include memberReviewerCount for member reviewers | ||
| if (!isAI) { | ||
| updatedReviewer.memberReviewerCount = currentReviewer.memberReviewerCount || 1 |
There was a problem hiding this comment.
Consider checking if currentReviewer.memberReviewerCount is a valid number before using it, to prevent potential issues with unexpected values.
| .filter(phase => { | ||
| const isReviewPhase = phase.name && phase.name.toLowerCase().includes('review') | ||
| const phaseName = phase.name ? phase.name.toLowerCase() : '' | ||
| const isReviewPhase = phaseName.includes('review') |
There was a problem hiding this comment.
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.
| // For AI reviewers, allow both review and submission phases | ||
| // For member reviewers, only allow review phases | ||
| if (reviewer.isAIReviewer) { | ||
| return (isReviewPhase || isSubmissionPhase) || isCurrentlySelected |
There was a problem hiding this comment.
The condition (isReviewPhase || isSubmissionPhase) || isCurrentlySelected can be simplified to isReviewPhase || isSubmissionPhase || isCurrentlySelected.
rishabhtc
changed the title
| {readOnly ? ( | ||
| <span> | ||
| {(() => { | ||
| const typeMap = { |
There was a problem hiding this comment.
this map either already exists somewhere, or you have to create it in a config/constants file. this is bad practice (to create it in the render function, it gets recreated a log of times).
3 participants
No description provided.