Validate reviewer errors on saving challenge #1682
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/components/ChallengeEditor/ChallengeReviewer-Field/ChallengeReviewer-Field.module.scss
Show resolved
Hide resolved
| return false | ||
| } | ||
|
|
||
| const memberCount = parseInt(reviewer.memberReviewerCount) || 1 |
There was a problem hiding this comment.
The parseInt function should specify a radix to avoid unexpected behavior. Consider using parseInt(reviewer.memberReviewerCount, 10).
| <span> | ||
| {(() => { | ||
| // In read-only mode, try to get the specific scorecard first | ||
| if (this.props.readOnly) { |
There was a problem hiding this comment.
The condition this.props.readOnly is already checked before this block, so it might be redundant to check it again here. Consider removing the redundant check.
| {(() => { | ||
| // In read-only mode, try to get the specific scorecard first | ||
| if (this.props.readOnly) { | ||
| const { metadata = {} } = this.props |
There was a problem hiding this comment.
Consider destructuring metadata and scorecardById directly from this.props for cleaner code.
src/components/ChallengeEditor/ChallengeReviewer-Field/index.js
Outdated
Show resolved
Hide resolved
| defaultReviewers: PropTypes.array, | ||
| workflows: PropTypes.array | ||
| workflows: PropTypes.array, | ||
| scorecardById: PropTypes.object |
There was a problem hiding this comment.
Consider adding a default value for scorecardById in metadata to prevent potential undefined errors.
| const { metadata = {} } = this.props | ||
| const specificScorecard = metadata.scorecardById | ||
| if (specificScorecard && specificScorecard.id === reviewer.scorecardId) { | ||
| return `${specificScorecard.name || 'Unknown'} - ${specificScorecard.type || 'Unknown'} (${specificScorecard.challengeTrack || 'Unknown'}) v${specificScorecard.version || 'Unknown'}` |
There was a problem hiding this comment.
Consider handling the case where specificScorecard.version is undefined or null separately, as using 'Unknown' for a version number might not be appropriate.
| // Fallback to searching in the general scorecards array | ||
| const scorecard = scorecards.find(s => s.id === reviewer.scorecardId) | ||
| return scorecard ? `${scorecard.name} - ${scorecard.type} (${scorecard.challengeTrack}) v${scorecard.version}` : 'Not selected' | ||
| return scorecard ? `${scorecard.name || 'Unknown'} - ${scorecard.type || 'Unknown'} (${scorecard.challengeTrack || 'Unknown'}) v${scorecard.version || 'Unknown'}` : 'Not selected' |
There was a problem hiding this comment.
Consider handling the case where scorecard.version is undefined or null separately, as using 'Unknown' for a version number might not be appropriate.
| {scorecards.map(scorecard => ( | ||
| <option key={scorecard.id} value={scorecard.id}> | ||
| {scorecard.name} - {scorecard.type} ({scorecard.challengeTrack}) v{scorecard.version} | ||
| {scorecard.name || 'Unknown'} - {scorecard.type || 'Unknown'} ({scorecard.challengeTrack || 'Unknown'}) v{scorecard.version || 'Unknown'} |
There was a problem hiding this comment.
Consider using a more descriptive placeholder than 'Unknown' for missing scorecard details to provide clearer context to the user. For example, 'Name not available', 'Type not specified', etc.
No description provided.