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

fix User Counts updates on user add/remove #31484

Closed
wants to merge 1 commit into from

Conversation

petre2dor
Copy link

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 [email protected]

@szaimen szaimen added this to the Nextcloud 24 milestone Mar 7, 2022
@szaimen szaimen requested review from blizzz, skjnldsv, a team, artonge and Pytal and removed request for a team March 7, 2022 23:22
@szaimen
Copy link
Contributor

szaimen commented Mar 7, 2022

/compile amend /

@skjnldsv
Copy link
Member

skjnldsv commented Mar 8, 2022

@petre2dor could you enable us to change this pr? :)

image

@petre2dor
Copy link
Author

Hi @skjnldsv,
That checkbox is already checked.
image

@Pytal
Copy link
Member

Pytal commented Mar 9, 2022

/compile amend /

@Pytal
Copy link
Member

Pytal commented Mar 9, 2022

👆 compile attempt 2

@szaimen
Copy link
Contributor

szaimen commented Mar 9, 2022

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

@szaimen
Copy link
Contributor

szaimen commented Mar 9, 2022

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? :)

@petre2dor
Copy link
Author

Hi everyone,
Just a friendly reminder about this PR in the hope we can move this along.

Thanks :)

@szaimen
Copy link
Contributor

szaimen commented Mar 22, 2022

@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 Node workflow by compiling the js locally and pushing it to the branch.

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' })
Copy link
Contributor

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':
Copy link
Contributor

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the review @artonge. I made the chenges and pushed here #31697
please review it again

@skjnldsv
Copy link
Member

@skjnldsv would you be able to do so? :)

invited @petre2dor

@szaimen
Copy link
Contributor

szaimen commented Mar 24, 2022

invited @petre2dor

@petre2dor did you accept? then I could push your changes to a new branch...

@petre2dor
Copy link
Author

invited @petre2dor

@petre2dor did you accept? then I could push your changes to a new branch...

Yes, I did today. Thanks!
I'll make my changes today or tomorrow.

@szaimen
Copy link
Contributor

szaimen commented Mar 24, 2022

Yes, I did today. Thanks!
I'll make my changes today or tomorrow.

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?

@petre2dor
Copy link
Author

Yes, I did today. Thanks!
I'll make my changes today or tomorrow.

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.
I destroyed my dev machine and I need to rebuild it. No need to use my fork and switch later to Nexcloud repo.

@szaimen
Copy link
Contributor

szaimen commented Mar 24, 2022

Superseded by #31697

@szaimen szaimen closed this Mar 24, 2022
@szaimen szaimen removed this from the Nextcloud 24 milestone Mar 24, 2022
@szaimen
Copy link
Contributor

szaimen commented Mar 24, 2022

Please push my changes to a new branch first.
I destroyed my dev machine and I need to rebuild it. No need to use my fork and switch later to Nexcloud repo.

done with #31697

@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants