Implement Draft Pull Request Feature #2319 (#2336)

src/main/resources/update/gitbucket-core_4.32.xml

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<changeSet>
+  <addColumn tableName="PULL_REQUEST">
+    <column name="IS_DRAFT" type="boolean" nullable="false" defaultValueBoolean="false" />
+  </addColumn>
+</changeSet>

src/main/scala/gitbucket/core/GitBucketCoreModule.scala

@@ -63,5 +63,6 @@ object GitBucketCoreModule
       new Version("4.30.1"),
       new Version("4.31.0", new LiquibaseMigration("update/gitbucket-core_4.31.xml")),
       new Version("4.31.1"),
-      new Version("4.31.2")
+      new Version("4.31.2"),
+      new Version("4.32.0", new LiquibaseMigration("update/gitbucket-core_4.32.xml"))
     )

src/main/scala/gitbucket/core/controller/PullRequestsController.scala

@@ -69,6 +69,7 @@ trait PullRequestsControllerBase extends ControllerBase {
     "requestBranch" -> trim(text(required, maxlength(100))),
     "commitIdFrom" -> trim(text(required, maxlength(40))),
     "commitIdTo" -> trim(text(required, maxlength(40))),
+    "isDraft" -> trim(boolean(required)),
     "assignedUserName" -> trim(optional(text())),
     "milestoneId" -> trim(optional(number())),
     "priorityId" -> trim(optional(number())),
@@ -77,7 +78,8 @@ trait PullRequestsControllerBase extends ControllerBase {
 
   val mergeForm = mapping(
     "message" -> trim(label("Message", text(required))),
-    "strategy" -> trim(label("Strategy", text(required)))
+    "strategy" -> trim(label("Strategy", text(required))),
+    "isDraft" -> trim(boolean(required))
   )(MergeForm.apply)
 
   case class PullRequestForm(
@@ -90,13 +92,14 @@ trait PullRequestsControllerBase extends ControllerBase {
     requestBranch: String,
     commitIdFrom: String,
     commitIdTo: String,
+    isDraft: Boolean,
     assignedUserName: Option[String],
     milestoneId: Option[Int],
     priorityId: Option[Int],
     labelNames: Option[String]
   )
 
-  case class MergeForm(message: String, strategy: String)
+  case class MergeForm(message: String, strategy: String, isDraft: Boolean)
 
   get("/:owner/:repository/pulls")(referrersOnly { repository =>
     val q = request.getParameter("q")
@@ -286,6 +289,7 @@ trait PullRequestsControllerBase extends ControllerBase {
           flash += "error" -> s"""Can't delete the default branch "${pullreq.requestBranch}"."""
         }
       }
+
       redirect(s"/${baseRepository.owner}/${baseRepository.name}/pull/${issueId}")
     }) getOrElse NotFound()
   })
@@ -338,14 +342,26 @@ trait PullRequestsControllerBase extends ControllerBase {
     }) getOrElse NotFound()
   })
 
