From 658d1bfac8c4706d83004c4069cd52ef63fb71cb Mon Sep 17 00:00:00 2001 From: Norwin Date: Sat, 13 Mar 2021 18:06:52 +0000 Subject: [PATCH] API: fix set milestone on PR creation (#14981) * API: fix set milestone on PR creation pr creation via API failed with 404, because we searched for milestoneID 0, due to uninitialized var usage D: * add tests * fix expected status codes * fix tests Co-authored-by: 6543 <6543@obermui.de> --- integrations/api_pull_test.go | 73 +++++++++++++++++++++++++++++++++- models/fixtures/label.yml | 8 ++++ models/fixtures/milestone.yml | 8 ++++ models/fixtures/repository.yml | 3 +- routers/api/v1/repo/pull.go | 4 +- 5 files changed, 91 insertions(+), 5 deletions(-) diff --git a/integrations/api_pull_test.go b/integrations/api_pull_test.go index 369f4ce31..7aa0bfc54 100644 --- a/integrations/api_pull_test.go +++ b/integrations/api_pull_test.go @@ -74,8 +74,79 @@ func TestAPICreatePullSuccess(t *testing.T) { Base: "master", Title: "create a failure pr", }) - session.MakeRequest(t, req, 201) + session.MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail +} + +func TestAPICreatePullWithFieldsSuccess(t *testing.T) { + defer prepareTestEnv(t)() + // repo10 have code, pulls units. + repo10 := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 10}).(*models.Repository) + owner10 := models.AssertExistsAndLoadBean(t, &models.User{ID: repo10.OwnerID}).(*models.User) + // repo11 only have code unit but should still create pulls + repo11 := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 11}).(*models.Repository) + owner11 := models.AssertExistsAndLoadBean(t, &models.User{ID: repo11.OwnerID}).(*models.User) + + session := loginUser(t, owner11.Name) + token := getTokenForLoggedInUser(t, session) + + opts := &api.CreatePullRequestOption{ + Head: fmt.Sprintf("%s:master", owner11.Name), + Base: "master", + Title: "create a failure pr", + Body: "foobaaar", + Milestone: 5, + Assignees: []string{owner10.Name}, + Labels: []int64{5}, + } + + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls?token=%s", owner10.Name, repo10.Name, token), opts) + + res := session.MakeRequest(t, req, 201) + pull := new(api.PullRequest) + DecodeJSON(t, res, pull) + + assert.NotNil(t, pull.Milestone) + assert.EqualValues(t, opts.Milestone, pull.Milestone.ID) + if assert.Len(t, pull.Assignees, 1) { + assert.EqualValues(t, opts.Assignees[0], owner10.Name) + } + assert.NotNil(t, pull.Labels) + assert.EqualValues(t, opts.Labels[0], pull.Labels[0].ID) +} + +func TestAPICreatePullWithFieldsFailure(t *testing.T) { + defer prepareTestEnv(t)() + // repo10 have code, pulls units. + repo10 := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 10}).(*models.Repository) + owner10 := models.AssertExistsAndLoadBean(t, &models.User{ID: repo10.OwnerID}).(*models.User) + // repo11 only have code unit but should still create pulls + repo11 := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 11}).(*models.Repository) + owner11 := models.AssertExistsAndLoadBean(t, &models.User{ID: repo11.OwnerID}).(*models.User) + + session := loginUser(t, owner11.Name) + token := getTokenForLoggedInUser(t, session) + + opts := &api.CreatePullRequestOption{ + Head: fmt.Sprintf("%s:master", owner11.Name), + Base: "master", + } + + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls?token=%s", owner10.Name, repo10.Name, token), opts) + session.MakeRequest(t, req, http.StatusUnprocessableEntity) + opts.Title = "is required" + + opts.Milestone = 666 + session.MakeRequest(t, req, http.StatusUnprocessableEntity) + opts.Milestone = 5 + + opts.Assignees = []string{"qweruqweroiuyqweoiruywqer"} + session.MakeRequest(t, req, http.StatusUnprocessableEntity) + opts.Assignees = []string{owner10.LoginName} + + opts.Labels = []int64{55555} + session.MakeRequest(t, req, http.StatusUnprocessableEntity) + opts.Labels = []int64{5} } func TestAPIEditPull(t *testing.T) { diff --git a/models/fixtures/label.yml b/models/fixtures/label.yml index 3ad82eebe..1b7ce7468 100644 --- a/models/fixtures/label.yml +++ b/models/fixtures/label.yml @@ -33,3 +33,11 @@ num_issues: 1 num_closed_issues: 0 +- + id: 5 + repo_id: 10 + org_id: 0 + name: pull-test-label + color: '#000000' + num_issues: 0 + num_closed_issues: 0 diff --git a/models/fixtures/milestone.yml b/models/fixtures/milestone.yml index b9894a100..2cd994e63 100644 --- a/models/fixtures/milestone.yml +++ b/models/fixtures/milestone.yml @@ -29,3 +29,11 @@ content: content random is_closed: false num_issues: 0 + +- + id: 5 + repo_id: 10 + name: milestone of repo 10 + content: for testing with PRs + is_closed: false + num_issues: 0 diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml index 952408b0c..492040316 100644 --- a/models/fixtures/repository.yml +++ b/models/fixtures/repository.yml @@ -148,6 +148,7 @@ num_closed_issues: 0 num_pulls: 1 num_closed_pulls: 0 + num_milestones: 1 is_mirror: false num_forks: 1 status: 0 @@ -734,4 +735,4 @@ num_watches: 0 num_projects: 0 num_closed_projects: 0 - status: 0 \ No newline at end of file + status: 0 diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 8eda94965..e3be5b4af 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -295,7 +295,6 @@ func CreatePullRequest(ctx *context.APIContext) { var ( repo = ctx.Repo.Repository labelIDs []int64 - assigneeID int64 milestoneID int64 ) @@ -356,7 +355,7 @@ func CreatePullRequest(ctx *context.APIContext) { } if form.Milestone > 0 { - milestone, err := models.GetMilestoneByRepoID(ctx.Repo.Repository.ID, milestoneID) + milestone, err := models.GetMilestoneByRepoID(ctx.Repo.Repository.ID, form.Milestone) if err != nil { if models.IsErrMilestoneNotExist(err) { ctx.NotFound() @@ -380,7 +379,6 @@ func CreatePullRequest(ctx *context.APIContext) { PosterID: ctx.User.ID, Poster: ctx.User, MilestoneID: milestoneID, - AssigneeID: assigneeID, IsPull: true, Content: form.Body, DeadlineUnix: deadlineUnix,