Skip to content

Engagements v3#1517

Merged
jmgasper merged 2 commits intodevfrom
engagements
Mar 6, 2026
Merged

Engagements v3#1517
jmgasper merged 2 commits intodevfrom
engagements

Conversation

@jmgasper
Copy link
Collaborator

@jmgasper jmgasper commented Mar 6, 2026

@jmgasper jmgasper requested a review from kkartunov as a code owner March 6, 2026 06:41
@jmgasper jmgasper merged commit c0a0cfe into dev Mar 6, 2026
7 checks passed
[EngagementStatus.OPEN]: 'Open',
[EngagementStatus.PENDING_ASSIGNMENT]: 'Pending Assignment',
[EngagementStatus.ACTIVE]: 'Active',
[EngagementStatus.ON_HOLD]: 'On Hold',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ correctness]
Ensure that EngagementStatus.ON_HOLD is defined in the EngagementStatus enum. If it is not defined, this could lead to runtime errors when attempting to access STATUS_LABELS[EngagementStatus.ON_HOLD].

coverLetter: '',
email: '',
mobileNumber: undefined,
mobileNumber: '',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
Changing mobileNumber from undefined to an empty string ('') as a default value might lead to unexpected behavior if the rest of the codebase expects undefined to signify the absence of a value. Ensure that all parts of the application that handle mobileNumber can correctly interpret an empty string as 'no value'.

shouldValidate: false,
})
setValue('mobileNumber', userData.mobileNumber, {
setValue('mobileNumber', userData.mobileNumber ?? '', {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
Using the nullish coalescing operator (??) to default userData.mobileNumber to an empty string ('') may cause issues if other parts of the application expect undefined to represent a missing value. Verify that this change aligns with the intended logic throughout the application.

mobileNumber: yup
.string()
.required(requiredMessage)
.required('Mobile Number must be defined')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ maintainability]
The change from .defined() to .required('Mobile Number must be defined') is redundant since .required() already implies .defined(). Consider removing the .required() at line 50 to avoid redundancy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant