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

Feat. Add Option to delete multiple users #114

Merged
merged 6 commits into from
Jun 18, 2024
Merged

Conversation

hunxjunedo
Copy link
Contributor

Fixes #92 , tested and works fine. If the user has any experiments, then inactivates him, otherwise deleted. Also provides the summary once action has been performed.

Screenshot from 2024-06-12 20-19-08
Screenshot from 2024-06-12 20-19-13
Screenshot from 2024-06-12 20-19-19

@tomlebl
Copy link
Contributor

tomlebl commented Jun 13, 2024

@hunxjunedo Looks good. I will pull it and have a look myself at some point soon. I would put the delete button in to the nav bar next to add button to keep it uniform with the rest of UI but that's something easy for me to fix.

@tomlebl tomlebl merged commit 2d63575 into nomad-nmr:main Jun 18, 2024
2 checks passed
@tomlebl
Copy link
Contributor

tomlebl commented Jun 19, 2024

@hunxjunedo
I have eventually merged your PR and had a proper look. It was working but I had to refactor it quite a bit. You can check my two following commits to see my changes. Here is some feedback from code review.

  • I have moved the action button into PageHeader.jsx component where I usually put these action buttons. It's bit trickier because you can use local state for checked rows state but at the end it's neater UI
  • Confirmation can be easily done by wrapping a button in higher order component Popconfirm available in AntD library.
  • Upon catching error in Redux action you can dispatch errorHandler function that deals with all possible errors.
  • The biggest problem was related to how the response was handled. Fetching the data for table after performing delete might work on as simple table or if you have only few users in DB. If you have thousands of users and some filters, pagination etc you get quite bad user experience. It's better to return arrays of ids/keys and then alter table data accordingly in reducer.
  • It's better use .findOne rather .find if you check whether user has an experiment. One is enough to make decision. Looking for all can take much longer and it's unnecessary.

At the end refactoring was faster than writing from the scratch. So, I appreciate your help and I hope that my feedback will help you to understand NOMAD's code base.

If you fancy another task let me know.

@hunxjunedo
Copy link
Contributor Author

@hunxjunedo
I have eventually merged your PR and had a proper look. It was working but I had to refactor it quite a bit. You can check my two following commits to see my changes. Here is some feedback from code review.

  • I have moved the action button into PageHeader.jsx component where I usually put these action buttons. It's bit trickier because you can use local state for checked rows state but at the end it's neater UI
  • Confirmation can be easily done by wrapping a button in higher order component Popconfirm available in AntD library.
  • Upon catching error in Redux action you can dispatch errorHandler function that deals with all possible errors.
  • The biggest problem was related to how the response was handled. Fetching the data for table after performing delete might work on as simple table or if you have only few users in DB. If you have thousands of users and some filters, pagination etc you get quite bad user experience. It's better to return arrays of ids/keys and then alter table data accordingly in reducer.
  • It's better use .findOne rather .find if you check whether user has an experiment. One is enough to make decision. Looking for all can take much longer and it's unnecessary.

At the end refactoring was faster than writing from the scratch. So, I appreciate your help and I hope that my feedback will help you to understand NOMAD's code base.

If you fancy another task let me know.

Acknowledged, hope that has a positive impact on my future contributions to this repository. Would love to contribute in the future.

@tomlebl
Copy link
Contributor

tomlebl commented Jun 19, 2024

@hunxjunedo
OK, if I will come across something that would be good for you than I will let you know and assign.

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.

Delete users
2 participants