Repository: palantir/policy-bot
Pull request #459 (2022-08-17 02:40:40)

fix ignore edited comments bug
I incorrectly [assumed](https://github.com/palantir/policy-bot/pull/357) that github would always set updatedAt and submittedAt to be the same upon review comment creation, but apparently that isn't so and it just bit us. Same for createdAt and updatedAt for issue comments.

```
                "updatedAt": "2022-08-16T18:58:18Z",
                "submittedAt": "2022-08-16T18:58:17Z"
```
I was able to easily switch out updatedAt for lastEditedAt and it seems to work well. lastEditedAt is just always null until it is actually edited which makes it much easier to discern programmatically. probably should have went with this from the beginning, but who knew?

at least this bug, although annoying, wasn't broken in the other direction by incorrectly counting edited approvals but instead only intermittently not counting appropriate approvals when the ignore option is enabled.



Patch:
diff --git a/policy/approval/approve.go b/policy/approval/approve.go
index ec42ad3..f9acbb2 100644
--- a/policy/approval/approve.go
+++ b/policy/approval/approve.go
@@ -286,7 +286,7 @@ func (r *Rule) filterEditedCandidates(ctx context.Context, prctx pull.Context, c
 	var allowedCandidates []*common.Candidate
 	for _, candidate := range candidates {
 		if r.Options.IgnoreEditedComments {
-			if candidate.UpdatedAt == candidate.CreatedAt {
+			if candidate.LastEditedAt.IsZero() {
 				allowedCandidates = append(allowedCandidates, candidate)
 			}
 		}
diff --git a/policy/common/methods.go b/policy/common/methods.go
index 4164ed2..6b4787f 100644
--- a/policy/common/methods.go
+++ b/policy/common/methods.go
@@ -35,9 +35,9 @@ type Methods struct {
 }
 
 type Candidate struct {
-	User      string
-	CreatedAt time.Time
-	UpdatedAt time.Time
+	User         string
+	CreatedAt    time.Time
+	LastEditedAt time.Time
 }
 
 type CandidatesByCreationTime []*Candidate
@@ -64,9 +64,9 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid
 		for _, c := range comments {
 			if m.CommentMatches(c.Body) {
 				candidates = append(candidates, &Candidate{
-					User:      c.Author,
-					CreatedAt: c.CreatedAt,
-					UpdatedAt: c.UpdatedAt,
+					User:         c.Author,
+					CreatedAt:    c.CreatedAt,
+					LastEditedAt: c.LastEditedAt,
 				})
 			}
 		}
@@ -83,16 +83,16 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid
 				if len(m.GithubReviewCommentPatterns) > 0 {
 					if m.githubReviewCommentMatches(r.Body) {
 						candidates = append(candidates, &Candidate{
-							User:      r.Author,
-							CreatedAt: r.CreatedAt,
-							UpdatedAt: r.UpdatedAt,
+							User:         r.Author,
+							CreatedAt:    r.CreatedAt,
+							LastEditedAt: r.LastEditedAt,
 						})
 					}
 				} else {
 					candidates = append(candidates, &Candidate{
-						User:      r.Author,
-						CreatedAt: r.CreatedAt,
-						UpdatedAt: r.UpdatedAt,
+						User:         r.Author,
+						CreatedAt:    r.CreatedAt,
+						LastEditedAt: r.LastEditedAt,
 					})
 				}
 			}
diff --git a/pull/context.go b/pull/context.go
index 82e8a30..79e5bed 100644
--- a/pull/context.go
+++ b/pull/context.go
@@ -181,10 +181,10 @@ type Signature struct {
 }
 
 type Comment struct {
-	CreatedAt time.Time
-	UpdatedAt time.Time
-	Author    string
-	Body      string
+	CreatedAt    time.Time
+	LastEditedAt time.Time
+	Author       string
+	Body         string
 }
 
 type ReviewState string
@@ -198,12 +198,12 @@ const (
 )
 
 type Review struct {
-	CreatedAt time.Time
-	UpdatedAt time.Time
-	Author    string
-	State     ReviewState
-	Body      string
-	SHA       string
+	CreatedAt    time.Time
+	LastEditedAt time.Time
+	Author       string
+	State        ReviewState
+	Body         string
+	SHA          string
 
 	Teams []string
 }
diff --git a/pull/github.go b/pull/github.go
index 2b88421..ecbaba4 100644
--- a/pull/github.go
+++ b/pull/github.go
@@ -978,11 +978,11 @@ func (pi v4PageInfo) UpdateCursor(vars map[string]interface{}, name string) bool
 }
 
 type v4PullRequestReview struct {
-	Author      v4Actor
-	State       string
-	Body        string
-	SubmittedAt time.Time
-	UpdatedAt   time.Time
+	Author       v4Actor
+	State        string
+	Body         string
+	SubmittedAt  time.Time
+	LastEditedAt time.Time
 
 	Commit struct {
 		OID string
@@ -1004,38 +1004,38 @@ func (r *v4PullRequestReview) ToReview() *Review {
 	}
 
 	return &Review{
-		CreatedAt: r.SubmittedAt,
-		UpdatedAt: r.UpdatedAt,
-		Author:    r.Author.GetV3Login(),
-		State:     ReviewState(strings.ToLower(r.State)),
-		Body:      r.Body,
-		SHA:       r.Commit.OID,
-		Teams:     teams,
+		CreatedAt:    r.SubmittedAt,
+		LastEditedAt: r.LastEditedAt,
+		Author:       r.Author.GetV3Login(),
+		State:        ReviewState(strings.ToLower(r.State)),
+		Body:         r.Body,
+		SHA:          r.Commit.OID,
+		Teams:        teams,
 	}
 }
 
 func (r *v4PullRequestReview) ToComment() *Comment {
 	return &Comment{
-		CreatedAt: r.SubmittedAt,
-		UpdatedAt: r.UpdatedAt,
-		Author:    r.Author.GetV3Login(),
-		Body:      r.Body,
+		CreatedAt:    r.SubmittedAt,
+		LastEditedAt: r.LastEditedAt,
+		Author:       r.Author.GetV3Login(),
+		Body:         r.Body,
 	}
 }
 
 type v4IssueComment struct {
-	Author    v4Actor
-	Body      string
-	CreatedAt time.Time
-	UpdatedAt time.Time
+	Author       v4Actor
+	Body         string
+	CreatedAt    time.Time
+	LastEditedAt time.Time
 }
 
 func (c *v4IssueComment) ToComment() *Comment {
 	return &Comment{
-		CreatedAt: c.CreatedAt,
-		UpdatedAt: c.UpdatedAt,
-		Author:    c.Author.GetV3Login(),
-		Body:      c.Body,
+		CreatedAt:    c.CreatedAt,
+		LastEditedAt: c.LastEditedAt,
+		Author:       c.Author.GetV3Login(),
+		Body:         c.Body,
 	}
 }
 


Test patch:
diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go
index a62ed60..743a588 100644
--- a/policy/approval/approve_test.go
+++ b/policy/approval/approve_test.go
@@ -39,69 +39,69 @@ func TestIsApproved(t *testing.T) {
 			AuthorValue: "mhaypenny",
 			CommentsValue: []*pull.Comment{
 				{
-					CreatedAt: now.Add(10 * time.Second),
-					UpdatedAt: now.Add(10 * time.Second),
-					Author:    "other-user",
-					Body:      "Why did you do this?",
+					CreatedAt:    now.Add(10 * time.Second),
+					LastEditedAt: time.Time{},
+					Author:       "other-user",
+					Body:         "Why did you do this?",
 				},
 				{
-					CreatedAt: now.Add(20 * time.Second),
-					UpdatedAt: now.Add(20 * time.Second),
-					Author:    "comment-approver",
-					Body:      "LGTM :+1: :shipit:",
+					CreatedAt:    now.Add(20 * time.Second),
+					LastEditedAt: time.Time{},
+					Author:       "comment-approver",
+					Body:         "LGTM :+1: :shipit:",
 				},
 				{
-					CreatedAt: now.Add(30 * time.Second),
-					UpdatedAt: now.Add(30 * time.Second),
-					Author:    "disapprover",
-					Body:      "I don't like things! :-1:",
+					CreatedAt:    now.Add(30 * time.Second),
+					LastEditedAt: time.Time{},
+					Author:       "disapprover",
+					Body:         "I don't like things! :-1:",
 				},
 				{
-					CreatedAt: now.Add(40 * time.Second),
-					UpdatedAt: now.Add(40 * time.Second),
-					Author:    "mhaypenny",
-					Body:      ":+1: my stuff is cool",
+					CreatedAt:    now.Add(40 * time.Second),
+					LastEditedAt: time.Time{},
+					Author:       "mhaypenny",
+					Body:         ":+1: my stuff is cool",
 				},
 				{
-					CreatedAt: now.Add(50 * time.Second),
-					UpdatedAt: now.Add(50 * time.Second),
-					Author:    "contributor-author",
-					Body:      ":+1: I added to this PR",
+					CreatedAt:    now.Add(50 * time.Second),
+					LastEditedAt: time.Time{},
+					Author:       "contributor-author",
+					Body:         ":+1: I added to this PR",
 				},
 				{
-					CreatedAt: now.Add(60 * time.Second),
-					UpdatedAt: now.Add(60 * time.Second),
-					Author:    "contributor-committer",
-					Body:      ":+1: I also added to this PR",
+					CreatedAt:    now.Add(60 * time.Second),
+					LastEditedAt: time.Time{},
+					Author:       "contributor-committer",
+					Body:         ":+1: I also added to this PR",
 				},
 				{
-					CreatedAt: now.Add(70 * time.Second),
-					UpdatedAt: now.Add(71 * time.Second),
-					Author:    "comment-editor",
-					Body:      "LGTM :+1: :shipit:",
+					CreatedAt:    now.Add(70 * time.Second),
+					LastEditedAt: now.Add(71 * time.Second),
+					Author:       "comment-editor",
+					Body:         "LGTM :+1: :shipit:",
 				},
 			},
 			ReviewsValue: []*pull.Review{
 				{
-					CreatedAt: now.Add(70 * time.Second),
-					UpdatedAt: now.Add(70 * time.Second),
-					Author:    "disapprover",
-					State:     pull.ReviewChangesRequested,
-					Body:      "I _really_ don't like things!",
+					CreatedAt:    now.Add(70 * time.Second),
+					LastEditedAt: time.Time{},
+					Author:       "disapprover",
+					State:        pull.ReviewChangesRequested,
+					Body:         "I _really_ don't like things!",
 				},
 				{
-					CreatedAt: now.Add(80 * time.Second),
-					UpdatedAt: now.Add(80 * time.Second),
-					Author:    "review-approver",
-					State:     pull.ReviewApproved,
-					Body:      "I LIKE THIS",
+					CreatedAt:    now.Add(80 * time.Second),
+					LastEditedAt: time.Time{},
+					Author:       "review-approver",
+					State:        pull.ReviewApproved,
+					Body:         "I LIKE THIS",
 				},
 				{
-					CreatedAt: now.Add(90 * time.Second),
-					UpdatedAt: now.Add(91 * time.Second),
-					Author:    "review-comment-editor",
-					State:     pull.ReviewApproved,
-					Body:      "I LIKE THIS",
+					CreatedAt:    now.Add(90 * time.Second),
+					LastEditedAt: now.Add(91 * time.Second),
+					Author:       "review-comment-editor",
+					State:        pull.ReviewApproved,
+					Body:         "I LIKE THIS",
 				},
 			},
 			CommitsValue: []*pull.Commit{
diff --git a/pull/github_test.go b/pull/github_test.go
index a249f2c..32c8fd1 100644
--- a/pull/github_test.go
+++ b/pull/github_test.go
@@ -201,19 +201,19 @@ func TestReviews(t *testing.T) {
 
 	assert.Equal(t, "mhaypenny", reviews[0].Author)
 	assert.Equal(t, expectedTime, reviews[0].CreatedAt)
-	assert.Equal(t, expectedTime, reviews[0].UpdatedAt)
+	assert.Equal(t, expectedTime, reviews[0].LastEditedAt)
 	assert.Equal(t, ReviewChangesRequested, reviews[0].State)
 	assert.Equal(t, "", reviews[0].Body)
 
 	assert.Equal(t, "bkeyes", reviews[1].Author)
 	assert.Equal(t, expectedTime.Add(time.Second), reviews[1].CreatedAt)
-	assert.Equal(t, expectedTime.Add(time.Second), reviews[1].UpdatedAt)
+	assert.Equal(t, expectedTime.Add(time.Second), reviews[1].LastEditedAt)
 	assert.Equal(t, ReviewApproved, reviews[1].State)
 	assert.Equal(t, "the body", reviews[1].Body)
 
 	assert.Equal(t, "jgiannuzzi", reviews[2].Author)
 	assert.Equal(t, expectedTime.Add(-4*time.Second).Add(5*time.Minute), reviews[2].CreatedAt)
-	assert.Equal(t, expectedTime.Add(-4*time.Second).Add(5*time.Minute), reviews[2].UpdatedAt)
+	assert.Equal(t, expectedTime.Add(-4*time.Second).Add(5*time.Minute), reviews[2].LastEditedAt)
 	assert.Equal(t, ReviewCommented, reviews[2].State)
 	assert.Equal(t, "A review comment", reviews[2].Body)
 
@@ -266,17 +266,17 @@ func TestComments(t *testing.T) {
 
 	assert.Equal(t, "bkeyes", comments[0].Author)
 	assert.Equal(t, expectedTime, comments[0].CreatedAt)
-	assert.Equal(t, expectedTime, comments[0].UpdatedAt)
+	assert.Equal(t, expectedTime, comments[0].LastEditedAt)
 	assert.Equal(t, ":+1:", comments[0].Body)
 
 	assert.Equal(t, "bulldozer[bot]", comments[1].Author)
 	assert.Equal(t, expectedTime.Add(time.Minute), comments[1].CreatedAt)
-	assert.Equal(t, expectedTime.Add(time.Minute), comments[1].UpdatedAt)
+	assert.Equal(t, expectedTime.Add(time.Minute), comments[1].LastEditedAt)
 	assert.Equal(t, "I merge!", comments[1].Body)
 
 	assert.Equal(t, "jgiannuzzi", comments[2].Author)
 	assert.Equal(t, expectedTime.Add(10*time.Minute), comments[2].CreatedAt)
-	assert.Equal(t, expectedTime.Add(10*time.Minute), comments[2].UpdatedAt)
+	assert.Equal(t, expectedTime.Add(10*time.Minute), comments[2].LastEditedAt)
 	assert.Equal(t, "A review comment", comments[2].Body)
 
 	// verify that the commit list is cached
diff --git a/pull/testdata/responses/pull_comments.yml b/pull/testdata/responses/pull_comments.yml
index bbca887..a913aeb 100644
--- a/pull/testdata/responses/pull_comments.yml
+++ b/pull/testdata/responses/pull_comments.yml
@@ -18,7 +18,7 @@
                   },
                   "body": ":+1:",
                   "createdAt": "2018-06-27T20:28:22Z",
-                  "updatedAt": "2018-06-27T20:28:22Z"
+                  "lastEditedAt": "2018-06-27T20:28:22Z"
                 }
               ]
             },
@@ -35,7 +35,7 @@
                   "state": "CHANGES_REQUESTED",
                   "body": "",
                   "submittedAt": "2018-06-27T20:33:26Z",
-                  "updatedAt": "2018-06-27T20:33:26Z"
+                  "lastEditedAt": "2018-06-27T20:33:26Z"
                 }
               ]
             }
