pr reviewer removal (#2269)

* add reviewer principal ID to activity payload
* pr reviewer removal

app/api/controller/pullreq/controller.go

@@ -145,9 +145,7 @@ func (c *Controller) verifyBranchExistence(ctx context.Context,
 	return ref.SHA, nil
 }
 
-func (c *Controller) getRepoCheckAccess(ctx context.Context,
+func (c *Controller) getRepo(ctx context.Context, repoRef string) (*types.Repository, error) {
-	session *auth.Session, repoRef string, reqPermission enum.Permission,
-) (*types.Repository, error) {
 	if repoRef == "" {
 		return nil, usererror.BadRequest("A valid repository reference must be provided.")
 	}
@@ -161,6 +159,17 @@ func (c *Controller) getRepoCheckAccess(ctx context.Context,
 		return nil, usererror.BadRequest("Repository is not ready to use.")
 	}
 
+	return repo, nil
+}
+
+func (c *Controller) getRepoCheckAccess(ctx context.Context,
+	session *auth.Session, repoRef string, reqPermission enum.Permission,
+) (*types.Repository, error) {
+	repo, err := c.getRepo(ctx, repoRef)
+	if err != nil {
+		return nil, err
+	}
+
 	if err = apiauth.CheckRepo(ctx, c.authorizer, session, repo, reqPermission); err != nil {
 		return nil, fmt.Errorf("access check failed: %w", err)
 	}

app/api/controller/pullreq/merge.go

@@ -408,13 +408,15 @@ func (c *Controller) Merge(
 
 	log.Ctx(ctx).Debug().Msgf("successfully merged PR")
 
+	mergedBy := session.Principal.ID
+
 	var activitySeqMerge, activitySeqBranchDeleted int64
 	pr, err = c.pullreqStore.UpdateOptLock(ctx, pr, func(pr *types.PullReq) error {
 		pr.State = enum.PullReqStateMerged
 
 		nowMilli := now.UnixMilli()
 		pr.Merged = &nowMilli
-		pr.MergedBy = &session.Principal.ID
+		pr.MergedBy = &mergedBy
 		pr.MergeMethod = &in.Method
 
 		// update all Merge specific information (might be empty if previous merge check failed)
@@ -455,7 +457,7 @@ func (c *Controller) Merge(
 		SourceSHA:     mergeOutput.HeadSHA.String(),
 		RulesBypassed: protection.IsBypassed(violations),
 	}
-	if _, errAct := c.activityStore.CreateWithPayload(ctx, pr, session.Principal.ID, activityPayload); errAct != nil {
+	if _, errAct := c.activityStore.CreateWithPayload(ctx, pr, mergedBy, activityPayload, nil); errAct != nil {
 		// non-critical error
 		log.Ctx(ctx).Err(errAct).Msgf("failed to write pull req merge activity")
 	}
@@ -483,8 +485,8 @@ func (c *Controller) Merge(
 			// NOTE: there is a chance someone pushed on the branch between merge and delete.
 			// Either way, we'll use the SHA that was merged with for the activity to be consistent from PR perspective.
 			pr.ActivitySeq = activitySeqBranchDeleted
-			if _, errAct := c.activityStore.CreateWithPayload(ctx, pr, session.Principal.ID,
+			if _, errAct := c.activityStore.CreateWithPayload(ctx, pr, mergedBy,
-				&types.PullRequestActivityPayloadBranchDelete{SHA: in.SourceSHA}); errAct != nil {
+				&types.PullRequestActivityPayloadBranchDelete{SHA: in.SourceSHA}, nil); errAct != nil {
 				// non-critical error
 				log.Ctx(ctx).Err(errAct).
 					Msgf("failed to write pull request activity for successful automatic branch delete")

app/api/controller/pullreq/pr_state.go

@@ -169,7 +169,7 @@ func (c *Controller) State(ctx context.Context,
 		OldDraft: oldDraft,
 		NewDraft: pr.IsDraft,
 	}
-	if _, errAct := c.activityStore.CreateWithPayload(ctx, pr, session.Principal.ID, payload); errAct != nil {
+	if _, errAct := c.activityStore.CreateWithPayload(ctx, pr, session.Principal.ID, payload, nil); errAct != nil {
 		// non-critical error
 		log.Ctx(ctx).Err(errAct).Msgf("failed to write pull request activity after state change")
 	}

app/api/controller/pullreq/pr_update.go

@@ -104,7 +104,7 @@ func (c *Controller) Update(ctx context.Context,
 			Old: oldTitle,
 			New: pr.Title,
 		}
-		if _, errAct := c.activityStore.CreateWithPayload(ctx, pr, session.Principal.ID, payload); errAct != nil {
+		if _, errAct := c.activityStore.CreateWithPayload(ctx, pr, session.Principal.ID, payload, nil); errAct != nil {
 			// non-critical error
 			log.Ctx(ctx).Err(errAct).Msgf("failed to write pull request activity after title change")
 		}

app/api/controller/pullreq/review_submit.go

@@ -76,6 +76,10 @@ func (c *Controller) ReviewSubmit(
 		return nil, fmt.Errorf("failed to find pull request by number: %w", err)
 	}
 
+	if pr.Merged != nil {
+		return nil, usererror.BadRequest("Can't submit a review for merged pull requests")
+	}
+
 	if pr.CreatedBy == session.Principal.ID {
 		return nil, usererror.BadRequest("Can't submit review to own pull requests.")
 	}
@@ -130,7 +134,7 @@ func (c *Controller) ReviewSubmit(
 			CommitSHA: commitSHA.String(),
 			Decision:  in.Decision,
 		}
-		_, err = c.activityStore.CreateWithPayload(ctx, pr, session.Principal.ID, payload)
+		_, err = c.activityStore.CreateWithPayload(ctx, pr, session.Principal.ID, payload, nil)
 		return err
 	}()
 	if err != nil {
@@ -142,8 +146,13 @@ func (c *Controller) ReviewSubmit(
 }
 
 // updateReviewer updates pull request reviewer object.
-func (c *Controller) updateReviewer(ctx context.Context, session *auth.Session,
+func (c *Controller) updateReviewer(
-	pr *types.PullReq, review *types.PullReqReview, sha string) (*types.PullReqReviewer, error) {
+	ctx context.Context,
+	session *auth.Session,
+	pr *types.PullReq,
+	review *types.PullReqReview,
+	sha string,
+) (*types.PullReqReviewer, error) {
 	reviewer, err := c.reviewerStore.Find(ctx, pr.ID, session.Principal.ID)
 	if err != nil && !errors.Is(err, store.ErrResourceNotFound) {
 		return nil, err

app/api/controller/pullreq/reviewer_add.go

@@ -53,6 +53,10 @@ func (c *Controller) ReviewerAdd(
 		return nil, fmt.Errorf("failed to find pull request by number: %w", err)
 	}
 
+	if pr.Merged != nil {
+		return nil, usererror.BadRequest("Can't request review for merged pull request")
+	}
+
 	if in.ReviewerID == 0 {
 		return nil, usererror.BadRequest("Must specify reviewer ID.")
 	}

app/api/controller/pullreq/reviewer_delete.go

@@ -18,16 +18,26 @@ import (
 	"context"
 	"fmt"
 
+	apiauth "github.com/harness/gitness/app/api/auth"
+	"github.com/harness/gitness/app/api/usererror"
 	"github.com/harness/gitness/app/auth"
+	"github.com/harness/gitness/types"
 	"github.com/harness/gitness/types/enum"
+
+	"github.com/rs/zerolog/log"
 )
 
-// ReviewerDelete deletes reviewer from the reviewerlist for the given PR.
+// ReviewerDelete deletes reviewer from the reviewer list for the given PR.
-func (c *Controller) ReviewerDelete(ctx context.Context, session *auth.Session,
+func (c *Controller) ReviewerDelete(
-	repoRef string, prNum, reviewerID int64) error {
+	ctx context.Context,
-	repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoEdit)
+	session *auth.Session,
+	repoRef string,
+	prNum int64,
+	reviewerID int64,
+) error {
+	repo, err := c.getRepo(ctx, repoRef)
 	if err != nil {
-		return fmt.Errorf("failed to acquire access to repo: %w", err)
+		return fmt.Errorf("failed to find repository: %w", err)
 	}
 
 	pr, err := c.pullreqStore.FindByNumber(ctx, repo.ID, prNum)
@@ -35,9 +45,66 @@ func (c *Controller) ReviewerDelete(ctx context.Context, session *auth.Session,
 		return fmt.Errorf("failed to find pull request: %w", err)
 	}
 
+	reviewer, err := c.reviewerStore.Find(ctx, pr.ID, reviewerID)
+	if err != nil {
+		return fmt.Errorf("failed to find reviewer: %w", err)
+	}
+
+	var reqPermission enum.Permission
+	switch {
+	case session.Principal.ID == reviewer.PrincipalID:
+		reqPermission = enum.PermissionRepoView // Anybody should be allowed to remove their own reviews.
+	case reviewer.ReviewDecision == enum.PullReqReviewDecisionPending:
+		reqPermission = enum.PermissionRepoPush // The reviewer was asked for a review but didn't submit it yet.
+	default:
+		reqPermission = enum.PermissionRepoEdit // RepoEdit permission is required to remove a submitted review.
+	}
+
+	// Make sure the caller has the right permission even if the PR is merged, so that we can return the correct error.
+	if err = apiauth.CheckRepo(ctx, c.authorizer, session, repo, reqPermission); err != nil {
+		return fmt.Errorf("access check failed: %w", err)
+	}
+
+	if pr.Merged != nil {
+		return usererror.BadRequest("Pull request is already merged")
+	}
+
 	err = c.reviewerStore.Delete(ctx, pr.ID, reviewerID)
 	if err != nil {
 		return fmt.Errorf("failed to delete reviewer: %w", err)
 	}
+
+	if reviewer.ReviewDecision == enum.PullReqReviewDecisionPending {
+		// We create a pull request activity entry only if a review has actually been submitted.
+		return nil
+	}
+
+	activityPayload := &types.PullRequestActivityPayloadReviewerDelete{
+		CommitSHA:   reviewer.SHA,
+		Decision:    reviewer.ReviewDecision,
+		PrincipalID: reviewer.PrincipalID,
+	}
+
+	metadata := &types.PullReqActivityMetadata{
+		Mentions: &types.PullReqActivityMentionsMetadata{IDs: []int64{reviewer.PrincipalID}},
+	}
+
+	err = func() error {
+		if pr, err = c.pullreqStore.UpdateActivitySeq(ctx, pr); err != nil {
+			return fmt.Errorf("failed to increment pull request activity sequence: %w", err)
+		}
+
+		_, err = c.activityStore.CreateWithPayload(ctx, pr, session.Principal.ID, activityPayload, metadata)
+		if err != nil {
+			return fmt.Errorf("failed to create pull request activity: %w", err)
+		}
+
+		return nil
+	}()
+	if err != nil {
+		// non-critical error
+		log.Ctx(ctx).Err(err).Msgf("failed to write pull request activity after reviewer removal")
+	}
+
 	return nil
 }

app/services/pullreq/handlers_branch.go

@@ -120,7 +120,7 @@ func (s *Service) triggerPREventOnBranchUpdate(ctx context.Context,
 			New: event.Payload.NewSHA,
 		}
 
-		_, err = s.activityStore.CreateWithPayload(ctx, pr, event.Payload.PrincipalID, payload)
+		_, err = s.activityStore.CreateWithPayload(ctx, pr, event.Payload.PrincipalID, payload, nil)
 		if err != nil {
 			// non-critical error
 			log.Ctx(ctx).Err(err).Msgf("failed to write pull request activity after branch update")
@@ -192,7 +192,7 @@ func (s *Service) closePullReqOnBranchDelete(ctx context.Context,
 		// Whatever is the source sha of the PR is most likely to be pointed at by the PR head ref.
 		pr.ActivitySeq = activitySeqBranchDeleted
 		_, err = s.activityStore.CreateWithPayload(ctx, pr, event.Payload.PrincipalID,
-			&types.PullRequestActivityPayloadBranchDelete{SHA: pr.SourceSHA})
+			&types.PullRequestActivityPayloadBranchDelete{SHA: pr.SourceSHA}, nil)
 		if err != nil {
 			// non-critical error
 			log.Ctx(ctx).Err(err).Msg("failed to write pull request activity for branch deletion")
@@ -205,7 +205,7 @@ func (s *Service) closePullReqOnBranchDelete(ctx context.Context,
 			OldDraft: pr.IsDraft,
 			NewDraft: pr.IsDraft,
 		}
-		if _, err := s.activityStore.CreateWithPayload(ctx, pr, event.Payload.PrincipalID, payload); err != nil {
+		if _, err := s.activityStore.CreateWithPayload(ctx, pr, event.Payload.PrincipalID, payload, nil); err != nil {
 			// non-critical error
 			log.Ctx(ctx).Err(err).Msg(
 				"failed to write pull request activity for pullrequest closure after branch deletion",

app/store/database.go

@@ -372,7 +372,11 @@ type (
 
 		// CreateWithPayload create a new system activity from the provided payload.
 		CreateWithPayload(ctx context.Context,
-			pr *types.PullReq, principalID int64, payload types.PullReqActivityPayload) (*types.PullReqActivity, error)
+			pr *types.PullReq,
+			principalID int64,
+			payload types.PullReqActivityPayload,
+			metadata *types.PullReqActivityMetadata,
+		) (*types.PullReqActivity, error)
 
 		// Update the pull request activity. It will set new values to the Version and Updated fields.
 		Update(ctx context.Context, act *types.PullReqActivity) error

app/store/database/pullreq_activity.go

@@ -230,7 +230,7 @@ func (s *PullReqActivityStore) Create(ctx context.Context, act *types.PullReqAct
 }
 
 func (s *PullReqActivityStore) CreateWithPayload(ctx context.Context,
-	pr *types.PullReq, principalID int64, payload types.PullReqActivityPayload,
+	pr *types.PullReq, principalID int64, payload types.PullReqActivityPayload, metadata *types.PullReqActivityMetadata,
 ) (*types.PullReqActivity, error) {
 	now := time.Now().UnixMilli()
 	act := &types.PullReqActivity{
@@ -246,6 +246,7 @@ func (s *PullReqActivityStore) CreateWithPayload(ctx context.Context,
 		Type:      payload.ActivityType(),
 		Kind:      enum.PullReqActivityKindSystem,
 		Text:      "",
+		Metadata:  metadata,
 	}
 
 	_ = act.SetPayload(payload)

types/enum/pullreq.go

@@ -78,14 +78,15 @@ func GetAllPullReqActivityTypes() ([]PullReqActivityType, PullReqActivityType) {
 
 // PullReqActivityType enumeration.
 const (
-	PullReqActivityTypeComment      PullReqActivityType = "comment"
+	PullReqActivityTypeComment        PullReqActivityType = "comment"
-	PullReqActivityTypeCodeComment  PullReqActivityType = "code-comment"
+	PullReqActivityTypeCodeComment    PullReqActivityType = "code-comment"
-	PullReqActivityTypeTitleChange  PullReqActivityType = "title-change"
+	PullReqActivityTypeTitleChange    PullReqActivityType = "title-change"
-	PullReqActivityTypeStateChange  PullReqActivityType = "state-change"
+	PullReqActivityTypeStateChange    PullReqActivityType = "state-change"
-	PullReqActivityTypeReviewSubmit PullReqActivityType = "review-submit"
+	PullReqActivityTypeReviewSubmit   PullReqActivityType = "review-submit"
-	PullReqActivityTypeBranchUpdate PullReqActivityType = "branch-update"
+	PullReqActivityTypeReviewerDelete PullReqActivityType = "reviewer-delete"
-	PullReqActivityTypeBranchDelete PullReqActivityType = "branch-delete"
+	PullReqActivityTypeBranchUpdate   PullReqActivityType = "branch-update"
-	PullReqActivityTypeMerge        PullReqActivityType = "merge"
+	PullReqActivityTypeBranchDelete   PullReqActivityType = "branch-delete"
+	PullReqActivityTypeMerge          PullReqActivityType = "merge"
 )
 
 var pullReqActivityTypes = sortEnum([]PullReqActivityType{
@@ -94,6 +95,7 @@ var pullReqActivityTypes = sortEnum([]PullReqActivityType{
 	PullReqActivityTypeTitleChange,
 	PullReqActivityTypeStateChange,
 	PullReqActivityTypeReviewSubmit,
+	PullReqActivityTypeReviewerDelete,
 	PullReqActivityTypeBranchUpdate,
 	PullReqActivityTypeBranchDelete,
 	PullReqActivityTypeMerge,

types/pullreq_activity_payload.go

@@ -130,6 +130,16 @@ func (a *PullRequestActivityPayloadReviewSubmit) ActivityType() enum.PullReqActi
 	return enum.PullReqActivityTypeReviewSubmit
 }
 
+type PullRequestActivityPayloadReviewerDelete struct {
+	CommitSHA   string                     `json:"commit_sha"`
+	Decision    enum.PullReqReviewDecision `json:"decision"`
+	PrincipalID int64                      `json:"principal_id"`
+}
+
+func (a *PullRequestActivityPayloadReviewerDelete) ActivityType() enum.PullReqActivityType {
+	return enum.PullReqActivityTypeReviewerDelete
+}
+
 type PullRequestActivityPayloadBranchUpdate struct {
 	Old string `json:"old"`
 	New string `json:"new"`