allow for approved status false after user sign up deactivated #351

mmulholla · 2023-10-05T20:07:46Z

Fix required for: codeready-toolchain/host-operator#868

it should not cause a regression because this condition:
approvedCondition.Reason != toolchainv1alpha1.UserSignupUserDeactivatedReason
can only be true without codeready-toolchain/host-operator#868

This is the issue it fixes as a result of codeready-toolchain/host-operator#868

=== NAME  TestSignupOK/test_activation-deactivation_workflow
    host.go:711: waiting for UserSignup 'testuser-e808fd7d-bf83-4cdc-abe8-1b72c80c1a9e' in namespace 'toolchain-host-05094958' to match criteria
    host.go:1982: waiting until Space 'testuser-e808fd7d-bf' in namespace 'toolchain-host-05094958' is deleted
    host.go:2028: waiting until SpaceBindings with labels 'map[toolchain.dev.openshift.com/space:testuser-e808fd7d-bf]' in namespace 'toolchain-host-05094958' are deleted
    regsvc.go:16: invoking http request: GET https://registration-service-toolchain-host-05094958.apps.martin-4.12-092615.qe.openshiftappsvc.org/api/v1/signup
    regsvc.go:26: response status code: 200
    regsvc.go:33: 
           Error Trace:   /Users/martinmulholland/codeready-toolchain/toolchain-e2e/testsupport/regsvc.go:33
                                   /Users/martinmulholland/codeready-toolchain/toolchain-e2e/test/e2e/parallel/registration_service_test.go:805
                                   /Users/martinmulholland/codeready-toolchain/toolchain-e2e/test/e2e/parallel/registration_service_test.go:381
           Error:         Not equal: 
                          expected: 404
                          actual  : 200
           Test:          TestSignupOK/test_activation-deactivation_workflow
           Messages:      unexpected response status with body: {"name":"testuser-e808fd7d-bf83-4cdc-abe8-1b72c80c1a9e","compliantUsername":"testuser-e808fd7d-bf","username":"testuser-e808fd7d-bf83-4cdc-abe8-1b72c80c1a9e","givenName":"","familyName":"","company":"","status":{"ready":false,"reason":"PendingApproval","verificationRequired":false}}

openshift-ci · 2023-10-05T20:07:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mmulholla
Once this PR has been reviewed and has the lgtm label, please assign alexeykazakov for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2023-10-05T20:07:58Z

Hi @mmulholla. Thanks for your PR.

I'm waiting for a codeready-toolchain member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

alexeykazakov · 2023-10-05T21:04:41Z

/ok-to-test

alexeykazakov

I think the unit tests should be updated to.

alexeykazakov · 2023-10-05T21:24:02Z

pkg/signup/service/signup_service.go

