From 8151b4591e66539f21a0280565d7eba36a6ac6e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ga=C4=87e=C5=A1a?= Date: Tue, 17 Jan 2023 16:04:30 +0100 Subject: [PATCH] PR change state API; Removed rejected state (#220) --- internal/api/controller/pullreq/controller.go | 38 ++++- internal/api/controller/pullreq/merge.go | 11 +- internal/api/controller/pullreq/pr_create.go | 20 +-- internal/api/controller/pullreq/pr_state.go | 154 ++++++++++++++++++ internal/api/handler/pullreq/pr_state.go | 49 ++++++ internal/api/openapi/pullreq.go | 16 ++ internal/api/request/pullreq.go | 3 - internal/router/api.go | 1 + .../0003_create_table_pullreqs.up.sql | 1 + .../sqlite/0003_create_table_pullreqs.up.sql | 1 + internal/store/database/pullreq.go | 9 +- types/enum/pullreq.go | 12 +- types/pullreq.go | 3 +- 13 files changed, 283 insertions(+), 35 deletions(-) create mode 100644 internal/api/controller/pullreq/pr_state.go create mode 100644 internal/api/handler/pullreq/pr_state.go diff --git a/internal/api/controller/pullreq/controller.go b/internal/api/controller/pullreq/controller.go index 275ceb84b..f2dbe7be8 100644 --- a/internal/api/controller/pullreq/controller.go +++ b/internal/api/controller/pullreq/controller.go @@ -63,7 +63,8 @@ func NewController( } func (c *Controller) verifyBranchExistence(ctx context.Context, - repo *types.Repository, branch string) error { + repo *types.Repository, branch string, +) error { if branch == "" { return usererror.BadRequest("branch name can't be empty") } @@ -87,7 +88,8 @@ func (c *Controller) verifyBranchExistence(ctx context.Context, } func (c *Controller) getRepoCheckAccess(ctx context.Context, - session *auth.Session, repoRef string, reqPermission enum.Permission) (*types.Repository, error) { + session *auth.Session, repoRef string, reqPermission enum.Permission, +) (*types.Repository, error) { if repoRef == "" { return nil, usererror.BadRequest("A valid repository reference must be provided.") } @@ -105,7 +107,8 @@ func (c *Controller) getRepoCheckAccess(ctx context.Context, } func (c *Controller) getCommentCheckEditAccess(ctx context.Context, - session *auth.Session, pr *types.PullReq, commentID int64) (*types.PullReqActivity, error) { + session *auth.Session, pr *types.PullReq, commentID int64, +) (*types.PullReqActivity, error) { if commentID <= 0 { return nil, usererror.BadRequest("A valid comment ID must be provided.") } @@ -172,3 +175,32 @@ func (c *Controller) writeReplyActivity(ctx context.Context, parent, act *types. return nil } + +func (c *Controller) checkIfAlreadyExists(ctx context.Context, + targetRepoID, sourceRepoID int64, targetBranch, sourceBranch string, +) error { + existing, err := c.pullreqStore.List(ctx, + targetRepoID, &types.PullReqFilter{ + SourceRepoID: sourceRepoID, + SourceBranch: sourceBranch, + TargetBranch: targetBranch, + States: []enum.PullReqState{enum.PullReqStateOpen}, + Size: 1, + Sort: enum.PullReqSortNumber, + Order: enum.OrderAsc, + }) + if err != nil { + return fmt.Errorf("failed to get existing pull requests: %w", err) + } + if len(existing) > 0 { + return usererror.BadRequest( + "a pull request for this target and source branch already exists", + map[string]any{ + "type": "pr already exists", + "number": existing[0].Number, + }, + ) + } + + return nil +} diff --git a/internal/api/controller/pullreq/merge.go b/internal/api/controller/pullreq/merge.go index c8eda7ac0..8752cb3f7 100644 --- a/internal/api/controller/pullreq/merge.go +++ b/internal/api/controller/pullreq/merge.go @@ -6,7 +6,6 @@ package pullreq import ( "context" - "errors" "fmt" "time" @@ -29,7 +28,7 @@ type MergeInput struct { // Merge merges the pull request. // -//nolint:gocognit // no need to refactor +//nolint:gocognit,funlen // no need to refactor func (c *Controller) Merge( ctx context.Context, session *auth.Session, @@ -66,11 +65,15 @@ func (c *Controller) Merge( } if pr.Merged != nil { - return errors.New("pull request already merged") + return usererror.BadRequest("Pull request already merged") } if pr.State != enum.PullReqStateOpen { - return fmt.Errorf("pull request state cannot be %v", pr.State) + return usererror.BadRequest("Pull request must be open") + } + + if pr.IsDraft { + return usererror.BadRequest("Draft pull requests can't be merged. Clear the draft flag first.") } sourceRepo := targetRepo diff --git a/internal/api/controller/pullreq/pr_create.go b/internal/api/controller/pullreq/pr_create.go index e96612f02..87f9dcc07 100644 --- a/internal/api/controller/pullreq/pr_create.go +++ b/internal/api/controller/pullreq/pr_create.go @@ -61,23 +61,8 @@ func (c *Controller) Create( return nil, errBranch } - existing, err := c.pullreqStore.List(ctx, targetRepo.ID, &types.PullReqFilter{ - SourceRepoID: sourceRepo.ID, - SourceBranch: in.SourceBranch, - TargetBranch: in.TargetBranch, - States: []enum.PullReqState{enum.PullReqStateOpen}, - Size: 1, - Sort: enum.PullReqSortNumber, - Order: enum.OrderAsc, - }) - if err != nil { - return nil, fmt.Errorf("failed to count existing pull requests: %w", err) - } - if len(existing) > 0 { - return nil, usererror.BadRequest( - "a pull request for this target and source branch already exists", - map[string]any{"number": existing[0].Number}, - ) + if err = c.checkIfAlreadyExists(ctx, targetRepo.ID, sourceRepo.ID, in.SourceBranch, in.TargetBranch); err != nil { + return nil, err } targetRepo, err = c.repoStore.UpdateOptLock(ctx, targetRepo, func(repo *types.Repository) error { @@ -111,6 +96,7 @@ func newPullReq(session *auth.Session, number int64, Updated: now, Edited: now, State: enum.PullReqStateOpen, + IsDraft: false, Title: in.Title, Description: in.Description, SourceRepoID: sourceRepo.ID, diff --git a/internal/api/controller/pullreq/pr_state.go b/internal/api/controller/pullreq/pr_state.go new file mode 100644 index 000000000..49618b70a --- /dev/null +++ b/internal/api/controller/pullreq/pr_state.go @@ -0,0 +1,154 @@ +// Copyright 2022 Harness Inc. All rights reserved. +// Use of this source code is governed by the Polyform Free Trial License +// that can be found in the LICENSE.md file for this repository. + +package pullreq + +import ( + "context" + "fmt" + "strings" + "time" + + apiauth "github.com/harness/gitness/internal/api/auth" + "github.com/harness/gitness/internal/api/usererror" + "github.com/harness/gitness/internal/auth" + "github.com/harness/gitness/internal/store/database/dbtx" + "github.com/harness/gitness/types" + "github.com/harness/gitness/types/enum" + + "github.com/rs/zerolog/log" +) + +type StateInput struct { + State enum.PullReqState `json:"state"` + IsDraft bool `json:"is_draft"` + Message string `json:"message"` +} + +// State updates the pull request's current state. +// +//nolint:gocognit +func (c *Controller) State(ctx context.Context, + session *auth.Session, repoRef string, pullreqNum int64, in *StateInput) (*types.PullReq, error) { + var pr *types.PullReq + + targetRepo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoEdit) + if err != nil { + return nil, fmt.Errorf("failed to acquire access to target repo: %w", err) + } + + state, ok := in.State.Sanitize() + if !ok { + return nil, usererror.BadRequest(fmt.Sprintf("Allowed states are: %s and %s", + enum.PullReqStateOpen, enum.PullReqStateClosed)) + } + + in.State = state + in.Message = strings.TrimSpace(in.Message) + + if in.State == enum.PullReqStateMerged { + return nil, usererror.BadRequest("Pull requests can't be merged with this API") + } + + var activity *types.PullReqActivity + + err = dbtx.New(c.db).WithTx(ctx, func(ctx context.Context) error { + pr, err = c.pullreqStore.FindByNumber(ctx, targetRepo.ID, pullreqNum) + if err != nil { + return fmt.Errorf("failed to get pull request by number: %w", err) + } + + if pr.SourceRepoID != pr.TargetRepoID { + var sourceRepo *types.Repository + + sourceRepo, err = c.repoStore.Find(ctx, pr.SourceRepoID) + if err != nil { + return fmt.Errorf("failed to get source repo by id: %w", err) + } + + if err = apiauth.CheckRepo(ctx, c.authorizer, session, sourceRepo, + enum.PermissionRepoView, false); err != nil { + return fmt.Errorf("failed to acquire access to source repo: %w", err) + } + } + + if pr.State == enum.PullReqStateMerged { + return usererror.BadRequest("Merged pull requests can't be modified.") + } + + if pr.State == in.State && in.IsDraft == pr.IsDraft { + return nil // no changes are necessary: state is the same and is_draft hasn't change + } + + if pr.State != enum.PullReqStateOpen && in.State == enum.PullReqStateOpen { + err = c.checkIfAlreadyExists(ctx, pr.TargetRepoID, pr.SourceRepoID, pr.TargetBranch, pr.SourceBranch) + if err != nil { + return err + } + } + + activity = getStateActivity(session, pr, in) + + pr.State = in.State + pr.IsDraft = in.IsDraft + pr.Edited = time.Now().UnixMilli() + + err = c.pullreqStore.Update(ctx, pr) + if err != nil { + return fmt.Errorf("failed to update pull request: %w", err) + } + + return nil + }) + if err != nil { + return nil, err + } + + // Write a row to the pull request activity + if activity != nil { + err = c.writeActivity(ctx, pr, activity) + if err != nil { + // non-critical error + log.Err(err).Msg("failed to write pull req activity") + } + } + + return pr, nil +} + +func getStateActivity(session *auth.Session, pr *types.PullReq, in *StateInput) *types.PullReqActivity { + now := time.Now().UnixMilli() + payload := map[string]interface{}{ + "old": pr.State, + "new": in.State, + "is_draft": in.IsDraft, + } + if len(in.Message) != 0 { + payload["message"] = in.Message + } + + act := &types.PullReqActivity{ + ID: 0, // Will be populated in the data layer + Version: 0, + CreatedBy: session.Principal.ID, + Created: now, + Updated: now, + Edited: now, + Deleted: nil, + RepoID: pr.TargetRepoID, + PullReqID: pr.ID, + Order: 0, // Will be filled in writeActivity + SubOrder: 0, + ReplySeq: 0, + Type: enum.PullReqActivityTypeStateChange, + Kind: enum.PullReqActivityKindSystem, + Text: "", + Payload: payload, + Metadata: nil, + ResolvedBy: nil, + Resolved: nil, + } + + return act +} diff --git a/internal/api/handler/pullreq/pr_state.go b/internal/api/handler/pullreq/pr_state.go new file mode 100644 index 000000000..11e443790 --- /dev/null +++ b/internal/api/handler/pullreq/pr_state.go @@ -0,0 +1,49 @@ +// Copyright 2022 Harness Inc. All rights reserved. +// Use of this source code is governed by the Polyform Free Trial License +// that can be found in the LICENSE.md file for this repository. + +package pullreq + +import ( + "encoding/json" + "net/http" + + "github.com/harness/gitness/internal/api/controller/pullreq" + "github.com/harness/gitness/internal/api/render" + "github.com/harness/gitness/internal/api/request" +) + +// HandleState handles API call to update pull request state. +func HandleState(pullreqCtrl *pullreq.Controller) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + session, _ := request.AuthSessionFrom(ctx) + + repoRef, err := request.GetRepoRefFromPath(r) + if err != nil { + render.TranslatedUserError(w, err) + return + } + + pullreqNumber, err := request.GetPullReqNumberFromPath(r) + if err != nil { + render.TranslatedUserError(w, err) + return + } + + in := new(pullreq.StateInput) + err = json.NewDecoder(r.Body).Decode(in) + if err != nil { + render.BadRequestf(w, "Invalid Request Body: %s.", err) + return + } + + pr, err := pullreqCtrl.State(ctx, session, repoRef, pullreqNumber, in) + if err != nil { + render.TranslatedUserError(w, err) + return + } + + render.JSON(w, http.StatusOK, pr) + } +} diff --git a/internal/api/openapi/pullreq.go b/internal/api/openapi/pullreq.go index 3e6e087a1..06bd5f391 100644 --- a/internal/api/openapi/pullreq.go +++ b/internal/api/openapi/pullreq.go @@ -40,6 +40,11 @@ type updatePullReqRequest struct { pullreq.UpdateInput } +type statePullReqRequest struct { + pullReqRequest + pullreq.StateInput +} + type listPullReqActivitiesRequest struct { pullReqRequest } @@ -297,6 +302,17 @@ func pullReqOperations(reflector *openapi3.Reflector) { _ = reflector.SetJSONResponse(&putPullReq, new(usererror.Error), http.StatusForbidden) _ = reflector.Spec.AddOperation(http.MethodPatch, "/repos/{repo_ref}/pullreq/{pullreq_number}", putPullReq) + statePullReq := openapi3.Operation{} + statePullReq.WithTags("pullreq") + statePullReq.WithMapOfAnything(map[string]interface{}{"operationId": "statePullReq"}) + _ = reflector.SetRequest(&statePullReq, new(statePullReqRequest), http.MethodPatch) + _ = reflector.SetJSONResponse(&statePullReq, new(types.PullReq), http.StatusOK) + _ = reflector.SetJSONResponse(&statePullReq, new(usererror.Error), http.StatusBadRequest) + _ = reflector.SetJSONResponse(&statePullReq, new(usererror.Error), http.StatusInternalServerError) + _ = reflector.SetJSONResponse(&statePullReq, new(usererror.Error), http.StatusUnauthorized) + _ = reflector.SetJSONResponse(&statePullReq, new(usererror.Error), http.StatusForbidden) + _ = reflector.Spec.AddOperation(http.MethodPost, "/repos/{repo_ref}/pullreq/{pullreq_number}/state", statePullReq) + listPullReqActivities := openapi3.Operation{} listPullReqActivities.WithTags("pullreq") listPullReqActivities.WithMapOfAnything(map[string]interface{}{"operationId": "listPullReqActivities"}) diff --git a/internal/api/request/pullreq.go b/internal/api/request/pullreq.go index c081c6d28..903c69332 100644 --- a/internal/api/request/pullreq.go +++ b/internal/api/request/pullreq.go @@ -35,9 +35,6 @@ func parsePullReqStates(r *http.Request) []enum.PullReqState { strStates := r.Form[QueryParamState] m := make(map[enum.PullReqState]struct{}) // use map to eliminate duplicates for _, s := range strStates { - if s == "" { - continue - } if state, ok := enum.PullReqState(s).Sanitize(); ok { m[state] = struct{}{} } diff --git a/internal/router/api.go b/internal/router/api.go index 37a7f3677..f532fe69f 100644 --- a/internal/router/api.go +++ b/internal/router/api.go @@ -232,6 +232,7 @@ func SetupPullReq(r chi.Router, pullreqCtrl *pullreq.Controller) { r.Route(fmt.Sprintf("/{%s}", request.PathParamPullReqNumber), func(r chi.Router) { r.Get("/", handlerpullreq.HandleFind(pullreqCtrl)) r.Patch("/", handlerpullreq.HandleUpdate(pullreqCtrl)) + r.Post("/state", handlerpullreq.HandleState(pullreqCtrl)) r.Get("/activities", handlerpullreq.HandleListActivities(pullreqCtrl)) r.Route("/comments", func(r chi.Router) { r.Post("/", handlerpullreq.HandleCommentCreate(pullreqCtrl)) diff --git a/internal/store/database/migrate/postgres/0003_create_table_pullreqs.up.sql b/internal/store/database/migrate/postgres/0003_create_table_pullreqs.up.sql index d61d37022..37833c598 100644 --- a/internal/store/database/migrate/postgres/0003_create_table_pullreqs.up.sql +++ b/internal/store/database/migrate/postgres/0003_create_table_pullreqs.up.sql @@ -7,6 +7,7 @@ pullreq_id SERIAL PRIMARY KEY ,pullreq_edited BIGINT NOT NULL ,pullreq_number INTEGER NOT NULL ,pullreq_state TEXT NOT NULL +,pullreq_is_draft TEXT NOT NULL DEFAULT FALSE ,pullreq_title TEXT NOT NULL ,pullreq_description TEXT NOT NULL ,pullreq_source_repo_id INTEGER NOT NULL diff --git a/internal/store/database/migrate/sqlite/0003_create_table_pullreqs.up.sql b/internal/store/database/migrate/sqlite/0003_create_table_pullreqs.up.sql index ddca7864b..e8a05dda5 100644 --- a/internal/store/database/migrate/sqlite/0003_create_table_pullreqs.up.sql +++ b/internal/store/database/migrate/sqlite/0003_create_table_pullreqs.up.sql @@ -7,6 +7,7 @@ pullreq_id INTEGER PRIMARY KEY AUTOINCREMENT ,pullreq_edited BIGINT NOT NULL ,pullreq_number INTEGER NOT NULL ,pullreq_state TEXT NOT NULL +,pullreq_is_draft TEXT NOT NULL DEFAULT FALSE ,pullreq_title TEXT NOT NULL ,pullreq_description TEXT NOT NULL ,pullreq_source_repo_id INTEGER NOT NULL diff --git a/internal/store/database/pullreq.go b/internal/store/database/pullreq.go index 4c8d1cac9..d530c850e 100644 --- a/internal/store/database/pullreq.go +++ b/internal/store/database/pullreq.go @@ -52,7 +52,8 @@ type pullReq struct { Updated int64 `db:"pullreq_updated"` Edited int64 `db:"pullreq_edited"` - State enum.PullReqState `db:"pullreq_state"` + State enum.PullReqState `db:"pullreq_state"` + IsDraft bool `db:"pullreq_is_draft"` Title string `db:"pullreq_title"` Description string `db:"pullreq_description"` @@ -81,6 +82,7 @@ const ( ,pullreq_updated ,pullreq_edited ,pullreq_state + ,pullreq_is_draft ,pullreq_title ,pullreq_description ,pullreq_source_repo_id @@ -155,6 +157,7 @@ func (s *PullReqStore) Create(ctx context.Context, pr *types.PullReq) error { ,pullreq_updated ,pullreq_edited ,pullreq_state + ,pullreq_is_draft ,pullreq_title ,pullreq_description ,pullreq_source_repo_id @@ -173,6 +176,7 @@ func (s *PullReqStore) Create(ctx context.Context, pr *types.PullReq) error { ,:pullreq_updated ,:pullreq_edited ,:pullreq_state + ,:pullreq_is_draft ,:pullreq_title ,:pullreq_description ,:pullreq_source_repo_id @@ -208,6 +212,7 @@ func (s *PullReqStore) Update(ctx context.Context, pr *types.PullReq) error { ,pullreq_updated = :pullreq_updated ,pullreq_edited = :pullreq_edited ,pullreq_state = :pullreq_state + ,pullreq_is_draft = :pullreq_is_draft ,pullreq_title = :pullreq_title ,pullreq_description = :pullreq_description ,pullreq_activity_seq = :pullreq_activity_seq @@ -401,6 +406,7 @@ func mapPullReq(pr *pullReq) *types.PullReq { Updated: pr.Updated, Edited: pr.Edited, State: pr.State, + IsDraft: pr.IsDraft, Title: pr.Title, Description: pr.Description, SourceRepoID: pr.SourceRepoID, @@ -428,6 +434,7 @@ func mapInternalPullReq(pr *types.PullReq) *pullReq { Updated: pr.Updated, Edited: pr.Edited, State: pr.State, + IsDraft: pr.IsDraft, Title: pr.Title, Description: pr.Description, SourceRepoID: pr.SourceRepoID, diff --git a/types/enum/pullreq.go b/types/enum/pullreq.go index a35b385a5..9e2cb4cbd 100644 --- a/types/enum/pullreq.go +++ b/types/enum/pullreq.go @@ -9,21 +9,19 @@ type PullReqState string func (PullReqState) Enum() []interface{} { return toInterfaceSlice(pullReqStates) } func (s PullReqState) Sanitize() (PullReqState, bool) { return Sanitize(s, GetAllPullReqStates) } -func GetAllPullReqStates() ([]PullReqState, PullReqState) { return pullReqStates, PullReqStateOpen } +func GetAllPullReqStates() ([]PullReqState, PullReqState) { return pullReqStates, "" } // PullReqState enumeration. const ( - PullReqStateOpen PullReqState = "open" - PullReqStateMerged PullReqState = "merged" - PullReqStateClosed PullReqState = "closed" - PullReqStateRejected PullReqState = "rejected" + PullReqStateOpen PullReqState = "open" + PullReqStateMerged PullReqState = "merged" + PullReqStateClosed PullReqState = "closed" ) var pullReqStates = sortEnum([]PullReqState{ PullReqStateOpen, PullReqStateMerged, PullReqStateClosed, - PullReqStateRejected, }) // PullReqSort defines pull request attribute that can be used for sorting. @@ -67,6 +65,7 @@ const ( PullReqActivityTypeComment PullReqActivityType = "comment" PullReqActivityTypeCodeComment PullReqActivityType = "code-comment" PullReqActivityTypeTitleChange PullReqActivityType = "title-change" + PullReqActivityTypeStateChange PullReqActivityType = "state-change" PullReqActivityTypeReviewSubmit PullReqActivityType = "review-submit" PullReqActivityTypeMerge PullReqActivityType = "merge" ) @@ -75,6 +74,7 @@ var pullReqActivityTypes = sortEnum([]PullReqActivityType{ PullReqActivityTypeComment, PullReqActivityTypeCodeComment, PullReqActivityTypeTitleChange, + PullReqActivityTypeStateChange, PullReqActivityTypeReviewSubmit, PullReqActivityTypeMerge, }) diff --git a/types/pullreq.go b/types/pullreq.go index c2b6cb9f8..053c1d636 100644 --- a/types/pullreq.go +++ b/types/pullreq.go @@ -19,7 +19,8 @@ type PullReq struct { Updated int64 `json:"-"` // not returned, it's updated by the server internally. Clients should use EditedAt. Edited int64 `json:"edited"` - State enum.PullReqState `json:"state"` + State enum.PullReqState `json:"state"` + IsDraft bool `json:"is_draft"` Title string `json:"title"` Description string `json:"description"`