diff --git a/pkg/application/service/services.go b/pkg/application/service/services.go index 2b8c2214..5322143a 100644 --- a/pkg/application/service/services.go +++ b/pkg/application/service/services.go @@ -9,10 +9,8 @@ import ( type SignupService interface { Signup(ctx *gin.Context) (*toolchainv1alpha1.UserSignup, error) - GetSignup(ctx *gin.Context, userID, username string) (*signup.Signup, error) - GetSignupFromInformer(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) + GetSignup(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) GetUserSignupFromIdentifier(userID, username string) (*toolchainv1alpha1.UserSignup, error) - UpdateUserSignup(userSignup *toolchainv1alpha1.UserSignup) (*toolchainv1alpha1.UserSignup, error) PhoneNumberAlreadyInUse(userID, username, phoneNumberOrHash string) error } diff --git a/pkg/controller/signup.go b/pkg/controller/signup.go index f9bacd80..66f4914b 100644 --- a/pkg/controller/signup.go +++ b/pkg/controller/signup.go @@ -111,7 +111,7 @@ func (s *Signup) GetHandler(ctx *gin.Context) { // Get the UserSignup resource from the service by the userID userID := ctx.GetString(context.SubKey) username := ctx.GetString(context.UsernameKey) - signupResource, err := s.app.SignupService().GetSignup(ctx, userID, username) + signupResource, err := s.app.SignupService().GetSignup(ctx, userID, username, true) if err != nil { log.Error(ctx, err, "error getting UserSignup resource") crterrors.AbortWithError(ctx, http.StatusInternalServerError, err, "error getting UserSignup resource") diff --git a/pkg/controller/signup_test.go b/pkg/controller/signup_test.go index de5de249..6731037a 100644 --- a/pkg/controller/signup_test.go +++ b/pkg/controller/signup_test.go @@ -207,7 +207,7 @@ func (s *TestSignupSuite) TestSignupGetHandler() { Reason: "Provisioning", }, } - svc.MockGetSignup = func(_ *gin.Context, id, _ string) (*signup.Signup, error) { + svc.MockGetSignup = func(_ *gin.Context, id, _ string, _ bool) (*signup.Signup, error) { if id == userID { return expected, nil } @@ -234,7 +234,7 @@ func (s *TestSignupSuite) TestSignupGetHandler() { ctx.Request = req ctx.Set(context.SubKey, userID) - svc.MockGetSignup = func(_ *gin.Context, _, _ string) (*signup.Signup, error) { + svc.MockGetSignup = func(_ *gin.Context, _, _ string, _ bool) (*signup.Signup, error) { return nil, nil } @@ -251,7 +251,7 @@ func (s *TestSignupSuite) TestSignupGetHandler() { ctx.Request = req ctx.Set(context.SubKey, userID) - svc.MockGetSignup = func(_ *gin.Context, _, _ string) (*signup.Signup, error) { + svc.MockGetSignup = func(_ *gin.Context, _, _ string, _ bool) (*signup.Signup, error) { return nil, errors.New("oopsie woopsie") } @@ -439,9 +439,6 @@ func (s *TestSignupSuite) TestInitVerificationHandler() { states.SetVerificationRequired(&us, true) return &us, nil }, - MockUpdateUserSignup: func(userSignup *crtapi.UserSignup) (userSignup2 *crtapi.UserSignup, e error) { - return userSignup, nil - }, MockPhoneNumberAlreadyInUse: func(_, _, _ string) error { return nil }, @@ -863,20 +860,14 @@ func initActivationCodeVerification(t *testing.T, handler gin.HandlerFunc, usern } type FakeSignupService struct { - MockGetSignup func(ctx *gin.Context, userID, username string) (*signup.Signup, error) - MockGetSignupFromInformer func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) + MockGetSignup func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) MockSignup func(ctx *gin.Context) (*crtapi.UserSignup, error) MockGetUserSignupFromIdentifier func(userID, username string) (*crtapi.UserSignup, error) - MockUpdateUserSignup func(userSignup *crtapi.UserSignup) (*crtapi.UserSignup, error) MockPhoneNumberAlreadyInUse func(userID, username, value string) error } -func (m *FakeSignupService) GetSignup(ctx *gin.Context, userID, username string) (*signup.Signup, error) { - return m.MockGetSignup(ctx, userID, username) -} - -func (m *FakeSignupService) GetSignupFromInformer(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) { - return m.MockGetSignupFromInformer(ctx, userID, username, checkUserSignupComplete) +func (m *FakeSignupService) GetSignup(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) { + return m.MockGetSignup(ctx, userID, username, checkUserSignupComplete) } func (m *FakeSignupService) Signup(ctx *gin.Context) (*crtapi.UserSignup, error) { @@ -887,10 +878,6 @@ func (m *FakeSignupService) GetUserSignupFromIdentifier(userID, username string) return m.MockGetUserSignupFromIdentifier(userID, username) } -func (m *FakeSignupService) UpdateUserSignup(userSignup *crtapi.UserSignup) (*crtapi.UserSignup, error) { - return m.MockUpdateUserSignup(userSignup) -} - func (m *FakeSignupService) PhoneNumberAlreadyInUse(userID, username, e164phoneNumber string) error { return m.MockPhoneNumberAlreadyInUse(userID, username, e164phoneNumber) } diff --git a/pkg/proxy/handlers/spacelister.go b/pkg/proxy/handlers/spacelister.go index 51779c37..fb1c8b15 100644 --- a/pkg/proxy/handlers/spacelister.go +++ b/pkg/proxy/handlers/spacelister.go @@ -35,7 +35,7 @@ type SpaceLister struct { func NewSpaceLister(client namespaced.Client, app application.Application, proxyMetrics *metrics.ProxyMetrics) *SpaceLister { return &SpaceLister{ Client: client, - GetSignupFunc: app.SignupService().GetSignupFromInformer, + GetSignupFunc: app.SignupService().GetSignup, ProxyMetrics: proxyMetrics, } } diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index 2643f111..56bece8c 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -548,7 +548,7 @@ func testSpaceListerGet(t *testing.T, publicViewerEnabled bool) { tc.mockFakeClient(fakeClient) } - signupProvider := fakeSignupService.GetSignupFromInformer + signupProvider := fakeSignupService.GetSignup if tc.overrideSignupFunc != nil { signupProvider = tc.overrideSignupFunc } @@ -724,7 +724,7 @@ func TestGetUserWorkspace(t *testing.T) { tc.mockFakeClient(fakeClient) } - signupProvider := fakeSignupService.GetSignupFromInformer + signupProvider := fakeSignupService.GetSignup if tc.overrideSignupFunc != nil { signupProvider = tc.overrideSignupFunc } @@ -834,7 +834,7 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { t.Run(k, func(t *testing.T) { // given - signupProvider := fakeSignupService.GetSignupFromInformer + signupProvider := fakeSignupService.GetSignup proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) s := &handlers.SpaceLister{ @@ -969,7 +969,7 @@ func TestGetUserWorkspaceWithBindingsWithPublicViewerEnabled(t *testing.T) { t.Run(k, func(t *testing.T) { // given - signupProvider := fakeSignupService.GetSignupFromInformer + signupProvider := fakeSignupService.GetSignup proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) s := &handlers.SpaceLister{ diff --git a/pkg/proxy/handlers/spacelister_list_test.go b/pkg/proxy/handlers/spacelister_list_test.go index 3aa9c73d..4b9d3933 100644 --- a/pkg/proxy/handlers/spacelister_list_test.go +++ b/pkg/proxy/handlers/spacelister_list_test.go @@ -71,7 +71,7 @@ func TestListUserWorkspaces(t *testing.T) { t.Run(k, func(t *testing.T) { // given - signupProvider := fakeSignupService.GetSignupFromInformer + signupProvider := fakeSignupService.GetSignup proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) @@ -196,7 +196,7 @@ func TestHandleSpaceListRequest(t *testing.T) { tc.mockFakeClient(fakeClient) } - signupProvider := fakeSignupService.GetSignupFromInformer + signupProvider := fakeSignupService.GetSignup if tc.overrideSignupFunc != nil { signupProvider = tc.overrideSignupFunc } diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index b76ed983..dd9930a5 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -378,7 +378,7 @@ func (p *Proxy) checkUserIsProvisioned(ctx echo.Context, userID, username string // // UserSignup complete status is not checked, since it might cause the proxy blocking the request // and returning an error when quick transitions from ready to provisioning are happening. - userSignup, err := p.app.SignupService().GetSignupFromInformer(nil, userID, username, false) + userSignup, err := p.app.SignupService().GetSignup(nil, userID, username, false) if err != nil { return err } @@ -414,7 +414,7 @@ func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, proxyPlugin // this function returns an error. func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, username, proxyPluginName string, workspace *toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { // retrieve the requesting user's UserSignup - userSignup, err := p.app.SignupService().GetSignupFromInformer(nil, userID, username, false) + userSignup, err := p.app.SignupService().GetSignup(nil, userID, username, false) if err != nil { log.Error(nil, err, fmt.Sprintf("error retrieving user signup for userID '%s' and username '%s'", userID, username)) return nil, crterrors.NewInternalError(errs.New("unable to get user info"), "error retrieving user") diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 67134e98..c2d5513f 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -153,7 +153,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr // configure proxy p.spaceLister = &handlers.SpaceLister{ Client: p.Client, - GetSignupFunc: fakeApp.SignupServiceMock.GetSignupFromInformer, + GetSignupFunc: fakeApp.SignupServiceMock.GetSignup, ProxyMetrics: p.metrics, } diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index 5e8fd5e2..d638a274 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -852,7 +852,7 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { p.spaceLister = &handlers.SpaceLister{ Client: p.Client, - GetSignupFunc: fakeApp.SignupServiceMock.GetSignupFromInformer, + GetSignupFunc: fakeApp.SignupServiceMock.GetSignup, ProxyMetrics: p.metrics, } if tc.OverrideGetSignupFunc != nil { diff --git a/pkg/proxy/service/cluster_service.go b/pkg/proxy/service/cluster_service.go index 4785458a..5f57adbc 100644 --- a/pkg/proxy/service/cluster_service.go +++ b/pkg/proxy/service/cluster_service.go @@ -100,7 +100,7 @@ func (s *ServiceImpl) getClusterAccessForDefaultWorkspace(userID, username, prox func (s *ServiceImpl) getSignupFromInformerForProvisionedUser(userID, username string) (*signup.Signup, error) { // don't check for usersignup complete status, since it might cause the proxy blocking the request // and returning an error when quick transitions from ready to provisioning are happening. - userSignup, err := s.SignupService.GetSignupFromInformer(nil, userID, username, false) + userSignup, err := s.SignupService.GetSignup(nil, userID, username, false) if err != nil { return nil, err } diff --git a/pkg/signup/service/signup_service.go b/pkg/signup/service/signup_service.go index e4a03564..61c5de1f 100644 --- a/pkg/signup/service/signup_service.go +++ b/pkg/signup/service/signup_service.go @@ -350,16 +350,10 @@ func (s *ServiceImpl) reactivateUserSignup(ctx *gin.Context, existing *toolchain // GetSignup returns Signup resource which represents the corresponding K8s UserSignup // and MasterUserRecord resources in the host cluster. -// Returns nil, nil if the UserSignup resource is not found or if it's deactivated. -func (s *ServiceImpl) GetSignup(ctx *gin.Context, userID, username string) (*signup.Signup, error) { - return s.DoGetSignup(ctx, s.Client, userID, username, true) -} - -// GetSignupFromInformer uses the same logic of the 'GetSignup' function, except it uses informers to get resources. -// This function and the ResourceProvider abstraction can replace the original GetSignup function once it is determined to be stable. // The checkUserSignupCompleted was introduced in order to avoid checking the readiness of the complete condition on the UserSignup in certain situations, // such as proxy calls for example. -func (s *ServiceImpl) GetSignupFromInformer(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) { +// Returns nil, nil if the UserSignup resource is not found or if it's deactivated. +func (s *ServiceImpl) GetSignup(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) { return s.DoGetSignup(ctx, s.Client, userID, username, checkUserSignupCompleted) } @@ -388,10 +382,8 @@ func (s *ServiceImpl) DoGetSignup(ctx *gin.Context, cl namespaced.Client, userID // If there is no need to update the UserSignup then break out of the loop here (by returning nil) // otherwise update the UserSignup if updated { - var updateErr error - userSignup, updateErr = s.UpdateUserSignup(userSignup) - if updateErr != nil { - return updateErr + if err := s.Update(gocontext.TODO(), userSignup); err != nil { + return err } } @@ -582,15 +574,6 @@ func (s *ServiceImpl) DoGetUserSignupFromIdentifier(cl namespaced.Client, userID return userSignup, nil } -// UpdateUserSignup is used to update the provided UserSignup resource, and returning the updated resource -func (s *ServiceImpl) UpdateUserSignup(userSignup *toolchainv1alpha1.UserSignup) (*toolchainv1alpha1.UserSignup, error) { - if err := s.Update(gocontext.TODO(), userSignup); err != nil { - return nil, err - } - - return userSignup, nil -} - var ( md5Matcher = regexp.MustCompile("(?i)[a-f0-9]{32}$") ) diff --git a/pkg/signup/service/signup_service_test.go b/pkg/signup/service/signup_service_test.go index 97d30f14..ab79cc1b 100644 --- a/pkg/signup/service/signup_service_test.go +++ b/pkg/signup/service/signup_service_test.go @@ -244,28 +244,10 @@ func (s *TestSignupServiceSuite) TestGetSignupFailsWithNotFoundThenOtherError() c, _ := gin.CreateTestContext(httptest.NewRecorder()) // when - _, err := application.SignupService().GetSignup(c, "000", "abc") + _, err := application.SignupService().GetSignup(c, "000", "abc", true) // then require.EqualError(s.T(), err, "something quite unfortunate happened: something bad") - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t) - fakeClient.MockGet = func(ctx gocontext.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { - if _, ok := obj.(*toolchainv1alpha1.UserSignup); ok && key.Name != "000" { - return errors2.NewInternalError(errors.New("something quite unfortunate happened"), "something bad") - } - return fakeClient.Client.Get(ctx, key, obj, opts...) - } - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - _, err = svc.GetSignupFromInformer(c, "000", "abc", true) - - // then - require.EqualError(t, err, "something quite unfortunate happened: something bad") - }) } func (s *TestSignupServiceSuite) TestSignupNoSpaces() { @@ -705,29 +687,10 @@ func (s *TestSignupServiceSuite) TestGetUserSignupFails() { } // when - _, err := application.SignupService().GetSignup(c, "", username) + _, err := application.SignupService().GetSignup(c, "", username, true) // then require.EqualError(s.T(), err, "an error occurred") - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t) - fakeClient.MockGet = func(ctx gocontext.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { - if key.Name != username { - return errors.New("an error occurred") - } - return fakeClient.Client.Get(ctx, key, obj, opts...) - } - - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - _, err = svc.GetSignupFromInformer(c, "johnsmith", "abc", true) - - // then - require.EqualError(t, err, "an error occurred") - }) } func (s *TestSignupServiceSuite) TestGetSignupNotFound() { @@ -738,24 +701,11 @@ func (s *TestSignupServiceSuite) TestGetSignupNotFound() { _, application := testutil.PrepareInClusterApp(s.T()) // when - signup, err := application.SignupService().GetSignup(c, userID.String(), "") + signup, err := application.SignupService().GetSignup(c, userID.String(), "", true) // then require.Nil(s.T(), signup) require.NoError(s.T(), err) - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t) - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - signup, err := svc.GetSignupFromInformer(c, userID.String(), "", true) - - // then - require.Nil(t, signup) - require.NoError(t, err) - }) } func (s *TestSignupServiceSuite) TestGetSignupStatusNotComplete() { @@ -800,7 +750,7 @@ func (s *TestSignupServiceSuite) TestGetSignupStatusNotComplete() { _, application := testutil.PrepareInClusterApp(s.T(), userSignupNotComplete) // when - response, err := application.SignupService().GetSignup(c, userID.String(), "") + response, err := application.SignupService().GetSignup(c, userID.String(), "", true) // then require.NoError(s.T(), err) @@ -823,35 +773,7 @@ func (s *TestSignupServiceSuite) TestGetSignupStatusNotComplete() { assert.Empty(s.T(), response.StartDate) assert.Empty(s.T(), response.EndDate) - s.T().Run("informer - with check for usersignup complete condition", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t, userSignupNotComplete) - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - response, err := svc.GetSignupFromInformer(c, userID.String(), "", true) - - // then - require.NoError(t, err) - require.NotNil(t, response) - - require.Equal(t, userID.String(), response.Name) - require.Equal(t, "bill", response.Username) - require.Equal(t, "bill", response.CompliantUsername) - require.False(t, response.Status.Ready) - require.Equal(t, "test_reason", response.Status.Reason) - require.Equal(t, "test_message", response.Status.Message) - require.True(t, response.Status.VerificationRequired) - require.Empty(t, response.ConsoleURL) - require.Empty(t, response.CheDashboardURL) - require.Empty(t, response.APIEndpoint) - require.Empty(t, response.ClusterName) - require.Empty(t, response.ProxyURL) - assert.Equal(t, "", response.DefaultUserNamespace) - assert.Equal(t, "", response.RHODSMemberURL) - }) - - s.T().Run("informer - with no check for UserSignup complete condition", func(t *testing.T) { + s.T().Run("with no check for UserSignup complete condition", func(t *testing.T) { // given states.SetVerificationRequired(userSignupNotComplete, false) mur := s.newProvisionedMUR("bill") @@ -864,7 +786,7 @@ func (s *TestSignupServiceSuite) TestGetSignupStatusNotComplete() { // when // we set checkUserSignupCompleted to false - response, err := svc.GetSignupFromInformer(c, userID.String(), userSignupNotComplete.Spec.IdentityClaims.PreferredUsername, false) + response, err := svc.GetSignup(c, userID.String(), userSignupNotComplete.Spec.IdentityClaims.PreferredUsername, false) // then require.NoError(t, err) @@ -941,7 +863,7 @@ func (s *TestSignupServiceSuite) TestGetSignupNoStatusNotCompleteCondition() { _, application := testutil.PrepareInClusterApp(s.T(), userSignup) // when - response, err := application.SignupService().GetSignup(c, userID.String(), "bill") + response, err := application.SignupService().GetSignup(c, userID.String(), "bill", true) // then require.NoError(s.T(), err) @@ -961,34 +883,6 @@ func (s *TestSignupServiceSuite) TestGetSignupNoStatusNotCompleteCondition() { require.Empty(s.T(), response.ProxyURL) assert.Equal(s.T(), "", response.DefaultUserNamespace) assert.Equal(s.T(), "", response.RHODSMemberURL) - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t, userSignup) - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - response, err := svc.GetSignupFromInformer(c, userID.String(), "", true) - - // then - require.NoError(t, err) - require.NotNil(t, response) - - require.Equal(t, userID.String(), response.Name) - require.Equal(t, "bill", response.Username) - require.Empty(t, response.CompliantUsername) - require.False(t, response.Status.Ready) - require.Equal(t, "PendingApproval", response.Status.Reason) - require.True(t, response.Status.VerificationRequired) - require.Empty(t, response.Status.Message) - require.Empty(t, response.ConsoleURL) - require.Empty(t, response.CheDashboardURL) - require.Empty(t, response.APIEndpoint) - require.Empty(t, response.ClusterName) - require.Empty(t, response.ProxyURL) - assert.Equal(t, "", response.DefaultUserNamespace) - assert.Equal(t, "", response.RHODSMemberURL) - }) } } @@ -999,28 +893,16 @@ func (s *TestSignupServiceSuite) TestGetSignupDeactivated() { us := s.newUserSignupComplete() us.Status.Conditions = deactivated() - fakeClient, application := testutil.PrepareInClusterApp(s.T(), us) + _, application := testutil.PrepareInClusterApp(s.T(), us) c, _ := gin.CreateTestContext(httptest.NewRecorder()) // when - signup, err := application.SignupService().GetSignup(c, us.Name, "") + signup, err := application.SignupService().GetSignup(c, us.Name, "", true) // then require.Nil(s.T(), signup) require.NoError(s.T(), err) - - s.T().Run("informer", func(t *testing.T) { - // given - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - signup, err := svc.GetSignupFromInformer(c, us.Name, "", true) - - // then - require.Nil(t, signup) - require.NoError(t, err) - }) } func (s *TestSignupServiceSuite) TestGetSignupStatusOK() { @@ -1041,7 +923,7 @@ func (s *TestSignupServiceSuite) TestGetSignupStatusOK() { c, _ := gin.CreateTestContext(httptest.NewRecorder()) // when - response, err := application.SignupService().GetSignup(c, us.Name, "") + response, err := application.SignupService().GetSignup(c, us.Name, "", true) // then require.NoError(t, err) @@ -1064,33 +946,6 @@ func (s *TestSignupServiceSuite) TestGetSignupStatusOK() { assert.Equal(t, "https://proxy-url.com", response.ProxyURL) assert.Equal(t, "ted-dev", response.DefaultUserNamespace) assert.Equal(t, fmt.Sprintf("https://rhods-dashboard-redhat-ods-applications%smember-123.com", appsSubDomain), response.RHODSMemberURL) - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t, us, mur, toolchainStatus, space, spacebinding) - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - response, err := svc.GetSignupFromInformer(c, us.Name, "", true) - - // then - require.NoError(t, err) - require.NotNil(t, response) - - require.Equal(t, "jsmith", response.Username) - require.Equal(t, "ted", response.CompliantUsername) - assert.True(t, response.Status.Ready) - assert.Equal(t, "mur_ready_reason", response.Status.Reason) - assert.Equal(t, "mur_ready_message", response.Status.Message) - assert.False(t, response.Status.VerificationRequired) - assert.Equal(t, fmt.Sprintf("https://console%smember-123.com", appsSubDomain), response.ConsoleURL) - assert.Equal(t, "http://che-toolchain-che.member-123.com", response.CheDashboardURL) - assert.Equal(t, "http://api.devcluster.openshift.com", response.APIEndpoint) - assert.Equal(t, "member-123", response.ClusterName) - assert.Equal(t, "https://proxy-url.com", response.ProxyURL) - assert.Equal(t, "ted-dev", response.DefaultUserNamespace) - assert.Equal(t, fmt.Sprintf("https://rhods-dashboard-redhat-ods-applications%smember-123.com", appsSubDomain), response.RHODSMemberURL) - }) }) } } @@ -1120,7 +975,7 @@ func (s *TestSignupServiceSuite) TestGetSignupByUsernameOK() { c, _ := gin.CreateTestContext(httptest.NewRecorder()) // when - response, err := svc.GetSignup(c, "foo", us.Spec.IdentityClaims.PreferredUsername) + response, err := svc.GetSignup(c, "foo", us.Spec.IdentityClaims.PreferredUsername, true) // then require.NoError(s.T(), err) @@ -1148,34 +1003,6 @@ func (s *TestSignupServiceSuite) TestGetSignupByUsernameOK() { assert.Equal(s.T(), "https://proxy-url.com", response.ProxyURL) assert.Equal(s.T(), "ted-dev", response.DefaultUserNamespace) assert.Equal(s.T(), "https://rhods-dashboard-redhat-ods-applications.apps.member-123.com", response.RHODSMemberURL) - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t, us, mur, toolchainStatus, space, spacebinding) - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - response, err := svc.GetSignupFromInformer(c, "foo", us.Spec.IdentityClaims.PreferredUsername, true) - - // then - require.NoError(t, err) - require.NotNil(t, response) - - require.Equal(t, us.Name, response.Name) - require.Equal(t, "jsmith", response.Username) - require.Equal(t, "ted", response.CompliantUsername) - assert.True(t, response.Status.Ready) - assert.Equal(t, "mur_ready_reason", response.Status.Reason) - assert.Equal(t, "mur_ready_message", response.Status.Message) - assert.False(t, response.Status.VerificationRequired) - assert.Equal(t, "https://console.apps.member-123.com", response.ConsoleURL) - assert.Equal(t, "http://che-toolchain-che.member-123.com", response.CheDashboardURL) - assert.Equal(t, "http://api.devcluster.openshift.com", response.APIEndpoint) - assert.Equal(t, "member-123", response.ClusterName) - assert.Equal(t, "https://proxy-url.com", response.ProxyURL) - assert.Equal(t, "ted-dev", response.DefaultUserNamespace) - assert.Equal(t, "https://rhods-dashboard-redhat-ods-applications.apps.member-123.com", response.RHODSMemberURL) - }) } func (s *TestSignupServiceSuite) newToolchainStatus(appsSubDomain string) *toolchainv1alpha1.ToolchainStatus { @@ -1229,22 +1056,10 @@ func (s *TestSignupServiceSuite) TestGetSignupStatusFailGetToolchainStatus() { _, application := testutil.PrepareInClusterApp(s.T(), us, mur, space) // when - _, err := application.SignupService().GetSignup(c, us.Name, "") + _, err := application.SignupService().GetSignup(c, us.Name, "", true) // then require.EqualError(s.T(), err, fmt.Sprintf("error when retrieving ToolchainStatus to set Che Dashboard for completed UserSignup %s: toolchainstatuses.toolchain.dev.openshift.com \"toolchain-status\" not found", us.Name)) - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t, us, mur, space) - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - _, err = svc.GetSignupFromInformer(c, us.Name, "", true) - - // then - require.EqualError(t, err, fmt.Sprintf("error when retrieving ToolchainStatus to set Che Dashboard for completed UserSignup %s: toolchainstatuses.toolchain.dev.openshift.com \"toolchain-status\" not found", us.Name)) - }) } func (s *TestSignupServiceSuite) TestGetSignupMURGetFails() { @@ -1265,28 +1080,10 @@ func (s *TestSignupServiceSuite) TestGetSignupMURGetFails() { } // when - _, err := application.SignupService().GetSignup(c, us.Name, "") + _, err := application.SignupService().GetSignup(c, us.Name, "", true) // then require.EqualError(s.T(), err, fmt.Sprintf("error when retrieving MasterUserRecord for completed UserSignup %s: an error occurred", us.Name)) - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t, us) - fakeClient.MockGet = func(ctx gocontext.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { - if _, ok := obj.(*toolchainv1alpha1.MasterUserRecord); ok && key.Name == us.Status.CompliantUsername { - return returnedErr - } - return fakeClient.Client.Get(ctx, key, obj, opts...) - } - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - _, err = svc.GetSignupFromInformer(c, us.Name, "", true) - - // then - require.EqualError(t, err, fmt.Sprintf("error when retrieving MasterUserRecord for completed UserSignup %s: an error occurred", us.Name)) - }) } func (s *TestSignupServiceSuite) TestGetSignupReadyConditionStatus() { @@ -1362,23 +1159,10 @@ func (s *TestSignupServiceSuite) TestGetSignupReadyConditionStatus() { tc.condition, }, } - fakeClient, application := testutil.PrepareInClusterApp(s.T(), us, mur, space, toolchainStatus) + _, application := testutil.PrepareInClusterApp(s.T(), us, mur, space, toolchainStatus) // when - response, err := application.SignupService().GetSignup(c, us.Name, "") - - // then - require.NoError(t, err) - require.Equal(t, tc.expectedConditionReady, response.Status.Ready) - require.Equal(t, tc.condition.Reason, response.Status.Reason) - require.Equal(t, tc.condition.Message, response.Status.Message) - - // informer case - // given - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - _, err = svc.GetSignupFromInformer(c, us.Name, "", true) + response, err := application.SignupService().GetSignup(c, us.Name, "", true) // then require.NoError(t, err) @@ -1394,7 +1178,7 @@ func (s *TestSignupServiceSuite) TestGetSignupBannedUserEmail() { s.ServiceConfiguration(true, "", 5) us := s.newBannedUserSignup() - fakeClient, application := testutil.PrepareInClusterApp(s.T(), us) + _, application := testutil.PrepareInClusterApp(s.T(), us) rr := httptest.NewRecorder() ctx, _ := gin.CreateTestContext(rr) @@ -1403,27 +1187,13 @@ func (s *TestSignupServiceSuite) TestGetSignupBannedUserEmail() { ctx.Set(context.EmailKey, "jsmith@gmail.com") // when - response, err := application.SignupService().GetSignup(ctx, us.Name, "") + response, err := application.SignupService().GetSignup(ctx, us.Name, "", true) // then // return not found signup require.NoError(s.T(), err) require.NotNil(s.T(), response) require.Equal(s.T(), toolchainv1alpha1.UserSignupPendingApprovalReason, response.Status.Reason) - - s.T().Run("informer", func(t *testing.T) { - // given - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - response, err := svc.GetSignupFromInformer(ctx, us.Name, "", true) - - // then - require.NoError(t, err) - require.NotNil(t, response) - require.Equal(t, toolchainv1alpha1.UserSignupPendingApprovalReason, response.Status.Reason) - - }) } func (s *TestSignupServiceSuite) TestGetDefaultUserNamespace() { @@ -1558,44 +1328,6 @@ func (s *TestSignupServiceSuite) TestGetUserSignup() { }) } -func (s *TestSignupServiceSuite) TestUpdateUserSignup() { - s.ServiceConfiguration(true, "", 5) - - us := s.newUserSignupComplete() - - s.Run("updateusersignup ok", func() { - _, application := testutil.PrepareInClusterApp(s.T(), us) - - val, err := application.SignupService().GetUserSignupFromIdentifier(us.Name, "") - require.NoError(s.T(), err) - - val.Spec.IdentityClaims.FamilyName = "Johnson" - - updated, err := application.SignupService().UpdateUserSignup(val) - require.NoError(s.T(), err) - - require.Equal(s.T(), val.Spec.IdentityClaims.FamilyName, updated.Spec.IdentityClaims.FamilyName) - }) - - s.Run("updateusersignup returns error", func() { - fakeClient, application := testutil.PrepareInClusterApp(s.T(), us) - - fakeClient.MockUpdate = func(ctx gocontext.Context, obj client.Object, opts ...client.UpdateOption) error { - if _, ok := obj.(*toolchainv1alpha1.UserSignup); ok { - return errors.New("update failed") - } - return fakeClient.Client.Update(ctx, obj, opts...) - } - - val, err := application.SignupService().GetUserSignupFromIdentifier(us.Name, "") - require.NoError(s.T(), err) - - updated, err := application.SignupService().UpdateUserSignup(val) - require.EqualError(s.T(), err, "update failed") - require.Nil(s.T(), updated) - }) -} - func (s *TestSignupServiceSuite) TestIsPhoneVerificationRequired() { commontest.SetEnvVarAndRestore(s.T(), commonconfig.WatchNamespaceEnvVar, commontest.HostOperatorNs) @@ -1745,7 +1477,7 @@ func (s *TestSignupServiceSuite) TestGetSignupUpdatesUserSignupIdentityClaims() c.Set(context.UsernameKey, "cocochanel") fakeClient, application := testutil.PrepareInClusterApp(s.T(), userSignup, mur) - _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername) + _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername, true) require.NoError(s.T(), err) modified := &toolchainv1alpha1.UserSignup{} @@ -1768,7 +1500,7 @@ func (s *TestSignupServiceSuite) TestGetSignupUpdatesUserSignupIdentityClaims() c, _ := gin.CreateTestContext(httptest.NewRecorder()) c.Set(context.GivenNameKey, "Jonathan") - _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername) + _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername, true) require.NoError(s.T(), err) modified := &toolchainv1alpha1.UserSignup{} @@ -1794,7 +1526,7 @@ func (s *TestSignupServiceSuite) TestGetSignupUpdatesUserSignupIdentityClaims() c.Set(context.FamilyNameKey, "Smythe") c.Set(context.CompanyKey, "Red Hat") - _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername) + _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername, true) require.NoError(s.T(), err) modified := &toolchainv1alpha1.UserSignup{} @@ -1822,7 +1554,7 @@ func (s *TestSignupServiceSuite) TestGetSignupUpdatesUserSignupIdentityClaims() c.Set(context.OriginalSubKey, "jsmythe-original-sub") c.Set(context.EmailKey, "jsmythe@redhat.com") - _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername) + _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername, true) require.NoError(s.T(), err) modified := &toolchainv1alpha1.UserSignup{} diff --git a/pkg/verification/service/verification_service.go b/pkg/verification/service/verification_service.go index c8ce7831..e4e990c5 100644 --- a/pkg/verification/service/verification_service.go +++ b/pkg/verification/service/verification_service.go @@ -1,6 +1,7 @@ package service import ( + gocontext "context" "crypto/rand" "errors" "fmt" @@ -185,8 +186,7 @@ func (s *ServiceImpl) InitVerification(ctx *gin.Context, userID, username, e164P for k, v := range annotationValues { signup.Annotations[k] = v } - _, err = s.Services().SignupService().UpdateUserSignup(signup) - if err != nil { + if err := s.Update(gocontext.TODO(), signup); err != nil { return err } @@ -343,8 +343,7 @@ func (s *ServiceImpl) VerifyPhoneCode(ctx *gin.Context, userID, username, code s delete(signup.Annotations, annotationName) } - _, err = s.Services().SignupService().UpdateUserSignup(signup) - if err != nil { + if err := s.Update(gocontext.TODO(), signup); err != nil { log.Error(ctx, err, fmt.Sprintf("error updating usersignup: %s", signup.Name)) return err } @@ -426,8 +425,7 @@ func (s *ServiceImpl) VerifyActivationCode(ctx *gin.Context, userID, username, c if targetCluster != "" { signup.Spec.TargetCluster = targetCluster } - _, err = s.Services().SignupService().UpdateUserSignup(signup) - if err != nil { + if err := s.Update(gocontext.TODO(), signup); err != nil { return err } diff --git a/test/fake/proxy.go b/test/fake/proxy.go index 952a29d4..f25b658b 100644 --- a/test/fake/proxy.go +++ b/test/fake/proxy.go @@ -94,11 +94,7 @@ func (m *SignupService) DefaultMockGetSignup() func(userID, username string) (*s } } -func (m *SignupService) GetSignup(_ *gin.Context, userID, username string) (*signup.Signup, error) { - return m.MockGetSignup(userID, username) -} - -func (m *SignupService) GetSignupFromInformer(_ *gin.Context, userID, username string, _ bool) (*signup.Signup, error) { +func (m *SignupService) GetSignup(_ *gin.Context, userID, username string, _ bool) (*signup.Signup, error) { return m.MockGetSignup(userID, username) } diff --git a/test/fake/informer.go b/test/fake/utils.go similarity index 100% rename from test/fake/informer.go rename to test/fake/utils.go