From d687080cf4896d319e7e6d59f8bb9ab70ef19e34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ga=C4=87e=C5=A1a?= Date: Thu, 25 Jul 2024 09:55:41 +0000 Subject: [PATCH] pr reviewer removal (#2269) * add reviewer principal ID to activity payload * pr reviewer removal --- app/api/controller/pullreq/controller.go | 15 +++- app/api/controller/pullreq/merge.go | 10 ++- app/api/controller/pullreq/pr_state.go | 2 +- app/api/controller/pullreq/pr_update.go | 2 +- app/api/controller/pullreq/review_submit.go | 15 +++- app/api/controller/pullreq/reviewer_add.go | 4 + app/api/controller/pullreq/reviewer_delete.go | 77 +++++++++++++++++-- app/services/pullreq/handlers_branch.go | 6 +- app/store/database.go | 6 +- app/store/database/pullreq_activity.go | 3 +- types/enum/pullreq.go | 18 +++-- types/pullreq_activity_payload.go | 10 +++ 12 files changed, 138 insertions(+), 30 deletions(-) diff --git a/app/api/controller/pullreq/controller.go b/app/api/controller/pullreq/controller.go index 9a36c50a9..b53ec605f 100644 --- a/app/api/controller/pullreq/controller.go +++ b/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, - session *auth.Session, repoRef string, reqPermission enum.Permission, -) (*types.Repository, error) { +func (c *Controller) getRepo(ctx context.Context, repoRef string) (*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) } diff --git a/app/api/controller/pullreq/merge.go b/app/api/controller/pullreq/merge.go index e96912c21..1bc5c2390 100644 --- a/app/api/controller/pullreq/merge.go +++ b/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, - &types.PullRequestActivityPayloadBranchDelete{SHA: in.SourceSHA}); errAct != nil { + if _, errAct := c.activityStore.CreateWithPayload(ctx, pr, mergedBy, + &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") diff --git a/app/api/controller/pullreq/pr_state.go b/app/api/controller/pullreq/pr_state.go index fa1b85541..0307e284c 100644 --- a/app/api/controller/pullreq/pr_state.go +++ b/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") } diff --git a/app/api/controller/pullreq/pr_update.go b/app/api/controller/pullreq/pr_update.go index 1f16fb2cd..45f1ef8b2 100644 --- a/app/api/controller/pullreq/pr_update.go +++ b/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") } diff --git a/app/api/controller/pullreq/review_submit.go b/app/api/controller/pullreq/review_submit.go index 7742c39a1..a3738201d 100644 --- a/app/api/controller/pullreq/review_submit.go +++ b/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, - pr *types.PullReq, review *types.PullReqReview, sha string) (*types.PullReqReviewer, error) { +func (c *Controller) updateReviewer( + 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 diff --git a/app/api/controller/pullreq/reviewer_add.go b/app/api/controller/pullreq/reviewer_add.go index bcd93c74d..9237924b5 100644 --- a/app/api/controller/pullreq/reviewer_add.go +++ b/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.") } diff --git a/app/api/controller/pullreq/reviewer_delete.go b/app/api/controller/pullreq/reviewer_delete.go index f31173fe1..f1be15ea3 100644 --- a/app/api/controller/pullreq/reviewer_delete.go +++ b/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. -func (c *Controller) ReviewerDelete(ctx context.Context, session *auth.Session, - repoRef string, prNum, reviewerID int64) error { - repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoEdit) +// ReviewerDelete deletes reviewer from the reviewer list for the given PR. +func (c *Controller) ReviewerDelete( + ctx context.Context, + 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 } diff --git a/app/services/pullreq/handlers_branch.go b/app/services/pullreq/handlers_branch.go index 6d6f5b44e..14f017415 100644 --- a/app/services/pullreq/handlers_branch.go +++ b/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", diff --git a/app/store/database.go b/app/store/database.go index 3eb1b9ff9..c1330b04b 100644 --- a/app/store/database.go +++ b/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 diff --git a/app/store/database/pullreq_activity.go b/app/store/database/pullreq_activity.go index 50dab106d..c5c288869 100644 --- a/app/store/database/pullreq_activity.go +++ b/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) diff --git a/types/enum/pullreq.go b/types/enum/pullreq.go index 39da208b0..311b46e96 100644 --- a/types/enum/pullreq.go +++ b/types/enum/pullreq.go @@ -78,14 +78,15 @@ func GetAllPullReqActivityTypes() ([]PullReqActivityType, PullReqActivityType) { // PullReqActivityType enumeration. const ( - PullReqActivityTypeComment PullReqActivityType = "comment" - PullReqActivityTypeCodeComment PullReqActivityType = "code-comment" - PullReqActivityTypeTitleChange PullReqActivityType = "title-change" - PullReqActivityTypeStateChange PullReqActivityType = "state-change" - PullReqActivityTypeReviewSubmit PullReqActivityType = "review-submit" - PullReqActivityTypeBranchUpdate PullReqActivityType = "branch-update" - PullReqActivityTypeBranchDelete PullReqActivityType = "branch-delete" - PullReqActivityTypeMerge PullReqActivityType = "merge" + PullReqActivityTypeComment PullReqActivityType = "comment" + PullReqActivityTypeCodeComment PullReqActivityType = "code-comment" + PullReqActivityTypeTitleChange PullReqActivityType = "title-change" + PullReqActivityTypeStateChange PullReqActivityType = "state-change" + PullReqActivityTypeReviewSubmit PullReqActivityType = "review-submit" + PullReqActivityTypeReviewerDelete PullReqActivityType = "reviewer-delete" + PullReqActivityTypeBranchUpdate PullReqActivityType = "branch-update" + 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, diff --git a/types/pullreq_activity_payload.go b/types/pullreq_activity_payload.go index d0b1bbc3a..00913e7b2 100644 --- a/types/pullreq_activity_payload.go +++ b/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"`