PR change state API; Removed rejected state (#220)

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

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

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,

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

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

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

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{}{}
 		}

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

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

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

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,

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

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"`