[Netmanager] Fix displaying users lists #2302
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to several files in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 4
🧹 Outside diff range and nitpick comments (10)
netmanager/src/views/pages/UserList/AvailableUserList.js (3)
34-41: Consider adding error handling in theuseEffecthook.Currently, there is no error handling for the
fetchAvailableNetworkUsersdispatch within theuseEffecthook. Incorporating error handling can improve robustness and user feedback in case of API failures.
43-51: Add error handling inhandlePageChange.Including error handling when dispatching
fetchAvailableNetworkUserswill help manage any potential errors during data fetching and enhance user experience.
53-61: Incorporate error handling inhandleRowsPerPageChange.Adding error handling to the dispatch action within
handleRowsPerPageChangecan prevent unhandled errors and provide better feedback to the user if the fetch operation fails.netmanager/src/views/pages/UserList/components/UsersTable/AvailableUsersTable.js (3)
Line range hint
74-89: Correct the loading state management insubmitEditUser.The
setLoading(false);call immediately after theifblock causes the loading state to end before the asynchronous operation completes, which might lead to inconsistent UI behavior. MovesetLoading(false);inside the.thenand.catchblocks to ensure it reflects the actual loading status.Apply this diff to fix the issue:
const submitEditUser = (user) => { setLoading(true); const userID = user._id; // assign user to network if (!isEmpty(activeNetwork) && !isEmpty(userID)) { assignUserNetworkApi(activeNetwork._id, userID) .then((response) => { dispatch(fetchAvailableNetworkUsers(activeNetwork._id)); dispatch( updateMainAlert({ message: 'User successfully added to the organisation. Check in assigned users table', show: true, severity: 'success' }) ); + setLoading(false); }) .catch((error) => { const errors = error.response && error.response.data && error.response.data.errors; dispatch( updateMainAlert({ message: error.response && error.response.data && error.response.data.message, show: true, severity: 'error', extra: createAlertBarExtraContentFromObject(errors || {}) }) ); + setLoading(false); }); } - setLoading(false); };
193-198: Remove console.log statements fromTablePaginationActions.The
console.logstatements used for debugging should be removed in the production code to keep the console output clean.Apply this diff to remove the console.log statements:
function TablePaginationActions(props) { const { count, page, rowsPerPage, onPageChange } = props; - console.log('TablePaginationActions props:', { - count, - page, - rowsPerPage, - hasOnPageChange: !!onPageChange - });
213-219: Eliminate debuggingconsole.logstatements from pagination handlers.Removing the console logs in the pagination handlers will prevent unnecessary clutter in the console output.
Apply this diff:
const handleNextButtonClick = (event) => { - console.log('Next button clicked, current page:', page); - console.log('onPageChange exists:', !!onPageChange); if (onPageChange) { - console.log('Calling onPageChange with:', page + 1); onPageChange(event, page + 1); } };netmanager/src/views/pages/UserList/components/UsersTable/UsersTable.js (2)
106-113: Remove console.log statements fromTablePaginationActions.The console logs within
TablePaginationActionsare for debugging and should be removed to maintain a clean console in production environments.Apply this diff to remove the console.log statements:
function TablePaginationActions(props) { const { count, page, rowsPerPage, onPageChange } = props; - console.log('TablePaginationActions props:', { - count, - page, - rowsPerPage, - hasOnPageChange: !!onPageChange - });
128-134: Delete debuggingconsole.logstatements in pagination handlers.Removing these console logs helps keep the console output clean and avoids potential performance issues.
Apply this diff:
const handleNextButtonClick = (event) => { - console.log('Next button clicked, current page:', page); - console.log('onPageChange exists:', !!onPageChange); if (onPageChange) { - console.log('Calling onPageChange with:', page + 1); onPageChange(event, page + 1); } };netmanager/src/redux/AccessControl/operations.js (2)
81-81: Consider using action type constants instead of string literalsThe loading state management is a good addition, but using string literals for action types ('SET_NETWORK_USERS_LOADING') makes the code more prone to typos and harder to maintain. Consider defining these as constants alongside other action types.
+ // In actions.js + export const SET_NETWORK_USERS_LOADING = 'SET_NETWORK_USERS_LOADING'; + export const SET_AVAILABLE_USERS_LOADING = 'SET_AVAILABLE_USERS_LOADING'; - dispatch({ type: 'SET_NETWORK_USERS_LOADING', payload: true }); + dispatch({ type: SET_NETWORK_USERS_LOADING, payload: true });Also applies to: 98-98
104-104: Apply the same action type constant pattern hereFor consistency with the previous suggestion, the loading state action type should also be defined as a constant.
- dispatch({ type: 'SET_AVAILABLE_USERS_LOADING', payload: true }); + dispatch({ type: SET_AVAILABLE_USERS_LOADING, payload: true });Also applies to: 121-121
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
netmanager/src/redux/AccessControl/operations.js(3 hunks)netmanager/src/redux/AccessControl/reducers.js(2 hunks)netmanager/src/views/pages/UserList/AvailableUserList.js(2 hunks)netmanager/src/views/pages/UserList/UserList.js(3 hunks)netmanager/src/views/pages/UserList/components/UsersTable/AvailableUsersTable.js(3 hunks)netmanager/src/views/pages/UserList/components/UsersTable/UsersTable.js(4 hunks)
🔇 Additional comments (8)
netmanager/src/views/pages/UserList/AvailableUserList.js (3)
12-12: Importing fetchAvailableNetworkUsers correctly.
The import statement for fetchAvailableNetworkUsers is appropriate and ensures the required action is available for dispatch.
26-27: Initializing pagination state variables.
The page and pageSize state variables are properly initialized using useState, allowing for effective pagination control.
30-31: Utilizing Redux state for users and loading status.
Accessing users, total, and loading from the Redux store with useSelector enhances state management consistency across the application.
netmanager/src/views/pages/UserList/UserList.js (3)
15-15: Successfully importing fetchNetworkUsers.
The import statement ensures that the fetchNetworkUsers action is available for dispatch, aligning the component with the Redux action flow.
29-30: Initializing pagination control state variables.
The page and pageSize state variables are correctly initialized, setting up the component for effective pagination management.
33-33: Accessing users and loading state from Redux store.
Using useSelector to retrieve users, total, and loading from the Redux state ensures consistent data and loading status throughout the application.
netmanager/src/redux/AccessControl/operations.js (2)
Line range hint 89-97: Well-implemented error handling pattern
The error handling implementation is robust and follows best practices:
- Dispatches failure actions for Redux state updates
- Rethrows errors for component-level handling
- Uses finally block to guarantee loading state reset
Also applies to: 112-120
Line range hint 81-98: Verify reducer handling of loading states
Let's verify that the reducer properly handles these new loading state actions.
Also applies to: 104-121
✅ Verification successful
Loading state actions are properly handled in the reducer ✅
The reducer correctly handles the SET_NETWORK_USERS_LOADING action by:
- Maintaining the loading state in
networkUsers.loading - Preserving other state properties during updates
- Having proper initial state with
loading: false
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the corresponding reducer cases
ast-grep --pattern 'case $ACTION_TYPE: {
$$$
loading: $$$
$$$
}'
# Also check for the initial state definition
rg -A 5 'initialState.*=.*{' netmanager/src/redux/AccessControl/reducers.js
Length of output: 250
Script:
#!/bin/bash
# Search for the reducer file content
cat netmanager/src/redux/AccessControl/reducers.js
# Alternatively, search for loading state related cases
rg "SET_NETWORK_USERS_LOADING" netmanager/src/redux/AccessControl/
Length of output: 2531
| if (!activeNetwork) return; | ||
| dispatch(loadRolesSummary(activeNetwork._id)); | ||
| }, []); | ||
|
|
||
| const fetchUsers = async (skipCount, limitCount) => { | ||
| if (!activeNetwork) return; | ||
| setLoading(true); | ||
| try { | ||
| const res = await getNetworkUsersListApi(activeNetwork._id, { | ||
| skip: skipCount, | ||
| limit: limitCount | ||
| }); | ||
| setUsers(res.assigned_users); | ||
| setTotalCount(res.total || 0); | ||
| dispatch(loadRolesSummary(activeNetwork._id)); | ||
| dispatch( | ||
| fetchNetworkUsers(activeNetwork._id, { | ||
| skip: page * pageSize, | ||
| limit: pageSize | ||
| }) | ||
| ); | ||
| } catch (error) { | ||
| let errorMessage = 'An error occurred'; | ||
| if (error.response && error.response.status >= 500) { | ||
| errorMessage = 'An error occurred. Please try again later'; | ||
| } else if (error.response && error.response.data && error.response.data.message) { | ||
| errorMessage = error.response.data.message; | ||
| } else if (error.message) { | ||
| errorMessage = error.message; | ||
| } | ||
| console.error('Error fetching network users:', error); | ||
| } | ||
| }, [activeNetwork]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reevaluate error handling in the useEffect hook.
Wrapping dispatch calls in a try-catch block may not effectively handle asynchronous errors from Redux actions. It's better to handle errors within the action creators or use middleware like Redux Thunk or Redux Saga for async operations.
| const handlePageChange = (event, newPage) => { | ||
| setPage(newPage); | ||
| try { | ||
| dispatch( | ||
| updateMainAlert({ | ||
| message: errorMessage, | ||
| show: true, | ||
| severity: 'error' | ||
| fetchNetworkUsers(activeNetwork._id, { | ||
| skip: newPage * pageSize, | ||
| limit: pageSize | ||
| }) | ||
| ); | ||
| } finally { | ||
| setLoading(false); | ||
| } catch (error) { | ||
| console.error('Error fetching network users:', error); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust error handling in handlePageChange.
The try-catch block may not catch errors from asynchronous dispatch operations. Consider managing errors within the fetchNetworkUsers action or utilizing middleware to handle async actions properly.
| const handleRowsPerPageChange = (newPageSize) => { | ||
| setPageSize(newPageSize); | ||
| setPage(0); | ||
| try { | ||
| dispatch( | ||
| fetchNetworkUsers(activeNetwork._id, { | ||
| skip: 0, | ||
| limit: newPageSize | ||
| }) | ||
| ); | ||
| } catch (error) { | ||
| console.error('Error fetching network users:', error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in handleRowsPerPageChange.
Similar to other dispatch calls, the current error handling may not be effective for asynchronous actions. Handling errors within the action creator or middleware can provide more reliable error management.
| case 'SET_NETWORK_USERS_LOADING': | ||
| return { | ||
| ...state, | ||
| networkUsers: { | ||
| ...state.networkUsers, | ||
| loading: action.payload | ||
| } | ||
| }; | ||
| case 'SET_AVAILABLE_USERS_LOADING': | ||
| return { | ||
| ...state, | ||
| availableUsers: { | ||
| ...state.availableUsers, | ||
| loading: action.payload | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define action types as constants for consistency.
Currently, 'SET_NETWORK_USERS_LOADING' and 'SET_AVAILABLE_USERS_LOADING' are string literals. Defining them as constants in the actions.js file enhances code maintainability and reduces the risk of typos.
Apply this diff:
In actions.js:
+export const SET_NETWORK_USERS_LOADING = 'SET_NETWORK_USERS_LOADING';
+export const SET_AVAILABLE_USERS_LOADING = 'SET_AVAILABLE_USERS_LOADING';In reducers.js:
import {
ASSIGN_USER_TO_NETWORK_SUCCESS,
LOAD_ALL_USER_ROLES_SUCCESS,
LOAD_AVAILABLE_USERS_SUCCESS,
LOAD_CURRENT_NETWORK_SUCCESS,
LOAD_CURRENT_USER_NETWORKS_SUCCESS,
LOAD_CURRENT_USER_ROLE_SUCCESS,
LOAD_GROUPS_SUMMARY_SUCCESS,
LOAD_NETWORK_USERS_SUCCESS,
LOAD_ROLES_SUMMARY_SUCCESS,
+ SET_NETWORK_USERS_LOADING,
+ SET_AVAILABLE_USERS_LOADING
} from './actions';
// ...
- case 'SET_NETWORK_USERS_LOADING':
+ case SET_NETWORK_USERS_LOADING:
return {
...state,
networkUsers: {
...state.networkUsers,
loading: action.payload
}
};
- case 'SET_AVAILABLE_USERS_LOADING':
+ case SET_AVAILABLE_USERS_LOADING:
return {
...state,
availableUsers: {
...state.availableUsers,
loading: action.payload
}
};Committable suggestion skipped: line range outside the PR's diff.
|
New netmanager changes available for preview here |
Summary of Changes (What does this PR do?)
This pull request includes significant changes to the
netmanagerproject to improve the handling of network users and available users. The changes mainly focus on migrating from local state management to Redux state management and enhancing the user interface for better usability.State Management Improvements:
netmanager/src/redux/AccessControl/operations.js: Added actions to set loading states for network users and available users during API calls. [1] [2] [3]netmanager/src/redux/AccessControl/reducers.js: Updated the reducer to handle new loading state actions and added loading properties to the initial state. [1] [2]User Interface Enhancements:
netmanager/src/views/pages/UserList/AvailableUserList.js: Refactored to use Redux state for users, total count, and loading state. Improved pagination handling and removed local state management.netmanager/src/views/pages/UserList/UserList.js: Similar refactoring to use Redux state and enhance pagination handling.Component Updates:
netmanager/src/views/pages/UserList/components/UsersTable/AvailableUsersTable.js: ReplacedCustomMaterialTablewith Material-UITablecomponents, added pagination controls, and improved the user table display. [1] [2]Miscellaneous:
These changes collectively aim to improve the performance, maintainability, and user experience of the
netmanagerproject.Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Documentation