improve IsAlreadyBanned (#416)

* improve IsAlreadyBanned

* requested changes

controllers/toolchaincluster/toolchaincluster_controller.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/codeready-toolchain/api/api/v1alpha1"
 	toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
 	"github.com/codeready-toolchain/toolchain-common/pkg/cluster"
 	"github.com/codeready-toolchain/toolchain-common/pkg/condition"
@@ -99,7 +98,7 @@ func (r *Reconciler) updateStatus(ctx context.Context, toolchainCluster *toolcha
 	return nil
 }
 
-func (r *Reconciler) getClusterHealthCondition(ctx context.Context, remoteClusterClientset *kubeclientset.Clientset) v1alpha1.Condition {
+func (r *Reconciler) getClusterHealthCondition(ctx context.Context, remoteClusterClientset *kubeclientset.Clientset) toolchainv1alpha1.Condition {
 	isHealthy, err := r.getClusterHealth(ctx, remoteClusterClientset)
 	if err != nil {
 		return clusterOfflineCondition(err.Error())

controllers/toolchaincluster/toolchaincluster_controller_test.go

@@ -20,7 +20,6 @@ import (
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
 	clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 	runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -217,7 +216,7 @@ func TestGetClusterHealth(t *testing.T) {
 		recResult, err := controller.Reconcile(context.TODO(), req)
 
 		// then
-		require.Equal(t, err, nil)
+		require.NoError(t, err)
 		require.Equal(t, reconcile.Result{RequeueAfter: requeAfter}, recResult)
 		assertClusterStatus(t, cl, "stable", clusterReadyCondition())
 	})
@@ -238,7 +237,7 @@ func TestGetClusterHealth(t *testing.T) {
 		recResult, err := controller.Reconcile(context.TODO(), req)
 
 		// then
-		require.Equal(t, err, nil)
+		require.NoError(t, err)
 		require.Equal(t, reconcile.Result{RequeueAfter: requeAfter}, recResult)
 		assertClusterStatus(t, cl, "stable", clusterNotReadyCondition())
 	})
@@ -293,7 +292,7 @@ func prepareReconcile(toolchainCluster *toolchainv1alpha1.ToolchainCluster, cl *
 	return controller, req
 }
 
-func assertClusterStatus(t *testing.T, cl client.Client, clusterName string, clusterConds ...toolchainv1alpha1.Condition) {
+func assertClusterStatus(t *testing.T, cl runtimeclient.Client, clusterName string, clusterConds ...toolchainv1alpha1.Condition) {
 	tc := &toolchainv1alpha1.ToolchainCluster{}
 	err := cl.Get(context.TODO(), test.NamespacedName("test-namespace", clusterName), tc)
 	require.NoError(t, err)

pkg/banneduser/banneduser.go

@@ -38,16 +38,20 @@ func NewBannedUser(userSignup *toolchainv1alpha1.UserSignup, bannedBy string) (*
 	return bannedUser, nil
 }
 
-// IsAlreadyBanned checks if the user was already banned
-func IsAlreadyBanned(ctx context.Context, userEmailHash string, hostClient client.Client, hostNamespace string) (bool, error) {
+// GetBannedUser returns BannedUser with the provided user email hash if found. Otherwise it returns nil.
+func GetBannedUser(ctx context.Context, userEmailHash string, hostClient client.Client, hostNamespace string) (*toolchainv1alpha1.BannedUser, error) {
 	emailHashLabelMatch := client.MatchingLabels(map[string]string{
 		toolchainv1alpha1.BannedUserEmailHashLabelKey: userEmailHash,
 	})
 	bannedUsers := &toolchainv1alpha1.BannedUserList{}
 
 	if err := hostClient.List(ctx, bannedUsers, emailHashLabelMatch, client.InNamespace(hostNamespace)); err != nil {
-		return false, err
+		return nil, err
 	}
 
-	return len(bannedUsers.Items) > 0, nil
+	if len(bannedUsers.Items) > 0 {
+		return &bannedUsers.Items[0], nil
+	}
+
+	return nil, nil
 }

pkg/banneduser/banneduser_test.go

@@ -111,7 +111,7 @@ func TestNewBannedUser(t *testing.T) {
 	}
 }
 
-func TestIsAlreadyBanned(t *testing.T) {
+func TestGetBannedUser(t *testing.T) {
 	userSignup1 := commonsignup.NewUserSignup(commonsignup.WithName("johny"), commonsignup.WithEmail("[email protected]"))
 	userSignup2 := commonsignup.NewUserSignup(commonsignup.WithName("bob"), commonsignup.WithEmail("[email protected]"))
 	userSignup3 := commonsignup.NewUserSignup(commonsignup.WithName("oliver"), commonsignup.WithEmail("[email protected]"))
@@ -129,36 +129,36 @@ func TestIsAlreadyBanned(t *testing.T) {
 	tests := []struct {
 		name       string
 		toBan      *toolchainv1alpha1.BannedUser
-		wantResult bool
+		wantResult *toolchainv1alpha1.BannedUser
 		wantError  bool
 		fakeClient *test.FakeClient
 	}{
 		{
 			name:       "user is already banned",
 			toBan:      bannedUser1,
-			wantResult: true,
+			wantResult: bannedUser1,
 			wantError:  false,
 			fakeClient: fakeClient,
 		},
 		{
 			name:       "user is not banned",
 			toBan:      bannedUser2,
-			wantResult: false,
+			wantResult: nil,
 			wantError:  false,
 			fakeClient: fakeClient,
 		},
 		{
 			name:       "cannot list banned users because the client does have type v1alpha1.BannedUserList registered in the scheme",
 			toBan:      bannedUser3,
-			wantResult: false,
+			wantResult: nil,
 			wantError:  true,
 			fakeClient: &test.FakeClient{Client: fake.NewClientBuilder().WithScheme(scheme.Scheme).Build(), T: t},
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			gotResult, err := IsAlreadyBanned(ctx, tt.toBan.Labels[toolchainv1alpha1.BannedUserEmailHashLabelKey], tt.fakeClient, test.HostOperatorNs)
+			gotResult, err := GetBannedUser(ctx, tt.toBan.Labels[toolchainv1alpha1.BannedUserEmailHashLabelKey], tt.fakeClient, test.HostOperatorNs)
 
 			if tt.wantError {
 				require.Error(t, err)