Add optional description field to approval rules (#251)

* Rename Description to StatusDescription
* Add optional description field to approval rules

config/policy-examples/complicated.yml

@@ -4,17 +4,19 @@
 policy:
   approval:
   - or:
-    - catskills can approve
-    - devtools can approve changes to staging
+    - catskills
+    - devtools
 
 approval_rules:
 
-- name: catskills can approve
+- name: catskills
+  description: catskills can approve
   requires:
     count: 1
     teams: ["palantir/catskills"]
 
-- name: devtools can approve changes to staging
+- name: devtools
+  description: devtools can approve changes to staging
   if:
     only_changed_files:
       paths:

policy/approval/approve.go

@@ -29,10 +29,11 @@ import (
 )
 
 type Rule struct {
-	Name       string     `yaml:"name"`
-	Predicates Predicates `yaml:"if"`
-	Options    Options    `yaml:"options"`
-	Requires   Requires   `yaml:"requires"`
+	Name        string     `yaml:"name"`
+	Description string     `yaml:"description"`
+	Predicates  Predicates `yaml:"if"`
+	Options     Options    `yaml:"options"`
+	Requires    Requires   `yaml:"requires"`
 }
 
 type Options struct {
@@ -99,6 +100,7 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res
 	log := zerolog.Ctx(ctx)
 
 	res.Name = r.Name
+	res.Description = r.Description
 	res.Status = common.StatusSkipped
 
 	for _, p := range r.Predicates.Predicates() {
@@ -111,9 +113,9 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res
 		if !satisfied {
 			log.Debug().Msgf("skipping rule, predicate of type %T was not satisfied", p)
 
-			res.Description = desc
+			res.StatusDescription = desc
 			if desc == "" {
-				res.Description = "The preconditions of this rule are not satisfied"
+				res.StatusDescription = "The preconditions of this rule are not satisfied"
 			}
 
 			return
@@ -126,7 +128,7 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res
 		return
 	}
 
-	res.Description = msg
+	res.StatusDescription = msg
 	if approved {
 		res.Status = common.StatusApproved
 	} else {

policy/approval/evaluate.go

@@ -42,7 +42,7 @@ func (eval *evaluator) Evaluate(ctx context.Context, prctx pull.Context) (res co
 		zerolog.Ctx(ctx).Debug().Msg("No approval policy defined; skipping")
 
 		res.Status = common.StatusApproved
-		res.Description = "No approval policy defined"
+		res.StatusDescription = "No approval policy defined"
 	}
 
 	res.Name = "approval"
@@ -63,7 +63,7 @@ func (r *RuleRequirement) Evaluate(ctx context.Context, prctx pull.Context) comm
 
 	result := r.rule.Evaluate(ctx, prctx)
 	if result.Error == nil {
-		log.Debug().Msgf("rule evaluation resulted in %s:\"%s\"", result.Status, result.Description)
+		log.Debug().Msgf("rule evaluation resulted in %s:\"%s\"", result.Status, result.StatusDescription)
 	}
 
 	return result
@@ -121,11 +121,11 @@ func (r *OrRequirement) Evaluate(ctx context.Context, prctx pull.Context) common
 	}
 
 	return common.Result{
-		Name:        "or",
-		Status:      status,
-		Description: description,
-		Error:       err,
-		Children:    children,
+		Name:              "or",
+		Status:            status,
+		StatusDescription: description,
+		Error:             err,
+		Children:          children,
 	}
 }
 
@@ -179,10 +179,10 @@ func (r *AndRequirement) Evaluate(ctx context.Context, prctx pull.Context) commo
 	}
 
 	return common.Result{
-		Name:        "and",
-		Status:      status,
-		Description: description,
-		Error:       err,
-		Children:    children,
+		Name:              "and",
+		Status:            status,
+		StatusDescription: description,
+		Error:             err,
+		Children:          children,
 	}
 }

policy/common/result.go

