diff --git a/app/api/controller/pullreq/merge.go b/app/api/controller/pullreq/merge.go index ce4ce3f2b..4d5a56123 100644 --- a/app/api/controller/pullreq/merge.go +++ b/app/api/controller/pullreq/merge.go @@ -225,6 +225,14 @@ func (c *Controller) Merge( } if in.DryRunRules { + for _, approvals := range ruleOut.DefaultReviewerApprovals { + principalInfos, err := c.principalInfoCache.Map(ctx, approvals.PrincipalIDs) + if err != nil { + return nil, nil, fmt.Errorf("failed to fetch principal infos from info cache: %w", err) + } + approvals.PrincipalInfos = maps.Values(principalInfos) + } + return &types.MergeResponse{ BranchDeleted: ruleOut.DeleteSourceBranch, RuleViolations: violations, diff --git a/app/services/protection/set.go b/app/services/protection/set.go index e951870da..6f2e5a9b2 100644 --- a/app/services/protection/set.go +++ b/app/services/protection/set.go @@ -54,21 +54,11 @@ func (s ruleSet) MergeVerify( out.DeleteSourceBranch = out.DeleteSourceBranch || rOut.DeleteSourceBranch out.MinimumRequiredApprovalsCount = maxInt(out.MinimumRequiredApprovalsCount, rOut.MinimumRequiredApprovalsCount) out.MinimumRequiredApprovalsCountLatest = maxInt(out.MinimumRequiredApprovalsCountLatest, rOut.MinimumRequiredApprovalsCountLatest) //nolint:lll - out.DefaultReviewerIDs = append(out.DefaultReviewerIDs, rOut.DefaultReviewerIDs...) out.RequiresCodeOwnersApproval = out.RequiresCodeOwnersApproval || rOut.RequiresCodeOwnersApproval out.RequiresCodeOwnersApprovalLatest = out.RequiresCodeOwnersApprovalLatest || rOut.RequiresCodeOwnersApprovalLatest out.RequiresCommentResolution = out.RequiresCommentResolution || rOut.RequiresCommentResolution out.RequiresNoChangeRequests = out.RequiresNoChangeRequests || rOut.RequiresNoChangeRequests - - if len(out.DefaultReviewerIDs) > 0 { - out.DefaultReviewerApprovals = append(out.DefaultReviewerApprovals, &types.DefaultReviewerApprovalsResponse{ - RuleInfo: r.RuleInfo, - MinimumRequiredCount: rOut.MinimumRequiredDefaultReviewerApprovalsCount, - MinimumRequiredCountLatest: rOut.MinimumRequiredDefaultReviewerApprovalsCountLatest, - PrincipalIDs: rOut.DefaultReviewerIDs, - CurrentCount: rOut.DefaultReviewerApprovalsCount, - }) - } + out.DefaultReviewerApprovals = append(out.DefaultReviewerApprovals, rOut.DefaultReviewerApprovals...) return nil }) diff --git a/app/services/protection/verify_pullreq.go b/app/services/protection/verify_pullreq.go index 1ae1d2169..6a2cac757 100644 --- a/app/services/protection/verify_pullreq.go +++ b/app/services/protection/verify_pullreq.go @@ -49,19 +49,15 @@ type ( } MergeVerifyOutput struct { - AllowedMethods []enum.MergeMethod - DeleteSourceBranch bool - MinimumRequiredApprovalsCount int - MinimumRequiredApprovalsCountLatest int - MinimumRequiredDefaultReviewerApprovalsCount int - MinimumRequiredDefaultReviewerApprovalsCountLatest int - RequiresCodeOwnersApproval bool - RequiresCodeOwnersApprovalLatest bool - RequiresCommentResolution bool - RequiresNoChangeRequests bool - DefaultReviewerIDs []int64 - DefaultReviewerApprovalsCount int - DefaultReviewerApprovals []*types.DefaultReviewerApprovalsResponse + AllowedMethods []enum.MergeMethod + DeleteSourceBranch bool + MinimumRequiredApprovalsCount int + MinimumRequiredApprovalsCountLatest int + RequiresCodeOwnersApproval bool + RequiresCodeOwnersApprovalLatest bool + RequiresCommentResolution bool + RequiresNoChangeRequests bool + DefaultReviewerApprovals []*types.DefaultReviewerApprovalsResponse } RequiredChecksInput struct { @@ -109,9 +105,8 @@ var ( const ( codePullReqApprovalReqMinCount = "pullreq.approvals.require_minimum_count" codePullReqApprovalReqMinCountLatest = "pullreq.approvals.require_minimum_count:latest_commit" - codePullReqDefaultReviewerApprovalReqMinCount = "pullreq.approvals.require_default_reviewer_minimum_count" - codePullReqDefaultReviewerApprovalReqMinCountLatest = "" + - "pullreq.approvals.require_default_reviewer_minimum_count:latest_commit" + codePullReqApprovalReqDefaultReviewerMinCount = "pullreq.approvals.require_default_reviewer_minimum_count" + codePullReqApprovalReqDefaultReviewerMinCountLatest = "pullreq.approvals.require_default_reviewer_minimum_count:latest_commit" //nolint:lll codePullReqApprovalReqLatestCommit = "pullreq.approvals.require_latest_commit" codePullReqApprovalReqChangeRequested = "pullreq.approvals.require_change_requested" @@ -146,23 +141,21 @@ func (v *DefPullReq) MergeVerify( if v.Approvals.RequireLatestCommit { out.RequiresCodeOwnersApprovalLatest = v.Approvals.RequireCodeOwners out.MinimumRequiredApprovalsCountLatest = v.Approvals.RequireMinimumCount - out.MinimumRequiredDefaultReviewerApprovalsCountLatest = v.Approvals.RequireMinimumDefaultReviewerCount } else { out.RequiresCodeOwnersApproval = v.Approvals.RequireCodeOwners out.MinimumRequiredApprovalsCount = v.Approvals.RequireMinimumCount - out.MinimumRequiredDefaultReviewerApprovalsCount = v.Approvals.RequireMinimumDefaultReviewerCount } // pullreq.approvals - approvedBy := make([]types.PrincipalInfo, 0, len(in.Reviewers)) + approvedBy := make(map[int64]struct{}) for _, reviewer := range in.Reviewers { switch reviewer.ReviewDecision { case enum.PullReqReviewDecisionApproved: if v.Approvals.RequireLatestCommit && reviewer.SHA != in.PullReq.SourceSHA { continue } - approvedBy = append(approvedBy, reviewer.Reviewer) + approvedBy[reviewer.Reviewer.ID] = struct{}{} case enum.PullReqReviewDecisionChangeReq: if v.Approvals.RequireNoChangeRequest { if reviewer.SHA == in.PullReq.SourceSHA { @@ -196,29 +189,51 @@ func (v *DefPullReq) MergeVerify( } } - defaultReviewerMap := make(map[int64]struct{}) - for _, approver := range approvedBy { - defaultReviewerMap[approver.ID] = struct{}{} - } - var defaultReviewersCount int + effectiveDefaultReviewerIDs := make([]int64, 0, len(v.Reviewers.DefaultReviewerIDs)) for _, id := range v.Reviewers.DefaultReviewerIDs { - if _, ok := defaultReviewerMap[id]; ok { - defaultReviewersCount++ + if id == in.PullReq.Author.ID { + continue } + effectiveDefaultReviewerIDs = append(effectiveDefaultReviewerIDs, id) } - if defaultReviewersCount < v.Approvals.RequireMinimumDefaultReviewerCount { + + // if author is default reviewer and required minimum == number of default reviewers, reduce minimum by one. + effectiveMinimumRequiredDefaultReviewerCount := v.Approvals.RequireMinimumDefaultReviewerCount + if len(effectiveDefaultReviewerIDs) < len(v.Reviewers.DefaultReviewerIDs) && + len(v.Reviewers.DefaultReviewerIDs) == v.Approvals.RequireMinimumDefaultReviewerCount { + effectiveMinimumRequiredDefaultReviewerCount-- + } + + //nolint:nestif + if effectiveMinimumRequiredDefaultReviewerCount > 0 { + var defaultReviewerApprovalCount int + for _, id := range effectiveDefaultReviewerIDs { + if _, ok := approvedBy[id]; ok { + defaultReviewerApprovalCount++ + } + } + if defaultReviewerApprovalCount < effectiveMinimumRequiredDefaultReviewerCount { + if v.Approvals.RequireLatestCommit { + violations.Addf(codePullReqApprovalReqDefaultReviewerMinCountLatest, + "Insufficient number of default reviewer approvals of the latest commit. Have %d but need at least %d.", + defaultReviewerApprovalCount, effectiveMinimumRequiredDefaultReviewerCount) + } else { + violations.Addf(codePullReqApprovalReqDefaultReviewerMinCount, + "Insufficient number of default reviewer approvals. Have %d but need at least %d.", + defaultReviewerApprovalCount, effectiveMinimumRequiredDefaultReviewerCount) + } + } + + out.DefaultReviewerApprovals = []*types.DefaultReviewerApprovalsResponse{{ + PrincipalIDs: effectiveDefaultReviewerIDs, + CurrentCount: defaultReviewerApprovalCount, + }} if v.Approvals.RequireLatestCommit { - violations.Addf(codePullReqDefaultReviewerApprovalReqMinCount, - "Insufficient number of default reviewer approvals of the latest commit. Have %d but need at least %d.", - defaultReviewersCount, v.Approvals.RequireMinimumDefaultReviewerCount) + out.DefaultReviewerApprovals[0].MinimumRequiredCountLatest = effectiveMinimumRequiredDefaultReviewerCount } else { - violations.Addf(codePullReqDefaultReviewerApprovalReqMinCountLatest, - "Insufficient number of default reviewer approvals. Have %d but need at least %d.", - defaultReviewersCount, v.Approvals.RequireMinimumDefaultReviewerCount) + out.DefaultReviewerApprovals[0].MinimumRequiredCount = effectiveMinimumRequiredDefaultReviewerCount } } - out.DefaultReviewerIDs = v.Reviewers.DefaultReviewerIDs - out.DefaultReviewerApprovalsCount = defaultReviewersCount if v.Approvals.RequireCodeOwners { for _, entry := range in.CodeOwners.EvaluationEntries { diff --git a/app/services/protection/verify_pullreq_test.go b/app/services/protection/verify_pullreq_test.go index dd02ddb47..401f60968 100644 --- a/app/services/protection/verify_pullreq_test.go +++ b/app/services/protection/verify_pullreq_test.go @@ -24,6 +24,12 @@ import ( "github.com/harness/gitness/types/enum" ) +var ( + reviewer1 = types.PrincipalInfo{ID: 1, DisplayName: "Reviewer 1", UID: "reviewer-1"} + reviewer2 = types.PrincipalInfo{ID: 2, DisplayName: "Reviewer 2", UID: "reviewer-2"} + reviewer3 = types.PrincipalInfo{ID: 3, DisplayName: "Reviewer 3", UID: "reviewer-3"} +) + // nolint:gocognit // it's a unit test func TestDefPullReq_MergeVerify(t *testing.T) { tests := []struct { @@ -58,7 +64,7 @@ func TestDefPullReq_MergeVerify(t *testing.T) { in: MergeVerifyInput{ PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, Reviewers: []*types.PullReqReviewer{ - {ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc"}, + {ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc", Reviewer: reviewer1}, }, Method: enum.MergeMethodMerge, }, @@ -75,8 +81,8 @@ func TestDefPullReq_MergeVerify(t *testing.T) { in: MergeVerifyInput{ PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, Reviewers: []*types.PullReqReviewer{ - {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, - {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer1}, + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer2}, }, Method: enum.MergeMethodMerge, }, @@ -109,9 +115,9 @@ func TestDefPullReq_MergeVerify(t *testing.T) { in: MergeVerifyInput{ PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, Reviewers: []*types.PullReqReviewer{ - {ReviewDecision: enum.PullReqReviewDecisionPending, SHA: "abc"}, - {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, - {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, + {ReviewDecision: enum.PullReqReviewDecisionPending, SHA: "abc", Reviewer: reviewer1}, + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer2}, + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer3}, }, Method: enum.MergeMethodMerge, }, @@ -120,6 +126,252 @@ func TestDefPullReq_MergeVerify(t *testing.T) { MinimumRequiredApprovalsCountLatest: 2, }, }, + { + name: codePullReqApprovalReqDefaultReviewerMinCount + "-fail", + def: DefPullReq{ + Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 1}, + Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID}}, + }, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, + Reviewers: []*types.PullReqReviewer{ + {ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc", Reviewer: reviewer1}, + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer2}, + }, + Method: enum.MergeMethodMerge, + }, + expCodes: []string{codePullReqApprovalReqDefaultReviewerMinCount}, + expParams: [][]any{{0, 1}}, + expOut: MergeVerifyOutput{ + AllowedMethods: enum.MergeMethods, + DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{ + PrincipalIDs: []int64{reviewer1.ID}, + CurrentCount: 0, + MinimumRequiredCount: 1, + }}, + }, + }, + { + name: codePullReqApprovalReqDefaultReviewerMinCount + "-success", + def: DefPullReq{ + Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 1}, + Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID}}, + }, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, + Reviewers: []*types.PullReqReviewer{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer1}, + {ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc", Reviewer: reviewer2}, + }, + Method: enum.MergeMethodMerge, + }, + expOut: MergeVerifyOutput{ + AllowedMethods: enum.MergeMethods, + DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{ + PrincipalIDs: []int64{reviewer1.ID}, + CurrentCount: 1, + MinimumRequiredCount: 1, + }}, + }, + }, + { + name: codePullReqApprovalReqDefaultReviewerMinCount + "-with-author-count-1-exact", + def: DefPullReq{ + Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 1}, + Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID}}, + }, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc", Author: reviewer1}, + Reviewers: nil, + Method: enum.MergeMethodMerge, + }, + expOut: MergeVerifyOutput{ + AllowedMethods: enum.MergeMethods, + DefaultReviewerApprovals: nil, + }, + }, + { + name: codePullReqApprovalReqDefaultReviewerMinCount + "-with-author-count-1-more-fail", + def: DefPullReq{ + Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 1}, + Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID, reviewer2.ID}}, + }, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc", Author: reviewer1}, + Reviewers: nil, + Method: enum.MergeMethodMerge, + }, + expCodes: []string{codePullReqApprovalReqDefaultReviewerMinCount}, + expParams: [][]any{{0, 1}}, + expOut: MergeVerifyOutput{ + AllowedMethods: enum.MergeMethods, + DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{ + PrincipalIDs: []int64{reviewer2.ID}, + CurrentCount: 0, + MinimumRequiredCount: 1, + }}, + }, + }, + { + name: codePullReqApprovalReqDefaultReviewerMinCount + "-with-author-count-1-more-success", + def: DefPullReq{ + Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 1}, + Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID, reviewer2.ID}}, + }, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc", Author: reviewer1}, + Reviewers: []*types.PullReqReviewer{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer2}, + }, + Method: enum.MergeMethodMerge, + }, + expOut: MergeVerifyOutput{ + AllowedMethods: enum.MergeMethods, + DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{ + PrincipalIDs: []int64{reviewer2.ID}, + CurrentCount: 1, + MinimumRequiredCount: 1, + }}, + }, + }, + { + name: codePullReqApprovalReqDefaultReviewerMinCount + "-with-author-count-2-exact-fail", + def: DefPullReq{ + Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 2}, + Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID, reviewer2.ID}}, + }, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc", Author: reviewer1}, + Reviewers: []*types.PullReqReviewer{}, + Method: enum.MergeMethodMerge, + }, + expCodes: []string{codePullReqApprovalReqDefaultReviewerMinCount}, + expParams: [][]any{{0, 1}}, + expOut: MergeVerifyOutput{ + AllowedMethods: enum.MergeMethods, + DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{ + PrincipalIDs: []int64{reviewer2.ID}, + CurrentCount: 0, + MinimumRequiredCount: 1, + }}, + }, + }, + { + name: codePullReqApprovalReqDefaultReviewerMinCount + "-with-author-count-2-exact-success", + def: DefPullReq{ + Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 2}, + Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID, reviewer2.ID}}, + }, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc", Author: reviewer1}, + Reviewers: []*types.PullReqReviewer{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer2}, + }, + Method: enum.MergeMethodMerge, + }, + expOut: MergeVerifyOutput{ + AllowedMethods: enum.MergeMethods, + DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{ + PrincipalIDs: []int64{reviewer2.ID}, + CurrentCount: 1, + MinimumRequiredCount: 1, + }}, + }, + }, + { + name: codePullReqApprovalReqDefaultReviewerMinCount + "-with-author-count-2-more-fail", + def: DefPullReq{ + Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 2}, + Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID, reviewer2.ID, reviewer3.ID}}, + }, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc", Author: reviewer1}, + Reviewers: []*types.PullReqReviewer{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer2}, + {ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc", Reviewer: reviewer3}, + }, + Method: enum.MergeMethodMerge, + }, + expCodes: []string{codePullReqApprovalReqDefaultReviewerMinCount}, + expParams: [][]any{{1, 2}}, + expOut: MergeVerifyOutput{ + AllowedMethods: enum.MergeMethods, + DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{ + PrincipalIDs: []int64{reviewer2.ID, reviewer3.ID}, + CurrentCount: 1, + MinimumRequiredCount: 2, + }}, + }, + }, + { + name: codePullReqApprovalReqDefaultReviewerMinCount + "-with-author-count-2-more-success", + def: DefPullReq{ + Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 2}, + Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID, reviewer2.ID, reviewer3.ID}}, + }, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc", Author: reviewer1}, + Reviewers: []*types.PullReqReviewer{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer2}, + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer3}, + }, + Method: enum.MergeMethodMerge, + }, + expOut: MergeVerifyOutput{ + AllowedMethods: enum.MergeMethods, + DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{ + PrincipalIDs: []int64{reviewer2.ID, reviewer3.ID}, + CurrentCount: 2, + MinimumRequiredCount: 2, + }}, + }, + }, + { + name: codePullReqApprovalReqDefaultReviewerMinCountLatest + "-fail", + def: DefPullReq{ + Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 1, RequireLatestCommit: true}, + Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID}}, + }, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, + Reviewers: []*types.PullReqReviewer{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "def", Reviewer: reviewer1}, + }, + Method: enum.MergeMethodMerge, + }, + expCodes: []string{codePullReqApprovalReqDefaultReviewerMinCountLatest}, + expParams: [][]any{{0, 1}}, + expOut: MergeVerifyOutput{ + AllowedMethods: enum.MergeMethods, + DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{ + PrincipalIDs: []int64{reviewer1.ID}, + CurrentCount: 0, + MinimumRequiredCountLatest: 1, + }}, + }, + }, + { + name: codePullReqApprovalReqDefaultReviewerMinCountLatest + "-success", + def: DefPullReq{ + Approvals: DefApprovals{RequireMinimumDefaultReviewerCount: 1, RequireLatestCommit: true}, + Reviewers: DefReviewers{DefaultReviewerIDs: []int64{reviewer1.ID}}, + }, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, + Reviewers: []*types.PullReqReviewer{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc", Reviewer: reviewer1}, + }, + Method: enum.MergeMethodMerge, + }, + expOut: MergeVerifyOutput{ + AllowedMethods: enum.MergeMethods, + DefaultReviewerApprovals: []*types.DefaultReviewerApprovalsResponse{{ + PrincipalIDs: []int64{reviewer1.ID}, + CurrentCount: 1, + MinimumRequiredCountLatest: 1, + }}, + }, + }, { name: codePullReqApprovalReqCodeOwnersNoApproval + "-fail", def: DefPullReq{Approvals: DefApprovals{RequireCodeOwners: true}}, @@ -390,7 +642,7 @@ func TestDefPullReq_MergeVerify(t *testing.T) { in: MergeVerifyInput{ PullReq: &types.PullReq{SourceSHA: "abc"}, Reviewers: []*types.PullReqReviewer{ - {ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc"}, + {ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc", Reviewer: reviewer1}, }, Method: enum.MergeMethodMerge, }, @@ -408,14 +660,14 @@ func TestDefPullReq_MergeVerify(t *testing.T) { Reviewers: []*types.PullReqReviewer{ { ReviewDecision: enum.PullReqReviewDecisionChangeReq, - Reviewer: types.PrincipalInfo{DisplayName: "John"}, + Reviewer: reviewer1, SHA: "abc", }, }, Method: enum.MergeMethodMerge, }, expCodes: []string{codePullReqApprovalReqChangeRequested}, - expParams: [][]any{{"John"}}, + expParams: [][]any{{reviewer1.DisplayName}}, expOut: MergeVerifyOutput{ AllowedMethods: enum.MergeMethods, RequiresNoChangeRequests: true, @@ -431,14 +683,14 @@ func TestDefPullReq_MergeVerify(t *testing.T) { Reviewers: []*types.PullReqReviewer{ { ReviewDecision: enum.PullReqReviewDecisionChangeReq, - Reviewer: types.PrincipalInfo{DisplayName: "John"}, + Reviewer: reviewer1, SHA: "def", }, }, Method: enum.MergeMethodMerge, }, expCodes: []string{codePullReqApprovalReqChangeRequestedOldSHA}, - expParams: [][]any{{"John"}}, + expParams: [][]any{{reviewer1.DisplayName}}, expOut: MergeVerifyOutput{ AllowedMethods: enum.MergeMethods, RequiresNoChangeRequests: true, diff --git a/types/pullreq.go b/types/pullreq.go index 93d5fddd1..4a07098a8 100644 --- a/types/pullreq.go +++ b/types/pullreq.go @@ -252,7 +252,6 @@ type PullReqFileView struct { } type DefaultReviewerApprovalsResponse struct { - RuleInfo RuleInfo `json:"rule_info"` MinimumRequiredCount int `json:"minimum_required_count"` MinimumRequiredCountLatest int `json:"minimum_required_count_latest"` CurrentCount int `json:"current_count"` diff --git a/web/src/components/BranchProtection/BranchProtectionForm/ProtectionRulesForm/DefaultReviewersSection.tsx b/web/src/components/BranchProtection/BranchProtectionForm/ProtectionRulesForm/DefaultReviewersSection.tsx index 11157f2ad..87a94e1e5 100644 --- a/web/src/components/BranchProtection/BranchProtectionForm/ProtectionRulesForm/DefaultReviewersSection.tsx +++ b/web/src/components/BranchProtection/BranchProtectionForm/ProtectionRulesForm/DefaultReviewersSection.tsx @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import React from 'react' +import React, { useMemo } from 'react' import cx from 'classnames' import { Container, FormInput, SelectOption, Text } from '@harnessio/uicore' import { Color } from '@harnessio/design-system' @@ -49,6 +49,28 @@ const DefaultReviewersSection = (props: { (item: SelectOption) => !defaultReviewersList?.includes(item.value as string) ) + const defReviewerWarning = useMemo(() => { + const minReviewers = Number(formik.values.minDefaultReviewers) + const reviewerCount = defaultReviewersList?.length || 0 + + if (formik.values.requireMinDefaultReviewers && minReviewers === reviewerCount) { + let message = '' + let showWarning = false + + if (reviewerCount === 1) { + message = getString('branchProtection.defaultReviewerWarning') + showWarning = true + } else if (reviewerCount > 1) { + message = getString('branchProtection.defaultReviewersWarning') + showWarning = true + } + + return { message, showWarning } + } + + return { message: '', showWarning: false } + }, [formik.values, defaultReviewersList]) + return ( <> )} + + + {defReviewerWarning.message} + + { defReviewerApprovalRequiredByRule, defReviewerLatestApprovalRequiredByRule, defReviewerApprovedLatestChanges, - defReviewerApprovedChanges, - changesRequestedByDefReviewersArr + defReviewerApprovedChanges } = getUnifiedDefaultReviewersState(updatedDefaultApprovalRes) const extractInfoForCodeOwnerContent = () => { @@ -690,29 +689,7 @@ const ChangesSection = (props: ChangesSectionProps) => { (defReviewerApprovalRequiredByRule || defReviewerLatestApprovalRequiredByRule) && ( - {changesRequestedByDefReviewersArr && changesRequestedByDefReviewersArr?.length > 0 ? ( - - {getString('changesSection.defaultReviewersChangesToPr')} - - ) : ( - renderDefaultReviewersStatus() - )} + {renderDefaultReviewersStatus()} {(defReviewerApprovalRequiredByRule || defReviewerLatestApprovalRequiredByRule) && ( diff --git a/web/src/pages/PullRequest/DefaultReviewers/DefaultReviewersPanel.tsx b/web/src/pages/PullRequest/DefaultReviewers/DefaultReviewersPanel.tsx index 27713ab0a..26976f32c 100644 --- a/web/src/pages/PullRequest/DefaultReviewers/DefaultReviewersPanel.tsx +++ b/web/src/pages/PullRequest/DefaultReviewers/DefaultReviewersPanel.tsx @@ -79,10 +79,7 @@ export const DefaultReviewersPanel: React.FC = ({ accessor: 'DefaultReviewers', Cell: ({ row }: CellProps) => { return ( - + {row.original.principals?.map((principal, idx) => { if (idx < 2) { return ( diff --git a/web/src/services/code/index.tsx b/web/src/services/code/index.tsx index 56276c4bc..769b779cc 100644 --- a/web/src/services/code/index.tsx +++ b/web/src/services/code/index.tsx @@ -1127,7 +1127,6 @@ export interface TypesDefaultReviewerApprovalsResponse { minimum_required_count?: number minimum_required_count_latest?: number principals?: TypesPrincipalInfo[] | null - rule_info?: TypesRuleInfo } export interface TypesDeleteBranchOutput { diff --git a/web/src/services/code/swagger.yaml b/web/src/services/code/swagger.yaml index 193b45bf7..3688ee227 100644 --- a/web/src/services/code/swagger.yaml +++ b/web/src/services/code/swagger.yaml @@ -14185,8 +14185,6 @@ components: $ref: '#/components/schemas/TypesPrincipalInfo' nullable: true type: array - rule_info: - $ref: '#/components/schemas/TypesRuleInfo' type: object TypesDeleteBranchOutput: properties: