nomad-nmr / nomad-server

Server side code for NOMAD system
https://www.nomad-nmr.uk/
GNU Affero General Public License v3.0
3 stars 2 forks source link

Feat. Add Option to delete multiple users #114

Closed hunxjunedo closed 5 months ago

hunxjunedo commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

@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.

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 commented 5 months ago

@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 commented 5 months ago

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