@@ -63,7 +63,7 @@
                   },
                   "body": "I merge!",
                   "createdAt": "2018-06-27T20:29:22Z",
-                  "updatedAt": "2018-06-27T20:29:22Z"
+                  "lastEditedAt": "2018-06-27T20:29:22Z"
                 }
               ]
             },
@@ -80,7 +80,7 @@
                   "state": "APPROVED",
                   "body": "the body",
                   "submittedAt": "2018-06-27T20:33:27Z",
-                  "updatedAt": "2018-06-27T20:33:27Z"
+                  "lastEditedAt": "2018-06-27T20:33:27Z"
                 },
                 {
                   "author": {
@@ -89,7 +89,7 @@
                   "state": "COMMENTED",
                   "body": "A review comment",
                   "submittedAt": "2018-06-27T20:38:22Z",
-                  "updatedAt": "2018-06-27T20:38:22Z"
+                  "lastEditedAt": "2018-06-27T20:38:22Z"
                 }
               ]
             }
diff --git a/pull/testdata/responses/pull_reviews.yml b/pull/testdata/responses/pull_reviews.yml
index 3bee176..acf958c 100644
--- a/pull/testdata/responses/pull_reviews.yml
+++ b/pull/testdata/responses/pull_reviews.yml
@@ -18,7 +18,7 @@
                   "state": "CHANGES_REQUESTED",
                   "body": "",
                   "submittedAt": "2018-06-27T20:33:26Z",
