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

Fixes #24318: Existing deleted user managed by file cannot be reactivated #5435

Conversation

clarktsiory
Copy link
Contributor

@clarktsiory clarktsiory commented Mar 5, 2024

https://issues.rudder.io/issues/24318

Another condition is added to the computeUpdatedUserList : deleted users that are in active users list should be active again if they are from the same origin.

The computeUpdatedUserList is being more patched over time, so I added unit tests to it, in addition to fixing the initial problem.

@clarktsiory clarktsiory requested a review from fanf March 5, 2024 11:03
@clarktsiory clarktsiory changed the base branch from branches/rudder/7.3 to backports/7.3.12/24146 March 5, 2024 11:03
Comment on lines 236 to 244
case k if zombies.get(k).map(u => u.status == UserStatus.Deleted && u.managedBy == origin).getOrElse(false) =>
(
k,
zombies(k)
.modify(_.status)
.setTo(UserStatus.Active)
.modify(_.statusHistory)
.using(StatusHistory(UserStatus.Active, trace) :: _)
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if there is a more elegant way, instead of having zombies.get(k) then zombies(k) later...

Copy link
Member

Choose a reason for hiding this comment

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

Put the condition below and pattern match on get(K) in place of having a guard. Iirc it's similar in perf and alloc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the case k if ... => is passed to a collect and should not be a function but a partial function.
There is always the option to change to a function and flatMap on Option but I don't know if that way it is worth the change...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just realized that maybe you were exactly referring to that 😅 : bf74ff1

@clarktsiory clarktsiory marked this pull request as draft March 5, 2024 11:12
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

1 similar comment
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

@clarktsiory clarktsiory marked this pull request as ready for review March 5, 2024 14:45
@fanf
Copy link
Member

fanf commented Mar 8, 2024

I will need to be rebased after retarget to branche 7.3

@fanf fanf changed the base branch from backports/7.3.12/24146 to branches/rudder/7.3 March 8, 2024 11:20
@clarktsiory clarktsiory force-pushed the bug_24318/existing_deleted_user_managed_by_file_cannot_be_reactivated branch from 4be737d to dec8ae9 Compare March 8, 2024 13:10
@clarktsiory
Copy link
Contributor Author

clarktsiory commented Mar 8, 2024

PR rebased and updated with a new commit

Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

Works as expected !

@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/5435
-- Your faithful QA
Kant merge: "All our knowledge begins with the senses, proceeds then to the understanding, and ends with reason. There is nothing higher than reason."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/81380/console)

@clarktsiory
Copy link
Contributor Author

OK, squash merging this PR

@clarktsiory clarktsiory force-pushed the bug_24318/existing_deleted_user_managed_by_file_cannot_be_reactivated branch from bf74ff1 to 6ff585c Compare March 12, 2024 14:37
@clarktsiory clarktsiory merged commit 6ff585c into Normation:branches/rudder/7.3 Mar 12, 2024
1 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants