Feat/challenge reviewer v6 #1673
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
Outdated
Show resolved
Hide resolved
src/components/ChallengeEditor/ChallengeReviewer-Field/ChallengeReviewer-Field.module.scss
Outdated
Show resolved
Hide resolved
src/components/ChallengeEditor/ChallengeReviewer-Field/ChallengeReviewer-Field.module.scss
Outdated
Show resolved
Hide resolved
| throw new Error('Failed to load scorecards') | ||
| } | ||
| } catch (error) { | ||
| console.error('Error loading scorecards:', error) |
There was a problem hiding this comment.
Consider handling specific error cases more explicitly instead of using a generic error message. This can help in debugging and provide more context to the user.
| // only load default reviewers if we have typeId and trackId | ||
| if (!challenge.typeId || !challenge.trackId) { | ||
| console.log('Cannot load default reviewers: missing typeId or trackId') | ||
| this.setState({ defaultReviewer: [] }) |
There was a problem hiding this comment.
There is a typo in the state update. It should be defaultReviewers instead of defaultReviewer.
| throw new Error('Failed to load default reviewers') | ||
| } | ||
| } catch (error) { | ||
| console.error('Error loading default reviewers:', error) |
There was a problem hiding this comment.
Consider handling specific error cases more explicitly instead of using a generic error message. This can help in debugging and provide more context to the user.
| const fieldUpdate = {} | ||
| fieldUpdate[field] = value | ||
| updatedReviewers[index] = Object.assign({}, updatedReviewers[index], fieldUpdate) | ||
| onUpdateOthers({ field: 'reviewers', value: updatedReviewers }) |
There was a problem hiding this comment.
Using Object.assign here is fine, but consider using the spread operator for a more modern approach: updatedReviewers[index] = { ...updatedReviewers[index], [field]: value }.
| onUpdateMultiSelect: () => {}, | ||
| readOnly: false | ||
| readOnly: false, | ||
| showReviewerField: false, |
There was a problem hiding this comment.
The addition of showReviewerField as a default prop is not accompanied by any explanation or usage in the surrounding code. Ensure that this prop is utilized appropriately in the component logic or remove it if unnecessary.
| readOnly: false | ||
| readOnly: false, | ||
| showReviewerField: false, | ||
| onUpdateOthers: () => {} |
There was a problem hiding this comment.
The onUpdateOthers function is added as a default prop but lacks context or usage in the component. Verify its necessity and ensure it is integrated into the component's functionality or remove it if not needed.
There was a problem hiding this comment.
please be more specific in the props naming. onUpdateOthers doesn't mean much. onUpdateOtherReviewers or onUpdateReviewers provides a better view over what it's supposed to mean.
There was a problem hiding this comment.
@rishabhtc this change should be handled, and you should rename onUpdateOthers to onUpdateOtherReviewers everywhere.
| shouldShowPrivateDescription: PropTypes.bool, | ||
| readOnly: PropTypes.bool | ||
| readOnly: PropTypes.bool, | ||
| showReviewerField: PropTypes.bool, |
There was a problem hiding this comment.
The showReviewerField prop is added but there is no context or usage shown in this diff. Ensure that this prop is utilized correctly in the component logic.
| readOnly: PropTypes.bool | ||
| readOnly: PropTypes.bool, | ||
| showReviewerField: PropTypes.bool, | ||
| onUpdateOthers: PropTypes.func |
There was a problem hiding this comment.
The onUpdateOthers prop is added but there is no context or usage shown in this diff. Ensure that this function is implemented and used appropriately within the component.
| onUpdateMultiSelect={this.onUpdateMultiSelect} | ||
| onUpdateMetadata={this.onUpdateMetadata} | ||
| showReviewerField | ||
| onUpdateOthers={this.onUpdateOthers} |
There was a problem hiding this comment.
The onUpdateOthers prop is added. Verify that the function this.onUpdateOthers is defined and implemented correctly to handle updates as expected.
There was a problem hiding this comment.
@vas3a onUpdateOthers in this component was present earlier as well. I can skip renaming it right ?
| // Add challenge track if available | ||
| if (challenge.trackId) { | ||
| // Map track ID to track name for the API | ||
| const trackMapping = { |
There was a problem hiding this comment.
do we have the track data in the API? if so, you should fetch it from API rather than hardcode it in here.
| scorecards: [], | ||
| defaultReviewers: [], | ||
| isLoadingScorecards: false, | ||
| isLoadingDefaults: false, |
There was a problem hiding this comment.
please rename to isLoadingDefaultReviewers
| this.setState({ defaultReviewer: [] }) | ||
| return | ||
| } | ||
| const response = await axios.get('https://api.topcoder-dev.com/v6/challenges/default-reviewers', { |
There was a problem hiding this comment.
please use src\services\challenges.js (add missing method as needed) to fetch default reviewers. make sure to follow the same code style as it's already in there.
| readOnly: false | ||
| readOnly: false, | ||
| showReviewerField: false, | ||
| onUpdateOthers: () => {} |
There was a problem hiding this comment.
please be more specific in the props naming. onUpdateOthers doesn't mean much. onUpdateOtherReviewers or onUpdateReviewers provides a better view over what it's supposed to mean.
| } catch (error) { | ||
| console.error('Error loading default reviewers:', error) | ||
| // Use mock data for development/testing | ||
| const mockDefaultReviewers = [ |
There was a problem hiding this comment.
please move this to other file and keep track to remove it when you're done with it.
| } catch (error) { | ||
| console.error('Error loading scorecards:', error) | ||
| // Use mock data for development/testing | ||
| // const mockScorecards = [ |
…cted payment logic
src/actions/challenges.js
Outdated
|
|
||
| if (status !== 'all') { | ||
| filters['status'] = !status ? undefined : status | ||
| filters['status'] = !status ? undefined : _.startCase(status.toLowerCase()) |
There was a problem hiding this comment.
Consider using _.capitalize(status.toLowerCase()) instead of _.startCase(status.toLowerCase()) if the intention is to only capitalize the first letter of the status. _.startCase will capitalize the first letter of each word, which might not be necessary if status is a single word.
There was a problem hiding this comment.
@vas3a
I have added this logic and also changed the ACTIVE to active due to challenge API expecting first letter of the status as uppercase Please verify it and let me know if needs to revert it.
There was a problem hiding this comment.
Is it not expecting all uppercase letters?
| metadataValue: scorecards.scoreCards || [] | ||
| }) | ||
| } catch (error) { | ||
| console.error('Error loading scorecards:', error) |
There was a problem hiding this comment.
Using console.error for error logging is fine for development, but consider using a more robust logging mechanism for production environments.
There was a problem hiding this comment.
@vas3a Do I need to remove console.error ?
| metadataValue: defaultReviewers | ||
| }) | ||
| } catch (error) { | ||
| console.error('Error loading default reviewers:', error) |
There was a problem hiding this comment.
Using console.error for error logging is fine for development, but consider using a more robust logging mechanism for production environments.
src/components/ChallengeEditor/ChallengeReviewer-Field/ChallengeReviewer-Field.module.scss
Show resolved
Hide resolved
|
|
||
| componentDidMount () { | ||
| this.loadScorecards() | ||
| this.isLoadingDefaultReviewers() |
There was a problem hiding this comment.
The method name isLoadingDefaultReviewers suggests it returns a boolean indicating loading status, but it actually initiates loading reviewers. Consider renaming it to better reflect its functionality, such as loadDefaultReviewers.
|
|
||
| renderReviewerForm (reviewer, index) { | ||
| const { challenge, metadata } = this.props | ||
| const { scorecards = [] } = metadata |
There was a problem hiding this comment.
The scorecards variable is being destructured from metadata with a default value of an empty array. Ensure that metadata is always defined to avoid potential runtime errors.
| const validationErrors = this.validateReviewer(reviewer) | ||
|
|
||
| return ( | ||
| <div key={`reviewer-${index}`} className={styles.reviewerForm}> |
There was a problem hiding this comment.
The key prop for the div element has been changed to use a template string with index. Ensure that index is unique and consistent across renders to prevent potential issues with React's reconciliation process.
| const currentReviewers = challenge.reviewers || [] | ||
| const updatedReviewers = currentReviewers.slice() | ||
|
|
||
| // Update both fields atomically to ensure XOR constraint is satisfied |
There was a problem hiding this comment.
Consider removing the comment about updating fields atomically, as comments are not encouraged in this context.
| const updatedReviewers = currentReviewers.slice() | ||
|
|
||
| // Update both fields atomically to ensure XOR constraint is satisfied | ||
| // Maintain correct field order as expected by API schema |
There was a problem hiding this comment.
Consider removing the comment about maintaining field order, as comments are not encouraged in this context.
| // Update both fields atomically to ensure XOR constraint is satisfied | ||
| // Maintain correct field order as expected by API schema | ||
| const currentReviewer = updatedReviewers[index] | ||
| updatedReviewers[index] = { |
There was a problem hiding this comment.
Ensure that updatedReviewers[index] is not undefined before attempting to access its properties to avoid potential runtime errors.
| } | ||
|
|
||
| render () { | ||
| const { challenge, metadata, isLoading } = this.props |
There was a problem hiding this comment.
The metadata prop is being destructured, but it is not clear if it is always provided. Consider adding a default value or a check to ensure metadata is defined before destructuring.
| const { scorecards = [], defaultReviewers = [] } = metadata | ||
| const reviewers = challenge.reviewers || [] | ||
|
|
||
| if (isLoading) { |
There was a problem hiding this comment.
The condition for loading has been changed to isLoading. Ensure that this new prop accurately reflects the loading state for both scorecards and defaults, as the previous condition checked two separate loading states.
| } | ||
|
|
||
| // Only show error if there's a real error, not just missing data | ||
| if (error && !scorecards.length && !defaultReviewers.length) { |
There was a problem hiding this comment.
The code has been updated to use scorecards and defaultReviewers directly instead of this.state.scorecards and this.state.defaultReviewers. Ensure that scorecards and defaultReviewers are defined and accessible in the current scope to avoid potential reference errors.
src/components/ChallengeEditor/ChallengeReviewer-Field/index.js
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| const mapStateToProps = (state) => ({ | ||
| metadata: state.challenges.metadata, |
There was a problem hiding this comment.
Consider verifying that state.challenges.metadata is correctly initialized in the Redux store to avoid potential issues with accessing properties on undefined.
|
|
||
| const mapStateToProps = (state) => ({ | ||
| metadata: state.challenges.metadata, | ||
| isLoading: state.challenges.isLoading |
There was a problem hiding this comment.
Ensure that state.challenges.isLoading is correctly managed in the Redux store to reflect the loading state accurately.
| </div> | ||
| )} | ||
|
|
||
| {readOnly && reviewers.length === 0 && ( |
There was a problem hiding this comment.
Consider adding a conditional check to ensure reviewers is defined before accessing its length property to prevent potential runtime errors.
| readOnly: PropTypes.bool | ||
| readOnly: PropTypes.bool, | ||
| showReviewerField: PropTypes.bool, | ||
| onUpdateReviewers: PropTypes.func |
There was a problem hiding this comment.
The prop name has been changed from onUpdateOthers to onUpdateReviewers. Ensure that all instances where this prop is used throughout the codebase are updated accordingly to prevent any potential errors or mismatches.
| onUpdateMultiSelect={this.onUpdateMultiSelect} | ||
| onUpdateMetadata={this.onUpdateMetadata} | ||
| showReviewerField | ||
| onUpdateReviewers={this.onUpdateOthers} |
There was a problem hiding this comment.
The function name onUpdateReviewers suggests that it updates reviewers, but it is still calling this.onUpdateOthers. Ensure that the function being called matches the intended functionality implied by the name change.
| scorecardId: (defaultReviewer && defaultReviewer.scorecardId) || '', | ||
| isMemberReview: true, | ||
| memberReviewerCount: (defaultReviewer && defaultReviewer.memberReviewerCount) || 1, | ||
| phaseId: (defaultReviewer && defaultReviewer.phaseId) || (firstPhase ? (firstPhase.id || firstPhase.phaseId) : ''), |
There was a problem hiding this comment.
Consider using a consistent type for basePayment. The change from a number to a string ('0') might lead to type-related issues elsewhere in the code where basePayment is expected to be a number.
| basePayment: (defaultReviewer && defaultReviewer.basePayment) || '0', | ||
| incrementalPayment: (defaultReviewer && defaultReviewer.incrementalPayment) || 0, | ||
| type: (defaultReviewer && defaultReviewer.opportunityType) || REVIEW_OPPORTUNITY_TYPES.REGULAR_REVIEW, | ||
| isAIReviewer: Boolean((defaultReviewer && defaultReviewer.isAIReviewer) || false) |
There was a problem hiding this comment.
The use of Boolean() is unnecessary here. The expression (defaultReviewer && defaultReviewer.isAIReviewer) || false already evaluates to a boolean value. Consider simplifying this line by removing Boolean().
| errors.push('Number of reviewers must be a positive integer') | ||
| } | ||
|
|
||
| const basePayment = convertDollarToInteger(reviewer.basePayment, '') |
There was a problem hiding this comment.
The convertDollarToInteger function is used here, but it's not clear if this function handles all edge cases, such as invalid input or currency formatting issues. Ensure that this function is robust and handles all potential input scenarios.
| return ( | ||
| <div key={`reviewer-${index}`} className={styles.reviewerForm}> | ||
| <div className={styles.reviewerHeader}> | ||
| {index > 0 && <h4>Reviewer {index + 1}</h4>} |
There was a problem hiding this comment.
Suggestion
Consider explaining why the header is conditionally rendered only when index > 0. If this is intentional to hide the header for the first reviewer, ensure that this logic aligns with the overall design requirements.
There was a problem hiding this comment.
Yes. I am implemented it because when number of reviewer value set to more than 1 , then it will not look nice as Reviewer 1 so removed it for first reviewer. @vas3a open for suggestions here
There was a problem hiding this comment.
you could hide it based on count. eg. if there are more than 1 reviewers render it, if there's a single reviewer hide it. BUT. let's leave it as is for now. it's pretty minor, we need to get this to QA.
| {validationErrors.length > 0 && ( | ||
| <div className={styles.validationErrors}> | ||
| {validationErrors.map((error, i) => ( | ||
| <div key={`error-${index}-${i}-${error}`} className={styles.validationError}>{error}</div> |
There was a problem hiding this comment.
Using index in the key generation suggests that index should be defined or imported in the current scope. Ensure that index is correctly defined or imported to avoid reference errors.
| updatedReviewers[index] = { | ||
| scorecardId: currentReviewer.scorecardId, | ||
| isMemberReview: !isAI, | ||
| memberReviewerCount: currentReviewer.memberReviewerCount || 1, |
There was a problem hiding this comment.
Consider verifying if currentReviewer.memberReviewerCount should default to 1. This change might affect logic that relies on the original value.
| isMemberReview: !isAI, | ||
| memberReviewerCount: currentReviewer.memberReviewerCount || 1, | ||
| phaseId: currentReviewer.phaseId, | ||
| basePayment: currentReviewer.basePayment || '0', |
There was a problem hiding this comment.
Ensure that defaulting currentReviewer.basePayment to '0' does not cause issues where a numeric value is expected, as this could lead to type mismatches.
| memberReviewerCount: currentReviewer.memberReviewerCount || 1, | ||
| phaseId: currentReviewer.phaseId, | ||
| basePayment: currentReviewer.basePayment || '0', | ||
| incrementalPayment: currentReviewer.incrementalPayment || 0, |
There was a problem hiding this comment.
Check if defaulting currentReviewer.incrementalPayment to 0 is appropriate, as it might affect calculations that assume a different default value.
| basePayment: currentReviewer.basePayment || '0', | ||
| incrementalPayment: currentReviewer.incrementalPayment || 0, | ||
| type: currentReviewer.type, | ||
| isAIReviewer: Boolean(isAI) |
There was a problem hiding this comment.
Using Boolean(isAI) ensures a boolean value, but verify if this change aligns with the intended logic, especially if isAI can have non-boolean values that need specific handling.
| <span> | ||
| {(() => { | ||
| const phase = challenge.phases && challenge.phases.find(p => | ||
| (p.id === reviewer.phaseId) || (p.phaseId === reviewer.phaseId) |
There was a problem hiding this comment.
The condition (p.phaseId === reviewer.phaseId) seems redundant if p.id and p.phaseId are intended to represent the same identifier. Consider verifying if both are necessary or if they can be consolidated into a single check.
| {challenge.phases && challenge.phases | ||
| .filter(phase => { | ||
| const isReviewPhase = phase.name && phase.name.toLowerCase().includes('review') | ||
| const isCurrentlySelected = (phase.id === reviewer.phaseId) || (phase.phaseId === reviewer.phaseId) |
There was a problem hiding this comment.
Consider renaming the variable isCurrentlySelected to isSelectedPhase for better clarity, as it checks if the phase is currently selected.
| return isReviewPhase || isCurrentlySelected | ||
| }) | ||
| .map(phase => ( | ||
| <option key={phase.id || phase.phaseId} value={phase.phaseId || phase.id}> |
There was a problem hiding this comment.
The logic for key and value attributes in the <option> element is slightly inconsistent. Consider using a consistent order for phase.id and phase.phaseId to avoid confusion.
| min='1' | ||
| value={reviewer.memberReviewerCount || 1} | ||
| onChange={(e) => { | ||
| const validatedValue = validateValue(e.target.value, VALIDATION_VALUE_TYPE.INTEGER) |
There was a problem hiding this comment.
The function validateValue is used here, but it's not clear from the context if it handles all potential edge cases or errors. Ensure that validateValue is robust and returns a valid integer string or a default value that can be safely parsed.
| value={reviewer.memberReviewerCount || 1} | ||
| onChange={(e) => { | ||
| const validatedValue = validateValue(e.target.value, VALIDATION_VALUE_TYPE.INTEGER) | ||
| const parsedValue = parseInt(validatedValue) || 1 |
There was a problem hiding this comment.
Consider checking if validatedValue is a valid number before parsing it with parseInt. This will help avoid potential issues with unexpected input.
| step='0.01' | ||
| value={reviewer.basePayment || '0'} | ||
| onChange={(e) => { | ||
| const validatedValue = validateValue(e.target.value, VALIDATION_VALUE_TYPE.INTEGER) |
There was a problem hiding this comment.
The function validateValue is being used here, but it is not clear if it handles all edge cases for a number input, such as empty strings or non-numeric characters. Ensure that validateValue properly validates and sanitizes the input to prevent potential issues.
| step='0.01' | ||
| value={reviewer.basePayment || '0'} | ||
| onChange={(e) => { | ||
| const validatedValue = validateValue(e.target.value, VALIDATION_VALUE_TYPE.INTEGER) |
There was a problem hiding this comment.
The VALIDATION_VALUE_TYPE.INTEGER constant is used here, but the input field allows decimal values due to step='0.01'. Consider using a validation type that supports decimal numbers if decimals are intended to be allowed.
| } | ||
|
|
||
| render () { | ||
| const { challenge, metadata = {}, isLoading, readOnly = false } = this.props |
There was a problem hiding this comment.
Consider providing a default value for challenge to prevent potential errors if challenge is undefined.
| </div> | ||
| )} | ||
|
|
||
| {!readOnly && reviewers && reviewers.length === 0 && ( |
There was a problem hiding this comment.
The condition reviewers && reviewers.length === 0 is checking if reviewers is truthy before checking its length. Ensure that reviewers is always initialized to an array to avoid potential issues if reviewers is null or undefined. Consider initializing reviewers to an empty array by default if not already done.
| <h4>Review Summary</h4> | ||
| <div className={styles.summaryRow}> | ||
| <span>Total Member Reviewers:</span> | ||
| <span>{reviewers.filter(r => Boolean(r.isAIReviewer) === false).reduce((sum, r) => sum + (parseInt(r.memberReviewerCount) || 0), 0)}</span> |
There was a problem hiding this comment.
Consider using Number.isInteger() to check if r.memberReviewerCount is a valid integer before parsing it with parseInt() to avoid potential issues with invalid inputs.
| const base = convertDollarToInteger(r.basePayment, '') | ||
| const count = parseInt(r.memberReviewerCount) || 1 | ||
| return sum + (base * count) | ||
| }, 0)}</span> |
There was a problem hiding this comment.
The .toFixed(2) method was removed from the total review cost calculation. If the intention is to maintain a fixed decimal format for currency display, consider re-adding .toFixed(2) to ensure the cost is displayed with two decimal places.
| reviewerTotal = challenge.reviewers | ||
| .filter(r => Boolean(r.isAIReviewer) === false) | ||
| .reduce((sum, r) => { | ||
| const base = convertDollarToInteger(r.basePayment, '') |
There was a problem hiding this comment.
The function convertDollarToInteger is used here, but it is unclear from the context whether this function handles cases where r.basePayment might be undefined or not a valid dollar amount. Ensure that convertDollarToInteger has proper error handling for such cases to prevent unexpected behavior.
| .filter(r => Boolean(r.isAIReviewer) === false) | ||
| .reduce((sum, r) => { | ||
| const base = convertDollarToInteger(r.basePayment, '') | ||
| const count = parseInt(r.memberReviewerCount) || 1 |
There was a problem hiding this comment.
Using parseInt without specifying a radix can lead to unexpected results if r.memberReviewerCount is a string with leading zeros. Consider using parseInt(r.memberReviewerCount, 10) to ensure the number is parsed as a decimal.
| <input | ||
| type='number' | ||
| min='0' | ||
| value={reviewer.basePayment || '0'} |
There was a problem hiding this comment.
Removing the step='0.01' attribute from the input field changes the behavior of the number input, potentially affecting user experience when entering decimal values. If the intention is to restrict input to whole numbers, consider updating the type attribute to reflect this change, or ensure that the validation logic handles decimal inputs appropriately.
| basePayment: currentReviewer.basePayment || '0', | ||
| incrementalPayment: currentReviewer.incrementalPayment || 0, | ||
| type: currentReviewer.type, | ||
| isAIReviewer: isAI |
There was a problem hiding this comment.
The isAIReviewer property is being set directly to isAI instead of using Boolean(isAI). If isAI is not already a boolean, this change could lead to unexpected behavior. Consider ensuring isAI is a boolean before assigning it to isAIReviewer.
| <span>Total Review Cost:</span> | ||
| <span>${reviewers.filter(r => !r.isAIReviewer).reduce((sum, r) => { | ||
| const base = convertDollarToInteger(r.basePayment || '0', '') | ||
| const count = parseInt(r.memberReviewerCount) || 1 |
There was a problem hiding this comment.
Consider providing a default value for r.basePayment directly in the convertDollarToInteger function if applicable, to centralize the handling of default values and avoid repetition.
| const base = convertDollarToInteger(r.basePayment || '0', '') | ||
| const count = parseInt(r.memberReviewerCount) || 1 | ||
| return sum + (base * count) | ||
| }, 0).toFixed(2)}</span> |
There was a problem hiding this comment.
The use of .toFixed(2) ensures that the total review cost is formatted to two decimal places, which is generally good for currency representation. However, ensure that this formatting does not interfere with any subsequent calculations or display requirements elsewhere in the application.
| reviewerTotal = challenge.reviewers | ||
| .filter(r => !r.isAIReviewer) | ||
| .reduce((sum, r) => { | ||
| const base = convertDollarToInteger(r.basePayment || '0', '') |
There was a problem hiding this comment.
Consider handling the case where r.basePayment is not a valid number string. Using || '0' ensures a default value, but it might be better to validate the input or handle potential errors more explicitly.
| .filter(r => !r.isAIReviewer) | ||
| .reduce((sum, r) => { | ||
| const base = convertDollarToInteger(r.basePayment || '0', '') | ||
| const count = parseInt(r.memberReviewerCount) || 1 |
There was a problem hiding this comment.
The use of parseInt(r.memberReviewerCount) || 1 assumes that memberReviewerCount is either a valid number string or undefined/null. If memberReviewerCount can be other invalid values, consider adding validation to ensure correct parsing.
vas3a
left a comment
vas3a
left a comment
There was a problem hiding this comment.
you can merge this. let's get it to qa.
| return ( | ||
| <div key={`reviewer-${index}`} className={styles.reviewerForm}> | ||
| <div className={styles.reviewerHeader}> | ||
| {index > 0 && <h4>Reviewer {index + 1}</h4>} |
There was a problem hiding this comment.
you could hide it based on count. eg. if there are more than 1 reviewers render it, if there's a single reviewer hide it. BUT. let's leave it as is for now. it's pretty minor, we need to get this to QA.
No description provided.