Modify DeactivationService to set scheduled deactivation time for active UserSignups #988
Conversation
sbryzak
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19 and
mfrancisc
as code owners
sbryzak
changed the title
|
/retest |
…ctivating state but no notification created
| @@ -48,6 +50,7 @@ type Reconciler struct { | |||
| // Note: | |||
| // The Controller will requeue the Request to be processed again if the returned error is non-nil or | |||
| // Result.Requeue is true, otherwise upon completion it will remove the work from the queue. | |||
| // nolint: gocyclo | |||
There was a problem hiding this comment.
the linter believes that the reconcile function is too complex, so we disabled that check
There was a problem hiding this comment.
Have you tried to re-enable it now? The code is less complex now then the initial one.
There was a problem hiding this comment.
Yes, the linter still doesn't like it.
There was a problem hiding this comment.
have you considered refactoring the code and splitting the method into multiple ones? TBH, the linter is correct that the method is really complex and should be simplified/refactored.
| @@ -128,6 +142,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. | |||
| if err := r.resetDeactivatingState(ctx, usersignup); err != nil { | |||
| return reconcile.Result{}, err | |||
| } | |||
| // Set the scheduled deactivation time to nil | |||
| if err := statusUpdater.SetScheduledDeactivationStatus(ctx, usersignup, nil); err != nil { | |||
There was a problem hiding this comment.
what is the expected UX for:
- user won't be deactivated (is either excluded or is assigned to non-deactivating tier)
- the value cannot be not calculated for the account for some reasons
Do you want to show the same message/value for these two cases?
There was a problem hiding this comment.
I guess since we can't actually differentiate between these two cases currently then a generic message or indicator would be most appropriate, perhaps for the "days remaining" box/field in the dashboard it can just display a - symbol or something.
There was a problem hiding this comment.
Yeah. Or N/A. Which can mean both Not Available and Not Applicable :)
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, sbryzak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #988 +/- ##
==========================================
+ Coverage 84.68% 84.72% +0.03%
==========================================
Files 55 55
Lines 4877 4909 +32
==========================================
+ Hits 4130 4159 +29
- Misses 574 577 +3
Partials 173 173
|
|
Merging now as I am on PTO from tomorrow. Codecov is lying about many of the lines not being covered, I added additional tests and confirmed with the debugger that it is indeed hitting those lines it's reporting as uncovered in the tests. |
|
Just to add - if there is something else that anyone thinks needs to be addressed with this feature please mention it on our slack channel and we can address in a followup PR. |
This PR updates the deactivation controller so that it will manage the
UserSignup.Status.ScheduledDeactivationTimestampproperty, setting it appropriately as the controller is reconciled, also when the user tier changes, and when the usersignup is deactivated.Related PR: codeready-toolchain/registration-service#411