@@ -57,10 +57,11 @@ type ReviewRequestRule struct {
 }
 
 type Result struct {
-	Name        string
-	Description string
-	Status      EvaluationStatus
-	Error       error
+	Name              string
+	Description       string
+	StatusDescription string
+	Status            EvaluationStatus
+	Error             error
 
 	ReviewRequestRule *ReviewRequestRule
 

policy/disapproval/disapprove.go

@@ -99,7 +99,7 @@ func (p *Policy) Evaluate(ctx context.Context, prctx pull.Context) (res common.R
 	if p.Requires.IsEmpty() {
 		log.Debug().Msg("no users are allowed to disapprove; skipping")
 
-		res.Description = "No disapproval policy is specified or the policy is empty"
+		res.StatusDescription = "No disapproval policy is specified or the policy is empty"
 		return
 	}
 
@@ -109,7 +109,7 @@ func (p *Policy) Evaluate(ctx context.Context, prctx pull.Context) (res common.R
 		return
 	}
 
-	res.Description = msg
+	res.StatusDescription = msg
 	if disapproved {
 		res.Status = common.StatusDisapproved
 	} else {

policy/policy.go

@@ -93,10 +93,10 @@ func (e evaluator) Evaluate(ctx context.Context, prctx pull.Context) (res common
 	case res.Error != nil:
 	case disapproval.Status == common.StatusDisapproved:
 		res.Status = common.StatusDisapproved
-		res.Description = disapproval.Description
+		res.StatusDescription = disapproval.StatusDescription
 	default:
 		res.Status = approval.Status
-		res.Description = approval.Description
+		res.StatusDescription = approval.StatusDescription
 	}
 	return
 }

policy/policy_test.go

@@ -47,23 +47,23 @@ func TestEvaluator(t *testing.T) {
 				Status: common.StatusApproved,
 			},
 			disapproval: &StaticEvaluator{
-				Status:      common.StatusDisapproved,
-				Description: "disapproved by test",
+				Status:            common.StatusDisapproved,
+				StatusDescription: "disapproved by test",
 			},
 		}
 
 		r := eval.Evaluate(ctx, prctx)
 		require.NoError(t, r.Error)
 
 		assert.Equal(t, common.StatusDisapproved, r.Status)
-		assert.Equal(t, "disapproved by test", r.Description)
+		assert.Equal(t, "disapproved by test", r.StatusDescription)
 	})
 
 	t.Run("approvalWinsByDefault", func(t *testing.T) {
 		eval := evaluator{
 			approval: &StaticEvaluator{
-				Status:      common.StatusPending,
-				Description: "2 approvals needed",
+				Status:            common.StatusPending,
+				StatusDescription: "2 approvals needed",
 			},
 			disapproval: &StaticEvaluator{
 				Status: common.StatusSkipped,
@@ -74,7 +74,7 @@ func TestEvaluator(t *testing.T) {
 		require.NoError(t, r.Error)
 
 		assert.Equal(t, common.StatusPending, r.Status)
-		assert.Equal(t, "2 approvals needed", r.Description)
+		assert.Equal(t, "2 approvals needed", r.StatusDescription)
 	})
 
 	t.Run("propagateError", func(t *testing.T) {

policy/reviewer/reviewer_test.go

@@ -31,11 +31,12 @@ import (
 
 func TestFindLeafResults(t *testing.T) {
 	result := makeResult(&common.Result{
-		Name:        "Skipped",
-		Description: "",
-		Status:      common.StatusSkipped,
-		Error:       nil,
-		Children:    nil,
+		Name:              "Skipped",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusSkipped,
+		Error:             nil,
+		Children:          nil,
 	}, "random-users")
 	actualResults := FindRequests(result)
 	require.Len(t, actualResults, 2, "incorrect number of leaf results")
@@ -171,9 +172,10 @@ func TestFindRepositoryCollaborators(t *testing.T) {
 func TestSelectReviewers(t *testing.T) {
 	r := rand.New(rand.NewSource(42))
 	results := makeResults(&common.Result{
-		Name:        "Owner",
-		Description: "",
-		Status:      common.StatusPending,
+		Name:              "Owner",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusPending,
 		ReviewRequestRule: &common.ReviewRequestRule{
 			Admins:        true,
 			RequiredCount: 1,
@@ -197,9 +199,10 @@ func TestSelectReviewers(t *testing.T) {
 func TestSelectAdminTeam(t *testing.T) {
 	r := rand.New(rand.NewSource(42))
 	results := makeResults(&common.Result{
-		Name:        "Owner",
-		Description: "",
-		Status:      common.StatusPending,
+		Name:              "Owner",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusPending,
 		ReviewRequestRule: &common.ReviewRequestRule{
 			Admins:        true,
 			RequiredCount: 1,
@@ -222,9 +225,10 @@ func TestSelectAdminTeam(t *testing.T) {
 func TestSelectReviewers_Team(t *testing.T) {
 	r := rand.New(rand.NewSource(42))
 	results := makeResults(&common.Result{
-		Name:        "Team",
-		Description: "",
-		Status:      common.StatusPending,
+		Name:              "Team",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusPending,
 		ReviewRequestRule: &common.ReviewRequestRule{
 			// Require a team approval
 			Teams:         []string{"everyone/team-write"},
@@ -249,9 +253,10 @@ func TestSelectReviewers_Team(t *testing.T) {
 func TestSelectReviewers_Team_teams(t *testing.T) {
 	r := rand.New(rand.NewSource(42))
 	results := makeResults(&common.Result{
-		Name:        "Team",
-		Description: "",
-		Status:      common.StatusPending,
+		Name:              "Team",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusPending,
 		ReviewRequestRule: &common.ReviewRequestRule{
 			// Require a team approval
 			Teams:         []string{"everyone/team-write", "everyone/team-not-collaborators"},
@@ -278,9 +283,10 @@ func TestSelectReviewers_Team_teams(t *testing.T) {
 func TestSelectReviewers_Team_teamsDefaultsToNothing(t *testing.T) {
 	r := rand.New(rand.NewSource(42))
 	results := makeResults(&common.Result{
-		Name:        "Team",
-		Description: "",
-		Status:      common.StatusPending,
+		Name:              "Team",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusPending,
 		ReviewRequestRule: &common.ReviewRequestRule{
 			// Require a team approval
 			Teams:         []string{"everyone/team-not-collaborators"},
@@ -302,9 +308,10 @@ func TestSelectReviewers_Team_teamsDefaultsToNothing(t *testing.T) {
 func TestSelectReviewers_Org(t *testing.T) {
 	r := rand.New(rand.NewSource(42))
 	results := makeResults(&common.Result{
-		Name:        "Team",
-		Description: "",
-		Status:      common.StatusPending,
+		Name:              "Team",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusPending,
 		ReviewRequestRule: &common.ReviewRequestRule{
 			// Require everyone org approval
 			Organizations: []string{"everyone"},
@@ -330,9 +337,10 @@ func makeResults(result *common.Result, mode string) []*common.Result {
 
 func makeResult(result *common.Result, mode string) *common.Result {
 	return &common.Result{
-		Name:        "One",
-		Description: "",
-		Status:      common.StatusPending,
+		Name:              "One",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusPending,
 		ReviewRequestRule: &common.ReviewRequestRule{
 			Users:         []string{"neverappears"},
 			RequiredCount: 0,
@@ -341,9 +349,10 @@ func makeResult(result *common.Result, mode string) *common.Result {
 		Error: nil,
 		Children: []*common.Result{
 			{
-				Name:        "Two",
-				Description: "",
-				Status:      common.StatusPending,
+				Name:              "Two",
+				Description:       "",
+				StatusDescription: "",
+				Status:            common.StatusPending,
 				ReviewRequestRule: &common.ReviewRequestRule{
 					Users:         []string{"mhaypenny", "review-approver"},
 					RequiredCount: 1,
@@ -356,6 +365,7 @@ func makeResult(result *common.Result, mode string) *common.Result {
 			{
 				Name:              "Three",
 				Description:       "",
+				StatusDescription: "",
 				Status:            common.StatusDisapproved,
 				ReviewRequestRule: &common.ReviewRequestRule{},
 				Error:             errors.New("foo"),
@@ -364,14 +374,16 @@ func makeResult(result *common.Result, mode string) *common.Result {
 			{
 				Name:              "Four",
 				Description:       "",
+				StatusDescription: "",
 				Status:            common.StatusPending,
 				ReviewRequestRule: &common.ReviewRequestRule{},
 				Error:             nil,
 				Children: []*common.Result{
 					{
-						Name:        "Five",
-						Description: "",
-						Status:      common.StatusPending,
+						Name:              "Five",
+						Description:       "",
+						StatusDescription: "",
+						Status:            common.StatusPending,
 						ReviewRequestRule: &common.ReviewRequestRule{
 							Users:              []string{"contributor-committer", "contributor-author", "not-a-collaborator"},
 							RequiredCount:      1,

server/handler/base.go

@@ -191,7 +191,7 @@ func (b *Base) EvaluateFetchedConfig(ctx context.Context, prctx pull.Context, cl
 		return err
 	}
 
-	statusDescription := result.Description
+	statusDescription := result.StatusDescription
 	var statusState string
 	switch result.Status {
 	case common.StatusApproved:

server/templates/details.html.tmpl

@@ -31,7 +31,7 @@
     <div class="status-banner {{$s}} flex flex-wrap items-center justify-between">
       <div>
         <h2 class="mb-1 text-lg font-bold">Status: {{$s | titlecase}}</h2>
-        <p>{{or .Result.Error .Result.Description}}</p>
+        <p>{{or .Result.Error .Result.StatusDescription}}</p>
       </div>
       <div class="p-2 rounded-md bg-white text-dark-gray1 text-sm text-shadow-none">
         <div class="toggle">
@@ -68,5 +68,8 @@
     <b class="font-bold">{{.Name}}</b>
     <span class="flex-none status-badge {{$s}}">{{$s | titlecase}}</span>
   </p>
-  <p class="text-dark-gray3 text-sm">{{or .Error .Description}}</p>
+  {{if (and .Description (ne $s "skipped"))}}
+  <p class="mb-2 text-dark-gray3 text-sm">{{.Description}}</p>
+  {{end}}
+  <p class="text-dark-gray3 text-sm">{{or .Error .StatusDescription}}</p>
 {{end}}