Make sure the status message is cleared when updating users #409
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #409 +/- ##
========================================
- Coverage 5.3% 5.16% -0.14%
========================================
Files 322 323 +1
Lines 15456 16003 +547
Branches 4655 4955 +300
========================================
+ Hits 820 827 +7
- Misses 12495 12876 +381
- Partials 2141 2300 +159
Continue to review full report at Codecov.
|
landonreed
left a comment
landonreed
left a comment
There was a problem hiding this comment.
Thanks for identifying this fix. One comment that I would like to address.
lib/manager/actions/user.js
Outdated
| // to be dispatched because the status message from the | ||
| // updatingUserData is still set to something, so we need to indicate | ||
| // that the request completed. | ||
| dispatch(clearStatusMessage()) |
There was a problem hiding this comment.
I wonder if rather than clearing the status message, we might want to replace the user profile that has been updated with the one contained in the store or alternatively, just fetch the user list again. Either way I think updating the user with its up-to-date info so that the user can verify the update matches their desired change is important.
|
I refactored this so that an error message is shown when the update fails and that the user list is refreshed upon success. |
landonreed
left a comment
landonreed
left a comment
There was a problem hiding this comment.
@evansiroky, I'm a bit confused about the new code. Could you explain why the logic to check if the updated user matches the current user was removed? Also, why does the error message say "Could not find user metadata"? That doesn't seem appropriate.
|
The new code changes the behavior after a successful update so that the user list is refreshed each time. Therefore, the check to update is no longer needed, because the most recent data gets refetched. That error message was copy-pasta-no-updata. Pushed a fix for that. |
|
Just added back logic to update current user permissions after a self-update is performed. |
lib/manager/actions/user.js
Outdated
| // may affect what they're allowed to do in the application. | ||
| dispatch(userProfileUpdated(responseJson)) | ||
| } | ||
| dispatch(fetchUsers()) |
There was a problem hiding this comment.
I don't think fetchUsers should always be called. This function is called in other places besides the user admin module (e.g., updateTargetForSubscription). So when a user updates their own subscriptions, we really don't want to be fetching all users. This was the original purpose of the code that updates the logged in user that was removed in a previous commit (and was added back here). Given how much confusion has surrounded this relatively minor code change, I think you should update this method's doc to read:
Update application/client ID specific datatools object with provided user data. This is used in the user admin module to update some user's permissions as well as throughout the application to update attributes for the logged in user (e.g., when a user subscribes/watches a feed source or project).
There was a problem hiding this comment.
Oh wow, yeah, I didn't realize how widely this action was used and how often times it's for updating subscriptions and other non-permissions stuff.
- add more comments - only fetch users after update if the user being updated was not the current user
|
I added more comments and refactored as suggest. |
|
🎉 This PR is included in version 4.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
fixes #408