fix: fix error to status code mapping for UpdateApproval

api/handler/v1beta1/approval.go

@@ -78,22 +78,24 @@ func (s *GRPCServer) UpdateApproval(ctx context.Context, req *guardianv1beta1.Up
 		Reason:       req.GetAction().GetReason(),
 	})
 	if err != nil {
-		switch err {
-		case appeal.ErrAppealStatusCanceled,
-			appeal.ErrAppealStatusApproved,
-			appeal.ErrAppealStatusRejected,
-			appeal.ErrAppealStatusUnrecognized,
-			appeal.ErrApprovalDependencyIsPending,
-			appeal.ErrApprovalStatusUnrecognized,
-			appeal.ErrApprovalStatusApproved,
-			appeal.ErrApprovalStatusRejected,
-			appeal.ErrApprovalStatusSkipped,
-			appeal.ErrActionInvalidValue:
-			return nil, status.Errorf(codes.InvalidArgument, "unable to process the request: %v", err)
-		case appeal.ErrActionForbidden:
+		switch {
+		case
+			errors.Is(err, appeal.ErrInvalidUpdateApprovalParameter),
+			errors.Is(err, appeal.ErrAppealIDEmptyParam),
+			errors.Is(err, appeal.ErrActionInvalidValue):
+			return nil, status.Errorf(codes.InvalidArgument, err.Error())
+		case
+			errors.Is(err, appeal.ErrAppealNotEligibleForApproval),
+			errors.Is(err, appeal.ErrAppealStatusUnrecognized),
+			errors.Is(err, appeal.ErrApprovalNotEligibleForAction),
+			errors.Is(err, appeal.ErrApprovalStatusUnrecognized):
+			return nil, status.Errorf(codes.FailedPrecondition, err.Error())
+		case errors.Is(err, appeal.ErrActionForbidden):
 			return nil, status.Error(codes.PermissionDenied, "permission denied")
-		case appeal.ErrApprovalNotFound:
-			return nil, status.Errorf(codes.NotFound, "approval not found: %v", id)
+		case
+			errors.Is(err, appeal.ErrAppealNotFound),
+			errors.Is(err, appeal.ErrApprovalNotFound):
+			return nil, status.Errorf(codes.NotFound, err.Error())
 		default:
 			return nil, s.internalError(ctx, "failed to update approval: %v", err)
 		}

api/handler/v1beta1/approval_test.go

@@ -493,49 +493,24 @@ func (s *GrpcHandlersSuite) TestUpdateApproval() {
 		}{
 
 			{
-				"should return invalid error if appeal status already cancelled",
-				appeal.ErrAppealStatusCanceled,
-				codes.InvalidArgument,
-			},
-			{
-				"should return invalid error if appeal status already approved",
-				appeal.ErrAppealStatusApproved,
-				codes.InvalidArgument,
-			},
-			{
-				"should return invalid error if appeal status already rejected",
-				appeal.ErrAppealStatusRejected,
-				codes.InvalidArgument,
+				"should return invalid error if appeal status is not eligible for approval",
+				appeal.ErrAppealNotEligibleForApproval,
+				codes.FailedPrecondition,
 			},
 			{
 				"should return invalid error if appeal status unrecognized",
 				appeal.ErrAppealStatusUnrecognized,
-				codes.InvalidArgument,
+				codes.FailedPrecondition,
 			},
 			{
 				"should return invalid error if approval dependency is still pending",
-				appeal.ErrApprovalDependencyIsPending,
-				codes.InvalidArgument,
+				appeal.ErrApprovalNotEligibleForAction,
+				codes.FailedPrecondition,
 			},
 			{
 				"should return invalid error if approval status unrecognized",
 				appeal.ErrApprovalStatusUnrecognized,
-				codes.InvalidArgument,
-			},
-			{
-				"should return invalid error if approval status already approved",
-				appeal.ErrApprovalStatusApproved,
-				codes.InvalidArgument,
-			},
-			{
-				"should return invalid error if approval status already rejected",
-				appeal.ErrApprovalStatusRejected,
-				codes.InvalidArgument,
-			},
-			{
-				"should return invalid error if approval status already skipped",
-				appeal.ErrApprovalStatusSkipped,
-				codes.InvalidArgument,
+				codes.FailedPrecondition,
 			},
 			{
 				"should return invalid error if action name is invalid",

core/appeal/errors.go

@@ -20,15 +20,13 @@ var (
 	ErrGrantNotEligibleForExtension   = errors.New("grant not eligible for extension")
 	ErrCannotCreateAppealForOtherUser = errors.New("creating appeal for other individual user (account_type=\"user\") is not allowed")
 
-	ErrApprovalDependencyIsBlocked = errors.New("found previous approval step that is still in blocked")
-	ErrApprovalDependencyIsPending = errors.New("found previous approval step that is still in pending")
-	ErrApprovalStatusApproved      = errors.New("approval already approved")
-	ErrApprovalStatusRejected      = errors.New("approval already rejected")
-	ErrApprovalStatusSkipped       = errors.New("approval already skipped")
-	ErrApprovalStatusUnrecognized  = errors.New("unrecognized approval status")
-	ErrApprovalNotFound            = errors.New("approval not found")
-	ErrUnableToAddApprover         = errors.New("unable to add a new approver")
-	ErrUnableToDeleteApprover      = errors.New("unable to remove approver")
+	ErrApprovalStatusApproved     = errors.New("approval already approved")
+	ErrApprovalStatusRejected     = errors.New("approval already rejected")
+	ErrApprovalStatusSkipped      = errors.New("approval already skipped")
+	ErrApprovalStatusUnrecognized = errors.New("unrecognized approval status")
+	ErrApprovalNotFound           = errors.New("approval not found")
+	ErrUnableToAddApprover        = errors.New("unable to add a new approver")
+	ErrUnableToDeleteApprover     = errors.New("unable to remove approver")
 
 	ErrActionForbidden    = errors.New("user is not allowed to make action on this approval step")
 	ErrActionInvalidValue = errors.New("invalid action value")
@@ -45,11 +43,15 @@ var (
 	ErrDurationNotAllowed                  = errors.New("duration value not allowed")
 	ErrDurationIsRequired                  = errors.New("having permanent access to this resource is not allowed, access duration is required")
 
-	ErrApproverKeyNotRecognized = errors.New("unrecognized approvers key")
-	ErrApproverInvalidType      = errors.New("invalid approver type, expected an email string or array of email string")
-	ErrApproverEmail            = errors.New("approver is not a valid email")
-	ErrApproverNotFound         = errors.New("approver not found")
-	ErrGrantNotFound            = errors.New("grant not found")
+	ErrApproverKeyNotRecognized       = errors.New("unrecognized approvers key")
+	ErrApproverInvalidType            = errors.New("invalid approver type, expected an email string or array of email string")
+	ErrApproverEmail                  = errors.New("approver is not a valid email")
+	ErrApproverNotFound               = errors.New("approver not found")
+	ErrGrantNotFound                  = errors.New("grant not found")
+	ErrInvalidUpdateApprovalParameter = errors.New("invalid parameter")
+
+	ErrAppealNotEligibleForApproval = errors.New("appeal status not eligible for approval")
+	ErrApprovalNotEligibleForAction = errors.New("approval not eligible for action")
 )
 
 type InvalidError struct {

core/appeal/service.go

@@ -439,12 +439,15 @@ func validateAppealOnBehalf(a *domain.Appeal, policy *domain.Policy) error {
 
 // UpdateApproval Approve an approval step
 func (s *Service) UpdateApproval(ctx context.Context, approvalAction domain.ApprovalAction) (*domain.Appeal, error) {
-	if err := utils.ValidateStruct(approvalAction); err != nil {
-		return nil, err
+	if err := approvalAction.Validate(); err != nil {
+		return nil, fmt.Errorf("%w: %v", ErrInvalidUpdateApprovalParameter, err)
 	}
 
 	appeal, err := s.GetByID(ctx, approvalAction.AppealID)
 	if err != nil {
+		if errors.Is(err, ErrAppealNotFound) {
+			return nil, fmt.Errorf("%w: %q", ErrAppealNotFound, approvalAction.AppealID)
+		}
 		return nil, err
 	}
 
@@ -454,16 +457,14 @@ func (s *Service) UpdateApproval(ctx context.Context, approvalAction domain.Appr
 
 	for i, approval := range appeal.Approvals {
 		if approval.Name != approvalAction.ApprovalName {
-			if err := checkPreviousApprovalStatus(approval.Status); err != nil {
+			if err := checkPreviousApprovalStatus(approval.Status, approval.Name); err != nil {
 				return nil, err
 			}
 			continue
 		}
 
-		if approval.Status != domain.ApprovalStatusPending {
-			if err := checkApprovalStatus(approval.Status); err != nil {
-				return nil, err
-			}
+		if err := checkApprovalStatus(approval.Status); err != nil {
+			return nil, err
 		}
 
 		if !utils.ContainsString(approval.Approvers, approvalAction.Actor) {
@@ -592,7 +593,7 @@ func (s *Service) UpdateApproval(ctx context.Context, approvalAction domain.Appr
 		return appeal, nil
 	}
 
-	return nil, ErrApprovalNotFound
+	return nil, fmt.Errorf("%w: %q", ErrApprovalNotFound, approvalAction.ApprovalName)
 }
 
 func (s *Service) Update(ctx context.Context, appeal *domain.Appeal) error {
@@ -904,57 +905,48 @@ func (s *Service) getApprovalNotifications(ctx context.Context, appeal *domain.A
 }
 
 func checkIfAppealStatusStillPending(status string) error {
-	if status == domain.AppealStatusPending {
-		return nil
-	}
-
-	var err error
 	switch status {
-	case domain.AppealStatusCanceled:
-		err = ErrAppealStatusCanceled
-	case domain.AppealStatusApproved:
-		err = ErrAppealStatusApproved
-	case domain.AppealStatusRejected:
-		err = ErrAppealStatusRejected
+	case domain.AppealStatusPending:
+		return nil
+	case
+		domain.AppealStatusCanceled,
+		domain.AppealStatusApproved,
+		domain.AppealStatusRejected:
+		return fmt.Errorf("%w: %q", ErrAppealNotEligibleForApproval, status)
 	default:
-		err = ErrAppealStatusUnrecognized
+		return fmt.Errorf("%w: %q", ErrAppealStatusUnrecognized, status)
 	}
-	return err
 }
 
-func checkPreviousApprovalStatus(status string) error {
-	var err error
+func checkPreviousApprovalStatus(status, name string) error {
 	switch status {
-	case domain.ApprovalStatusApproved,
+	case
+		domain.ApprovalStatusApproved,
 		domain.ApprovalStatusSkipped:
-		err = nil
-	case domain.ApprovalStatusBlocked:
-		err = ErrApprovalDependencyIsBlocked
-	case domain.ApprovalStatusPending:
-		err = ErrApprovalDependencyIsPending
-	case domain.ApprovalStatusRejected:
-		err = ErrAppealStatusRejected
+		return nil
+	case
+		domain.ApprovalStatusBlocked,
+		domain.ApprovalStatusPending,
+		domain.ApprovalStatusRejected:
+		return fmt.Errorf("%w: found previous approval %q with status %q", ErrApprovalNotEligibleForAction, name, status)
 	default:
-		err = ErrApprovalStatusUnrecognized
+		return fmt.Errorf("%w: found previous approval %q with unrecognized status %q", ErrApprovalStatusUnrecognized, name, status)
 	}
-	return err
 }
 
 func checkApprovalStatus(status string) error {
-	var err error
 	switch status {
-	case domain.ApprovalStatusBlocked:
-		err = ErrAppealStatusBlocked
-	case domain.ApprovalStatusApproved:
-		err = ErrApprovalStatusApproved
-	case domain.ApprovalStatusRejected:
-		err = ErrApprovalStatusRejected
-	case domain.ApprovalStatusSkipped:
-		err = ErrApprovalStatusSkipped
+	case domain.ApprovalStatusPending:
+		return nil
+	case
+		domain.ApprovalStatusBlocked,
+		domain.ApprovalStatusApproved,
+		domain.ApprovalStatusRejected,
+		domain.ApprovalStatusSkipped:
+		return fmt.Errorf("%w: approval status %q is not actionable", ErrApprovalNotEligibleForAction, status)
 	default:
-		err = ErrApprovalStatusUnrecognized
+		return fmt.Errorf("%w: %q", ErrApprovalStatusUnrecognized, status)
 	}
-	return err
 }
 
 func (s *Service) handleAppealRequirements(ctx context.Context, a *domain.Appeal, p *domain.Policy) error {

core/appeal/service_test.go

@@ -1626,7 +1626,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 
 		s.mockRepository.AssertExpectations(s.T())
 		s.Nil(actualResult)
-		s.EqualError(actualError, expectedError.Error())
+		s.ErrorIs(actualError, expectedError)
 	})
 
 	s.Run("should return error if appeal not found", func() {
@@ -1637,7 +1637,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 
 		s.mockRepository.AssertExpectations(s.T())
 		s.Nil(actualResult)
-		s.EqualError(actualError, appeal.ErrAppealNotFound.Error())
+		s.ErrorIs(actualError, appeal.ErrAppealNotFound)
 	})
 
 	s.Run("should return error based on statuses conditions", func() {
@@ -1647,15 +1647,20 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 			approvals     []*domain.Approval
 			expectedError error
 		}{
+			{
+				name:          "appeal not eligible, status: canceled",
+				appealStatus:  domain.AppealStatusCanceled,
+				expectedError: appeal.ErrAppealNotEligibleForApproval,
+			},
 			{
 				name:          "appeal not eligible, status: approved",
 				appealStatus:  domain.AppealStatusApproved,
-				expectedError: appeal.ErrAppealStatusApproved,
+				expectedError: appeal.ErrAppealNotEligibleForApproval,
 			},
 			{
 				name:          "appeal not eligible, status: rejected",
 				appealStatus:  domain.AppealStatusRejected,
-				expectedError: appeal.ErrAppealStatusRejected,
+				expectedError: appeal.ErrAppealNotEligibleForApproval,
 			},
 			{
 				name:          "invalid appeal status",
@@ -1675,7 +1680,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 						Status: domain.ApprovalStatusPending,
 					},
 				},
-				expectedError: appeal.ErrApprovalDependencyIsPending,
+				expectedError: appeal.ErrApprovalNotEligibleForAction,
 			},
 			{
 				name:         "found one previous approval is reject",
@@ -1690,7 +1695,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 						Status: domain.ApprovalStatusPending,
 					},
 				},
-				expectedError: appeal.ErrAppealStatusRejected,
+				expectedError: appeal.ErrApprovalNotEligibleForAction,
 			},
 			{
 				name:         "invalid approval status",
@@ -1720,7 +1725,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 						Status: domain.ApprovalStatusApproved,
 					},
 				},
-				expectedError: appeal.ErrApprovalStatusApproved,
+				expectedError: appeal.ErrApprovalNotEligibleForAction,
 			},
 			{
 				name:         "approval step already rejected",
@@ -1735,7 +1740,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 						Status: domain.ApprovalStatusRejected,
 					},
 				},
-				expectedError: appeal.ErrApprovalStatusRejected,
+				expectedError: appeal.ErrApprovalNotEligibleForAction,
 			},
 			{
 				name:         "approval step already skipped",
@@ -1750,7 +1755,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 						Status: domain.ApprovalStatusSkipped,
 					},
 				},
-				expectedError: appeal.ErrApprovalStatusSkipped,
+				expectedError: appeal.ErrApprovalNotEligibleForAction,
 			},
 			{
 				name:         "invalid approval status",
@@ -1813,7 +1818,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 			actualResult, actualError := s.service.UpdateApproval(context.Background(), validApprovalActionParam)
 
 			s.Nil(actualResult)
-			s.EqualError(actualError, tc.expectedError.Error())
+			s.ErrorIs(actualError, tc.expectedError)
 		}
 	})