+  post("/:owner/:repository/pull/:id/update_draft")(readableUsersOnly { baseRepository =>
+    (for {
+      issueId <- params("id").toIntOpt
+      (_, pullreq) <- getPullRequest(baseRepository.owner, baseRepository.name, issueId)
+      owner = pullreq.requestUserName
+      name = pullreq.requestRepositoryName
+      if hasDeveloperRole(owner, name, context.loginAccount)
+    } yield {
+      updateDraftToPullRequest(baseRepository.owner, baseRepository.name, issueId)
+    }) getOrElse NotFound()
+  })
+
   post("/:owner/:repository/pull/:id/merge", mergeForm)(writableUsersOnly { (form, repository) =>
     params("id").toIntOpt.flatMap { issueId =>
       val owner = repository.owner
       val name = repository.name
 
-      mergePullRequest(repository, issueId, context.loginAccount.get, form.message, form.strategy) match {
+      mergePullRequest(repository, issueId, context.loginAccount.get, form.message, form.strategy, form.isDraft) match {
         case Right(objectId) => redirect(s"/${owner}/${name}/pull/${issueId}")
-        case Left(message)   => Some(BadRequest())
+        case Left(message)   => Some(BadRequest(message))
       }
     } getOrElse NotFound()
   })
@@ -543,6 +559,7 @@ trait PullRequestsControllerBase extends ControllerBase {
           requestBranch = form.requestBranch,
           commitIdFrom = form.commitIdFrom,
           commitIdTo = form.commitIdTo,
+          isDraft = form.isDraft,
           loginAccount = context.loginAccount.get
         )
 

src/main/scala/gitbucket/core/controller/api/ApiPullRequestControllerBase.scala

@@ -114,6 +114,7 @@ trait ApiPullRequestControllerBase extends ControllerBase {
                       requestBranch = reqBranch,
                       commitIdFrom = commitIdFrom.getName,
                       commitIdTo = commitIdTo.getName,
+                      isDraft = false,
                       loginAccount = context.loginAccount.get
                     )
                     getApiPullRequest(repository, issueId).map(JsonFormat(_))
@@ -141,6 +142,7 @@ trait ApiPullRequestControllerBase extends ControllerBase {
                       requestBranch = reqBranch,
                       commitIdFrom = commitIdFrom.getName,
                       commitIdTo = commitIdTo.getName,
+                      isDraft = false,
                       loginAccount = context.loginAccount.get
                     )
                     getApiPullRequest(repository, createPullReqAlt.issue).map(JsonFormat(_))

src/main/scala/gitbucket/core/model/PullRequest.scala

@@ -12,6 +12,7 @@ trait PullRequestComponent extends TemplateComponent { self: Profile =>
     val requestBranch = column[String]("REQUEST_BRANCH")
     val commitIdFrom = column[String]("COMMIT_ID_FROM")
     val commitIdTo = column[String]("COMMIT_ID_TO")
+    val isDraft = column[Boolean]("IS_DRAFT")
     def * =
       (
         userName,
@@ -22,7 +23,8 @@ trait PullRequestComponent extends TemplateComponent { self: Profile =>
         requestRepositoryName,
         requestBranch,
         commitIdFrom,
-        commitIdTo
+        commitIdTo,
+        isDraft
       ) <> (PullRequest.tupled, PullRequest.unapply)
 
     def byPrimaryKey(userName: String, repositoryName: String, issueId: Int) =
@@ -41,5 +43,6 @@ case class PullRequest(
   requestRepositoryName: String,
   requestBranch: String,
   commitIdFrom: String,
-  commitIdTo: String
+  commitIdTo: String,
+  isDraft: Boolean
 )

src/main/scala/gitbucket/core/service/MergeService.scala

@@ -252,135 +252,137 @@ trait MergeService {
     issueId: Int,
     loginAccount: Account,
     message: String,
-    strategy: String
+    strategy: String,
+    isDraft: Boolean
   )(implicit s: Session, c: JsonFormat.Context, context: Context): Either[String, ObjectId] = {
-    if (repository.repository.options.mergeOptions.split(",").contains(strategy)) {
-      LockUtil.lock(s"${repository.owner}/${repository.name}") {
-        getPullRequest(repository.owner, repository.name, issueId)
-          .map {
-            case (issue, pullreq) =>
-              using(Git.open(getRepositoryDir(repository.owner, repository.name))) { git =>
-                // mark issue as merged and close.
-                val commentId =
-                  createComment(repository.owner, repository.name, loginAccount.userName, issueId, message, "merge")
-                createComment(repository.owner, repository.name, loginAccount.userName, issueId, "Close", "close")
-                updateClosed(repository.owner, repository.name, issueId, true)
+    if (!isDraft) {
+      if (repository.repository.options.mergeOptions.split(",").contains(strategy)) {
+        LockUtil.lock(s"${repository.owner}/${repository.name}") {
+          getPullRequest(repository.owner, repository.name, issueId)
+            .map {
+              case (issue, pullreq) =>
+                using(Git.open(getRepositoryDir(repository.owner, repository.name))) { git =>
+                  // mark issue as merged and close.
+                  val commentId =
+                    createComment(repository.owner, repository.name, loginAccount.userName, issueId, message, "merge")
+                  createComment(repository.owner, repository.name, loginAccount.userName, issueId, "Close", "close")
+                  updateClosed(repository.owner, repository.name, issueId, true)
 
-                // record activity
-                recordMergeActivity(repository.owner, repository.name, loginAccount.userName, issueId, message)
+                  // record activity
+                  recordMergeActivity(repository.owner, repository.name, loginAccount.userName, issueId, message)
 
-                val (commits, _) = getRequestCompareInfo(
-                  repository.owner,
-                  repository.name,
-                  pullreq.commitIdFrom,
-                  pullreq.requestUserName,
-                  pullreq.requestRepositoryName,
-                  pullreq.commitIdTo
-                )
+                  val (commits, _) = getRequestCompareInfo(
+                    repository.owner,
+                    repository.name,
+                    pullreq.commitIdFrom,
+                    pullreq.requestUserName,
+                    pullreq.requestRepositoryName,
+                    pullreq.commitIdTo
+                  )
 
-                val revCommits = using(new RevWalk(git.getRepository)) { revWalk =>
-                  commits.flatten.map { commit =>
-                    revWalk.parseCommit(git.getRepository.resolve(commit.id))
-                  }
-                }.reverse
+                  val revCommits = using(new RevWalk(git.getRepository)) { revWalk =>
+                    commits.flatten.map { commit =>
+                      revWalk.parseCommit(git.getRepository.resolve(commit.id))
+                    }
+                  }.reverse
 
-                // merge git repository
-                (strategy match {
-                  case "merge-commit" =>
-                    Some(
-                      mergePullRequest(
-                        git,
-                        pullreq.branch,
-                        issueId,
-                        s"Merge pull request #${issueId} from ${pullreq.requestUserName}/${pullreq.requestBranch}\n\n" + message,
-                        new PersonIdent(loginAccount.fullName, loginAccount.mailAddress)
+                  // merge git repository
+                  (strategy match {
+                    case "merge-commit" =>
+                      Some(
+                        mergePullRequest(
+                          git,
+                          pullreq.branch,
+                          issueId,
+                          s"Merge pull request #${issueId} from ${pullreq.requestUserName}/${pullreq.requestBranch}\n\n" + message,
+                          new PersonIdent(loginAccount.fullName, loginAccount.mailAddress)
+                        )
                       )
-                    )
-                  case "rebase" =>
-                    Some(
-                      rebasePullRequest(
-                        git,
-                        pullreq.branch,
-                        issueId,
-                        revCommits,
-                        new PersonIdent(loginAccount.fullName, loginAccount.mailAddress)
+                    case "rebase" =>
+                      Some(
+                        rebasePullRequest(
+                          git,
+                          pullreq.branch,
+                          issueId,
+                          revCommits,
+                          new PersonIdent(loginAccount.fullName, loginAccount.mailAddress)
+                        )
                       )
-                    )
-                  case "squash" =>
-                    Some(
-                      squashPullRequest(
-                        git,
-                        pullreq.branch,
-                        issueId,
-                        s"${issue.title} (#${issueId})\n\n" + message,
-                        new PersonIdent(loginAccount.fullName, loginAccount.mailAddress)
+                    case "squash" =>
+                      Some(
+                        squashPullRequest(
+                          git,
+                          pullreq.branch,
+                          issueId,
+                          s"${issue.title} (#${issueId})\n\n" + message,
+                          new PersonIdent(loginAccount.fullName, loginAccount.mailAddress)
+                        )
                       )
-                    )
-                  case _ =>
-                    None
-                }) match {
-                  case Some(newCommitId) =>
-                    // close issue by content of pull request
-                    val defaultBranch = getRepository(repository.owner, repository.name).get.repository.defaultBranch
-                    if (pullreq.branch == defaultBranch) {
-                      commits.flatten.foreach { commit =>
+                    case _ =>
+                      None
+                  }) match {
+                    case Some(newCommitId) =>
+                      // close issue by content of pull request
+                      val defaultBranch = getRepository(repository.owner, repository.name).get.repository.defaultBranch
+                      if (pullreq.branch == defaultBranch) {
+                        commits.flatten.foreach { commit =>
+                          closeIssuesFromMessage(
+                            commit.fullMessage,
+                            loginAccount.userName,
+                            repository.owner,
+                            repository.name
+                          ).foreach { issueId =>
+                            getIssue(repository.owner, repository.name, issueId.toString).foreach { issue =>
+                              callIssuesWebHook("closed", repository, issue, loginAccount)
+                              PluginRegistry().getIssueHooks
+                                .foreach(_.closedByCommitComment(issue, repository, commit.fullMessage, loginAccount))
+                            }
+                          }
+                        }
+                        val issueContent = issue.title + " " + issue.content.getOrElse("")
                         closeIssuesFromMessage(
-                          commit.fullMessage,
+                          issueContent,
                           loginAccount.userName,
                           repository.owner,
                           repository.name
                         ).foreach { issueId =>
-                          getIssue(repository.owner, repository.name, issueId.toString).foreach { issue =>
-                            callIssuesWebHook("closed", repository, issue, loginAccount)
-                            PluginRegistry().getIssueHooks
-                              .foreach(_.closedByCommitComment(issue, repository, commit.fullMessage, loginAccount))
-                          }
-                        }
-                      }
-                      val issueContent = issue.title + " " + issue.content.getOrElse("")
-                      closeIssuesFromMessage(
-                        issueContent,
-                        loginAccount.userName,
-                        repository.owner,
-                        repository.name
-                      ).foreach { issueId =>
-                        getIssue(repository.owner, repository.name, issueId.toString).foreach { issue =>
-                          callIssuesWebHook("closed", repository, issue, loginAccount)
-                          PluginRegistry().getIssueHooks
-                            .foreach(_.closedByCommitComment(issue, repository, issueContent, loginAccount))
-                        }
-                      }
-                      closeIssuesFromMessage(message, loginAccount.userName, repository.owner, repository.name)
-                        .foreach { issueId =>
                           getIssue(repository.owner, repository.name, issueId.toString).foreach { issue =>
                             callIssuesWebHook("closed", repository, issue, loginAccount)
                             PluginRegistry().getIssueHooks
                               .foreach(_.closedByCommitComment(issue, repository, issueContent, loginAccount))
                           }
                         }
-                    }
+                        closeIssuesFromMessage(message, loginAccount.userName, repository.owner, repository.name)
+                          .foreach { issueId =>
+                            getIssue(repository.owner, repository.name, issueId.toString).foreach { issue =>
+                              callIssuesWebHook("closed", repository, issue, loginAccount)
+                              PluginRegistry().getIssueHooks
+                                .foreach(_.closedByCommitComment(issue, repository, issueContent, loginAccount))
+                            }
+                          }
+                      }
 
-                    callPullRequestWebHook("closed", repository, issueId, context.loginAccount.get)
+                      callPullRequestWebHook("closed", repository, issueId, context.loginAccount.get)
 
-                    updatePullRequests(repository.owner, repository.name, pullreq.branch, loginAccount, "closed")
+                      updatePullRequests(repository.owner, repository.name, pullreq.branch, loginAccount, "closed")
 
-                    // call hooks
-                    PluginRegistry().getPullRequestHooks.foreach { h =>
-                      h.addedComment(commentId, message, issue, repository)
-                      h.merged(issue, repository)
-                    }
+                      // call hooks
+                      PluginRegistry().getPullRequestHooks.foreach { h =>
+                        h.addedComment(commentId, message, issue, repository)
+                        h.merged(issue, repository)
+                      }
 
-                    Right(newCommitId)
-                  case None =>
-                    Left("Unknown strategy")
+                      Right(newCommitId)
+                    case None =>
+                      Left("Unknown strategy")
+                  }
                 }
-              }
-            case _ => Left("Unknown error")
-          }
-          .getOrElse(Left("Pull request not found"))
-      }
-    } else Left("Strategy not allowed")
-
+              case _ => Left("Unknown error")
+            }
+            .getOrElse(Left("Pull request not found"))
+        }
+      } else Left("Strategy not allowed")
+    } else Left("Draft pull requests cannot be merged")
   }
 }
 

src/main/scala/gitbucket/core/service/PullRequestService.scala

@@ -48,6 +48,14 @@ trait PullRequestService {
       .map(pr => pr.commitIdTo -> pr.commitIdFrom)
       .update((commitIdTo, commitIdFrom))
 
+  def updateDraftToPullRequest(owner: String, repository: String, issueId: Int)(
+    implicit s: Session
+  ): Unit =
+    PullRequests
+      .filter(_.byPrimaryKey(owner, repository, issueId))
+      .map(pr => pr.isDraft)
+      .update(false)
+
   def getPullRequestCountGroupByUser(closed: Boolean, owner: Option[String], repository: Option[String])(
     implicit s: Session
   ): List[PullRequestCount] =
@@ -97,6 +105,7 @@ trait PullRequestService {
     requestBranch: String,
     commitIdFrom: String,
     commitIdTo: String,
+    isDraft: Boolean,
     loginAccount: Account
   )(implicit s: Session, context: Context): Unit = {
     getIssue(originRepository.owner, originRepository.name, issueId.toString).foreach { baseIssue =>
@@ -109,7 +118,8 @@ trait PullRequestService {
         requestRepositoryName,
         requestBranch,
         commitIdFrom,
-        commitIdTo
+        commitIdTo,
+        isDraft
       )
 
       // fetch requested branch
@@ -215,6 +225,7 @@ trait PullRequestService {
 
   /**
    * Fetch pull request contents into refs/pull/${issueId}/head and update pull request table.
+   *
    */
   def updatePullRequests(owner: String, repository: String, branch: String, loginAccount: Account, action: String)(
     implicit s: Session,

src/main/twirl/gitbucket/core/pulls/compare.scala.html

@@ -79,6 +79,7 @@
                 completionContext  = "issues",
                 style              = "height: 200px;"
               )
+              <div class="text-right">
               <input type="hidden" name="targetUserName" value="@originRepository.owner"/>
               <input type="hidden" name="targetBranch" value="@originId"/>
               <input type="hidden" name="requestUserName" value="@forkedRepository.owner"/>
@@ -86,8 +87,20 @@
               <input type="hidden" name="requestBranch" value="@forkedId"/>
               <input type="hidden" name="commitIdFrom" value="@sourceId"/>
               <input type="hidden" name="commitIdTo" value="@commitId"/>
-              <div class="align-right">
-                <input type="submit" class="btn btn-success" value="Create pull request"/>
+               <input type="hidden" id="is-draft" name="isDraft" value=false />
+
+<!--                <input type="submit" class="btn btn-success" value="Create pull request"/>-->
+<!--                formaction="@context.path/@originRepository.owner/@originRepository.name/pulls/new"-->
+                <div class="btn-group dropup">
+                <input type="submit" class="btn btn-success" tabindex="2" value="Create pull request" id="submit-button" validate="true" formaction="@context.path/@originRepository.owner/@originRepository.name/pulls/new"/>
+                <button type="button" class="btn btn-success dropdown-toggle" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
+                  <span class="caret"></span>
+                </button>
+                <ul class="dropdown-menu dropdown-menu-right">
+                  <li><a id="pull-request">Create pull request</a></li>
+                  <li><a id="draft-request">Create draft request</a></li>
+                </ul>
+                </div>
               </div>
             </div>
             <div class="col-md-3">
@@ -159,6 +172,19 @@
   }
 }
 <script>
+
+$(function(){
+  $('#draft-request').click(function(){
+    $("#is-draft").val(true);
+    $('#submit-button').attr('value', 'Create draft request')
+  });
+  $('#pull-request').click(function(){
+  $("#is-draft").val(false);
+    $('#submit-button').attr('value', 'Create pull request')
+  });
+});
+
+
 $(function(){
   function updateSelector(e){
     e.parents('ul').find('i').attr('class', 'octicon');

src/main/twirl/gitbucket/core/pulls/conversation.scala.html

@@ -1,4 +1,4 @@
-@(issue: gitbucket.core.model.Issue,
+ @(issue: gitbucket.core.model.Issue,
   pullreq: gitbucket.core.model.PullRequest,
   commits: Seq[gitbucket.core.util.JGitUtil.CommitInfo],
   comments: Seq[gitbucket.core.model.Comment],

src/main/twirl/gitbucket/core/pulls/mergeguide.scala.html

@@ -76,24 +76,35 @@
             </div>
           } else {
             <div class="merge-indicator merge-indicator-success"><span class="octicon octicon-check"></span></div>
+
+        @if(pullreq.isDraft){
+          <span class="strong">This pull request is still a work in progress.</span>
+        <div class="pull-right">
+          <input type="button" class="btn btn-default" value="Ready for review" id="ready-for-review" />
+        </div>
+          <div class="small">
+            Draft pull requests cannot be merged.
+          </div>
+        } else {
             @if(status.hasMergePermission){
-              <span class="strong">Merging can be performed automatically.</span>
-              <div class="small">
-                Merging can be performed automatically.
-              </div>
+                  <span class="strong">Merging can be performed automatically.</span>
+                  <div class="small">
+                  Merging can be performed automatically.
+                 </div>
             } else {
-              <span class="strong">This branch has no conflicts with the base branch.</span>
-              <div class="small">
-                Only those with write access to this repository can merge pull requests.
-              </div>
+                    <span class="strong">This branch has no conflicts with the base branch.</span>
+                    <div class="small">
+                      Only those with write access to this repository can merge pull requests.
+                    </div>
             }
+           }
           }
         }
       }
       </div>
       @if(status.hasMergePermission){
         <div style="padding:15px; border-top:solid 1px #e5e5e5; background:#fafafa">
-          <input type="button" class="btn @if(!status.hasProblem){btn-success} else {btn-default}" id="merge-pull-request-button" value="Merge pull request"@if(!status.canMerge){ disabled="true"}/>
+          <input type="button" class="btn @if(!status.hasProblem){btn-success} else {btn-default}" id="merge-pull-request-button" value="Merge pull request"@if(!status.canMerge || pullreq.isDraft){ disabled="true"}/>
           &nbsp;&nbsp;You can also merge branches on the <a href="#" class="show-command-line">command line</a>.
           <div id="command-line" style="display: none;margin-top: 15px;">
             <hr />
@@ -191,6 +202,7 @@
             <input type="button" class="btn btn-default" value="Cancel" id="cancel-merge-pull-request"/>
             <input type="submit" class="btn btn-success" value="Confirm merge"/>
             <input type="hidden" name="strategy" value="@originRepository.repository.options.defaultMergeOption"/>
+            <input type="hidden" name="isDraft" value="@pullreq.isDraft" />
           </div>
         </div>
       </form>
@@ -199,6 +211,15 @@
 </div>
 
 <script>
+
+$(function(){
+  $('#ready-for-review').click(function(){
+    $.post('@helpers.url(originRepository)/pull/@issue.issueId/update_draft', function(data, status){
+      location.reload();
+    })
+  });
+});
+
 $(function(){
   $('.show-command-line').click(function(){
     $('#command-line').toggle();

src/test/scala/gitbucket/core/api/ApiSpecModels.scala

@@ -139,7 +139,8 @@ object ApiSpecModels {
     requestRepositoryName = repo1Name.name,
     requestBranch = "new-topic",
     commitIdFrom = sha1,
-    commitIdTo = sha1
+    commitIdTo = sha1,
+    isDraft = true
   )
 
   val commitComment = CommitComment(

src/test/scala/gitbucket/core/service/ServiceSpecBase.scala

@@ -138,6 +138,7 @@ trait ServiceSpecBase extends MockitoSugar {
       requestBranch = requestBranch,
       commitIdFrom = baesBranch,
       commitIdTo = requestBranch,
+      isDraft = false,
       loginAccount = loginAccount.get
     )
     dummyService.getPullRequest(baseUserName, baseRepositoryName, issueId).get