From e19b9653ea138c2ba9a5325e6fcbc696a0e758a3 Mon Sep 17 00:00:00 2001 From: Aravinth Manivannan Date: Sat, 29 Jan 2022 17:33:20 +0000 Subject: [PATCH] GitLab reviews may not have the updated_at field set (#18450) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- services/migrations/gitlab.go | 11 ++- services/migrations/gitlab_test.go | 140 +++++++++++++++++++++++++++++ services/migrations/main_test.go | 18 ++-- 3 files changed, 159 insertions(+), 10 deletions(-) diff --git a/services/migrations/gitlab.go b/services/migrations/gitlab.go index a9856739c2..97ebc4dd8b 100644 --- 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 6b77aa62ef..ad61577653 100644 --- 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 ddf73df98e..b040df83d1 100644 --- 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) }