diff options
author | Aravinth Manivannan | 2022-01-30 13:56:39 +0000 |
---|---|---|
committer | GitHub | 2022-01-30 14:56:39 +0100 |
commit | f93d72c09be24f5a11f6543867736f46d1f905fd (patch) | |
tree | a8f6adcada04fde58a56a3143900d8b56d89489d | |
parent | 2f22337125f496ada3a4d17148bb6b2a82c639e5 (diff) |
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
-rw-r--r-- | services/migrations/gitlab.go | 56 | ||||
-rw-r--r-- | services/migrations/gitlab_test.go | 140 | ||||
-rw-r--r-- | services/migrations/main_test.go | 18 |
3 files changed, 181 insertions, 33 deletions
diff --git a/services/migrations/gitlab.go b/services/migrations/gitlab.go index 4eb7e3e47..97ebc4dd8 100644 --- a/services/migrations/gitlab.go +++ b/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 - var state = "all" - var milestones = make([]*base.Milestone, 0, perPage) + perPage := g.maxPerPage + state := "all" + 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 - var labels = make([]*base.Label, 0, perPage) + perPage := g.maxPerPage + 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 - var releases = make([]*base.Release, 0, perPage) + perPage := g.maxPerPage + 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, }) diff --git a/services/migrations/gitlab_test.go b/services/migrations/gitlab_test.go index 6b77aa62e..ad6157765 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 ddf73df98..b040df83d 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) } |