GitLab reviews may not have the updated_at field set (#18450)

* GitLab reviews may not have the updated_at field set

Fallback to created_at if that the case and to time.Now() if it is
also missing.

Fixes: 18434
* use assert.WithinDuration

Co-authored-by: Loïc Dachary <loic@dachary.org>

diff --git a/services/migrations/gitlab.go b/services/migrations/gitlab.go
index a9856739c242bc00bd28d37972c902970409f5e6..97ebc4dd8b488245918eb6d9f01d88e76ad6ebd1 100644 (file)
--- a/services/migrations/gitlab.go
+++ b/services/migrations/gitlab.go
@@ -640,13 +640,22 @@ func (g *GitlabDownloader) GetReviews(context base.IssueContext) ([]*base.Review
                return nil, err
        }
 
+       var createdAt time.Time
+       if approvals.CreatedAt != nil {
+               createdAt = *approvals.CreatedAt
+       } else if approvals.UpdatedAt != nil {
+               createdAt = *approvals.UpdatedAt
+       } else {
+               createdAt = time.Now()
+       }
+
        reviews := make([]*base.Review, 0, len(approvals.ApprovedBy))
        for _, user := range approvals.ApprovedBy {
                reviews = append(reviews, &base.Review{
                        IssueIndex:   context.LocalID(),
                        ReviewerID:   int64(user.User.ID),
                        ReviewerName: user.User.Username,
-                       CreatedAt:    *approvals.UpdatedAt,
+                       CreatedAt:    createdAt,
                        // All we get are approvals
                        State: base.ReviewStateApproved,
                })

diff --git a/services/migrations/gitlab_test.go b/services/migrations/gitlab_test.go
index 6b77aa62efbfbad7fd74ab515e8322342256738a..ad61577653c177e0fd9931f89b4b0654e0b8dc78 100644 (file)
--- a/services/migrations/gitlab_test.go
+++ b/services/migrations/gitlab_test.go
@@ -8,13 +8,16 @@ import (
        "context"
        "fmt"
        "net/http"
+       "net/http/httptest"
        "os"
+       "strconv"
        "testing"
        "time"
 
        base "code.gitea.io/gitea/modules/migration"
 
        "github.com/stretchr/testify/assert"
+       "github.com/xanzy/go-gitlab"
 )
 
 func TestGitlabDownloadRepo(t *testing.T) {
@@ -310,12 +313,14 @@ func TestGitlabDownloadRepo(t *testing.T) {
        assert.NoError(t, err)
        assertReviewsEqual(t, []*base.Review{
                {
+                       IssueIndex:   1,
                        ReviewerID:   4102996,
                        ReviewerName: "zeripath",
                        CreatedAt:    time.Date(2019, 11, 28, 16, 2, 8, 377000000, time.UTC),
                        State:        "APPROVED",
                },
                {
+                       IssueIndex:   1,
                        ReviewerID:   527793,
                        ReviewerName: "axifive",
                        CreatedAt:    time.Date(2019, 11, 28, 16, 2, 8, 377000000, time.UTC),
@@ -327,6 +332,7 @@ func TestGitlabDownloadRepo(t *testing.T) {
        assert.NoError(t, err)
        assertReviewsEqual(t, []*base.Review{
                {
+                       IssueIndex:   2,
                        ReviewerID:   4575606,
                        ReviewerName: "real6543",
                        CreatedAt:    time.Date(2020, 4, 19, 19, 24, 21, 108000000, time.UTC),
@@ -334,3 +340,137 @@ func TestGitlabDownloadRepo(t *testing.T) {
                },
        }, rvs)
 }
+
+func gitlabClientMockSetup(t *testing.T) (*http.ServeMux, *httptest.Server, *gitlab.Client) {
+       // mux is the HTTP request multiplexer used with the test server.
+       mux := http.NewServeMux()
+
+       // server is a test HTTP server used to provide mock API responses.
+       server := httptest.NewServer(mux)
+
+       // client is the Gitlab client being tested.
+       client, err := gitlab.NewClient("", gitlab.WithBaseURL(server.URL))
+       if err != nil {
+               server.Close()
+               t.Fatalf("Failed to create client: %v", err)
+       }
+
+       return mux, server, client
+}
+
+func gitlabClientMockTeardown(server *httptest.Server) {
+       server.Close()
+}
+
+type reviewTestCase struct {
+       repoID, prID, reviewerID int
+       reviewerName             string
+       createdAt, updatedAt     *time.Time
+       expectedCreatedAt        time.Time
+}
+
+func convertTestCase(t reviewTestCase) (func(w http.ResponseWriter, r *http.Request), base.Review) {
+       var updatedAtField string
+       if t.updatedAt == nil {
+               updatedAtField = ""
+       } else {
+               updatedAtField = `"updated_at": "` + t.updatedAt.Format(time.RFC3339) + `",`
+       }
+
+       var createdAtField string
+       if t.createdAt == nil {
+               createdAtField = ""
+       } else {
+               createdAtField = `"created_at": "` + t.createdAt.Format(time.RFC3339) + `",`
+       }
+
+       handler := func(w http.ResponseWriter, r *http.Request) {
+               fmt.Fprint(w, `
+{
+  "id": 5,
+  "iid": `+strconv.Itoa(t.prID)+`,
+  "project_id": `+strconv.Itoa(t.repoID)+`,
+  "title": "Approvals API",
+  "description": "Test",
+  "state": "opened",
+  `+createdAtField+`
+  `+updatedAtField+`
+  "merge_status": "cannot_be_merged",
+  "approvals_required": 2,
+  "approvals_left": 1,
+  "approved_by": [
+    {
+      "user": {
+        "name": "Administrator",
+        "username": "`+t.reviewerName+`",
+        "id": `+strconv.Itoa(t.reviewerID)+`,
+        "state": "active",
+        "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80\u0026d=identicon",
+        "web_url": "http://localhost:3000/root"
+      }
+    }
+  ]
+}`)
+       }
+       review := base.Review{
+               IssueIndex:   int64(t.prID),
+               ReviewerID:   int64(t.reviewerID),
+               ReviewerName: t.reviewerName,
+               CreatedAt:    t.expectedCreatedAt,
+               State:        "APPROVED",
+       }
+
+       return handler, review
+}
+
+func TestGitlabGetReviews(t *testing.T) {
+       mux, server, client := gitlabClientMockSetup(t)
+       defer gitlabClientMockTeardown(server)
+
+       repoID := 1324
+
+       downloader := &GitlabDownloader{
+               ctx:    context.Background(),
+               client: client,
+               repoID: repoID,
+       }
+
+       createdAt := time.Date(2020, 4, 19, 19, 24, 21, 0, time.UTC)
+
+       for _, testCase := range []reviewTestCase{
+               {
+                       repoID:            repoID,
+                       prID:              1,
+                       reviewerID:        801,
+                       reviewerName:      "someone1",
+                       createdAt:         nil,
+                       updatedAt:         &createdAt,
+                       expectedCreatedAt: createdAt,
+               },
+               {
+                       repoID:            repoID,
+                       prID:              2,
+                       reviewerID:        802,
+                       reviewerName:      "someone2",
+                       createdAt:         &createdAt,
+                       updatedAt:         nil,
+                       expectedCreatedAt: createdAt,
+               },
+               {
+                       repoID:            repoID,
+                       prID:              3,
+                       reviewerID:        803,
+                       reviewerName:      "someone3",
+                       createdAt:         nil,
+                       updatedAt:         nil,
+                       expectedCreatedAt: time.Now(),
+               },
+       } {
+               mock, review := convertTestCase(testCase)
+               mux.HandleFunc(fmt.Sprintf("/api/v4/projects/%d/merge_requests/%d/approvals", testCase.repoID, testCase.prID), mock)
+
+               rvs, err := downloader.GetReviews(base.BasicIssueContext(testCase.prID))
+               assert.NoError(t, err)
+               assertReviewsEqual(t, []*base.Review{&review}, rvs)
+       }
+}

diff --git a/services/migrations/main_test.go b/services/migrations/main_test.go
index ddf73df98e47ca49e0eeab07ee311375dc446794..b040df83d14bd8a6caefe1ac9031208aaaefb625 100644 (file)
--- a/services/migrations/main_test.go
+++ b/services/migrations/main_test.go
@@ -223,15 +223,15 @@ func assertRepositoryEqual(t *testing.T, expected, actual *base.Repository) {
 }
 
 func assertReviewEqual(t *testing.T, expected, actual *base.Review) {
-       assert.Equal(t, expected.ID, actual.ID)
-       assert.Equal(t, expected.IssueIndex, actual.IssueIndex)
-       assert.Equal(t, expected.ReviewerID, actual.ReviewerID)
-       assert.Equal(t, expected.ReviewerName, actual.ReviewerName)
-       assert.Equal(t, expected.Official, actual.Official)
-       assert.Equal(t, expected.CommitID, actual.CommitID)
-       assert.Equal(t, expected.Content, actual.Content)
-       assertTimeEqual(t, expected.CreatedAt, actual.CreatedAt)
-       assert.Equal(t, expected.State, actual.State)
+       assert.Equal(t, expected.ID, actual.ID, "ID")
+       assert.Equal(t, expected.IssueIndex, actual.IssueIndex, "IsssueIndex")
+       assert.Equal(t, expected.ReviewerID, actual.ReviewerID, "ReviewerID")
+       assert.Equal(t, expected.ReviewerName, actual.ReviewerName, "ReviewerName")
+       assert.Equal(t, expected.Official, actual.Official, "Official")
+       assert.Equal(t, expected.CommitID, actual.CommitID, "CommitID")
+       assert.Equal(t, expected.Content, actual.Content, "Content")
+       assert.WithinDuration(t, expected.CreatedAt, actual.CreatedAt, 10*time.Second)
+       assert.Equal(t, expected.State, actual.State, "State")
        assertReviewCommentsEqual(t, expected.Comments, actual.Comments)
 }