feat(approval): support search query and filter by resource type and …

…account_type (#51)

* feat(approval): support search query and filter by resource type and account_type

* feat(approval): add indexes for some filterable/searchable columns

* chore: update proton commit

* feat: include total count in list approvals result

* chore: update proton commit

* fix: fix gorm query building order

* test: add test cases to ensure grouped condition for "q"

* chore: add comment to document the issue

Makefile

@@ -3,7 +3,7 @@ COMMIT := $(shell git rev-parse --short HEAD)
 TAG := "$(shell git rev-list --tags --max-count=1)"
 VERSION := "$(shell git describe --tags ${TAG})-next"
 BUILD_DIR=dist
-PROTON_COMMIT := "869341885f6b35fc3b647987958c8d2feb9a459d"
+PROTON_COMMIT := "07884ade7983c5de0fde7d426f23574d82ce31a6"
 
 .PHONY: all build clean test tidy vet proto setup format generate
 

api/handler/v1beta1/approval.go

@@ -7,6 +7,7 @@ import (
 	guardianv1beta1 "github.com/goto/guardian/api/proto/gotocompany/guardian/v1beta1"
 	"github.com/goto/guardian/core/appeal"
 	"github.com/goto/guardian/domain"
+	"golang.org/x/sync/errgroup"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -17,8 +18,11 @@ func (s *GRPCServer) ListUserApprovals(ctx context.Context, req *guardianv1beta1
 		return nil, status.Error(codes.Unauthenticated, err.Error())
 	}
 
-	approvals, err := s.listApprovals(ctx, &domain.ListApprovalsFilter{
+	approvals, total, err := s.listApprovals(ctx, &domain.ListApprovalsFilter{
+		Q:              req.GetQ(),
 		AccountID:      req.GetAccountId(),
+		AccountTypes:   req.GetAccountTypes(),
+		ResourceTypes:  req.GetResourceTypes(),
 		CreatedBy:      user,
 		Statuses:       req.GetStatuses(),
 		OrderBy:        req.GetOrderBy(),
@@ -32,12 +36,16 @@ func (s *GRPCServer) ListUserApprovals(ctx context.Context, req *guardianv1beta1
 
 	return &guardianv1beta1.ListUserApprovalsResponse{
 		Approvals: approvals,
+		Total:     int32(total),
 	}, nil
 }
 
 func (s *GRPCServer) ListApprovals(ctx context.Context, req *guardianv1beta1.ListApprovalsRequest) (*guardianv1beta1.ListApprovalsResponse, error) {
-	approvals, err := s.listApprovals(ctx, &domain.ListApprovalsFilter{
+	approvals, total, err := s.listApprovals(ctx, &domain.ListApprovalsFilter{
+		Q:              req.GetQ(),
 		AccountID:      req.GetAccountId(),
+		AccountTypes:   req.GetAccountTypes(),
+		ResourceTypes:  req.GetResourceTypes(),
 		CreatedBy:      req.GetCreatedBy(),
 		Statuses:       req.GetStatuses(),
 		OrderBy:        req.GetOrderBy(),
@@ -51,6 +59,7 @@ func (s *GRPCServer) ListApprovals(ctx context.Context, req *guardianv1beta1.Lis
 
 	return &guardianv1beta1.ListApprovalsResponse{
 		Approvals: approvals,
+		Total:     int32(total),
 	}, nil
 }
 
@@ -150,20 +159,41 @@ func (s *GRPCServer) DeleteApprover(ctx context.Context, req *guardianv1beta1.De
 	}, nil
 }
 
-func (s *GRPCServer) listApprovals(ctx context.Context, filters *domain.ListApprovalsFilter) ([]*guardianv1beta1.Approval, error) {
-	approvals, err := s.approvalService.ListApprovals(ctx, filters)
-	if err != nil {
-		return nil, status.Errorf(codes.Internal, "failed to get approval list: %s", err)
+func (s *GRPCServer) listApprovals(ctx context.Context, filters *domain.ListApprovalsFilter) ([]*guardianv1beta1.Approval, int64, error) {
+	eg, ctx := errgroup.WithContext(ctx)
+	var approvals []*domain.Approval
+	var total int64
+
+	eg.Go(func() error {
+		approvalRecords, err := s.approvalService.ListApprovals(ctx, filters)
+		if err != nil {
+			return status.Errorf(codes.Internal, "failed to get approval list: %s", err)
+		}
+		approvals = approvalRecords
+		return nil
+	})
+
+	eg.Go(func() error {
+		totalRecord, err := s.approvalService.GetApprovalsTotalCount(ctx, filters)
+		if err != nil {
+			return status.Errorf(codes.Internal, "failed to get approval list: %v", err)
+		}
+		total = totalRecord
+		return nil
+	})
+
+	if err := eg.Wait(); err != nil {
+		return nil, 0, err
 	}
 
 	approvalProtos := []*guardianv1beta1.Approval{}
 	for _, a := range approvals {
 		approvalProto, err := s.adapter.ToApprovalProto(a)
 		if err != nil {
-			return nil, status.Errorf(codes.Internal, "failed to parse approval: %v: %s", a.ID, err)
+			return nil, 0, status.Errorf(codes.Internal, "failed to parse approval: %v: %s", a.ID, err)
 		}
 		approvalProtos = append(approvalProtos, approvalProto)
 	}
 
-	return approvalProtos, nil
+	return approvalProtos, total, nil
 }

api/handler/v1beta1/approval_test.go

@@ -107,9 +107,12 @@ func (s *GrpcHandlersSuite) TestListUserApprovals() {
 					},
 				},
 			},
+			Total: 1,
 		}
-		s.approvalService.EXPECT().ListApprovals(mock.AnythingOfType("*context.valueCtx"), expectedFilters).
+		s.approvalService.EXPECT().ListApprovals(mock.AnythingOfType("*context.cancelCtx"), expectedFilters).
 			Return(expectedApprovals, nil).Once()
+		s.approvalService.EXPECT().GetApprovalsTotalCount(mock.AnythingOfType("*context.cancelCtx"), expectedFilters).
+			Return(int64(1), nil).Once()
 
 		req := &guardianv1beta1.ListUserApprovalsRequest{
 			AccountId: "test-account-id",
@@ -138,12 +141,32 @@ func (s *GrpcHandlersSuite) TestListUserApprovals() {
 		s.approvalService.AssertExpectations(s.T())
 	})
 
-	s.Run("should return internal error if approval service returns an error", func() {
+	s.Run("should return internal error if approvalService.ListApprovals returns an error", func() {
 		s.setup()
 
 		expectedError := errors.New("random error")
-		s.approvalService.EXPECT().ListApprovals(mock.AnythingOfType("*context.valueCtx"), mock.Anything).
+		s.approvalService.EXPECT().ListApprovals(mock.AnythingOfType("*context.cancelCtx"), mock.Anything).
 			Return(nil, expectedError).Once()
+		s.approvalService.EXPECT().GetApprovalsTotalCount(mock.AnythingOfType("*context.cancelCtx"), mock.Anything).
+			Return(int64(0), nil).Once()
+
+		req := &guardianv1beta1.ListUserApprovalsRequest{}
+		ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, "test-user")
+		res, err := s.grpcServer.ListUserApprovals(ctx, req)
+
+		s.Equal(codes.Internal, status.Code(err))
+		s.Nil(res)
+		s.approvalService.AssertExpectations(s.T())
+	})
+
+	s.Run("should return internal error if approvalService.GetApprovalsTotalCount returns an error", func() {
+		s.setup()
+
+		s.approvalService.EXPECT().ListApprovals(mock.AnythingOfType("*context.cancelCtx"), mock.Anything).
+			Return([]*domain.Approval{}, nil).Once()
+		expectedError := errors.New("random error")
+		s.approvalService.EXPECT().GetApprovalsTotalCount(mock.AnythingOfType("*context.cancelCtx"), mock.Anything).
+			Return(int64(0), expectedError).Once()
 
 		req := &guardianv1beta1.ListUserApprovalsRequest{}
 		ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, "test-user")
@@ -166,8 +189,10 @@ func (s *GrpcHandlersSuite) TestListUserApprovals() {
 				},
 			},
 		}
-		s.approvalService.EXPECT().ListApprovals(mock.AnythingOfType("*context.valueCtx"), mock.Anything).
+		s.approvalService.EXPECT().ListApprovals(mock.AnythingOfType("*context.cancelCtx"), mock.Anything).
 			Return(invalidApprovals, nil).Once()
+		s.approvalService.EXPECT().GetApprovalsTotalCount(mock.AnythingOfType("*context.cancelCtx"), mock.Anything).
+			Return(int64(1), nil).Once()
 
 		req := &guardianv1beta1.ListUserApprovalsRequest{}
 		ctx := context.WithValue(context.Background(), authEmailTestContextKey{}, "test-user")
@@ -268,9 +293,12 @@ func (s *GrpcHandlersSuite) TestListApprovals() {
 					},
 				},
 			},
+			Total: 1,
 		}
-		s.approvalService.EXPECT().ListApprovals(mock.AnythingOfType("*context.emptyCtx"), expectedFilters).
+		s.approvalService.EXPECT().ListApprovals(mock.AnythingOfType("*context.cancelCtx"), expectedFilters).
 			Return(expectedApprovals, nil).Once()
+		s.approvalService.EXPECT().GetApprovalsTotalCount(mock.AnythingOfType("*context.cancelCtx"), expectedFilters).
+			Return(int64(1), nil).Once()
 
 		req := &guardianv1beta1.ListApprovalsRequest{
 			AccountId: "test-account-id",
@@ -289,8 +317,10 @@ func (s *GrpcHandlersSuite) TestListApprovals() {
 		s.setup()
 
 		expectedError := errors.New("random error")
-		s.approvalService.EXPECT().ListApprovals(mock.AnythingOfType("*context.emptyCtx"), mock.Anything).
+		s.approvalService.EXPECT().ListApprovals(mock.AnythingOfType("*context.cancelCtx"), mock.Anything).
 			Return(nil, expectedError).Once()
+		s.approvalService.EXPECT().GetApprovalsTotalCount(mock.AnythingOfType("*context.cancelCtx"), mock.Anything).
+			Return(int64(0), nil).Once()
 
 		req := &guardianv1beta1.ListApprovalsRequest{}
 		res, err := s.grpcServer.ListApprovals(context.Background(), req)
@@ -312,8 +342,10 @@ func (s *GrpcHandlersSuite) TestListApprovals() {
 				},
 			},
 		}
-		s.approvalService.EXPECT().ListApprovals(mock.AnythingOfType("*context.emptyCtx"), mock.Anything).
+		s.approvalService.EXPECT().ListApprovals(mock.AnythingOfType("*context.cancelCtx"), mock.Anything).
 			Return(invalidApprovals, nil).Once()
+		s.approvalService.EXPECT().GetApprovalsTotalCount(mock.AnythingOfType("*context.cancelCtx"), mock.Anything).
+			Return(int64(1), nil).Once()
 
 		req := &guardianv1beta1.ListApprovalsRequest{}
 		res, err := s.grpcServer.ListApprovals(context.Background(), req)

api/handler/v1beta1/grpc.go

@@ -95,6 +95,7 @@ type appealService interface {
 //go:generate mockery --name=approvalService --exported --with-expecter
 type approvalService interface {
 	ListApprovals(context.Context, *domain.ListApprovalsFilter) ([]*domain.Approval, error)
+	GetApprovalsTotalCount(context.Context, *domain.ListApprovalsFilter) (int64, error)
 	BulkInsert(context.Context, []*domain.Approval) error
 }