Updates for anticipated start, payment popup changes#1721
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }) | ||
|
|
||
| const parsedAmount = Number(amount) | ||
| const trimmedDescription = typeof description === 'string' ? description.trim() : '' |
There was a problem hiding this comment.
[correctness]
The trimmedDescription variable is assigned a trimmed version of description only if it is a string. If description is not a string, it defaults to an empty string. Consider handling non-string values more explicitly, or validating the input type earlier to ensure description is always a string.
| if (!value) { | ||
| return '-' | ||
| } | ||
| return ANTICIPATED_START_LABELS[value] || value |
There was a problem hiding this comment.
[correctness]
Consider adding validation to ensure value is one of the expected keys in ANTICIPATED_START_LABELS before using it as a fallback. This will prevent unexpected behavior if value is not a recognized key.
| title: PropTypes.string, | ||
| description: PropTypes.string, | ||
| applicationDeadline: PropTypes.any, | ||
| anticipatedStart: PropTypes.string, |
There was a problem hiding this comment.
[correctness]
Ensure that anticipatedStart is always a string that matches one of the keys in ANTICIPATED_START_LABELS. Consider using a more specific PropType or validation to enforce this constraint.
| @@ -169,17 +171,6 @@ const EngagementEditor = ({ | |||
| const selectedCountries = (engagement.countries || []).map(code => { | |||
| return countryOptionsByValue[code] || { label: code, value: code } | |||
| }) | |||
There was a problem hiding this comment.
[💡 maintainability]
The function getMinApplicationDeadline is removed, but it might still be useful if there's a need to validate or set a minimum date for any future date-related fields. Consider keeping it if there's a possibility of reusing it.
| } | ||
|
|
||
| const selectedRoleOption = useMemo(() => { | ||
| if (!engagement.role) { |
There was a problem hiding this comment.
[correctness]
The function isValidApplicationDeadlineDate is removed. Ensure that any date validation logic that was previously handled by this function is not needed elsewhere, or consider implementing a similar validation if necessary for future date fields.
| {canEdit ? ( | ||
| <DateInput | ||
| <Select | ||
| className={styles.selectInput} |
There was a problem hiding this comment.
[💡 design]
The Select component for Anticipated Start is set with isClearable={false}. Consider whether users should be able to clear their selection, as this could impact user experience if they need to reset the field.
| Immediate: 1, | ||
| 'In a few days': 2, | ||
| 'In a few weeks': 3, | ||
| ...Object.keys(ANTICIPATED_START_LABELS).reduce((acc, key, index) => { |
There was a problem hiding this comment.
[maintainability]
The ANTICIPATED_START_ORDER object is being constructed with both hardcoded values and dynamically generated values from ANTICIPATED_START_LABELS. This could lead to unexpected behavior if the keys in ANTICIPATED_START_LABELS do not match the hardcoded keys. Consider ensuring consistency between these two structures or using one approach consistently.
| if (!anticipatedStart) { | ||
| return null | ||
| } | ||
| return ANTICIPATED_START_ORDER[anticipatedStart] || 0 |
There was a problem hiding this comment.
[correctness]
The getSortValue function returns 0 when ANTICIPATED_START_ORDER does not contain the anticipatedStart value. This could lead to incorrect sorting behavior if 0 is not a valid or expected sort value. Consider returning null or another distinct value to indicate an unrecognized anticipatedStart value.
| if (value instanceof Date) { | ||
| return Number.isNaN(value.getTime()) ? null : value | ||
| } | ||
| const parsed = new Date(value) |
There was a problem hiding this comment.
[correctness]
The normalizeWeekEndingDate function uses new Date(value) for parsing, which can lead to inconsistent results depending on the input format and the user's locale. Consider using moment(value) for consistent date parsing.
| @@ -124,8 +162,10 @@ const PaymentForm = ({ engagement, member, availableMembers, isProcessing, onSub | |||
| const memberHandle = getMemberHandle(selectedMember) | |||
| const parsedAmount = Number(amount) | |||
| const isAmountValid = Number.isFinite(parsedAmount) && parsedAmount > 0 | |||
There was a problem hiding this comment.
[correctness]
The isAmountValid check uses Number.isFinite(parsedAmount), which will return false for NaN, Infinity, and -Infinity. Ensure that parsedAmount is a valid number before this check, or handle these cases explicitly.
| setMemberError('') | ||
| setTitleError('') | ||
| onSubmit(selectedMember, trimmedTitle, parsedAmount) | ||
| onSubmit(selectedMember, trimmedTitle, parsedAmount, remarks.trim()) |
There was a problem hiding this comment.
[design]
The onSubmit function now includes remarks.trim() as an argument. Ensure that the onSubmit handler is updated to handle this additional parameter correctly.
| } | ||
|
|
||
| async onSubmitPayment (member, paymentTitle, amount) { | ||
| async onSubmitPayment (member, paymentTitle, amount, remarks) { |
There was a problem hiding this comment.
[❗❗ correctness]
The onSubmitPayment function signature has changed to include a new remarks parameter. Ensure that all calls to this function throughout the codebase are updated to pass this new parameter, or provide a default value to avoid potential runtime errors.
| if (ANTICIPATED_START_TO_API[normalized]) { | ||
| return ANTICIPATED_START_TO_API[normalized] | ||
| } | ||
| const upper = normalized.toUpperCase().replace(/\s+/g, '_') |
There was a problem hiding this comment.
[correctness]
The toEngagementAnticipatedStartApi function attempts to convert anticipatedStart to an API format by first checking against ANTICIPATED_START_TO_API and then ANTICIPATED_START_FROM_API. This could lead to unexpected results if the input is not exactly as expected. Consider logging a warning or handling unexpected values more explicitly.
| if (!anticipatedStart) { | ||
| return anticipatedStart | ||
| } | ||
| const normalized = anticipatedStart.toString().trim().toUpperCase().replace(/\s+/g, '_') |
There was a problem hiding this comment.
[correctness]
In fromEngagementAnticipatedStartApi, the normalization process assumes that the input will always match one of the keys in ANTICIPATED_START_FROM_API. If the input is not as expected, it will return the original input, which might not be the desired behavior. Consider adding a fallback or logging for unexpected values.
| const workload = fromEngagementWorkloadApi(engagement.workload) | ||
| const compensationRange = engagement.compensationRange || '' | ||
| const anticipatedStart = fromEngagementAnticipatedStartApi( | ||
| engagement.anticipatedStart || engagement.anticipated_start |
There was a problem hiding this comment.
[correctness]
The use of engagement.anticipatedStart || engagement.anticipated_start in normalizeEngagement assumes that if anticipatedStart is falsy, anticipated_start should be used. This could lead to issues if anticipatedStart is intentionally set to a falsy value. Consider explicitly checking for undefined instead.
1 participant
No description provided.