From 2e9461aa589676ef9700679085e9b816c168f187 Mon Sep 17 00:00:00 2001
From: Anshul Sirur <anshul@3dhubs.com>
Date: Thu, 15 Oct 2020 16:50:13 +0200
Subject: [PATCH] Use closedAt and mergedAt when comparing version timestamps

When we're considering PRs in the closed or merged state, we need to use
their closedAt or mergedAt timestamps to check how recent the version
is. This is because PRs will not necessarily be closed or merged in the
same order their commits have been made.
---
 check.go      | 32 +++++++++++++++++++-------------
 check_test.go |  2 +-
 in_test.go    |  2 ++
 models.go     | 20 ++++++++++++++++----
 4 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/check.go b/check.go
index 34fb58a3..a0100bf3 100644
--- a/check.go
+++ b/check.go
@@ -6,6 +6,8 @@ import (
 	"regexp"
 	"sort"
 	"strings"
+
+	"github.com/shurcooL/githubv4"
 )
 
 // Check (business logic)
@@ -25,33 +27,37 @@ Loop:
 		if !disableSkipCI && ContainsSkipCI(p.Title) {
 			continue
 		}
+
 		// [ci skip]/[skip ci] in Commit message
 		if !disableSkipCI && ContainsSkipCI(p.Tip.Message) {
 			continue
 		}
+
 		// Filter pull request if the BaseBranch does not match the one specified in source
 		if request.Source.BaseBranch != "" && p.PullRequestObject.BaseRefName != request.Source.BaseBranch {
 			continue
 		}
-		// Filter out commits that are too old.
-		if !p.Tip.CommittedDate.Time.After(request.Version.CommittedDate) {
-			continue
-		}
 
 		// Filter out pull request if it does not have a filtered state
+		filterStates := []githubv4.PullRequestState{githubv4.PullRequestStateOpen}
 		if len(request.Source.States) > 0 {
-			stateFound := false
+			filterStates = request.Source.States
+		}
 
-			for _, state := range request.Source.States {
-				if p.State == state {
-					stateFound = true
-					break
-				}
+		stateFound := false
+		for _, state := range filterStates {
+			if p.State == state {
+				stateFound = true
+				break
 			}
+		}
+		if !stateFound {
+			continue
+		}
 
-			if !stateFound {
-				continue
-			}
+		// Filter out commits that are too old.
+		if !p.UpdatedDate().Time.After(request.Version.CommittedDate) {
+			continue
 		}
 
 		// Filter out pull request if it does not contain at least one of the desired labels
diff --git a/check_test.go b/check_test.go
index 86fabf66..fd88bae1 100644
--- a/check_test.go
+++ b/check_test.go
@@ -229,8 +229,8 @@ func TestCheck(t *testing.T) {
 			pullRequests: testPullRequests,
 			files:        [][]string{},
 			expected: resource.CheckResponse{
-				resource.NewVersion(testPullRequests[10]),
 				resource.NewVersion(testPullRequests[9]),
+				resource.NewVersion(testPullRequests[10]),
 			},
 		},
 	}
diff --git a/in_test.go b/in_test.go
index 3f595ee7..0d328600 100644
--- a/in_test.go
+++ b/in_test.go
@@ -357,6 +357,8 @@ func createTestPR(
 			},
 			IsCrossRepository: isCrossRepo,
 			State:             state,
+			ClosedAt:          githubv4.DateTime{Time: time.Now()},
+			MergedAt:          githubv4.DateTime{Time: time.Now()},
 		},
 		Tip: resource.CommitObject{
 			ID:            fmt.Sprintf("commit%s", n),
diff --git a/models.go b/models.go
index 2fafbfa3..30c08b7b 100644
--- a/models.go
+++ b/models.go
@@ -51,9 +51,6 @@ func (s *Source) Validate() error {
 			return errors.New(fmt.Sprintf("states value \"%s\" must be one of: OPEN, MERGED, CLOSED", state))
 		}
 	}
-	if len(s.States) == 0 {
-		s.States = []githubv4.PullRequestState{githubv4.PullRequestStateOpen}
-	}
 	return nil
 }
 
@@ -85,7 +82,7 @@ func NewVersion(p *PullRequest) Version {
 	return Version{
 		PR:                  strconv.Itoa(p.Number),
 		Commit:              p.Tip.OID,
-		CommittedDate:       p.Tip.CommittedDate.Time,
+		CommittedDate:       p.UpdatedDate().Time,
 		ApprovedReviewCount: strconv.Itoa(p.ApprovedReviewCount),
 		State:               p.State,
 	}
@@ -113,6 +110,21 @@ type PullRequestObject struct {
 	}
 	IsCrossRepository bool
 	State             githubv4.PullRequestState
+	ClosedAt          githubv4.DateTime
+	MergedAt          githubv4.DateTime
+}
+
+// UpdatedDate returns the last time a PR was updated, either by commit
+// or being closed/merged.
+func (p *PullRequest) UpdatedDate() githubv4.DateTime {
+	date := p.Tip.CommittedDate
+	switch p.State {
+	case githubv4.PullRequestStateClosed:
+		date = p.ClosedAt
+	case githubv4.PullRequestStateMerged:
+		date = p.MergedAt
+	}
+	return date
 }
 
 // CommitObject represents the GraphQL commit node.