feat(notifier): sending notification concurrently (#136)

* feat: sending notification concurrently

* test: update unit test

* test: fix unit test

* test: fix race data

* chore: update ctx

* test: update test

* test: adding sleep for temporary solution

* test: update test sleep

---------

Co-authored-by: Muhammad Idil Haq Amir <[email protected]>

Author: idilhaq

core/appeal/service.go

@@ -413,11 +413,14 @@ func (s *Service) Create(ctx context.Context, appeals []*domain.Appeal, opts ...
 	}
 
 	if len(notifications) > 0 {
-		if errs := s.notifier.Notify(ctx, notifications); errs != nil {
-			for _, err1 := range errs {
-				s.logger.Error(ctx, "failed to send notifications", "error", err1.Error())
+		go func() {
+			ctx := context.WithoutCancel(ctx)
+			if errs := s.notifier.Notify(ctx, notifications); errs != nil {
+				for _, err1 := range errs {
+					s.logger.Error(ctx, "failed to send notifications", "error", err1.Error())
+				}
 			}
-		}
+		}()
 	}
 
 	return nil
@@ -627,11 +630,14 @@ func (s *Service) UpdateApproval(ctx context.Context, approvalAction domain.Appr
 			notifications = append(notifications, s.getApprovalNotifications(ctx, appeal)...)
 		}
 		if len(notifications) > 0 {
-			if errs := s.notifier.Notify(ctx, notifications); errs != nil {
-				for _, err1 := range errs {
-					s.logger.Error(ctx, "failed to send notifications", "error", err1.Error())
+			go func() {
+				ctx := context.WithoutCancel(ctx)
+				if errs := s.notifier.Notify(ctx, notifications); errs != nil {
+					for _, err1 := range errs {
+						s.logger.Error(ctx, "failed to send notifications", "error", err1.Error())
+					}
 				}
-			}
+			}()
 		}
 
 		var auditKey string
@@ -744,37 +750,40 @@ func (s *Service) AddApprover(ctx context.Context, appealID, approvalID, email s
 		}
 	}
 
-	if errs := s.notifier.Notify(ctx, []domain.Notification{
-		{
-			User: email,
-			Labels: map[string]string{
-				"appeal_id": appeal.ID,
-			},
-			Message: domain.NotificationMessage{
-				Type: domain.NotificationTypeApproverNotification,
-				Variables: map[string]interface{}{
-					"resource_name": fmt.Sprintf("%s (%s: %s)", appeal.Resource.Name, appeal.Resource.ProviderType, appeal.Resource.URN),
-					"role":          appeal.Role,
-					"requestor":     appeal.CreatedBy,
-					"appeal_id":     appeal.ID,
-					"account_id":    appeal.AccountID,
-					"account_type":  appeal.AccountType,
-					"provider_type": appeal.Resource.ProviderType,
-					"resource_type": appeal.Resource.Type,
-					"created_at":    appeal.CreatedAt,
-					"approval_step": approval.Name,
-					"actor":         email,
-					"details":       appeal.Details,
-					"duration":      duration,
-					"creator":       appeal.Creator,
+	go func() {
+		ctx := context.WithoutCancel(ctx)
+		if errs := s.notifier.Notify(ctx, []domain.Notification{
+			{
+				User: email,
+				Labels: map[string]string{
+					"appeal_id": appeal.ID,
+				},
+				Message: domain.NotificationMessage{
+					Type: domain.NotificationTypeApproverNotification,
+					Variables: map[string]interface{}{
+						"resource_name": fmt.Sprintf("%s (%s: %s)", appeal.Resource.Name, appeal.Resource.ProviderType, appeal.Resource.URN),
+						"role":          appeal.Role,
+						"requestor":     appeal.CreatedBy,
+						"appeal_id":     appeal.ID,
+						"account_id":    appeal.AccountID,
+						"account_type":  appeal.AccountType,
+						"provider_type": appeal.Resource.ProviderType,
+						"resource_type": appeal.Resource.Type,
+						"created_at":    appeal.CreatedAt,
+						"approval_step": approval.Name,
+						"actor":         email,
+						"details":       appeal.Details,
+						"duration":      duration,
+						"creator":       appeal.Creator,
+					},
 				},
 			},
-		},
-	}); errs != nil {
-		for _, err1 := range errs {
-			s.logger.Error(ctx, "failed to send notifications", "error", err1.Error())
+		}); errs != nil {
+			for _, err1 := range errs {
+				s.logger.Error(ctx, "failed to send notifications", "error", err1.Error())
+			}
 		}
-	}
+	}()
 
 	return appeal, nil
 }

core/appeal/service_test.go

@@ -719,7 +719,6 @@ func (s *ServiceTestSuite) TestCreate() {
 
 	s.Run("should return appeals on success", func() {
 		h := newServiceTestHelper()
-		defer h.assertExpectations(s.T())
 		resources := []*domain.Resource{
 			{
 				ID:           "1",
@@ -1106,11 +1105,13 @@ func (s *ServiceTestSuite) TestCreate() {
 
 		s.Nil(actualError)
 		s.Equal(expectedResult, appeals)
+
+		time.Sleep(time.Millisecond)
+		h.assertExpectations(s.T())
 	})
 
 	s.Run("should return appeals on success with latest policy", func() {
 		h := newServiceTestHelper()
-		defer h.assertExpectations(s.T())
 		expDate := timeNow.Add(23 * time.Hour)
 
 		resources := []*domain.Resource{
@@ -1570,12 +1571,14 @@ func (s *ServiceTestSuite) TestCreate() {
 
 		s.Nil(actualError)
 		s.Equal(expectedResult, appeals)
+
+		time.Sleep(time.Millisecond)
+		h.assertExpectations(s.T())
 	})
 
 	s.Run("additional appeal creation", func() {
 		s.Run("should use the overridding policy", func() {
 			h := newServiceTestHelper()
-			defer h.assertExpectations(s.T())
 			input := &domain.Appeal{
 				ResourceID:    uuid.New().String(),
 				AccountID:     "[email protected]",
@@ -1668,13 +1671,15 @@ func (s *ServiceTestSuite) TestCreate() {
 			s.NoError(err)
 			s.Equal("test-approval", input.Approvals[0].Name)
 			s.Equal(expectedPermissions, input.Permissions)
+
+			time.Sleep(time.Millisecond)
+			h.assertExpectations(s.T())
 		})
 	})
 }
 
 func (s *ServiceTestSuite) TestCreateAppeal__WithExistingAppealAndWithAutoApprovalSteps() {
 	h := newServiceTestHelper()
-	defer h.assertExpectations(s.T())
 
 	appeal.TimeNow = func() time.Time {
 		return timeNow
@@ -1943,11 +1948,13 @@ func (s *ServiceTestSuite) TestCreateAppeal__WithExistingAppealAndWithAutoApprov
 
 	s.Nil(actualError)
 	s.Equal(expectedResult, appeals)
+
+	time.Sleep(time.Millisecond)
+	h.assertExpectations(s.T())
 }
 
 func (s *ServiceTestSuite) TestCreateAppeal__WithAdditionalAppeals() {
 	h := newServiceTestHelper()
-	defer h.assertExpectations(s.T())
 	providerType := "test-provider-type"
 	providerURN := "test-provider-urn"
 	resourceType := "test-resource-type"
@@ -2138,6 +2145,9 @@ func (s *ServiceTestSuite) TestCreateAppeal__WithAdditionalAppeals() {
 	err := h.service.Create(context.Background(), appealsPayload)
 
 	s.NoError(err)
+
+	time.Sleep(time.Millisecond)
+	h.assertExpectations(s.T())
 }
 
 func (s *ServiceTestSuite) TestCreate__WithAppealMetadata() {
@@ -2705,7 +2715,6 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 
 	s.Run("should terminate existing active grant if present", func() {
 		h := newServiceTestHelper()
-		defer h.assertExpectations(s.T())
 		action := domain.ApprovalAction{
 			AppealID:     appealID,
 			ApprovalName: "test-approval-step",
@@ -2772,6 +2781,9 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 		_, actualError := h.service.UpdateApproval(context.Background(), action)
 
 		s.Nil(actualError)
+
+		time.Sleep(time.Millisecond)
+		h.assertExpectations(s.T())
 	})
 
 	s.Run("should return updated appeal on success", func() {
@@ -3128,7 +3140,6 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 		for _, tc := range testCases {
 			s.Run(tc.name, func() {
 				h := newServiceTestHelper()
-				defer h.assertExpectations(s.T())
 
 				h.mockRepository.EXPECT().
 					GetByID(h.ctxMatcher, validApprovalActionParam.AppealID).
@@ -3171,10 +3182,12 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 					Return(nil).Once()
 
 				actualResult, actualError := h.service.UpdateApproval(context.Background(), tc.expectedApprovalAction)
-
 				s.NoError(actualError)
 				tc.expectedResult.Policy = actualResult.Policy
 				s.Equal(tc.expectedResult, actualResult)
+
+				time.Sleep(time.Millisecond)
+				h.assertExpectations(s.T())
 			})
 		}
 	})
@@ -3305,8 +3318,6 @@ func (s *ServiceTestSuite) TestCancel() {
 
 func (s *ServiceTestSuite) TestAddApprover() {
 	s.Run("should return appeal on success", func() {
-		h := newServiceTestHelper()
-		defer h.assertExpectations(s.T())
 		appealID := uuid.New().String()
 		approvalID := uuid.New().String()
 		approvalName := "test-approval-name"
@@ -3327,6 +3338,7 @@ func (s *ServiceTestSuite) TestAddApprover() {
 
 		for _, tc := range testCases {
 			s.Run(tc.name, func() {
+				h := newServiceTestHelper()
 				expectedAppeal := &domain.Appeal{
 					ID:     appealID,
 					Status: domain.AppealStatusPending,
@@ -3369,10 +3381,10 @@ func (s *ServiceTestSuite) TestAddApprover() {
 				h.mockNotifier.EXPECT().
 					Notify(h.ctxMatcher, mock.Anything).
 					Run(func(ctx context.Context, notifications []domain.Notification) {
-						s.Len(notifications, 1)
+						assert.Equal(s.T(), len(notifications), 1)
 						n := notifications[0]
-						s.Equal(tc.newApprover, n.User)
-						s.Equal(domain.NotificationTypeApproverNotification, n.Message.Type)
+						assert.Equal(s.T(), tc.newApprover, n.User)
+						assert.Equal(s.T(), domain.NotificationTypeApproverNotification, n.Message.Type)
 					}).
 					Return(nil).Once()
 
@@ -3381,6 +3393,8 @@ func (s *ServiceTestSuite) TestAddApprover() {
 				s.NoError(actualError)
 				s.Equal(expectedApproval, actualAppeal.Approvals[0])
 
+				time.Sleep(time.Millisecond)
+				h.assertExpectations(s.T())
 			})
 		}
 	})