Skip to content

drop service factory and service context abstractions #506

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 12, 2025
Merged

drop service factory and service context abstractions #506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions pkg/application/service/base/base_service.go
View file
Open in desktop

This file was deleted.

13 changes: 0 additions & 13 deletions pkg/application/service/context/service_context.go
View file
Open in desktop

This file was deleted.

102 changes: 0 additions & 102 deletions pkg/application/service/factory/service_factory.go
View file
Open in desktop

This file was deleted.

94 changes: 60 additions & 34 deletions pkg/controller/signup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,20 +224,15 @@ func (s *TestSignupSuite) TestInitVerificationHandler() {
testusersignup.WithAnnotation(crtapi.UserSignupVerificationCounterAnnotationKey, "0"),
testusersignup.WithAnnotation(crtapi.UserSignupVerificationCodeAnnotationKey, ""),
testusersignup.VerificationRequiredAgo(time.Second))
fakeClient, application := testutil.PrepareInClusterApp(s.T(), userSignup)
defer gock.Off()

// Create Signup controller instance.
ctrl := controller.NewSignup(application)
handler := gin.HandlerFunc(ctrl.InitVerificationHandler)

assertInitVerificationSuccess := func(phoneNumber, expectedHash string, expectedCounter int) {
gock.New("https://api.twilio.com").
Reply(http.StatusNoContent).
BodyString("")

assertInitVerificationSuccess := func(handler gin.HandlerFunc, fakeClient *commontest.FakeClient, phoneNumber, expectedHash string, expectedCounter int) {
// given
data := []byte(fmt.Sprintf(`{"phone_number": "%s", "country_code": "1"}`, phoneNumber))

// when
rr := initPhoneVerification(s.T(), handler, gin.Param{}, data, "johnny@kubesaw", http.MethodPut, "/api/v1/signup/verification")

// then
require.Equal(s.T(), http.StatusNoContent, rr.Code)

updatedUserSignup := &crtapi.UserSignup{}
Expand All @@ -252,26 +247,41 @@ func (s *TestSignupSuite) TestInitVerificationHandler() {
}

s.Run("init verification success", func() {
assertInitVerificationSuccess("2268213044", "fd276563a8232d16620da8ec85d0575f", 1)
})
gock.New("https://api.twilio.com").
Persist().
Reply(http.StatusNoContent).
BodyString("")
defer gock.OffAll()
fakeClient, handler := prepareVerificationHandler(s.T(), userSignup)

s.Run("init verification success phone number with parenthesis and spaces", func() {
assertInitVerificationSuccess("(226) 821 3045", "9691252ac0ea2cb55295ac9b98df1c51", 2)
})
assertInitVerificationSuccess(handler, fakeClient, "2268213044", "fd276563a8232d16620da8ec85d0575f", 1)

s.Run("init verification success phone number with dashes", func() {
assertInitVerificationSuccess("226-821-3044", "fd276563a8232d16620da8ec85d0575f", 3)
})
s.Run("init verification success phone number with spaces", func() {
assertInitVerificationSuccess("2 2 6 8 2 1 3 0 4 7", "ce3e697125f35efb76357ed8e3b768b7", 4)
s.Run("init verification success phone number with parenthesis and spaces", func() {
assertInitVerificationSuccess(handler, fakeClient, "(226) 821 3045", "9691252ac0ea2cb55295ac9b98df1c51", 2)

s.Run("init verification success phone number with dashes", func() {
assertInitVerificationSuccess(handler, fakeClient, "226-821-3044", "fd276563a8232d16620da8ec85d0575f", 3)

s.Run("init verification success phone number with spaces", func() {
assertInitVerificationSuccess(handler, fakeClient, "2 2 6 8 2 1 3 0 4 7", "ce3e697125f35efb76357ed8e3b768b7", 4)
})
})
})
})
Comment on lines -258 to 270
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 had to refactor the format because the sub-tests depend on the previous ones. Previously it wasn't a problem because the factory created a new instance of the service for every call of the VerificationService method.


s.Run("init verification fails with invalid country code", func() {
// given
gock.New("https://api.twilio.com").
Reply(http.StatusNoContent).
BodyString("")

defer gock.OffAll()
_, handler := prepareVerificationHandler(s.T(), userSignup)
data := []byte(`{"phone_number": "2268213044", "country_code": "(1)"}`)

// when
rr := initPhoneVerification(s.T(), handler, gin.Param{}, data, "johnny@kubesaw", http.MethodPut, "/api/v1/signup/verification")

// then
require.Equal(s.T(), http.StatusBadRequest, rr.Code)

bodyParams := make(map[string]interface{})
Expand All @@ -284,9 +294,14 @@ func (s *TestSignupSuite) TestInitVerificationHandler() {
require.Equal(s.T(), "invalid country_code", bodyParams["details"])
})
s.Run("init verification request body could not be read", func() {
// given
_, handler := prepareVerificationHandler(s.T(), userSignup)
data := []byte(`{"test_number": "2268213044", "test_code": "1"}`)

// when
rr := initPhoneVerification(s.T(), handler, gin.Param{}, data, "johnny@kubesaw", http.MethodPut, "/api/v1/signup/verification")

// then
// Check the status code is what we expect.
assert.Equal(s.T(), http.StatusBadRequest, rr.Code)

Expand All @@ -303,31 +318,34 @@ func (s *TestSignupSuite) TestInitVerificationHandler() {
})

s.Run("init verification daily limit exceeded", func() {
// given
_, handler := prepareVerificationHandler(s.T(), userSignup)
cfg := configuration.GetRegistrationServiceConfig()
originalValue := cfg.Verification().DailyLimit()
s.SetConfig(testconfig.RegistrationService().Verification().DailyLimit(0))
defer s.SetConfig(testconfig.RegistrationService().Verification().DailyLimit(originalValue))

data := []byte(`{"phone_number": "2268213044", "country_code": "1"}`)

// when
rr := initPhoneVerification(s.T(), handler, gin.Param{}, data, "johnny@kubesaw", http.MethodPut, "/api/v1/signup/verification")

// then
// Check the status code is what we expect.
assert.Equal(s.T(), http.StatusForbidden, rr.Code, "handler returned wrong status code")
})

s.Run("init verification handler fails when verification not required", func() {
// given
// Create UserSignup
userSignup := testusersignup.NewUserSignup(testusersignup.WithEncodedName("johnny@kubesaw"))

_, application := testutil.PrepareInClusterApp(s.T(), userSignup)

// Create Signup controller instance.
ctrl := controller.NewSignup(application)
handler := gin.HandlerFunc(ctrl.InitVerificationHandler)

_, handler := prepareVerificationHandler(s.T(), userSignup)
data := []byte(`{"phone_number": "2268213044", "country_code": "1"}`)

// when
rr := initPhoneVerification(s.T(), handler, gin.Param{}, data, "johnny@kubesaw", http.MethodPut, "/api/v1/signup/verification")

// then
// Check the status code is what we expect.
assert.Equal(s.T(), http.StatusBadRequest, rr.Code)

Expand All @@ -342,21 +360,29 @@ func (s *TestSignupSuite) TestInitVerificationHandler() {
})

s.Run("init verification handler fails when invalid phone number provided", func() {
_, application := testutil.PrepareInClusterApp(s.T(), userSignup)

// Create Signup controller instance.
ctrl := controller.NewSignup(application)
handler := gin.HandlerFunc(ctrl.InitVerificationHandler)
// given
_, handler := prepareVerificationHandler(s.T(), userSignup)

// We create a ResponseRecorder (which satisfies http.ResponseWriter) to record the response.
data := []byte(`{"phone_number": "!226%213044", "country_code": "1"}`)

// when
rr := initPhoneVerification(s.T(), handler, gin.Param{}, data, "johnny@kubesaw", http.MethodPut, "/api/v1/signup/verification")

// Check the status code is what we expect.
assert.Equal(s.T(), http.StatusBadRequest, rr.Code)
})
}

func prepareVerificationHandler(t *testing.T, initObjects ...client.Object) (*commontest.FakeClient, gin.HandlerFunc) {
fakeClient, application := testutil.PrepareInClusterApp(t, initObjects...)

// Create Signup controller instance.
ctrl := controller.NewSignup(application)
handler := gin.HandlerFunc(ctrl.InitVerificationHandler)
return fakeClient, handler
}

func (s *TestSignupSuite) TestVerifyPhoneCodeHandler() {
// Create UserSignup
userSignup := testusersignup.NewUserSignup(
Expand Down
22 changes: 10 additions & 12 deletions pkg/server/in_cluster_application.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,30 +3,28 @@
import (
"github.com/codeready-toolchain/registration-service/pkg/application"
"github.com/codeready-toolchain/registration-service/pkg/application/service"
"github.com/codeready-toolchain/registration-service/pkg/application/service/factory"
"github.com/codeready-toolchain/registration-service/pkg/namespaced"
signupservice "github.com/codeready-toolchain/registration-service/pkg/signup/service"
verificationservice "github.com/codeready-toolchain/registration-service/pkg/verification/service"
)

// NewInClusterApplication creates a new in-cluster application with the specified configuration and options. This
// application type is intended to run inside a Kubernetes cluster, where it makes use of the rest.InClusterConfig()
// function to determine which Kubernetes configuration to use to create the REST client that interacts with the
// Kubernetes service endpoints.
func NewInClusterApplication(client namespaced.Client, options ...factory.Option) application.Application {
// NewInClusterApplication creates a new in-cluster application.
func NewInClusterApplication(client namespaced.Client) application.Application {
return &InClusterApplication{
serviceFactory: factory.NewServiceFactory(append(options,
factory.WithServiceContextOptions(
factory.NamespacedClientOption(client)))...),
signupService: signupservice.NewSignupService(client),
verificationService: verificationservice.NewVerificationService(client),
}
}

type InClusterApplication struct {
serviceFactory *factory.ServiceFactory
signupService service.SignupService
verificationService service.VerificationService
}
Comment on lines 19 to 22
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the next PR(s), we will be able to deal with the services separately as they don't depend on each other anymore


func (r InClusterApplication) SignupService() service.SignupService {
return r.serviceFactory.SignupService()
return r.signupService

Check warning on line 25 in pkg/server/in_cluster_application.go

View check run for this annotation

Codecov / codecov/patch

pkg/server/in_cluster_application.go#L25 

Added line #L25 was not covered by tests
}

func (r InClusterApplication) VerificationService() service.VerificationService {
return r.serviceFactory.VerificationService()
return r.verificationService

Check warning on line 29 in pkg/server/in_cluster_application.go

View check run for this annotation

Codecov / codecov/patch

pkg/server/in_cluster_application.go#L29 

Added line #L29 was not covered by tests
}
11 changes: 2 additions & 9 deletions pkg/signup/service/signup_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"time"

toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
"github.com/codeready-toolchain/registration-service/pkg/application/service"
"github.com/codeready-toolchain/registration-service/pkg/configuration"
"github.com/codeready-toolchain/registration-service/pkg/context"
"github.com/codeready-toolchain/registration-service/pkg/log"
Expand Down Expand Up @@ -48,17 +47,11 @@ type ServiceImpl struct { // nolint:revive
type SignupServiceOption func(svc *ServiceImpl)

// NewSignupService creates a service object for performing user signup-related activities.
func NewSignupService(client namespaced.Client, opts ...SignupServiceOption) service.SignupService {
s := &ServiceImpl{
func NewSignupService(client namespaced.Client) *ServiceImpl {
return &ServiceImpl{
CaptchaChecker: captcha.Helper{},
Client: client,
}

for _, opt := range opts {
opt(s)
}

return s
}

// newUserSignup generates a new UserSignup resource with the specified username and available claims.
Expand Down
Loading
Loading