-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
fix User Counts updates on user add/remove #31484
Conversation
Signed-off-by: Petre T <petre.tudor@dorkfarm.com>
/compile amend / |
@petre2dor could you enable us to change this pr? :) |
Hi @skjnldsv, |
/compile amend / |
👆 compile attempt 2 |
I guess it would probably make sense to push this to a branch in this repo and invite Petre to the org for easier collaboration... |
@skjnldsv would you be able to do so? :) |
Hi everyone, Thanks :) |
@petre2dor thanks for the ping! I would be able to push your changes to a branch in this repo but I fear I am not able to invite you to the org which would mean that you would not be able to push to the new branch... Apart from that, you can also fix the |
user.groups.forEach(group => { | ||
// Increment disabled count | ||
state.groups.find(groupSearch => groupSearch.id === group).disabled += enabled ? -1 : 1 | ||
// this.commit('updateUserCounts', { user, actionType: enabled ? 'enable' : 'disable' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer the enable
/disable
action type so the update
one is not wrongly used in the future.
updateUserCounts(state, { user, actionType }) { | ||
const disabledGroup = state.groups.find(group => group.id === 'disabled') | ||
switch (actionType) { | ||
case 'update': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To not duplicate the code you can handle both enable
and disable
like so:
case 'enable':
case 'disable':
...
and keeping the current logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invited @petre2dor |
@petre2dor did you accept? then I could push your changes to a new branch... |
Yes, I did today. Thanks! |
Shall I push your changes to the new branch in the meantime or do you want to make your new changes first and then push it to the new branch afterwards? |
Please push my changes to a new branch first. |
Superseded by #31697 |
done with #31697 |
Fix for #27607
I refactored the existing code that update the counts on user enable/disable and added code to update the counts on user add/remove
Signed-off-by: Petre T petre.tudor@dorkfarm.com