V6 -> develop #1694
V6 -> develop #1694
Conversation
…cted payment logic
…er-v6 Feat/challenge reviewer v6
…nt-coefficients When adding/editing reviewers, re-assign the payment coefficients
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const phase = challenge.phases && challenge.phases.find(p => p.phaseId === item.phaseId) | ||
| return { | ||
| ...item, | ||
| name: phase?.name |
There was a problem hiding this comment.
[correctness]
Using optional chaining (phase?.name) is a good practice to avoid runtime errors when phase is undefined. Ensure that this change is intentional and that undefined is an acceptable value for name in the context where it is used.
| defaultPhaseId = fallbackPhase.phaseId || fallbackPhase.id | ||
| } | ||
|
|
||
| const defaultReviewer = this.findDefaultReviewer(defaultPhaseId) ?? defaultTrackReviewer; |
There was a problem hiding this comment.
[❗❗ correctness]
The logic for determining defaultPhaseId has been moved and refactored. Ensure that the new logic correctly handles all edge cases, especially when defaultTrackReviewer, firstReviewPhase, and fallbackPhase are all null or undefined.
| defaultPhaseId = fallbackPhase.phaseId || fallbackPhase.id | ||
| } | ||
|
|
||
| const defaultReviewer = this.findDefaultReviewer(defaultPhaseId) ?? defaultTrackReviewer; |
There was a problem hiding this comment.
[❗❗ correctness]
The assignment of defaultReviewer now uses a fallback to defaultTrackReviewer. Verify that this change does not introduce unintended side effects, particularly in scenarios where defaultPhaseId is not found.
| this.handlePhaseChangeWithReassign(index, value) | ||
|
|
||
| // update payment based on default reviewer | ||
| const defaultReviewer = this.findDefaultReviewer(value) ?? updatedReviewers[index]; |
There was a problem hiding this comment.
[correctness]
The use of Object.assign to update fieldUpdate with fixedAmount, baseCoefficient, and incrementalCoefficient from defaultReviewer assumes these properties are always defined. Consider adding checks or defaults to prevent potential undefined values.
| return null | ||
| } | ||
|
|
||
| return phaseId ? defaultReviewers.find(dr => dr.phaseId === phaseId) : defaultReviewers[0]; |
There was a problem hiding this comment.
[design]
The findDefaultReviewer function now accepts a phaseId parameter and returns the first default reviewer if phaseId is not provided. Ensure that this behavior aligns with the intended logic and that it does not inadvertently select an incorrect default reviewer.
| const reviewersCost = reviewers | ||
| .filter((r) => !this.isAIReviewer(r)) | ||
| .reduce((sum, r) => { | ||
| const fixedAmount = r.fixedAmount || 0 |
There was a problem hiding this comment.
[correctness]
The calculation of reviewersCost now includes fixedAmount. Verify that fixedAmount is correctly initialized and that its inclusion in the cost calculation is intentional and accurate.
| const phase = challenge.phases && challenge.phases.find(p => p.phaseId === item.phaseId) | ||
| return { | ||
| ...item, | ||
| name: phase && phase.name |
There was a problem hiding this comment.
[💡 readability]
The change from optional chaining (phase?.name) to a logical AND (phase && phase.name) is functionally equivalent in this context. However, optional chaining is generally more concise and modern. Consider reverting to optional chaining for improved readability.
| defaultPhaseId = fallbackPhase.phaseId || fallbackPhase.id | ||
| } | ||
|
|
||
| const defaultReviewer = this.findDefaultReviewer(defaultPhaseId) || defaultTrackReviewer |
There was a problem hiding this comment.
[correctness]
The change from nullish coalescing (??) to logical OR (||) is functionally equivalent here since defaultTrackReviewer is not expected to be null or undefined. However, if defaultReviewer can be 0 or false, this change could lead to unexpected behavior. Ensure that defaultReviewer is always expected to be an object or null/undefined.
| this.handlePhaseChangeWithReassign(index, value) | ||
|
|
||
| // update payment based on default reviewer | ||
| const defaultReviewer = this.findDefaultReviewer(value) || updatedReviewers[index] |
There was a problem hiding this comment.
[correctness]
The change from nullish coalescing (??) to logical OR (||) is functionally equivalent here since updatedReviewers[index] is not expected to be null or undefined. However, if defaultReviewer can be 0 or false, this change could lead to unexpected behavior. Ensure that defaultReviewer is always expected to be an object or null/undefined.
| return null | ||
| } | ||
|
|
||
| return phaseId ? defaultReviewers.find(dr => dr.phaseId === phaseId) : defaultReviewers[0] |
There was a problem hiding this comment.
[correctness]
The change from nullish coalescing (??) to logical OR (||) is functionally equivalent here since defaultReviewers[0] is not expected to be null or undefined. However, if defaultReviewers[0] can be 0 or false, this change could lead to unexpected behavior. Ensure that defaultReviewers[0] is always expected to be an object or null/undefined.
|
|
||
| if (challenge.trackId === DEV_TRACK_ID) { | ||
| const topgearTypes = metadata.challengeTypes.filter(type => type.name && type.name.toLowerCase() === 'topgear task') | ||
| filteredTypes = _.uniqBy([...filteredTypes, ...topgearTypes], 'id') |
There was a problem hiding this comment.
[correctness]
Using _.uniqBy to merge filteredTypes and topgearTypes based on id is correct, but ensure that id is a unique identifier for challenge types. If id is not guaranteed to be unique, consider using a more reliable property for deduplication.
| } | ||
|
|
||
| if (selectedType && !filteredTypes.find(type => type.id === selectedType.id)) { | ||
| filteredTypes = [...filteredTypes, selectedType] |
There was a problem hiding this comment.
[correctness]
Appending selectedType to filteredTypes without checking for duplicates might lead to redundant entries. Consider using _.uniqBy([...filteredTypes, selectedType], 'id') to ensure uniqueness.
| const { scorecards = [], defaultReviewers = [], workflows = [] } = metadata | ||
| const reviewers = challenge.reviewers || [] | ||
| const firstPlacePrize = this.getFirstPlacePrizeValue(challenge) | ||
| const estimatedSubmissionsCount = 2 // Estimate assumes two submissions |
There was a problem hiding this comment.
[design]
The estimatedSubmissionsCount is hardcoded to 2. Consider making this a configurable parameter or deriving it from historical data to improve accuracy and flexibility.
| const reviewersCost = reviewers | ||
| .filter((r) => !this.isAIReviewer(r)) | ||
| .reduce((sum, r) => { | ||
| const fixedAmount = parseFloat(r.fixedAmount || 0) |
There was a problem hiding this comment.
[💡 readability]
Ensure that parseFloat is necessary for fixedAmount, baseCoefficient, and incrementalCoefficient. If these values are already numbers, this conversion is redundant and could be removed for clarity.
fix(PM-2456): Review setup overwriting reviewer when scorecard is changed
|
|
||
| componentDidMount () { | ||
| const { challenge, challengeResources } = this.props | ||
| if (challenge && challenge.id && challengeResources) { |
There was a problem hiding this comment.
[maintainability]
The check challenge && challenge.id && challengeResources is repeated in both componentDidMount and componentDidUpdate. Consider refactoring this logic into a helper function to improve maintainability and reduce duplication.
| } | ||
| } | ||
|
|
||
| const reviewersChanged = (() => { |
There was a problem hiding this comment.
[performance]
The reviewersChanged function uses JSON.stringify to compare reviewer objects, which can be inefficient for large objects and may not handle all edge cases correctly (e.g., different key orders). Consider using a deep comparison utility like lodash.isEqual for more robust comparison.
fix(PM-2531): Avoid duplicate PATCH calls when accepting invite
| filters: &filters-dev | ||
| branches: | ||
| only: ["develop", "PM-803_wm-regression-fixes", "PM-902_show-all-projects-on-challenge-page", "pm-1365"] | ||
| only: ["v6", "develop", "pm-2531"] |
There was a problem hiding this comment.
[maintainability]
Adding the branch pm-2531 to the filters for development builds might be temporary. Ensure that this branch is intended to be included in the long term or consider documenting the purpose of this addition elsewhere to avoid confusion in the future.
| }, [projectId, auth, projectDetail, isProjectLoading, history]) | ||
|
|
||
| const updateInvite = useCallback(async (status, source) => { | ||
| if (isUpdateInprogress) { |
There was a problem hiding this comment.
[❗❗ correctness]
The variable isUpdateInprogress is used to prevent concurrent updates. However, the function updateInvite is asynchronous, and there is a potential race condition if updateInvite is called multiple times in quick succession. Consider using a more robust mechanism to handle concurrent updates, such as a queue or a lock.
|
|
||
| // await for the project details to propagate | ||
| await delay(1000) | ||
| setIsUpdateInprogress(false) |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that setIsUpdateInprogress(false) is called in the catch block as well to reset the state in case of an error. Currently, if an error occurs, isUpdateInprogress will remain true, potentially blocking further updates.
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -1,5 +1,5 @@ | |||
| # Use the base image with Node.js | |||
| FROM node:12 | |||
| # Use Node.js 22 base image | |||
Check notice
Code scanning / Trivy
No HEALTHCHECK defined Low
Moving V6 stuff to dev env to prepare the switch off "-v6" .