-
Notifications
You must be signed in to change notification settings - Fork 21
Fix 2446 #1268
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.
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
Fix 2446 #1268
Conversation
| columnId: `score-${index}`, | ||
| label: `Score ${index + 1}`, | ||
| renderer: (submission: SubmissionRow) => renderScoreCell(submission, index, scoreVisibilityConfig), | ||
| renderer: (submission: SubmissionRow) => renderScoreCell( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
The renderScoreCell function now takes additional parameters: challengeInfo, pendingReopen, canManageCompletedReviews, isReopening, and openReopenDialog. Ensure that renderScoreCell is updated to handle these new parameters appropriately, and verify that they are necessary for its logic. If any of these parameters are not used within renderScoreCell, consider removing them to maintain clarity and simplicity.
| return | ||
| } | ||
|
|
||
| if (!isReviewPhaseCurrentlyOpen(challengeInfo, reviewInfo.phaseId)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The function isReviewPhaseCurrentlyOpen is used to check if the review phase is open. Ensure that this function handles all edge cases, such as time zone differences or unexpected phase IDs, to prevent incorrect button rendering.
| buttons.push( | ||
| // eslint-disable-next-line jsx-a11y/control-has-associated-label | ||
| <button | ||
| key={`reopen-${reviewInfo.id}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[accessibility]
Consider adding an accessible label to the button to improve accessibility for screen readers. This will help users who rely on assistive technologies to understand the purpose of the button.
| canManageCompletedReviews: !!canManageCompletedReviews, | ||
| isReopening: !!isReopening, | ||
| openReopenDialog: openReopenDialog as (submission: SubmissionRow, review: AggregatedReviewDetail) => void, | ||
| pendingReopen, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
The openReopenDialog function is cast to a specific type. Ensure that the function passed in matches this type signature to avoid runtime errors.
|
|
||
| <MarkdownReview | ||
| value={commentItem.content} | ||
| value={commentItem.content.replace(/\n\n/g, '<br><br>')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ security]
Using replace to convert newlines to <br> tags could lead to unexpected behavior if commentItem.content contains HTML or special characters. Consider using a library like DOMPurify to sanitize the content before rendering it as HTML to prevent XSS vulnerabilities.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-2446
https://topcoder.atlassian.net/browse/PM-2356
What's in this PR?
Adds reopen icon beside individual scores
\n\n is being converted to
<p>by markdown. Replaced it with<br>before passing it to markdown