Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BugFix : Fixed Improper Handling for Non-Numeric Values in Allotted hours section of Create Action item modal #3417

Closed
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
184 changes: 184 additions & 0 deletions src/screens/OrganizationActionItems/ItemModal.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,48 @@ describe('Testing ItemModal', () => {
});
});

it('should not allow letters or negative values in allotted hours', async () => {
renderItemModal(link1, itemProps[0]);

const allottedHours = screen.getByLabelText(t.allottedHours);
expect(allottedHours).toBeInTheDocument();

// Test letter input
fireEvent.change(allottedHours, { target: { value: 'abc' } });
await waitFor(() => {
expect(allottedHours).toHaveValue('');
});

// Test negative value
fireEvent.change(allottedHours, { target: { value: '-5' } });
await waitFor(() => {
expect(allottedHours).toHaveValue('');
});

// Test valid positive number
fireEvent.change(allottedHours, { target: { value: '5' } });
await waitFor(() => {
expect(allottedHours).toHaveValue('5');
});
});

it('validates allottedHours edge cases', async () => {
renderItemModal(link1, itemProps[0]);
const allottedHours = screen.getByLabelText(t.allottedHours);

// Test invalid string
fireEvent.change(allottedHours, { target: { value: 'invalid' } });
expect(allottedHours).toHaveValue('');

// Test NaN
fireEvent.change(allottedHours, { target: { value: NaN } });
expect(allottedHours).toHaveValue('');

// Test negative number
fireEvent.change(allottedHours, { target: { value: -5 } });
expect(allottedHours).toHaveValue('');
});

it('should fail to Create Action Item', async () => {
renderItemModal(link2, itemProps[0]);
// Click Submit
Expand All @@ -802,6 +844,148 @@ describe('Testing ItemModal', () => {
});
});