@@ -438,7 +438,8 @@ func (s *ServiceImpl) DoGetSignup(ctx *gin.Context, provider ResourceProvider, u
 	// Check UserSignup status to determine whether user signup is complete
 	approvedCondition, approvedFound := condition.FindConditionByType(userSignup.Status.Conditions, toolchainv1alpha1.UserSignupApproved)
 	completeCondition, completeFound := condition.FindConditionByType(userSignup.Status.Conditions, toolchainv1alpha1.UserSignupComplete)
-	if !approvedFound || !completeFound || approvedCondition.Status != apiv1.ConditionTrue {
+	if !approvedFound || !completeFound ||
+		( approvedCondition.Status != apiv1.ConditionTrue && approvedCondition.Reason != toolchainv1alpha1.UserSignupUserDeactivatedReason) {


I think we should replace it by:

Suggested change

( approvedCondition.Status != apiv1.ConditionTrue && approvedCondition.Reason != toolchainv1alpha1.UserSignupUserDeactivatedReason) {

condition.IsFalseWithReason(userSignup.Status.Conditions, toolchainv1alpha1.UserSignupApproved, toolchainv1alpha1.UserSignupPendingApprovalReason {

This is the actual condition we set for UserSignup when it's pending approval:

- reason: PendingApproval status: "False" type: Approved

I can do that if you prefer but my concern would be that is is more strict than the current code which could lead to other behavior changes.

Yeah, I still think it's better to rely on the PendingApproval reason.

MatousJobanek · 2023-10-09T14:30:11Z

pkg/signup/service/signup_service.go

 	approvedCondition, approvedFound := condition.FindConditionByType(userSignup.Status.Conditions, toolchainv1alpha1.UserSignupApproved)
 	completeCondition, completeFound := condition.FindConditionByType(userSignup.Status.Conditions, toolchainv1alpha1.UserSignupComplete)
-	if !approvedFound || !completeFound || approvedCondition.Status != apiv1.ConditionTrue {
+	if !approvedFound || !completeFound ||
+		(approvedCondition.Status != apiv1.ConditionTrue && approvedCondition.Reason != toolchainv1alpha1.UserSignupUserDeactivatedReason) {


EDIT: I misread the conditions - all good ;-)

this would mean that if the approved is found, but the status is not true (is pending approval), then the if statement would be evaluated as "false", which would mean (in this context) as approved, which is wrong, right?

~~Please try to play with the condition. package - you may found some useful functions there that could simplify it.~~

I'm idiot 🤦‍♂️
It's Monday and I cannot read the if statement correctly. Your version should work fine.
Could you please add/update some unit tests?

Have updated the tests.

Also changed the if statement per Alexey's comment .

openshift-ci · 2023-10-09T19:04:46Z

@mmulholla: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e	`f2b1b0a`	link	true	`/test e2e`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

sonarqubecloud · 2023-10-09T19:21:50Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

alexeykazakov · 2023-10-09T19:31:15Z

pkg/signup/service/signup_service_test.go

@@ -1093,7 +1093,8 @@ func (s *TestSignupServiceSuite) TestGetSignupDeactivated() {
 	// given
 	s.ServiceConfiguration(configuration.Namespace(), true, "", 5)

-	us := s.newUserSignupCompleteWithReason("Deactivated")
+	us := s.newUserSignupComplete()
+	us.Status.Conditions = deactivated()


Maybe I'm missing something but this test doesn't seem to check the signup payload status in lines 1132-1133. So it doesn't really test the changed signup service. Could you please add it to the test? Check the status.

mmulholla · 2023-10-09T19:52:54Z

Had to rename the branch because it was picking up the toolchain-e2e branch in my fork which was causing failures but are needed for host-operator changes. New PR is: #354

allow for approved status false after user sign up deactivated

bcb040f

openshift-ci bot requested review from alexeykazakov and MatousJobanek October 5, 2023 20:07

openshift-ci bot added the needs-ok-to-test label Oct 5, 2023

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Oct 5, 2023

alexeykazakov reviewed Oct 5, 2023

View reviewed changes

after make format

caef9d9

MatousJobanek reviewed Oct 9, 2023

View reviewed changes

Code review comments and test update

f2b1b0a

Merge branch 'master' into RemoveUserSignUpApprovalAfterDeactivation

3110294

alexeykazakov reviewed Oct 9, 2023

View reviewed changes

mmulholla mentioned this pull request Oct 9, 2023

Fix pending approval error #354

Merged

mmulholla closed this Oct 9, 2023

mmulholla deleted the RemoveUserSignUpApprovalAfterDeactivation branch October 11, 2023 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow for approved status false after user sign up deactivated #351

allow for approved status false after user sign up deactivated #351

mmulholla commented Oct 5, 2023 •

edited

Loading

openshift-ci bot commented Oct 5, 2023

openshift-ci bot commented Oct 5, 2023

alexeykazakov commented Oct 5, 2023

alexeykazakov left a comment

alexeykazakov Oct 5, 2023

mmulholla Oct 5, 2023

alexeykazakov Oct 6, 2023

mmulholla Oct 9, 2023

MatousJobanek Oct 9, 2023 •

edited

Loading

MatousJobanek Oct 9, 2023

mmulholla Oct 9, 2023

mmulholla Oct 9, 2023

openshift-ci bot commented Oct 9, 2023

sonarqubecloud bot commented Oct 9, 2023

alexeykazakov Oct 9, 2023

mmulholla commented Oct 9, 2023

	( approvedCondition.Status != apiv1.ConditionTrue && approvedCondition.Reason != toolchainv1alpha1.UserSignupUserDeactivatedReason) {
	condition.IsFalseWithReason(userSignup.Status.Conditions, toolchainv1alpha1.UserSignupApproved, toolchainv1alpha1.UserSignupPendingApprovalReason {

allow for approved status false after user sign up deactivated #351

allow for approved status false after user sign up deactivated #351

Conversation

mmulholla commented Oct 5, 2023 • edited Loading

openshift-ci bot commented Oct 5, 2023

openshift-ci bot commented Oct 5, 2023

alexeykazakov commented Oct 5, 2023

alexeykazakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatousJobanek Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 9, 2023

sonarqubecloud bot commented Oct 9, 2023

Choose a reason for hiding this comment

mmulholla commented Oct 9, 2023

mmulholla commented Oct 5, 2023 •

edited

Loading

MatousJobanek Oct 9, 2023 •

edited

Loading