Feat/user page test cases #48
Conversation
WalkthroughThe pull request introduces a comprehensive test suite for the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SB as SearchBar
participant UP as UsersPage
participant UA as UserActions
U->>SB: Types search query
SB->>UP: Calls onSearchChange (setSearchQuery)
UP->>UA: Fetches updated user list
U->>UP: Clicks "Next" button
UP->>UA: setCurrentPage(currentPage + 1)
UA-->>UP: Returns paginated results
U->>UP: Updates role / Requests deletion
UP->>UA: Executes update or delete action
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/app/(app)/users/page.js (1)
10-45: 🛠️ Refactor suggestionConsider implementing pagination state reset when search changes.
Currently, when a user performs a search, the pagination state isn't reset. This could lead to confusion if the filtered results have fewer pages than the original data, but the currentPage state remains unchanged.
Add a useEffect hook to reset pagination when search query changes:
useEffect(() => { const lowerCaseQuery = searchQuery.toLowerCase(); const results = users.filter( (user) => user.name.toLowerCase().includes(lowerCaseQuery) || user.email.toLowerCase().includes(lowerCaseQuery) || user.roles.some((role) => role.name.toLowerCase().includes(lowerCaseQuery) ) ); setFilteredUsers(results); + // Reset to first page when search query changes + if (searchQuery && currentPage !== 1) { + setCurrentPage(1); + } - }, [searchQuery, users]); + }, [searchQuery, users, currentPage]);
🧹 Nitpick comments (3)
__tests__/UsersPage.test.js (2)
72-84: Test for role updates is effective but could be more comprehensive.While the test correctly verifies that the update function is called with the right parameters and that the user list is refreshed, consider adding assertions to verify UI feedback such as the toast message.
await waitFor(() => { expect(mockUpdateUserRoles).toHaveBeenCalledWith(2, ["Admin"]); expect(mockFetchUsers).toHaveBeenCalledTimes(2); + // Verify toast message is shown + expect(screen.getByText("Role updated successfully!")).toBeInTheDocument(); });Note: You would need to mock the showToast function for this to work.
103-115: Pagination test verifies Next button functionality.The test correctly verifies that clicking the Next button fetches the next page of users. Consider adding a similar test for the Previous button to ensure complete pagination coverage.
test("handles pagination with Previous button", async () => { mockFetchUsers.mockResolvedValueOnce({ data: mockUsers, last_page: 2 }); render(<UsersPage />); // Set current page to 2 await waitFor(() => expect(mockFetchUsers).toHaveBeenCalledTimes(1)); const nextButton = await screen.findByTestId("pagination-next"); fireEvent.click(nextButton); await waitFor(() => expect(mockFetchUsers).toHaveBeenCalledWith(2)); // Now test Previous button const prevButton = await screen.findByTestId("pagination-previous"); fireEvent.click(prevButton); await waitFor(() => expect(mockFetchUsers).toHaveBeenCalledWith(1)); });src/app/(app)/users/page.js (1)
58-69: handleRoleChange could be optimized to avoid unnecessary fetches.The current implementation fetches all users after a role update. For better performance, especially with large user lists, consider updating the user locally first before requesting a refresh.
const handleRoleChange = async (userId, selectedRole) => { try { const rolePayload = selectedRole ? [selectedRole] : []; await updateUserRoles(userId, rolePayload); showToast("Role updated successfully!"); - const updatedUsers = await fetchUsers(currentPage); - setUsers(updatedUsers?.data || []); + // Update the user locally first + setUsers(users.map(user => { + if (user.id === userId) { + return { + ...user, + roles: selectedRole ? [{ name: selectedRole }] : [] + }; + } + return user; + })); + // Then refresh data in the background + fetchUsers(currentPage).then(updatedUsers => { + setUsers(updatedUsers?.data || []); + }); } catch (error) { showToast("An error occurred. Please try again."); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
__tests__/UsersPage.test.js(1 hunks)src/app/(app)/users/page.js(3 hunks)
🔇 Additional comments (12)
__tests__/UsersPage.test.js (5)
1-10: Test setup follows good practices with proper imports and mocking.The test file is well-structured with appropriate imports from testing libraries and proper mocking of the hooks. Using jest.mock() for the external hooks is a good approach for unit testing.
11-29: Good test data preparation.The mock data for users and roles is well-structured and provides sufficient test coverage for different scenarios (admin vs regular user). This will allow the tests to verify various aspects of the UI rendering and interactions.
31-50: Well-structured test setup with proper isolation.The beforeEach setup ensures each test runs with a fresh mock implementation, preventing test pollution. The mock implementation is comprehensive, covering all the necessary functions from the hooks.
52-70: Good initial rendering test with loading state verification.This test effectively checks both the loading state and the fully rendered user list. It verifies that the loading indicator appears and disappears at the right time, and that user data and role dropdowns are properly rendered.
86-101: Delete user test covers the full workflow effectively.This test comprehensively checks the user deletion flow, including the confirmation dialog and the subsequent API call. It correctly verifies that the user list is refreshed after deletion.
src/app/(app)/users/page.js (7)
86-86: Added data-testid for loading state to improve testability.Adding the data-testid attribute to the loading message enhances testability by providing a reliable selector for tests.
94-94: Simplified event handler with inline arrow function.Replacing a named handler function with an inline arrow function makes the code more concise in this case. Since the logic is simple (just setting state), this approach is reasonable.
117-118: Added data-testid to role select for improved testability.The data-testid attribute with dynamic userId makes it easier to target specific role dropdowns in tests, which is a good practice for testability.
134-135: Added data-testid to delete button for improved testability.Similar to the role select, the delete button now has a data-testid with the userId, making it easier to target in tests.
155-157: Added data-testid to pagination buttons and simplified with inline function.The pagination buttons now have data-testid attributes and use inline arrow functions for the onClick handlers. This improves testability and makes the code more concise.
165-167: Added data-testid to Next button and simplified with inline function.Similar to the Previous button, the Next button now has a data-testid and uses an inline arrow function, which improves testability and code conciseness.
177-177: Simplified onCancel handler with inline function.Replacing a named handler function with an inline arrow function is appropriate here as the logic is simple (just resetting state).
| roles: [{ name: "Admin" }], | ||
| }, | ||
| { | ||
| id: 2, | ||
| name: "Jane Smith", | ||
| email: "[email protected]", | ||
| roles: [{ name: "User" }], | ||
| }, | ||
| ]; | ||
|
|
||
| const mockRoles = [ | ||
| { id: 1, name: "Admin" }, | ||
| { id: 2, name: "User" }, | ||
| ]; | ||
|
|
||
| describe("UsersPage Component", () => { | ||
| let mockFetchUsers, mockFetchRoles, mockUpdateUserRoles, mockDeleteUser; | ||
|
|
||
| beforeEach(() => { | ||
| mockFetchUsers = jest | ||
| .fn() | ||
| .mockResolvedValue({ data: mockUsers, last_page: 1 }); | ||
| mockFetchRoles = jest.fn().mockResolvedValue(mockRoles); | ||
| mockUpdateUserRoles = jest.fn().mockResolvedValue({}); | ||
| mockDeleteUser = jest.fn().mockResolvedValue({}); | ||
|
|
||
| useUserActions.mockReturnValue({ | ||
| fetchUsers: mockFetchUsers, | ||
| fetchRoles: mockFetchRoles, | ||
| updateUserRoles: mockUpdateUserRoles, | ||
| deleteUser: mockDeleteUser, | ||
| }); | ||
|
|
||
| useAuth.mockReturnValue({ user: { id: 3, name: "Admin User" } }); | ||
| }); | ||
|
|
||
| test("renders user list and roles dropdown correctly", async () => { | ||
| render(<UsersPage />); | ||
|
|
||
| // Check loading state | ||
| expect(screen.getByTestId("loading-users")).toBeInTheDocument(); | ||
|
|
||
| await waitFor(() => { | ||
| expect(mockFetchUsers).toHaveBeenCalledTimes(1); | ||
| expect(screen.queryByTestId("loading-users")).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| // Verify user data | ||
| expect(screen.getByText("John Doe")).toBeInTheDocument(); | ||
| expect(screen.getByText("Jane Smith")).toBeInTheDocument(); | ||
|
|
||
| // Verify role selects | ||
| expect(screen.getByTestId("role-select-1")).toBeInTheDocument(); | ||
| expect(screen.getByTestId("role-select-2")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test("updates user role and refreshes data", async () => { | ||
| render(<UsersPage />); | ||
|
|
||
| await waitFor(() => expect(mockFetchUsers).toHaveBeenCalledTimes(1)); | ||
|
|
||
| const roleSelect = await screen.findByTestId("role-select-2"); | ||
| fireEvent.change(roleSelect, { target: { value: "Admin" } }); | ||
|
|
||
| await waitFor(() => { | ||
| expect(mockUpdateUserRoles).toHaveBeenCalledWith(2, ["Admin"]); | ||
| expect(mockFetchUsers).toHaveBeenCalledTimes(2); | ||
| }); | ||
| }); | ||
|
|
||
| test("deletes a user after confirmation", async () => { | ||
| render(<UsersPage />); | ||
|
|
||
| await waitFor(() => expect(mockFetchUsers).toHaveBeenCalledTimes(1)); | ||
|
|
||
| const deleteButton = await screen.findByTestId("delete-user-2"); | ||
| fireEvent.click(deleteButton); | ||
|
|
||
| const confirmButton = await screen.findByText("Confirm"); | ||
| fireEvent.click(confirmButton); | ||
|
|
||
| await waitFor(() => { | ||
| expect(mockDeleteUser).toHaveBeenCalledWith(2); | ||
| expect(mockFetchUsers).toHaveBeenCalledTimes(2); | ||
| }); | ||
| }); | ||
|
|
||
| test("handles pagination with Next button", async () => { | ||
| mockFetchUsers.mockResolvedValueOnce({ data: mockUsers, last_page: 2 }); | ||
|
|
||
| render(<UsersPage />); | ||
|
|
||
| await waitFor(() => expect(mockFetchUsers).toHaveBeenCalledTimes(1)); | ||
|
|
||
| const nextButton = await screen.findByTestId("pagination-next"); | ||
| fireEvent.click(nextButton); | ||
|
|
||
| await waitFor(() => expect(mockFetchUsers).toHaveBeenCalledWith(2)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding tests for search functionality and error handling.
The current test suite covers rendering, role updates, deletion, and pagination but doesn't test the search functionality or error handling. Adding tests for these scenarios would provide more comprehensive coverage.
For search functionality:
test("filters users based on search query", async () => {
render(<UsersPage />);
await waitFor(() => expect(mockFetchUsers).toHaveBeenCalledTimes(1));
const searchBar = screen.getByPlaceholderText("Search...");
fireEvent.change(searchBar, { target: { value: "john" } });
expect(screen.getByText("John Doe")).toBeInTheDocument();
expect(screen.queryByText("Jane Smith")).not.toBeInTheDocument();
});For error handling:
test("shows error message when API call fails", async () => {
// Mock failed API call
mockFetchUsers.mockRejectedValueOnce(new Error("API Error"));
render(<UsersPage />);
await waitFor(() => {
expect(screen.getByText("An error occurred. Please try again.")).toBeInTheDocument();
});
});| {filteredUsers.map((user) => ( | ||
| <tr key={user.id} className="odd:bg-gray-50 even:bg-white"> | ||
| <td className="p-4 text-gray-700 truncate max-w-xs"> | ||
| {user.name} | ||
| </td> | ||
| <td className="p-4 text-gray-700 truncate max-w-xs"> | ||
| {user.email} | ||
| </td> | ||
| <td className="p-4 text-center"> | ||
| <select | ||
| data-testid={`role-select-${user.id}`} | ||
| value={user?.roles?.[0]?.name || ""} | ||
| onChange={(e) => | ||
| handleRoleChange(user.id, e.target.value) | ||
| } | ||
| className="w-32 p-2 border rounded" | ||
| > | ||
| <option value="">Select Role</option> | ||
| {roles.map((role) => ( | ||
| <option key={role.id} value={role.name}> | ||
| {role.name} | ||
| </option> | ||
| ))} | ||
| </select> | ||
| </td> | ||
| <td className="p-4 text-center"> | ||
| <Button | ||
| data-testid={`delete-user-${user.id}`} | ||
| onClick={() => { | ||
| if (user.id !== currentUser.id) { | ||
| setUserToDelete(user.id); | ||
| setIsPopupOpen(true); | ||
| } | ||
| className="w-32 p-2 border rounded" | ||
| > | ||
| <option value="">Select Role</option> | ||
| {roles.map((role) => ( | ||
| <option key={role.id} value={role.name}> | ||
| {role.name} | ||
| </option> | ||
| ))} | ||
| </select> | ||
| </td> | ||
| <td className="p-4 text-center"> | ||
| <Button | ||
| onClick={() => { | ||
| if (user.id !== currentUser.id) { | ||
| setUserToDelete(user.id); | ||
| setIsPopupOpen(true); | ||
| } | ||
| }} | ||
| disabled={user.id === currentUser.id} | ||
| className="bg-red-500 hover:bg-red-600" | ||
| > | ||
| Delete | ||
| </Button> | ||
| </td> | ||
| </tr> | ||
| )) | ||
| ) : ( | ||
| <tr> | ||
| <td colSpan="4" className="p-4 text-center"> | ||
| No users found. | ||
| }} | ||
| disabled={user.id === currentUser.id} | ||
| className="bg-red-500 hover:bg-red-600" | ||
| > | ||
| Delete | ||
| </Button> | ||
| </td> | ||
| </tr> | ||
| )} | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
User list rendering has been simplified but lacks empty state handling.
The code now directly maps over filteredUsers without checking if the array is empty. While this simplifies the code, it doesn't handle the case when no users match the search criteria.
Add a conditional to handle empty state:
-{filteredUsers.map((user) => (
+{filteredUsers.length > 0 ? (
+ filteredUsers.map((user) => (
<tr key={user.id} className="odd:bg-gray-50 even:bg-white">
{/* Row content */}
</tr>
- ))}
+ ))
+) : (
+ <tr>
+ <td colSpan="4" className="p-4 text-center text-gray-500">
+ No users found
+ </td>
+ </tr>
+)}📝 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.
| {filteredUsers.map((user) => ( | |
| <tr key={user.id} className="odd:bg-gray-50 even:bg-white"> | |
| <td className="p-4 text-gray-700 truncate max-w-xs"> | |
| {user.name} | |
| </td> | |
| <td className="p-4 text-gray-700 truncate max-w-xs"> | |
| {user.email} | |
| </td> | |
| <td className="p-4 text-center"> | |
| <select | |
| data-testid={`role-select-${user.id}`} | |
| value={user?.roles?.[0]?.name || ""} | |
| onChange={(e) => | |
| handleRoleChange(user.id, e.target.value) | |
| } | |
| className="w-32 p-2 border rounded" | |
| > | |
| <option value="">Select Role</option> | |
| {roles.map((role) => ( | |
| <option key={role.id} value={role.name}> | |
| {role.name} | |
| </option> | |
| ))} | |
| </select> | |
| </td> | |
| <td className="p-4 text-center"> | |
| <Button | |
| data-testid={`delete-user-${user.id}`} | |
| onClick={() => { | |
| if (user.id !== currentUser.id) { | |
| setUserToDelete(user.id); | |
| setIsPopupOpen(true); | |
| } | |
| className="w-32 p-2 border rounded" | |
| > | |
| <option value="">Select Role</option> | |
| {roles.map((role) => ( | |
| <option key={role.id} value={role.name}> | |
| {role.name} | |
| </option> | |
| ))} | |
| </select> | |
| </td> | |
| <td className="p-4 text-center"> | |
| <Button | |
| onClick={() => { | |
| if (user.id !== currentUser.id) { | |
| setUserToDelete(user.id); | |
| setIsPopupOpen(true); | |
| } | |
| }} | |
| disabled={user.id === currentUser.id} | |
| className="bg-red-500 hover:bg-red-600" | |
| > | |
| Delete | |
| </Button> | |
| </td> | |
| </tr> | |
| )) | |
| ) : ( | |
| <tr> | |
| <td colSpan="4" className="p-4 text-center"> | |
| No users found. | |
| }} | |
| disabled={user.id === currentUser.id} | |
| className="bg-red-500 hover:bg-red-600" | |
| > | |
| Delete | |
| </Button> | |
| </td> | |
| </tr> | |
| )} | |
| ))} | |
| {filteredUsers.length > 0 ? ( | |
| filteredUsers.map((user) => ( | |
| <tr key={user.id} className="odd:bg-gray-50 even:bg-white"> | |
| <td className="p-4 text-gray-700 truncate max-w-xs"> | |
| {user.name} | |
| </td> | |
| <td className="p-4 text-gray-700 truncate max-w-xs"> | |
| {user.email} | |
| </td> | |
| <td className="p-4 text-center"> | |
| <select | |
| data-testid={`role-select-${user.id}`} | |
| value={user?.roles?.[0]?.name || ""} | |
| onChange={(e) => | |
| handleRoleChange(user.id, e.target.value) | |
| } | |
| className="w-32 p-2 border rounded" | |
| > | |
| <option value="">Select Role</option> | |
| {roles.map((role) => ( | |
| <option key={role.id} value={role.name}> | |
| {role.name} | |
| </option> | |
| ))} | |
| </select> | |
| </td> | |
| <td className="p-4 text-center"> | |
| <Button | |
| data-testid={`delete-user-${user.id}`} | |
| onClick={() => { | |
| if (user.id !== currentUser.id) { | |
| setUserToDelete(user.id); | |
| setIsPopupOpen(true); | |
| } | |
| }} | |
| disabled={user.id === currentUser.id} | |
| className="bg-red-500 hover:bg-red-600" | |
| > | |
| Delete | |
| </Button> | |
| </td> | |
| </tr> | |
| )) | |
| ) : ( | |
| <tr> | |
| <td colSpan="4" className="p-4 text-center text-gray-500"> | |
| No users found | |
| </td> | |
| </tr> | |
| )} |
There was a problem hiding this comment.
@belwalshubham I think we need empty state handling.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
#12
Covered test cases for:
Summary by CodeRabbit
Tests
Refactor