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

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

Fixes: #18434

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

Conflicts:
	services/migrations/gitlab.go
	trivial context conflict because var reviews became reviews := in 1.17

services/migrations/gitlab.go

@@ -32,8 +32,7 @@ func init() {
 }
 
 // GitlabDownloaderFactory defines a gitlab downloader factory
-type GitlabDownloaderFactory struct {
+type GitlabDownloaderFactory struct{}
-}
 
 // New returns a Downloader related to this factory according MigrateOptions
 func (f *GitlabDownloaderFactory) New(ctx context.Context, opts base.MigrateOptions) (base.Downloader, error) {
@@ -184,16 +183,17 @@ func (g *GitlabDownloader) GetTopics() ([]string, error) {
 
 // GetMilestones returns milestones
 func (g *GitlabDownloader) GetMilestones() ([]*base.Milestone, error) {
-	var perPage = g.maxPerPage
+	perPage := g.maxPerPage
-	var state = "all"
+	state := "all"
-	var milestones = make([]*base.Milestone, 0, perPage)
+	milestones := make([]*base.Milestone, 0, perPage)
 	for i := 1; ; i++ {
 		ms, _, err := g.client.Milestones.ListMilestones(g.repoID, &gitlab.ListMilestonesOptions{
 			State: &state,
 			ListOptions: gitlab.ListOptions{
 				Page:    i,
 				PerPage: perPage,
-			}}, nil, gitlab.WithContext(g.ctx))
+			},
+		}, nil, gitlab.WithContext(g.ctx))
 		if err != nil {
 			return nil, err
 		}
@@ -203,7 +203,7 @@ func (g *GitlabDownloader) GetMilestones() ([]*base.Milestone, error) {
 			if m.Description != "" {
 				desc = m.Description
 			}
-			var state = "open"
+			state := "open"
 			var closedAt *time.Time
 			if m.State != "" {
 				state = m.State
@@ -255,8 +255,8 @@ func (g *GitlabDownloader) normalizeColor(val string) string {
 
 // GetLabels returns labels
 func (g *GitlabDownloader) GetLabels() ([]*base.Label, error) {
-	var perPage = g.maxPerPage
+	perPage := g.maxPerPage
-	var labels = make([]*base.Label, 0, perPage)
+	labels := make([]*base.Label, 0, perPage)
 	for i := 1; ; i++ {
 		ls, _, err := g.client.Labels.ListLabels(g.repoID, &gitlab.ListLabelsOptions{ListOptions: gitlab.ListOptions{
 			Page:    i,
@@ -327,8 +327,8 @@ func (g *GitlabDownloader) convertGitlabRelease(rel *gitlab.Release) *base.Relea
 
 // GetReleases returns releases
 func (g *GitlabDownloader) GetReleases() ([]*base.Release, error) {
-	var perPage = g.maxPerPage
+	perPage := g.maxPerPage
-	var releases = make([]*base.Release, 0, perPage)
+	releases := make([]*base.Release, 0, perPage)
 	for i := 1; ; i++ {
 		ls, _, err := g.client.Releases.ListReleases(g.repoID, &gitlab.ListReleasesOptions{
 			Page:    i,
@@ -381,7 +381,7 @@ func (g *GitlabDownloader) GetIssues(page, perPage int) ([]*base.Issue, bool, er
 		},
 	}
 
-	var allIssues = make([]*base.Issue, 0, perPage)
+	allIssues := make([]*base.Issue, 0, perPage)
 
 	issues, _, err := g.client.Issues.ListProjectIssues(g.repoID, opt, nil, gitlab.WithContext(g.ctx))
 	if err != nil {
@@ -389,7 +389,7 @@ func (g *GitlabDownloader) GetIssues(page, perPage int) ([]*base.Issue, bool, er
 	}
 	for _, issue := range issues {
 
-		var labels = make([]*base.Label, 0, len(issue.Labels))
+		labels := make([]*base.Label, 0, len(issue.Labels))
 		for _, l := range issue.Labels {
 			labels = append(labels, &base.Label{
 				Name: l,
@@ -402,7 +402,7 @@ func (g *GitlabDownloader) GetIssues(page, perPage int) ([]*base.Issue, bool, er
 		}
 
 		var reactions []*base.Reaction
-		var awardPage = 1
+		awardPage := 1
 		for {
 			awards, _, err := g.client.AwardEmoji.ListIssueAwardEmoji(g.repoID, issue.IID, &gitlab.ListAwardEmojiOptions{Page: awardPage, PerPage: perPage}, gitlab.WithContext(g.ctx))
 			if err != nil {
@@ -456,9 +456,9 @@ func (g *GitlabDownloader) GetComments(opts base.GetCommentOptions) ([]*base.Com
 		return nil, false, fmt.Errorf("unexpected context: %+v", opts.Context)
 	}
 
-	var allComments = make([]*base.Comment, 0, g.maxPerPage)
+	allComments := make([]*base.Comment, 0, g.maxPerPage)
 
-	var page = 1
+	page := 1
 
 	for {
 		var comments []*gitlab.Discussion
@@ -503,7 +503,6 @@ func (g *GitlabDownloader) GetComments(opts base.GetCommentOptions) ([]*base.Com
 					Created:     *c.CreatedAt,
 				})
 			}
-
 		}
 		if resp.NextPage == 0 {
 			break
@@ -526,7 +525,7 @@ func (g *GitlabDownloader) GetPullRequests(page, perPage int) ([]*base.PullReque
 		},
 	}
 
-	var allPRs = make([]*base.PullRequest, 0, perPage)
+	allPRs := make([]*base.PullRequest, 0, perPage)
 
 	prs, _, err := g.client.MergeRequests.ListProjectMergeRequests(g.repoID, opt, nil, gitlab.WithContext(g.ctx))
 	if err != nil {
@@ -534,7 +533,7 @@ func (g *GitlabDownloader) GetPullRequests(page, perPage int) ([]*base.PullReque
 	}
 	for _, pr := range prs {
 
-		var labels = make([]*base.Label, 0, len(pr.Labels))
+		labels := make([]*base.Label, 0, len(pr.Labels))
 		for _, l := range pr.Labels {
 			labels = append(labels, &base.Label{
 				Name: l,
@@ -547,12 +546,12 @@ func (g *GitlabDownloader) GetPullRequests(page, perPage int) ([]*base.PullReque
 			pr.State = "closed"
 		}
 
-		var mergeTime = pr.MergedAt
+		mergeTime := pr.MergedAt
 		if merged && pr.MergedAt == nil {
 			mergeTime = pr.UpdatedAt
 		}
 
-		var closeTime = pr.ClosedAt
+		closeTime := pr.ClosedAt
 		if merged && pr.ClosedAt == nil {
 			closeTime = pr.UpdatedAt
 		}
@@ -568,7 +567,7 @@ func (g *GitlabDownloader) GetPullRequests(page, perPage int) ([]*base.PullReque
 		}
 
 		var reactions []*base.Reaction
-		var awardPage = 1
+		awardPage := 1
 		for {
 			awards, _, err := g.client.AwardEmoji.ListMergeRequestAwardEmoji(g.repoID, pr.IID, &gitlab.ListAwardEmojiOptions{Page: awardPage, PerPage: perPage}, gitlab.WithContext(g.ctx))
 			if err != nil {
@@ -641,13 +640,22 @@ func (g *GitlabDownloader) GetReviews(context base.IssueContext) ([]*base.Review
 		return nil, err
 	}
 
-	var reviews = make([]*base.Review, 0, len(approvals.ApprovedBy))
+	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,
 		})

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)
+	}
+}

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.ID, actual.ID, "ID")
-	assert.Equal(t, expected.IssueIndex, actual.IssueIndex)
+	assert.Equal(t, expected.IssueIndex, actual.IssueIndex, "IsssueIndex")
-	assert.Equal(t, expected.ReviewerID, actual.ReviewerID)
+	assert.Equal(t, expected.ReviewerID, actual.ReviewerID, "ReviewerID")
-	assert.Equal(t, expected.ReviewerName, actual.ReviewerName)
+	assert.Equal(t, expected.ReviewerName, actual.ReviewerName, "ReviewerName")
-	assert.Equal(t, expected.Official, actual.Official)
+	assert.Equal(t, expected.Official, actual.Official, "Official")
-	assert.Equal(t, expected.CommitID, actual.CommitID)
+	assert.Equal(t, expected.CommitID, actual.CommitID, "CommitID")
-	assert.Equal(t, expected.Content, actual.Content)
+	assert.Equal(t, expected.Content, actual.Content, "Content")
-	assertTimeEqual(t, expected.CreatedAt, actual.CreatedAt)
+	assert.WithinDuration(t, expected.CreatedAt, actual.CreatedAt, 10*time.Second)
-	assert.Equal(t, expected.State, actual.State)
+	assert.Equal(t, expected.State, actual.State, "State")
 	assertReviewCommentsEqual(t, expected.Comments, actual.Comments)
 }