Conversation
PM-3342 Fix markdown rendering
PM-3834 Declutter engagement list
PM-3087 virus scan fix
…hallenge PM-3541 home points challenge
PM-3718 Wrap skills on Challenge details page
…ions-in-challenge-details PM-3686 group submissions in challenge details
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| .filter(set => set.type === 'PLACEMENT') | ||
| .flatMap(item => item.prizes); | ||
| const prizeTotal = _.sum(placementPrizes.map(prize => prize.value)); | ||
| const prizeType = placementPrizes.length > 0 ? placementPrizes[0].type : null; |
There was a problem hiding this comment.
[correctness]
The prizeType is determined by the first element of placementPrizes. If placementPrizes contains mixed types, this logic might not accurately reflect the prize type. Consider verifying that all prizes are of the same type or adjusting the logic to handle mixed types appropriately.
| styleName="rawHtml" | ||
| /> | ||
| > | ||
| <MarkdownRenderer markdown={challenge.description} /> |
There was a problem hiding this comment.
[❗❗ security]
Replacing dangerouslySetInnerHTML with MarkdownRenderer is a positive change for security, as it reduces the risk of XSS vulnerabilities. However, ensure that MarkdownRenderer itself is secure and properly sanitizes the input to prevent any potential security issues.
| const formatDate = date => moment(+new Date(date)).format('MMM DD, YYYY hh:mm A'); | ||
| const onDownloadSubmission = onDownload.bind(1, submissionObject.id); | ||
| const safeForDownloadCheck = safeForDownload(submissionObject.url); | ||
| const safeForDownloadCheck = safeForDownload(submissionObject); |
There was a problem hiding this comment.
[❗❗ correctness]
The safeForDownload function now takes submissionObject instead of submissionObject.url. Ensure that safeForDownload is designed to handle the entire object and not just the URL, as this change might affect its behavior.
| const finalScoreValue = parseScore(finalScore); | ||
| const submissionMoment = submissionTime ? moment(submissionTime) : null; | ||
|
|
||
| const timeField = isMM ? submissionTime : createdAt; |
There was a problem hiding this comment.
[correctness]
The logic for selecting timeField based on isMM could lead to unexpected behavior if both submissionTime and createdAt are null. Consider adding a fallback or handling this case explicitly to avoid potential issues.
| </div> | ||
| ) | ||
| } | ||
| <div styleName={`${isMM ? 'col-4' : 'col-3'} col ${isMM ? 'mm' : ''}`}> |
There was a problem hiding this comment.
[💡 readability]
The conditional logic for setting the class name of the div is a bit complex and could be simplified for better readability. Consider extracting the class name logic into a separate variable to improve clarity.
| }; | ||
|
|
||
| // For non-MM challenges, use createdAt field for submission date | ||
| const submissionDateField = isMM ? submissionTime : (created || createdAt); |
There was a problem hiding this comment.
[maintainability]
The logic for determining submissionDateField could be simplified by using createdAt || created, as createdAt is more likely to be the standardized field. Consider verifying which field is consistently populated and using that primarily.
| const memberLinkTarget = `${_.includes(window.origin, 'www') ? '_self' : '_blank'}`; | ||
| const memberForHistory = memberHandle || memberDisplay; | ||
| const latestSubmissionId = submissionId || 'N/A'; | ||
| const latestSubmissionId = latestSubmission.submissionId || latestSubmission.id || 'N/A'; |
There was a problem hiding this comment.
[❗❗ correctness]
The fallback logic for latestSubmissionId could lead to unexpected results if both submissionId and id are present but different. Ensure that the correct field is prioritized based on the application requirements.
| auth={auth} | ||
| isLoggedIn={isLoggedIn} | ||
| submissionId={submissionHistory.submissionId} | ||
| createdAt={submissionHistory.created || submissionHistory.createdAt} |
There was a problem hiding this comment.
[maintainability]
The logic for determining createdAt in SubmissionHistoryRow could be simplified by using submissionHistory.createdAt || submissionHistory.created, as createdAt is more likely to be the standardized field. Consider verifying which field is consistently populated and using that primarily.
| * @return {Array} grouped submissions by member | ||
| */ | ||
| function groupSubmissionsByMember(submissions) { | ||
| if (!Array.isArray(submissions)) { |
There was a problem hiding this comment.
[correctness]
Consider adding a type check for submissions to ensure it is an array of objects, as the function assumes each element has specific properties like registrant.memberHandle.
| const memberEntry = memberMap.get(memberHandle); | ||
| memberEntry.submissions.push(submission); | ||
| // Update rating to the latest | ||
| if (submission.rating !== undefined) { |
There was a problem hiding this comment.
[correctness]
The logic to update the rating assumes that the latest submission always has the most accurate rating. Confirm that this assumption holds true in all cases, as it might lead to incorrect data if the assumption is violated.
| return Array.from(memberMap.values()).map(memberGroup => ({ | ||
| ...memberGroup, | ||
| submissions: memberGroup.submissions.sort((a, b) => { | ||
| const timeA = new Date(a.created || a.createdAt).getTime(); |
There was a problem hiding this comment.
[correctness]
Ensure that created and createdAt are always valid date strings or objects. Consider adding error handling for invalid dates to prevent potential runtime errors.
| && !_.isEmpty(submission) | ||
| && submission.initialScore | ||
| const parsedScore = Number(_.get(submission, 'initialScore')); | ||
| const hasScore = Number.isFinite(parsedScore); |
There was a problem hiding this comment.
[correctness]
The conversion of initialScore to a number should handle potential NaN values more explicitly, perhaps by providing a default value or logging an error.
| } | ||
|
|
||
| const parsedScore = Number(_.get(submission, 'finalScore')); | ||
| if (!Number.isFinite(parsedScore)) { |
There was a problem hiding this comment.
[correctness]
The conversion of finalScore to a number should handle potential NaN values more explicitly, perhaps by providing a default value or logging an error.
|
|
||
| // Group submissions by member for non-MM challenges | ||
| if (!isMM) { | ||
| sortedSubmissions = groupSubmissionsByMember(sortedSubmissions); |
There was a problem hiding this comment.
[design]
The groupSubmissionsByMember function is called for non-MM challenges. Ensure that this logic is correct and that MM challenges should not be grouped by member.
| )); | ||
|
|
||
| // For non-MM submissions that are grouped by member, we need to adjust the sorting logic | ||
| const isGrouped = !isMM && submissions.length > 0 && submissions[0].submissions; |
There was a problem hiding this comment.
[correctness]
The isGrouped logic assumes that the presence of a submissions array in the first element indicates grouping. Ensure this assumption is valid for all cases.
|
|
||
| const getPrimarySubmission = (entry) => { | ||
| if (isGrouped) { | ||
| return _.get(entry, ['submissions', 0]); |
There was a problem hiding this comment.
[design]
The getPrimarySubmission function assumes that the first submission in the group is the primary one. Verify that this assumption is correct and that the first submission is always the one intended for primary operations.
| valueA = Number(valueA.score); | ||
| valueB = Number(valueB.score); | ||
| if (hasFinalScore) { | ||
| valueA = toScoreValue(_.get(primaryA, 'finalScore')); |
There was a problem hiding this comment.
[correctness]
The logic for determining valueA and valueB when sorting by 'Initial Score' or 'Final Score' should ensure that both scores are consistently available and valid. Consider adding a fallback or error handling for missing or invalid scores.
| .col-1 { | ||
| padding-left: 30px; | ||
| width: 10%; | ||
| flex: 20; |
There was a problem hiding this comment.
[maintainability]
Switching from a fixed width to a flex value without specifying a flex-basis or flex-grow might lead to unexpected layout behavior, especially if the parent container's size changes. Consider verifying the layout in different scenarios to ensure it behaves as expected.
|
|
||
| .col-2 { | ||
| width: 15%; | ||
| flex: 20; |
There was a problem hiding this comment.
[maintainability]
The change from a fixed width to flex: 20 should be reviewed to ensure that the layout remains consistent across different screen sizes and container widths. This change might affect the distribution of space among columns.
|
|
||
| .col-3 { | ||
| width: 20.5%; | ||
| flex: 20; |
There was a problem hiding this comment.
[maintainability]
Changing from a fixed width to flex: 20 could lead to layout issues if the parent container's size is not controlled. Ensure that the flex properties align with the intended design across various screen sizes.
|
|
||
| .col-4 { | ||
| width: 20.5%; | ||
| flex: 20; |
There was a problem hiding this comment.
[maintainability]
Using flex: 20 instead of a fixed width may cause layout shifts if the parent container size changes. Verify that this change maintains the desired layout across different resolutions.
|
|
||
| .col-5 { | ||
| width: 22%; | ||
| flex: 20; |
There was a problem hiding this comment.
[maintainability]
The change from a fixed width to flex: 20 should be tested to ensure that the column maintains its intended size relative to other columns, especially in responsive designs.
|
|
||
| .col-6 { | ||
| width: 22%; | ||
| flex: 20; |
There was a problem hiding this comment.
[maintainability]
Switching to flex: 20 from a fixed width might affect the column's size relative to others. Ensure that this change aligns with the overall design and layout goals.
| } | ||
|
|
||
| .col-8 { | ||
| flex: 13; |
There was a problem hiding this comment.
[maintainability]
The introduction of .col-8 with flex: 13 should be checked for consistency with other columns. Ensure that the flex values across columns achieve the desired layout and spacing.
| @@ -1,3 +1,3 @@ | |||
| import React from 'react'; | |||
| import PT from 'prop-types'; | |||
| import moment from 'moment-timezone'; | |||
There was a problem hiding this comment.
[💡 maintainability]
The moment-timezone library is imported but no longer used in the modified code. Consider removing this import to clean up unused dependencies.
| .job-infos { | ||
| display: grid; | ||
| grid-template-columns: 1.2fr 1fr 1.2fr 1.2fr 1fr 1.3fr 0.9fr 1fr 141px; | ||
| grid-template-columns: 1.2fr 1.2fr 1fr 1.3fr 0.9fr 141px; |
There was a problem hiding this comment.
[❗❗ correctness]
The change in grid-template-columns reduces the number of columns from 9 to 6. Ensure that this change aligns with the intended design and that all content is properly displayed within the new grid layout. This could potentially impact the layout and visibility of elements.
|
|
||
| if (needReload === false && mySubmissions) { | ||
| if (mySubmissions.find(item => safeForDownload(item.url) !== true)) { | ||
| if (mySubmissions.find(item => safeForDownload(item) !== true)) { |
There was a problem hiding this comment.
[❗❗ correctness]
The change from safeForDownload(item.url) to safeForDownload(item) suggests that the safeForDownload function now expects the entire item object rather than just the url. Ensure that safeForDownload is designed to handle the entire object and that this change is intentional. If safeForDownload still expects a URL, this change could lead to incorrect behavior.
| if (nextChallengeId | ||
| && nextChallengeId !== previousChallengeId | ||
| && checkIsMM(nextProps.challenge)) { | ||
| && nextChallengeId !== previousChallengeId) { |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of checkIsMM(nextProps.challenge) from the condition might lead to incorrect behavior if fetchChallengeStatistics is intended to be called only for MM challenges. Please verify if this change is intentional.
| const lookupActions = actions.lookup; | ||
|
|
||
| const dispatchReviewSummations = (challengeId, tokenV3) => { | ||
| const dispatchReviewSummations = (challengeId, tokenV3, options = {}) => { |
There was a problem hiding this comment.
[design]
The dispatchReviewSummations function now accepts an options parameter with default values. Ensure that all calls to this function are updated to pass the appropriate options, or confirm that the default behavior is intended.
| }, | ||
| fetchChallengeStatistics: (tokens, challengeDetails) => { | ||
| if (!tokens || !tokens.tokenV3 || !challengeDetails || !checkIsMM(challengeDetails)) { | ||
| if (!challengeDetails) { |
There was a problem hiding this comment.
[❗❗ security]
The check for tokens and tokens.tokenV3 has been removed. This could lead to potential issues if tokens is null or undefined. Consider adding a check to ensure tokens is valid before proceeding.
| Authorization: `Bearer ${tokenV3}`, | ||
| }); | ||
| const headers = new Headers(); | ||
| if (tokenV3) { |
There was a problem hiding this comment.
[security]
Consider adding a check for the validity of tokenV3 before using it in the Authorization header. This ensures that the token is not only present but also in a valid format, which can prevent potential issues with malformed tokens.
|
|
||
| if (url.toLowerCase().indexOf('submissions-quarantine/') !== -1) { | ||
| const { url } = submission; | ||
| if (url.toLowerCase().indexOf('submissions-quarantine/') !== -1 || submission.virusScan === false) { |
There was a problem hiding this comment.
[❗❗ correctness]
The condition submission.virusScan === false and !submission.virusScan are not equivalent. If submission.virusScan is undefined, the second condition will evaluate to true, potentially leading to incorrect results. Consider explicitly checking for false in both conditions for consistency.
4 participants
No description provided.