feat(provider): update provider with the latest value (#238)

* Update provider record with the latest changes always without merging with the existing record * Fix test case BREAKING CHANGE: Current provider api works in a patch way although with issues. This changes makes sure provider takes in the latest value only without any merge with existing value.
raystack · Aug 3, 2022 · d6df0cf · d6df0cf
1 parent f05bb58
commit d6df0cf
Show file tree

Hide file tree

Showing 2 changed files with 5 additions and 58 deletions.
diff --git a/core/provider/service.go b/core/provider/service.go
@@ -12,7 +12,6 @@ import (
 	"time"
 
 	"github.com/go-playground/validator/v10"
-	"github.com/imdario/mergo"
 	"github.com/odpf/guardian/domain"
 	"github.com/odpf/guardian/plugins/providers"
 	"github.com/odpf/guardian/utils"
@@ -164,22 +163,6 @@ func (s *Service) GetOne(ctx context.Context, pType, urn string) (*domain.Provid
 
 // Update updates the non-zero value(s) only
 func (s *Service) Update(ctx context.Context, p *domain.Provider) error {
-	var currentProvider *domain.Provider
-	var err error
-
-	if len(p.ID) > 0 {
-		currentProvider, err = s.GetByID(ctx, p.ID)
-	} else {
-		currentProvider, err = s.GetOne(ctx, p.Type, p.URN)
-	}
-	if err != nil {
-		return err
-	}
-
-	if err := mergo.Merge(p, currentProvider); err != nil {
-		return err
-	}
-
 	c := s.getClient(p.Type)
 	if c == nil {
 		return ErrInvalidProviderType

diff --git a/core/provider/service_test.go b/core/provider/service_test.go
@@ -204,60 +204,25 @@ func (s *ServiceTestSuite) TestUpdateValidation() {
 }
 
 func (s *ServiceTestSuite) TestUpdate() {
-	s.Run("should return error if got error getting existing record", func() {
-		testCases := []struct {
-			expectedExistingProvider *domain.Provider
-			expectedRepositoryError  error
-			expectedError            error
-		}{
-			{
-				expectedExistingProvider: nil,
-				expectedRepositoryError:  provider.ErrRecordNotFound,
-				expectedError:            provider.ErrRecordNotFound,
-			},
-			{
-				expectedExistingProvider: nil,
-				expectedRepositoryError:  errors.New("repository error"),
-				expectedError:            errors.New("repository error"),
-			},
-		}
-
-		for _, tc := range testCases {
-			expectedProvider := &domain.Provider{
-				ID: "1",
-			}
-			expectedError := tc.expectedError
-			s.mockProviderRepository.On("GetByID", expectedProvider.ID).Return(tc.expectedExistingProvider, tc.expectedRepositoryError).Once()
-
-			actualError := s.service.Update(context.Background(), expectedProvider)
-
-			s.EqualError(actualError, expectedError.Error())
-		}
-	})
-
-	s.Run("should update only non-zero values", func() {
+	s.Run("should update record on success", func() {
 		testCases := []struct {
 			updatePayload       *domain.Provider
 			existingProvider    *domain.Provider
 			expectedNewProvider *domain.Provider
 		}{
 			{
 				updatePayload: &domain.Provider{
-					ID: "1",
-					Config: &domain.ProviderConfig{
-						Labels: map[string]string{
-							"foo": "bar",
-						},
-					},
-				},
-				existingProvider: &domain.Provider{
 					ID:   "1",
 					Type: mockProviderType,
 					Config: &domain.ProviderConfig{
 						Appeal: &domain.AppealConfig{
 							AllowPermanentAccess:         true,
 							AllowActiveAccessExtensionIn: "1h",
 						},
+						AllowedAccountTypes: []string{"user"},
+						Labels: map[string]string{
+							"foo": "bar",
+						},
 						Type: mockProviderType,
 						URN:  "urn",
 					},
@@ -282,7 +247,6 @@ func (s *ServiceTestSuite) TestUpdate() {
 		}
 
 		for _, tc := range testCases {
-			s.mockProviderRepository.On("GetByID", tc.updatePayload.ID).Return(tc.existingProvider, nil).Once()
 			s.mockProvider.On("GetAccountTypes").Return([]string{"user"}).Once()
 			s.mockProvider.On("CreateConfig", mock.Anything).Return(nil).Once()
 			s.mockProviderRepository.On("Update", tc.expectedNewProvider).Return(nil)