-                  "updatedAt": "2018-06-27T20:33:26Z"
+                  "lastEditedAt": "2018-06-27T20:33:26Z"
                 }
               ]
             }
@@ -46,7 +46,7 @@
                   "state": "APPROVED",
                   "body": "the body",
                   "submittedAt": "2018-06-27T20:33:27Z",
-                  "updatedAt": "2018-06-27T20:33:27Z"
+                  "lastEditedAt": "2018-06-27T20:33:27Z"
                 },
                 {
                   "author": {
@@ -55,7 +55,7 @@
                   "state": "COMMENTED",
                   "body": "A review comment",
                   "submittedAt": "2018-06-27T20:38:22Z",
-                  "updatedAt": "2018-06-27T20:38:22Z"
+                  "lastEditedAt": "2018-06-27T20:38:22Z"
                 }
               ]
             }
diff --git a/pull/testdata/responses/pull_reviews_comments.yml b/pull/testdata/responses/pull_reviews_comments.yml
index d1120da..59c5652 100644
--- a/pull/testdata/responses/pull_reviews_comments.yml
+++ b/pull/testdata/responses/pull_reviews_comments.yml
@@ -18,7 +18,7 @@
                   },
                   "body": ":+1:",
                   "createdAt": "2018-06-27T20:28:22Z",