//checking for empty and null values
it('handles empty and null form values correctly', async () => {
renderItemModal(link1, itemProps[0]);

await waitFor(async () => {
const allottedHours = screen.getByLabelText(t.allottedHours);
const preCompletionNotes = screen.getByLabelText(t.preCompletionNotes);

fireEvent.change(allottedHours, { target: { value: '' } });
expect(allottedHours).toHaveValue('');

fireEvent.change(preCompletionNotes, { target: { value: '' } });
expect(preCompletionNotes).toHaveValue('');

fireEvent.change(allottedHours, { target: { value: null } });
expect(allottedHours).toHaveValue('');
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add assertions for form submission with empty values.

While the test verifies that empty/null values are handled correctly in the UI, it should also verify the form submission behavior with these values.

   it('handles empty and null form values correctly', async () => {
     renderItemModal(link1, itemProps[0]);
 
     await waitFor(async () => {
       const allottedHours = screen.getByLabelText(t.allottedHours);
       const preCompletionNotes = screen.getByLabelText(t.preCompletionNotes);
 
       fireEvent.change(allottedHours, { target: { value: '' } });
       expect(allottedHours).toHaveValue('');
 
       fireEvent.change(preCompletionNotes, { target: { value: '' } });
       expect(preCompletionNotes).toHaveValue('');
 
       fireEvent.change(allottedHours, { target: { value: null } });
       expect(allottedHours).toHaveValue('');
+
+      // Verify form submission with empty values
+      const submitButton = screen.getByTestId('submitBtn');
+      fireEvent.click(submitButton);
+      
+      // Add expectations based on your requirements
+      // For example:
+      // expect(toast.warning).toHaveBeenCalledWith('Please fill in required fields');
     });
   });

Committable suggestion skipped: line range outside the PR's diff.


// validation of catergory selection
it('validates category selection', async () => {
renderItemModal(link1, itemProps[0]);

await waitFor(async () => {
const categorySelect = await screen.findByTestId('categorySelect');
const inputField = within(categorySelect).getByRole('combobox');

const submitButton = screen.getByTestId('submitBtn');
fireEvent.click(submitButton);
expect(inputField).toBeRequired();

fireEvent.mouseDown(inputField);
const categoryOption = await screen.findByText('Category 1');
fireEvent.click(categoryOption);
expect(inputField).toHaveValue('Category 1');

fireEvent.change(inputField, { target: { value: '' } });
expect(inputField).toHaveValue('');
});
});

// changing of assignee type handling
it('handles assignee type changes correctly', async () => {
renderItemModal(link1, itemProps[1]);

await waitFor(async () => {
const groupRadio = await screen.findByText(t.groups);
const individualRadio = await screen.findByText(t.individuals);

fireEvent.click(individualRadio);
expect(await screen.findByTestId('volunteerSelect')).toBeInTheDocument();

fireEvent.click(groupRadio);
expect(
await screen.findByTestId('volunteerGroupSelect'),
).toBeInTheDocument();

fireEvent.click(individualRadio);
expect(await screen.findByTestId('volunteerSelect')).toBeInTheDocument();
});
});

// validating when dates are due
it('validates due date handling', async () => {
renderItemModal(link1, itemProps[0]);

await waitFor(async () => {
const dateInput = screen.getByLabelText(t.dueDate);

fireEvent.change(dateInput, { target: { value: 'invalid date' } });
expect(dateInput).toHaveValue('');

fireEvent.change(dateInput, { target: { value: '01/01/2020' } });
expect(dateInput).toHaveValue('01/01/2020');

fireEvent.change(dateInput, { target: { value: '01/01/2025' } });
expect(dateInput).toHaveValue('01/01/2025');
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation for past dates.

The test should verify that past dates are not accepted as valid due dates.

 it('validates due date handling', async () => {
   renderItemModal(link1, itemProps[0]);

   await waitFor(async () => {
     const dateInput = screen.getByLabelText(t.dueDate);

     fireEvent.change(dateInput, { target: { value: 'invalid date' } });
     expect(dateInput).toHaveValue('');

-    fireEvent.change(dateInput, { target: { value: '01/01/2020' } });
-    expect(dateInput).toHaveValue('01/01/2020');
+    // Test past date
+    const pastDate = '01/01/2020';
+    fireEvent.change(dateInput, { target: { value: pastDate } });
+    expect(dateInput).toHaveValue('');
+    expect(screen.queryByText(/date must be in the future/i)).toBeInTheDocument();

     fireEvent.change(dateInput, { target: { value: '01/01/2025' } });
     expect(dateInput).toHaveValue('01/01/2025');
   });
 });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// validating when dates are due
it('validates due date handling', async () => {
renderItemModal(link1, itemProps[0]);
await waitFor(async () => {
const dateInput = screen.getByLabelText(t.dueDate);
fireEvent.change(dateInput, { target: { value: 'invalid date' } });
expect(dateInput).toHaveValue('');
fireEvent.change(dateInput, { target: { value: '01/01/2020' } });
expect(dateInput).toHaveValue('01/01/2020');
fireEvent.change(dateInput, { target: { value: '01/01/2025' } });
expect(dateInput).toHaveValue('01/01/2025');
});
});
// validating when dates are due
it('validates due date handling', async () => {
renderItemModal(link1, itemProps[0]);
await waitFor(async () => {
const dateInput = screen.getByLabelText(t.dueDate);
fireEvent.change(dateInput, { target: { value: 'invalid date' } });
expect(dateInput).toHaveValue('');
// Test past date
const pastDate = '01/01/2020';
fireEvent.change(dateInput, { target: { value: pastDate } });
expect(dateInput).toHaveValue('');
expect(screen.queryByText(/date must be in the future/i)).toBeInTheDocument();
fireEvent.change(dateInput, { target: { value: '01/01/2025' } });
expect(dateInput).toHaveValue('01/01/2025');
});
});


// for testing network handling
it('handles network errors gracefully', async () => {
renderItemModal(link2, itemProps[0]);

await waitFor(async () => {
const categorySelect = await screen.findByTestId('categorySelect');
const inputField = within(categorySelect).getByRole('combobox');
fireEvent.mouseDown(inputField);
const categoryOption = await screen.findByText('Category 1');
fireEvent.click(categoryOption);

const memberSelect = await screen.findByTestId('memberSelect');
const memberInput = within(memberSelect).getByRole('combobox');
fireEvent.mouseDown(memberInput);
const memberOption = await screen.findByText('Harve Lance');
fireEvent.click(memberOption);

const submitButton = screen.getByTestId('submitBtn');
fireEvent.click(submitButton);
});

await waitFor(() => {
expect(toast.error).toHaveBeenCalledWith('Mock Graphql Error');
});
});

it('handles null date change', async () => {
renderItemModal(link1, itemProps[0]);

const dateInput = screen.getByLabelText(t.dueDate);
fireEvent.change(dateInput, { target: { value: null } });

await waitFor(() => {
expect(dateInput).toHaveValue('');
});
});

// for handling edge cases in timezone
it('handles timezone edge cases', async () => {
renderItemModal(link1, itemProps[0]);

await waitFor(async () => {
const dateInput = screen.getByLabelText(t.dueDate);

// Test dates around DST changes
const dstDates = [
'03/12/2025', // Spring forward
'11/05/2025', // Fall back
];

for (const date of dstDates) {
fireEvent.change(dateInput, { target: { value: date } });
expect(dateInput).toHaveValue(date);
}

// Test midnight boundary dates
fireEvent.change(dateInput, { target: { value: '01/01/2025' } });
expect(dateInput).toHaveValue('01/01/2025');
});
});

// For testing failure of updating action item
it('should fail to Update Action Item', async () => {
renderItemModal(link2, itemProps[2]);
expect(screen.getAllByText(t.updateActionItem)).toHaveLength(2);
Expand Down
16 changes: 14 additions & 2 deletions src/screens/OrganizationActionItems/ItemModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,20 @@ const ItemModal: FC<InterfaceItemModalProps> = ({
field: keyof InterfaceFormStateType,
value: string | number | boolean | Date | undefined | null,
): void => {
// Special handling for allottedHours
if (field === 'allottedHours') {
// If the value is not a valid number or is negative, set to null
const numValue = typeof value === 'string' ? Number(value) : value;
if (
typeof numValue !== 'number' ||
Number.isNaN(numValue) ||
numValue < 0
) {
setFormState((prevState) => ({ ...prevState, [field]: null }));
return;
}
}

setFormState((prevState) => ({ ...prevState, [field]: value }));
};

Expand Down Expand Up @@ -574,8 +588,6 @@ const ItemModal: FC<InterfaceItemModalProps> = ({
className={styles.noOutline}
value={dayjs(dueDate)}
onChange={(date: Dayjs | null): void => {
// Added istanbul ignore else, which will ignore else condition, we are not using else condition here
/* istanbul ignore else -- @preserve */
if (date) handleFormChange('dueDate', date.toDate());
}}
/>
Expand Down
Loading