[PROD RELEASE V6] #7128
[PROD RELEASE V6] #7128
Conversation
update the review opportunity bucket to work with new review api Fixes #1760
normalise response and test Fixes #PM-1760
Previously, the filter function was defined in topcoder-react-lib Fixes #1760
remove unnecessary param Fixes #1760
Adds error handling to the API call for fetching review opportunities Fixes #1760
migrates review opportunity details page to v6 review api fixes #1761
ai review feedbacks on return types and logging Fixes #1760
removes link for how to be a reviewer, we can add it later when process is in place fixes #1761
migrate: review opportunity details page to v6 review API
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| id, | ||
| track, | ||
| } = challenge; | ||
| const trackName = (track && typeof track === 'object') ? (track.name || '') : track; |
There was a problem hiding this comment.
[💡 readability]
Consider using a more descriptive variable name than trackName to clarify that it can be either a string or an object with a name property. This could improve readability and maintainability.
| <div styleName="details-footer"> | ||
| <span styleName="date"> | ||
| {challenge.status === 'Active' ? 'Ends ' : 'Ended '} | ||
| {challenge.status === 'ACTIVE' ? 'Ends ' : 'Ended '} |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that changing the challenge status comparison from 'Active' to 'ACTIVE' aligns with the rest of the application logic and data sources. This change could impact the correctness if other parts of the codebase still use 'Active'.
| if (!_.includes(roles, 'administrator')) { | ||
| filteredChallenges = sortedChallenges.filter((ch) => { | ||
| if (ch.type === 'Task' | ||
| const typeName = getTypeName(ch); |
There was a problem hiding this comment.
[performance]
The function getTypeName(ch) is called twice for the same challenge object within the same scope. Consider storing the result in a variable to avoid redundant function calls and improve performance.
| <ReviewOpportunityCard | ||
| challengesUrl={challengesUrl} | ||
| challengeType={_.find(challengeTypes, { name: item.challenge.type }) || {}} | ||
| challengeType={_.find(challengeTypes, { name: item.challengeData.track }) || {}} |
There was a problem hiding this comment.
[❗❗ correctness]
The change from item.challenge.type to item.challengeData.track should be verified to ensure that challengeData.track is always available and correctly represents the intended data. This change could impact the correctness of the challenge type mapping.
| } | ||
| { | ||
| loadMore && !loading && filterState.reviewOpportunityTypes.length ? ( | ||
| loadMore && !loading ? ( |
There was a problem hiding this comment.
[correctness]
Removing the check for filterState.reviewOpportunityTypes.length could lead to unintended behavior if loadMore is triggered without any review opportunity types being selected. Ensure this change aligns with the intended functionality.
| challengeType, | ||
| }) { | ||
| const { challenge } = opportunity; | ||
| const { challengeData: challenge } = opportunity; |
There was a problem hiding this comment.
[❗❗ correctness]
The destructuring assignment const { challengeData: challenge } = opportunity; changes the property name from challenge to challengeData. Ensure that this change is intentional and consistent with the rest of the codebase, as it could lead to confusion or errors if other parts of the code expect challenge.
| ]); | ||
| tags = tags.filter(tag => tag.trim().length); | ||
| const { track } = challenge.track; | ||
| const { track } = challenge; |
There was a problem hiding this comment.
[❗❗ correctness]
The line const { track } = challenge; appears to be corrected from const { track } = challenge.track;. Verify that challenge.track is not an object, as this change assumes track is a direct property of challenge.
| <div styleName="challenge-details"> | ||
| <Link | ||
| to={`${challengesUrl}/${challenge.id}`} | ||
| to={`${challengesUrl}/${opportunity.challengeId}`} |
There was a problem hiding this comment.
[❗❗ correctness]
The URL construction to={${challengesUrl}/${opportunity.challengeId}} changes from using challenge.id to opportunity.challengeId. Ensure that opportunity.challengeId is the correct identifier for the URL, as this could affect navigation if incorrect.
| </div> | ||
| <Link | ||
| to={`/challenges/${challenge.legacyId || challenge.id}/review-opportunities`} | ||
| to={`/challenges/${opportunity.challengeId}/review-opportunities?opportunityId=${opportunity.id}`} |
There was a problem hiding this comment.
[❗❗ correctness]
The URL construction to={/challenges/${opportunity.challengeId}/review-opportunities?opportunityId=${opportunity.id}} changes from using challenge.legacyId || challenge.id to opportunity.challengeId. Ensure that opportunity.challengeId is the correct identifier for the URL, as this could affect navigation if incorrect.
| <span> | ||
| Late by<br /> | ||
| { start.isAfter() ? formatDuration(start.diff()) : ` ${formatDuration(-start.diff())}` } | ||
| {isLate ? 'Late by' : 'Time left'}<br /> |
There was a problem hiding this comment.
[correctness]
The logic for determining if the opportunity is late has been changed to use isLate ? 'Late by' : 'Time left'. Ensure that the isLate calculation using now.isAfter(start) is correct and that the formatting of the duration is accurate.
|
|
||
| let TrackTag; | ||
| switch (track.toLowerCase()) { | ||
| switch ((getTrackName(track) || '').toLowerCase()) { |
There was a problem hiding this comment.
[correctness]
The use of getTrackName(track) || '' suggests that getTrackName might return null or undefined. Ensure that getTrackName handles all possible inputs gracefully and returns a valid string or explicitly handles errors. This will prevent unexpected behavior if track is not recognized.
| cancelApplications(challengeId, rolesToCancel, tokenV3); | ||
| await loadDetails(challengeId, opportunityId, tokenV3); | ||
| } catch (err) { | ||
| logger.error('submitApplications failed', err); |
There was a problem hiding this comment.
[maintainability]
Consider handling the error more gracefully by providing user feedback or retrying the operation. Simply logging the error might not be sufficient for a better user experience.
|
|
||
| toggleApplyModal(); | ||
| loadDetails(challengeId, tokenV3); | ||
| loadDetails(challengeId, opportunityId, tokenV3); |
There was a problem hiding this comment.
[performance]
The call to loadDetails(challengeId, opportunityId, tokenV3) is duplicated after the catch block. This could lead to unnecessary API calls if the error is caught. Consider removing the duplicate call.
| dispatch(api.getDetailsDone(challengeId, tokenV3)); | ||
| loadDetails: (challengeId, opportunityId, tokenV3) => { | ||
| dispatch(page.getDetailsInit()); | ||
| return dispatch(page.getDetailsDone(challengeId, opportunityId, tokenV3)); |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that page.getDetailsDone(challengeId, opportunityId, tokenV3) handles the case where opportunityId might be null or undefined. This could lead to unexpected behavior if not properly validated.
| setState(s => ({ ...s, loading: true, error: null })); | ||
|
|
||
| try { | ||
| const res = await fetch(`${config.API.V6}/reports${reportsPath}`); |
There was a problem hiding this comment.
[performance]
Consider adding a timeout to the fetch request to handle cases where the network is slow or unresponsive. This will prevent the application from hanging indefinitely.
| }); | ||
| } | ||
| } catch (err) { | ||
| if (!cancelled) { |
There was a problem hiding this comment.
[maintainability]
The error handling here only sets the error message in the state but does not log or report the error. Consider logging the error for better debugging and monitoring.
| if (!reportsPath) { | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| // eslint-disable-next-line no-console | ||
| console.error(`SmartLooker: no reports mapping for Looker ID ${lookerId}`); |
There was a problem hiding this comment.
[💡 maintainability]
Using console.error directly in the component can lead to noisy logs in development. Consider using a logging utility that can be toggled or configured based on the environment.
| } | ||
|
|
||
| SmartLooker.propTypes = { | ||
| lookerId: PT.string.isRequired, |
There was a problem hiding this comment.
[correctness]
The lookerId prop is marked as required but is expected to be a string. Ensure that the component handles non-string values gracefully, as this could lead to runtime errors if incorrect types are passed.
| } | ||
|
|
||
| return { | ||
| ...submission, |
There was a problem hiding this comment.
[maintainability]
The mergeSubmissionWithSummations function adds both reviewSummations and reviewSummation properties to the submission object, which appear to contain the same data. This redundancy can lead to confusion and potential bugs. Consider removing one of these properties to improve clarity and maintainability.
| this.pendingReviewSummationChallengeId = challengeIdStr; | ||
|
|
||
| try { | ||
| const { data } = await getReviewSummationsService(tokenV3, challengeIdStr); |
There was a problem hiding this comment.
[correctness]
The loadReviewSummations function does not handle errors from getReviewSummationsService in a way that provides feedback to the user or logs the error for debugging purposes. Consider adding error logging or user feedback to improve the robustness of the application.
| this.refreshSubmissionScores(); | ||
| }); | ||
| } catch (error) { | ||
| if (!this.isComponentMounted || this.pendingReviewSummationChallengeId !== challengeIdStr) { |
There was a problem hiding this comment.
[maintainability]
In the catch block of loadReviewSummations, the error is silently ignored, which can make debugging difficult. Consider logging the error or providing user feedback to improve error handling.
| tokenV2: state.auth.tokenV2, | ||
| tokenV3: state.auth.tokenV3, | ||
| track: details.track, | ||
| track: (details && details.track && details.track.name) ? details.track.name : details.track, |
There was a problem hiding this comment.
[correctness]
The conditional (details && details.track && details.track.name) ? details.track.name : details.track could potentially lead to an error if details.track is undefined and details.track.name is accessed. Consider using optional chaining (details?.track?.name ?? details.track) to safely handle undefined values.
| this.initialDefaultChallengeTypes = true; | ||
| } else if (validTypes.length && currentTypes.length) { | ||
| const validAbbreviations = validTypes.map(item => item.abbreviation); | ||
| const sanitizedTypes = currentTypes.filter(type => validAbbreviations.includes(type)); |
There was a problem hiding this comment.
[❗❗ correctness]
The sanitizedTypes filtering logic assumes that currentTypes is always an array. Consider adding a check to ensure currentTypes is indeed an array before calling .filter() to prevent potential runtime errors.
| function mapStateToProps(state, ownProps) { | ||
| const cl = state.challengeListing; | ||
| const tc = state.tcCommunities; | ||
| const filteredChallengeTypes = cl.challengeTypes |
There was a problem hiding this comment.
[maintainability]
The filtering logic for filteredChallengeTypes and excludedTypeAbbreviations is duplicated. Consider refactoring this into a utility function to improve maintainability and reduce code duplication.
| .filter(type => EXCLUDED_CHALLENGE_TYPE_NAMES.includes(type.name)) | ||
| .map(type => type.abbreviation); | ||
| let filterState = cl.filter; | ||
| const existingTypes = Array.isArray(cl.filter.types) ? cl.filter.types : []; |
There was a problem hiding this comment.
[❗❗ correctness]
The check Array.isArray(cl.filter.types) ensures existingTypes is an array, but similar checks should be applied before using .filter() on existingTypes to avoid potential runtime errors if cl.filter.types is unexpectedly not an array.
| if (!allReviewOpportunitiesLoaded) { | ||
| loadMoreReviewOpportunities = () => getReviewOpportunities( | ||
| 1 + lastRequestedPageOfReviewOpportunities, tokenV3, | ||
| 1 + lastRequestedPageOfReviewOpportunities, |
There was a problem hiding this comment.
[❗❗ security]
The tokenV3 parameter has been removed from the getReviewOpportunities function call. Ensure that this change is intentional and that the function does not require the token for authentication or authorization purposes.
| dispatch(a.getPastChallengesDone(uuid, page, filter, token, frontFilter)); | ||
| }, | ||
| getReviewOpportunities: (page, token) => { | ||
| getReviewOpportunities: (page) => { |
There was a problem hiding this comment.
[❗❗ security]
The token parameter has been removed from the getReviewOpportunities function in mapDispatchToProps. Verify that this change is intentional and that the function does not need the token for any security-related operations.
| filter = Filter.getFilterFunction(filter.challengeFilter); | ||
| challenges = activeChallenges | ||
| .filter(x => x.status === 'Active') | ||
| .filter(x => x.status === 'ACTIVE') |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that the change from 'Active' to 'ACTIVE' is consistent with the rest of the application and any external systems that interact with this code. If this change is part of a broader update, verify that all related components and documentation are updated accordingly.
| /* Validation of filter parameters: they may come from URL query, thus | ||
| * validation is not a bad idea. As you may note, at the moment we do not | ||
| * do it very carefuly (many params are not validated). */ | ||
| const basePayload = _.isPlainObject(payload) ? payload : {}; |
There was a problem hiding this comment.
[💡 maintainability]
The use of _.isPlainObject(payload) is a good check, but consider handling the case where payload is not an object more explicitly, perhaps by logging a warning or error. This can help in debugging if unexpected data types are passed.
| .filter(type => EXCLUDED_CHALLENGE_TYPE_NAMES.includes(type.name)) | ||
| .map(type => type.abbreviation); | ||
| if (excludedTypeAbbreviations.length && Array.isArray(basePayload.types)) { | ||
| sanitizedPayload.types = basePayload.types |
There was a problem hiding this comment.
[correctness]
The filtering logic for sanitizedPayload.types assumes that basePayload.types is always an array when it exists. Consider adding a check to ensure basePayload.types is an array before applying .filter(), to prevent potential runtime errors.
|
|
||
| const ids = new Set(); | ||
| loaded.forEach(item => ids.add(item.id)); | ||
| loaded.forEach(item => ids.add(item.challengeId)); |
There was a problem hiding this comment.
[❗❗ correctness]
The change from item.id to item.challengeId in the loaded.forEach loop and subsequent filtering logic assumes that challengeId is always present and unique. Ensure that challengeId is consistently available and serves the same purpose as id to avoid logical errors.
| * Generates a list of unique terms ids required for the open review roles | ||
| * with an agreed field | ||
| * | ||
| * @param {Object} details Review Opportuny details from API |
There was a problem hiding this comment.
[💡 readability]
Typo in the comment: 'Opportuny' should be 'Opportunity'.
| * @param {Object} action Payload will be JSON from api call | ||
| * @return {Object} New state | ||
| */ | ||
| function onGetDetailsDone(state, { payload, error }) { |
There was a problem hiding this comment.
[correctness]
Consider handling the case where payload.details might be undefined or null to prevent potential runtime errors.
| ...state, | ||
| details: payload.details, | ||
| isLoadingDetails: false, | ||
| requiredTerms: buildRequiredTermsList(payload.details), |
There was a problem hiding this comment.
[correctness]
The function buildRequiredTermsList assumes details.challenge.terms is always defined. Consider adding a check to handle cases where terms might be undefined to avoid potential errors.
| filter: { | ||
| ...meta.challengeFilter, | ||
| status: 'Active', | ||
| status: 'ACTIVE', |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that the change from 'Active' to 'ACTIVE' aligns with the expected values in the system. If the system expects status values to be case-sensitive, this change might lead to incorrect filtering behavior.
| this.private = { | ||
| api: getApi('V5', tokenV5), | ||
| tokenV5, | ||
| api: getApi('V6', tokenV6), |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that the getApi('V6', tokenV6) call is compatible with the expected API changes from V5 to V6. Verify that the API endpoints and authentication mechanisms have not changed in a way that could break functionality.
| if (!lastInstance || tokenV5 !== lastInstance.private.tokenV5) { | ||
| lastInstance = new DashboardService(tokenV5); | ||
| export function getService(tokenV6) { | ||
| if (!lastInstance || tokenV6 !== lastInstance.private.tokenV6) { |
There was a problem hiding this comment.
[maintainability]
Consider adding a mechanism to handle token expiration or invalidation. Currently, if the token changes, a new instance is created, but there is no check for token validity, which could lead to unexpected behavior if the token is expired or invalid.
| * @returns {Promise<Object>} The fetched data. | ||
| */ | ||
| export default async function getReviewOpportunities(page, pageSize) { | ||
| const offset = page * pageSize; |
There was a problem hiding this comment.
[❗❗ correctness]
The calculation of offset should be (page - 1) * pageSize to ensure the pagination starts correctly from the first item on the specified page. Currently, it skips the first page entirely.
| * @param {Number} challengeId The ID of the challenge (not the opportunity id) | ||
| * @return {Object} The combined data of the review opportunity and challenge details | ||
| */ | ||
| export async function getDetails(challengeId, opportunityId) { |
There was a problem hiding this comment.
[💡 maintainability]
The function getDetails is missing a JSDoc parameter description for opportunityId. This can lead to confusion about the expected input.
| * @param {Array} roleIds List of review role IDs to apply for | ||
| * @return {Promise} Resolves to the api response in JSON. | ||
| */ | ||
| export async function submitApplications(opportunityId, tokenV3) { |
There was a problem hiding this comment.
[💡 maintainability]
The function submitApplications is missing a JSDoc parameter description for opportunityId and tokenV3. This can lead to confusion about the expected input.
|
|
||
| return res.json(); | ||
| } catch (error) { | ||
| return 'There was an error while submitting the application.'; |
There was a problem hiding this comment.
[correctness]
Returning a string error message is inconsistent with the rest of the function, which returns a Promise. Consider rejecting the promise with an error object instead.
| aggregated, | ||
| meta, | ||
| }) { | ||
| const url = `${v6ApiUrl}${baseUrl}?challengeId=${encodeURIComponent(challengeId)}&perPage=${DEFAULT_PER_PAGE}&page=${page}`; |
There was a problem hiding this comment.
[💡 readability]
Consider using URLSearchParams to construct query parameters for the URL to ensure proper encoding and readability.
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch review summations: ${response.status} ${response.statusText}`); |
There was a problem hiding this comment.
[💡 maintainability]
The error message could include the URL to aid in debugging, especially if the URL construction is dynamic.
|
|
||
| export default async function getReviewSummations(tokenV3, challengeId) { | ||
| const headers = new Headers({ | ||
| Authorization: `Bearer ${tokenV3}`, |
There was a problem hiding this comment.
[❗❗ security]
Ensure that tokenV3 is validated before using it in the Authorization header to prevent potential security issues with malformed tokens.
|
|
||
| export default function getReviewTypes(tokenV3) { | ||
| return fetch(`${v5ApiUrl}${baseUrl}?perPage=500&page=1`, { | ||
| return fetch(`${v6ApiUrl}${baseUrl}?perPage=500&page=1`, { |
There was a problem hiding this comment.
[maintainability]
Consider handling potential errors from the fetch call, such as network issues or non-200 HTTP responses, to improve robustness and error handling.
| import { config } from 'topcoder-react-utils'; | ||
|
|
||
| const v5ApiUrl = config.API.V5; | ||
| const v5ApiUrl = config.API.V6; |
There was a problem hiding this comment.
[💡 readability]
The variable name v5ApiUrl is misleading now that it points to the V6 API. Consider renaming it to v6ApiUrl to reflect the correct API version and improve code clarity.
| const type = getTypeName(challenge); | ||
| let phases = challenge.phases || []; | ||
| if (type === 'First2Finish' && challenge.status === 'Completed') { | ||
| if (type === 'First2Finish' && challenge.status === 'COMPLETED') { |
There was a problem hiding this comment.
[❗❗ correctness]
The change from challenge.status === 'Completed' to challenge.status === 'COMPLETED' suggests a change in the expected status format. Ensure that all parts of the codebase and any external systems interacting with this status are updated accordingly to prevent mismatches.
| const placementPrizes = _.find(challenge.prizeSets, { type: 'placement' }); | ||
| const placementPrizes = _.find( | ||
| challenge.prizeSets, | ||
| prizeSet => ((prizeSet && prizeSet.type) || '').toLowerCase() === 'placement', |
There was a problem hiding this comment.
[💡 readability]
The use of ((prizeSet && prizeSet.type) || '').toLowerCase() is a defensive programming technique to handle potential null or undefined values. While this is correct, consider using optional chaining (prizeSet?.type?.toLowerCase()) for improved readability and modern syntax.
| } | ||
|
|
||
| const combined = []; | ||
| toArray(_.get(submission, 'reviewSummations')).forEach(item => combined.push(item)); |
There was a problem hiding this comment.
[performance]
Using forEach to push items into combined could be replaced with concat for better readability and performance.
|
|
||
| const combined = []; | ||
| toArray(_.get(submission, 'reviewSummations')).forEach(item => combined.push(item)); | ||
| toArray(_.get(submission, 'reviewSummation')).forEach(item => combined.push(item)); |
There was a problem hiding this comment.
[performance]
Using forEach to push items into combined could be replaced with concat for better readability and performance.
| } | ||
|
|
||
| export function getSubmissionStatus(submission) { | ||
| const targetIdRaw = _.get(submission, 'submissionId', _.get(submission, 'id', null)); |
There was a problem hiding this comment.
[correctness]
The fallback logic for targetIdRaw could lead to unexpected behavior if both submissionId and id are present but submissionId is falsy. Consider explicitly checking for the presence of submissionId before falling back to id.
| const hasReviewSummation = reviewSummations.length > 0; | ||
|
|
||
| const isAccepted = reviewSummations.some((summation) => { | ||
| const type = _.toLower(_.toString(_.get(summation, 'type', '') || '').trim()); |
There was a problem hiding this comment.
[💡 performance]
The conversion logic for type could be simplified by using _.get(summation, 'type', '').toLowerCase().trim() directly, which avoids unnecessary conversions.
| const EXCLUDED_CHALLENGE_TYPE_NAMES = ['Topgear Task']; | ||
|
|
||
| export { EXCLUDED_CHALLENGE_TYPE_NAMES }; | ||
| export default EXCLUDED_CHALLENGE_TYPE_NAMES; |
There was a problem hiding this comment.
[maintainability]
Exporting the same constant both as a named export and a default export can lead to confusion and potential issues if the module is imported in different ways. Consider choosing one export method to maintain clarity and avoid unexpected behavior.
| * @returns {String} | ||
| */ | ||
| export function getTrackName(trackOrChallenge) { | ||
| const value = (trackOrChallenge && trackOrChallenge.track !== undefined) |
There was a problem hiding this comment.
[correctness]
The logic assumes that trackOrChallenge.track is not null or undefined when it exists. Consider using _.get(trackOrChallenge, 'track', trackOrChallenge) to safely handle cases where track might be null or undefined.
| * @returns {String} | ||
| */ | ||
| export function getTypeName(typeOrChallenge) { | ||
| const value = (typeOrChallenge && typeOrChallenge.type !== undefined) |
There was a problem hiding this comment.
[correctness]
Similar to getTrackName, the logic assumes that typeOrChallenge.type is not null or undefined when it exists. Consider using _.get(typeOrChallenge, 'type', typeOrChallenge) to safely handle cases where type might be null or undefined.
|
|
||
| export function hasOpenSubmissionPhase(phases) { | ||
| if (!Array.isArray(phases)) return false; | ||
| return phases.some(phase => SUBMISSION_PHASE_NAMES.includes(phase.name) && phase.isOpen); |
There was a problem hiding this comment.
[performance]
Consider using Set for SUBMISSION_PHASE_NAMES to improve lookup performance from O(n) to O(1).
| import JoinCommunity from 'containers/tc-communities/JoinCommunity'; | ||
| import VideoModalButton from 'components/VideoModalButton'; | ||
| import Looker from 'containers/Looker'; | ||
| import Looker from 'containers/SmartLooker'; |
There was a problem hiding this comment.
[❗❗ correctness]
The import path has been changed from containers/Looker to containers/SmartLooker. Ensure that SmartLooker is correctly implemented and tested, as this change might affect the functionality dependent on the Looker component.
| finalScore: null, | ||
| status: 'completed', | ||
| reviewSummations: [], | ||
| reviewSummation: [], |
There was a problem hiding this comment.
[💡 maintainability]
The reviewSummation field seems redundant as it duplicates the reviewSummations array. Consider removing it unless there's a specific need for both.
| finalMeta: finalResult.meta, | ||
| finalScore: finalResult.value, | ||
| reviewSummations, | ||
| reviewSummation: [...reviewSummations], |
There was a problem hiding this comment.
[💡 maintainability]
The reviewSummation field duplicates reviewSummations. Consider removing it to avoid redundancy unless both are needed for a specific reason.
| finalScore: _.isNil(submission.finalScore) ? null : submission.finalScore, | ||
| status: submission.status || 'completed', | ||
| reviewSummations: [...submission.reviewSummations], | ||
| reviewSummation: [...submission.reviewSummations], |
There was a problem hiding this comment.
[💡 maintainability]
The reviewSummation field duplicates reviewSummations. Consider removing it to avoid redundancy unless both are needed for a specific reason.
| const positions = openPositionsByRole(details); | ||
| const apps = details.applications | ||
| ? details.applications.filter(app => _.toString(app.handle) === _.toString(handle) && app.status !== 'Cancelled') : []; | ||
| ? details.applications.filter(app => _.toString(app.handle) === _.toString(handle) && app.status !== 'CANCELLED') : []; |
There was a problem hiding this comment.
[❗❗ correctness]
The change from Cancelled to CANCELLED should be verified against the source of truth for application statuses to ensure consistency and correctness across the application.
|
|
||
| function filterByText(challenge, state) { | ||
| if (!state.search) return true; | ||
| const str = `${challenge.name} ${challenge.tags} ${challenge.platforms} ${challenge.tags}` |
There was a problem hiding this comment.
[performance]
The string concatenation in filterByText includes challenge.tags twice. This redundancy should be removed to improve performance and maintainability.
| function filterByTrack(challenge, state) { | ||
| // if (!state.tracks) return true; | ||
| // eslint-disable-next-line max-len | ||
| return state.tracks[challenge.track] === true; |
There was a problem hiding this comment.
[💡 maintainability]
The commented-out code in filterByTrack should be removed if it is not needed, as it can clutter the codebase and reduce maintainability.
| const { challengeData } = opp; | ||
|
|
||
| // const newType = _.find(validTypes, { name: opp.challenge.type }) || {}; | ||
| const newType = _.find(validTypes, { name: challengeData.subTrack === 'FIRST_2_FINISH' ? 'First2Finish' : 'Challenge' }) || {}; |
There was a problem hiding this comment.
[correctness]
The logic for determining newType based on challengeData.subTrack should be verified to ensure it aligns with the intended business logic, especially since the original commented code suggests a different approach.
| const filterSubmissions = handle ? submissions.filter(s => s.createdBy === handle) : submissions; | ||
| const sortedSubmissions = filterSubmissions.sort((a, b) => (a.created < b.created ? 1 : -1)); | ||
| const sortedSubmissions = filterSubmissions.sort((a, b) => ( | ||
| (a.created || a.createdAt) < (b.created || b.createdAt) ? 1 : -1 |
There was a problem hiding this comment.
[correctness]
The use of a.created || a.createdAt and b.created || b.createdAt assumes that if created is falsy, createdAt will be a valid date. Ensure that both fields are consistently formatted and that this fallback logic is intentional and safe. Consider documenting the expected structure of the submissions array elsewhere in the codebase to prevent future misuse.
No description provided.