Skip to content

Conversation

@suppermancool
Copy link
Contributor

import { format } from 'date-fns'

import { CheckIcon, XIcon } from '@heroicons/react/solid'
import { CheckIcon } from '@heroicons/react/solid'
Copy link

Choose a reason for hiding this comment

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

The XIcon import has been removed. Ensure that this icon is not used elsewhere in the component. If it is needed, it should be re-imported.

onApproveApplication: ReviewerListProps['onApproveApplication']
onUnapproveApplication: ReviewerListProps['onUnapproveApplication']
}> = props => {
const handleApprove = useEventCallback((): void => {
Copy link

Choose a reason for hiding this comment

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

The onUnapproveApplication prop has been removed from the component's props. Ensure that this is intentional and that the functionality for unapproving an application is no longer required.


const isApproving = props.approvingReviewerId === props.reviewer.userId
const isOtherApproving = props.approvingReviewerId > 0
const hideApproveButton
Copy link

Choose a reason for hiding this comment

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

The handleRemove function has been removed. Verify that the removal of this function aligns with the intended functionality changes, as it was previously used to handle unapproving applications.

{isApproving ? (
<LoadingSpinner
inline
className={styles.approvingLoadingSpinner}
Copy link

Choose a reason for hiding this comment

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

The showRemoveButton logic has been removed. Confirm that the removal of this logic is intended and that the UI no longer requires a remove button for approved applications.

)
) : (
!hideApproveButton && (
<Button
Copy link

Choose a reason for hiding this comment

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

The variant='danger' prop was removed from the Button component. If this was intentional, ensure that the styling and behavior of the button still meet the requirements. If it was not intentional, consider adding it back to maintain the previous styling.

*/
export type TableRolesFilter = {
id: string
id?: string
Copy link

Choose a reason for hiding this comment

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

The id field has been changed to optional. Ensure that this change aligns with the intended functionality and that all parts of the application that rely on this field being present are updated accordingly.

modifiedAtString: string
createdByHandle: string
modifiedByHandle: string
createdAtString?: string
Copy link

Choose a reason for hiding this comment

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

The createdAtString field has been made optional. Verify that this change does not affect any logic that assumes this field is always present.

createdByHandle: string
modifiedByHandle: string
createdAtString?: string
modifiedAtString?: string
Copy link

Choose a reason for hiding this comment

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

The modifiedAtString field is now optional. Check if there are any dependencies or logic that require this field to be non-optional.

modifiedByHandle: string
createdAtString?: string
modifiedAtString?: string
createdByHandle?: string
Copy link

Choose a reason for hiding this comment

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

The createdByHandle field has been changed to optional. Ensure that this change does not break any existing functionality that expects this field to be mandatory.

createdAtString?: string
modifiedAtString?: string
createdByHandle?: string
modifiedByHandle?: string
Copy link

Choose a reason for hiding this comment

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

The modifiedByHandle field is now optional. Confirm that this change is consistent with the application's requirements and does not introduce any issues.

* validation schema for form new billing account
*/
export const formEditBillingAccountSchema: Yup.ObjectSchema<FormEditBillingAccount>
export const formAddBillingAccountSchema: Yup.ObjectSchema<FormEditBillingAccount>
Copy link

Choose a reason for hiding this comment

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

The variable name formAddBillingAccountSchema suggests that this schema is for adding a new billing account, but the type FormEditBillingAccount implies it is for editing. Ensure the naming is consistent with the intended use of the schema.

/**
* validation schema for form new billing account
*/
export const formEditBillingAccountSchema: Yup.ObjectSchema<FormEditBillingAccount>
Copy link

Choose a reason for hiding this comment

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

The variable formEditBillingAccountSchema is declared twice in the file. Consider renaming the newly added schema to avoid conflicts and ensure clarity.

.trim()
.required('Description is required.'),
endDate: Yup.date()
.required('End date is required.'),
Copy link

Choose a reason for hiding this comment

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

Consider adding a .nullable() method to the endDate field to handle cases where the date might be null.

salesTax: Yup.number()
.required('Sales tax is required.'),
startDate: Yup.date()
.required('Start date is required.'),
Copy link

Choose a reason for hiding this comment

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

Consider adding a .nullable() method to the startDate field to handle cases where the date might be null.

.trim()
.required('PO Number is required.'),
salesTax: Yup.number()
.required('Sales tax is required.'),
Copy link

Choose a reason for hiding this comment

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

The salesTax field should have a .typeError('Invalid number.') similar to other number fields for consistency in error messaging.

useState,
} from 'react'
import { NavigateFunction, useNavigate, useParams } from 'react-router-dom'
import { useParams } from 'react-router-dom'
Copy link

Choose a reason for hiding this comment

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

The NavigateFunction and useNavigate imports have been removed. Ensure that these are not used elsewhere in the file, as their removal could lead to runtime errors if they are still needed.

}>()
const [challengeUuid, setChallengeUuid] = useState('')
const navigate: NavigateFunction = useNavigate()
const [filterCriteria, setFilterCriteria]: [
Copy link

Choose a reason for hiding this comment

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

The navigate variable declaration has been removed. Ensure that this removal does not affect any navigation functionality within the component. If navigate was used elsewhere in the code, consider refactoring those parts to avoid errors.

primary
onClick={handleRejectPendingConfirmDialog}
size='lg'
to={`${rootRoute}/challenge-management/${challengeUuid}/manage-user`}
Copy link

Choose a reason for hiding this comment

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

The to prop in the LinkButton component should be reviewed to ensure it correctly constructs the URL. Verify that rootRoute, challengeUuid, and the rest of the path are correctly defined and concatenated to form a valid URL.

<ReviewerList
reviewers={reviewers}
openReviews={openReviews}
onApproveApplication={approve}
Copy link

Choose a reason for hiding this comment

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

The onUnapproveApplication prop has been removed. Ensure that this is intentional and that the functionality for unapproving applications is no longer needed. If it is still required, consider re-adding it or implementing the functionality elsewhere.

@jmgasper jmgasper merged commit a82243b into feat/system-admin Jun 5, 2025
3 checks passed
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.

3 participants