-                  "updatedAt": "2018-06-27T20:28:22Z"
+                  "lastEditedAt": "2018-06-27T20:28:22Z"
                 },
                 {
                   "author": {
@@ -27,7 +27,7 @@
                   },
                   "body": "I comment!",
                   "createdAt": "2018-06-27T20:28:22Z",
-                  "updatedAt": "2018-06-27T20:28:22Z"
+                  "lastEditedAt": "2018-06-27T20:28:22Z"
                 }
               ]
             },
@@ -44,7 +44,7 @@
                   "state": "CHANGES_REQUESTED",
                   "body": "",
                   "submittedAt": "2018-06-27T20:33:26Z",
-                  "updatedAt": "2018-06-27T20:33:26Z"
+                  "lastEditedAt": "2018-06-27T20:33:26Z"
                 }
               ]
             }
@@ -79,7 +79,7 @@
                   "state": "APPROVED",
                   "body": "the body",
                   "submittedAt": "2018-06-27T20:33:27Z",
-                  "updatedAt": "2018-06-27T20:33:27Z"
+                  "lastEditedAt": "2018-06-27T20:33:27Z"
                 },
                 {
                   "author": {
@@ -88,7 +88,7 @@
                   "state": "COMMENTED",
                   "body": "A review comment",
                   "submittedAt": "2018-06-27T20:38:22Z",
-                  "updatedAt": "2018-06-27T20:38:22Z"
+                  "lastEditedAt": "2018-06-27T20:38:22Z"
                 }
               ]
             }