diff --git a/src/main/scala/gitbucket/core/api/ApiBranchProtection.scala b/src/main/scala/gitbucket/core/api/ApiBranchProtection.scala index 85d2f4d..2af86f2 100644 --- a/src/main/scala/gitbucket/core/api/ApiBranchProtection.scala +++ b/src/main/scala/gitbucket/core/api/ApiBranchProtection.scala @@ -12,16 +12,26 @@ /** form for enabling-and-disabling-branch-protection */ case class EnablingAndDisabling(protection: ApiBranchProtection) - def apply(info: Option[ProtectedBrancheService.ProtectedBranchInfo]): ApiBranchProtection = info match { - case None => ApiBranchProtection(false, Some(statusNone)) - case Some(info) => ApiBranchProtection(true, Some(Status(if(info.includeAdministrators){ Everyone }else{ NonAdmins }, info.requireStatusChecksToPass))) - } + def apply(info: ProtectedBrancheService.ProtectedBranchInfo): ApiBranchProtection = ApiBranchProtection( + enabled = info.enabled, + required_status_checks = Some(Status(EnforcementLevel(info.enabled, info.includeAdministrators), info.contexts))) val statusNone = Status(Off, Seq.empty) case class Status(enforcement_level: EnforcementLevel, contexts: Seq[String]) sealed class EnforcementLevel(val name: String) case object Off extends EnforcementLevel("off") case object NonAdmins extends EnforcementLevel("non_admins") case object Everyone extends EnforcementLevel("everyone") + object EnforcementLevel { + def apply(enabled: Boolean, includeAdministrators: Boolean): EnforcementLevel = if(enabled){ + if(includeAdministrators){ + Everyone + }else{ + NonAdmins + } + }else{ + Off + } + } implicit val enforcementLevelSerializer = new CustomSerializer[EnforcementLevel](format => ( { diff --git a/src/main/scala/gitbucket/core/controller/PullRequestsController.scala b/src/main/scala/gitbucket/core/controller/PullRequestsController.scala index 8cbb317..9c23927 100644 --- a/src/main/scala/gitbucket/core/controller/PullRequestsController.scala +++ b/src/main/scala/gitbucket/core/controller/PullRequestsController.scala @@ -27,13 +27,13 @@ class PullRequestsController extends PullRequestsControllerBase with RepositoryService with AccountService with IssuesService with PullRequestService with MilestonesService with LabelsService with CommitsService with ActivityService with WebHookPullRequestService with ReferrerAuthenticator with CollaboratorsAuthenticator - with CommitStatusService with MergeService + with CommitStatusService with MergeService with ProtectedBrancheService trait PullRequestsControllerBase extends ControllerBase { self: RepositoryService with AccountService with IssuesService with MilestonesService with LabelsService with CommitsService with ActivityService with PullRequestService with WebHookPullRequestService with ReferrerAuthenticator with CollaboratorsAuthenticator - with CommitStatusService with MergeService => + with CommitStatusService with MergeService with ProtectedBrancheService => private val logger = LoggerFactory.getLogger(classOf[PullRequestsControllerBase]) @@ -171,17 +171,21 @@ val owner = repository.owner val name = repository.name getPullRequest(owner, name, issueId) map { case(issue, pullreq) => - val statuses = getCommitStatues(owner, name, pullreq.commitIdTo) + val branchProtection = getProtectedBranchInfo(owner, name, pullreq.branch) + val statuses = branchProtection.withRequireStatues(getCommitStatues(owner, name, pullreq.commitIdTo)) val hasConfrict = LockUtil.lock(s"${owner}/${name}"){ checkConflict(owner, name, pullreq.branch, issueId) } - val hasProblem = hasConfrict || (!statuses.isEmpty && CommitState.combine(statuses.map(_.state).toSet) != CommitState.SUCCESS) + val hasRequiredStatusProblem = branchProtection.hasProblem(statuses, pullreq.commitIdTo, context.loginAccount.get.userName) + val hasProblem = hasRequiredStatusProblem || hasConfrict || (!statuses.isEmpty && CommitState.combine(statuses.map(_.state).toSet) != CommitState.SUCCESS) html.mergeguide( hasConfrict, hasProblem, issue, pullreq, statuses, + branchProtection, + hasRequiredStatusProblem, repository, getRepository(pullreq.requestUserName, pullreq.requestRepositoryName, context.baseUrl).get) } diff --git a/src/main/scala/gitbucket/core/service/ProtectedBrancheService.scala b/src/main/scala/gitbucket/core/service/ProtectedBrancheService.scala index cafcd7a..2d2fd9a 100644 --- a/src/main/scala/gitbucket/core/service/ProtectedBrancheService.scala +++ b/src/main/scala/gitbucket/core/service/ProtectedBrancheService.scala @@ -1,6 +1,6 @@ package gitbucket.core.service -import gitbucket.core.model.{Collaborator, Repository, Account, CommitState} +import gitbucket.core.model.{Collaborator, Repository, Account, CommitState, CommitStatus} import gitbucket.core.model.Profile._ import gitbucket.core.util.JGitUtil import profile.simple._ @@ -16,14 +16,17 @@ trait ProtectedBrancheService { import ProtectedBrancheService._ - def getProtectedBranchInfo(owner: String, repository: String, branch: String)(implicit session: Session): Option[ProtectedBranchInfo] = { + private def getProtectedBranchInfoOpt(owner: String, repository: String, branch: String)(implicit session: Session): Option[ProtectedBranchInfo] = { // TODO: mock - MockDB.data.get((owner, repository, branch)).map{ case (includeAdministrators, requireStatusChecksToPass) => - new ProtectedBranchInfo(owner, repository, requireStatusChecksToPass, includeAdministrators) + MockDB.data.get((owner, repository, branch)).map{ case (includeAdministrators, contexts) => + new ProtectedBranchInfo(owner, repository, true, contexts, includeAdministrators) } } + def getProtectedBranchInfo(owner: String, repository: String, branch: String)(implicit session: Session): ProtectedBranchInfo = { + getProtectedBranchInfoOpt(owner, repository, branch).getOrElse(ProtectedBranchInfo.disabled(owner, repository)) + } def isProtectedBranchNeedStatusCheck(owner: String, repository: String, branch: String, user: String)(implicit session: Session): Boolean = - getProtectedBranchInfo(owner, repository, branch).map{a => println(a); a.needStatusCheck(user)}.getOrElse(false) + getProtectedBranchInfo(owner, repository, branch).needStatusCheck(user) def getProtectedBranchList(owner: String, repository: String)(implicit session: Session): List[String] = { // TODO: mock MockDB.data.filter{ @@ -31,9 +34,9 @@ case _ => false }.map{ case ((_, _, branch), _) => branch }.toList } - def enableBranchProtection(owner: String, repository: String, branch:String, includeAdministrators: Boolean, requireStatusChecksToPass: Seq[String])(implicit session: Session): Unit = { + def enableBranchProtection(owner: String, repository: String, branch:String, includeAdministrators: Boolean, contexts: Seq[String])(implicit session: Session): Unit = { // TODO: mock - MockDB.data.put((owner, repository, branch), includeAdministrators -> requireStatusChecksToPass) + MockDB.data.put((owner, repository, branch), includeAdministrators -> contexts) } def disableBranchProtection(owner: String, repository: String, branch:String)(implicit session: Session): Unit = { // TODO: mock @@ -43,7 +46,7 @@ def getBranchProtectedReason(owner: String, repository: String, receivePack: ReceivePack, command: ReceiveCommand, pusher: String)(implicit session: Session): Option[String] = { val branch = command.getRefName.stripPrefix("refs/heads/") if(branch != command.getRefName){ - getProtectedBranchInfo(owner, repository, branch).flatMap(_.getStopReason(receivePack, command, pusher)) + getProtectedBranchInfo(owner, repository, branch).getStopReason(receivePack, command, pusher) }else{ None } @@ -53,13 +56,14 @@ case class ProtectedBranchInfo( owner: String, repository: String, + enabled: Boolean, /** * Require status checks to pass before merging * Choose which status checks must pass before branches can be merged into test. * When enabled, commits must first be pushed to another branch, * then merged or pushed directly to test after status checks have passed. */ - requireStatusChecksToPass: Seq[String], + contexts: Seq[String], /** * Include administrators * Enforce required status checks for repository administrators. @@ -74,32 +78,56 @@ * Can't have changes merged into them until required status checks pass */ def getStopReason(receivePack: ReceivePack, command: ReceiveCommand, pusher: String)(implicit session: Session): Option[String] = { - command.getType() match { - case ReceiveCommand.Type.UPDATE|ReceiveCommand.Type.UPDATE_NONFASTFORWARD if receivePack.isAllowNonFastForwards => - Some("Cannot force-push to a protected branch") - case ReceiveCommand.Type.UPDATE|ReceiveCommand.Type.UPDATE_NONFASTFORWARD if needStatusCheck(pusher) => - unSuccessedContexts(command.getNewId.name) match { - case s if s.size == 1 => Some(s"""Required status check "${s.toSeq(0)}" is expected""") - case s if s.size >= 1 => Some(s"${s.size} of ${requireStatusChecksToPass.size} required status checks are expected") - case _ => None - } - case ReceiveCommand.Type.DELETE => - Some("Cannot delete a protected branch") - case _ => None + if(enabled){ + command.getType() match { + case ReceiveCommand.Type.UPDATE|ReceiveCommand.Type.UPDATE_NONFASTFORWARD if receivePack.isAllowNonFastForwards => + Some("Cannot force-push to a protected branch") + case ReceiveCommand.Type.UPDATE|ReceiveCommand.Type.UPDATE_NONFASTFORWARD if needStatusCheck(pusher) => + unSuccessedContexts(command.getNewId.name) match { + case s if s.size == 1 => Some(s"""Required status check "${s.toSeq(0)}" is expected""") + case s if s.size >= 1 => Some(s"${s.size} of ${contexts.size} required status checks are expected") + case _ => None + } + case ReceiveCommand.Type.DELETE => + Some("Cannot delete a protected branch") + case _ => None + } + }else{ + None } } - def unSuccessedContexts(sha1: String)(implicit session: Session): Set[String] = if(requireStatusChecksToPass.isEmpty){ + def unSuccessedContexts(sha1: String)(implicit session: Session): Set[String] = if(contexts.isEmpty){ Set.empty } else { - requireStatusChecksToPass.toSet -- getCommitStatues(owner, repository, sha1).filter(_.state == CommitState.SUCCESS).map(_.context).toSet + contexts.toSet -- getCommitStatues(owner, repository, sha1).filter(_.state == CommitState.SUCCESS).map(_.context).toSet } def needStatusCheck(pusher: String)(implicit session: Session): Boolean = - if(requireStatusChecksToPass.isEmpty){ + if(!enabled || contexts.isEmpty){ false }else if(includeAdministrators){ true }else{ !isAdministrator(pusher) } + def withRequireStatues(statuses: List[CommitStatus]): List[CommitStatus] = { + statuses ++ (contexts.toSet -- statuses.map(_.context).toSet).map{ context => CommitStatus( + commitStatusId = 0, + userName = owner, + repositoryName = repository, + commitId = "", + context = context, + state = CommitState.PENDING, + targetUrl = None, + description = Some("Waiting for status to be reported"), + creator = "", + registeredDate = new java.util.Date(), + updatedDate = new java.util.Date()) + } + } + def hasProblem(statuses: List[CommitStatus], sha1: String, account: String)(implicit session: Session): Boolean = + needStatusCheck(account) && contexts.exists(context => statuses.find(_.context == context).map(_.state) != Some(CommitState.SUCCESS)) + } + object ProtectedBranchInfo{ + def disabled(owner: String, repository: String): ProtectedBranchInfo = ProtectedBranchInfo(owner, repository, false, Nil, false) } } diff --git a/src/main/scala/gitbucket/core/view/helpers.scala b/src/main/scala/gitbucket/core/view/helpers.scala index 69418fc..7f11915 100644 --- a/src/main/scala/gitbucket/core/view/helpers.scala +++ b/src/main/scala/gitbucket/core/view/helpers.scala @@ -288,10 +288,10 @@ } def commitStateIcon(state: CommitState) = Html(state match { - case CommitState.PENDING => "●" - case CommitState.SUCCESS => "✔" - case CommitState.ERROR => "×" - case CommitState.FAILURE => "×" + case CommitState.PENDING => """""" + case CommitState.SUCCESS => """""" + case CommitState.ERROR => """""" + case CommitState.FAILURE => """""" }) def commitStateText(state: CommitState, commitId:String) = state match { diff --git a/src/main/twirl/gitbucket/core/pulls/mergeguide.scala.html b/src/main/twirl/gitbucket/core/pulls/mergeguide.scala.html index 6b9d41f..2ca5fd3 100644 --- a/src/main/twirl/gitbucket/core/pulls/mergeguide.scala.html +++ b/src/main/twirl/gitbucket/core/pulls/mergeguide.scala.html @@ -3,6 +3,8 @@ issue: gitbucket.core.model.Issue, pullreq: gitbucket.core.model.PullRequest, statuses: List[model.CommitStatus], + branchProtection: gitbucket.core.service.ProtectedBrancheService.ProtectedBranchInfo, + hasRequiredStatusProblem: Boolean, originRepository: gitbucket.core.service.RepositoryService.RepositoryInfo, forkedRepository: gitbucket.core.service.RepositoryService.RepositoryInfo)(implicit context: gitbucket.core.controller.Context) @import gitbucket.core.service.SystemSettingsService @@ -14,35 +16,27 @@
@if(!statuses.isEmpty){
- @if(statuses.size==1){ - @defining(statuses.head){ status => -
- @status.targetUrl.map{ url => Details } - @commitStateIcon(status.state) - @commitStateText(status.state, pullreq.commitIdTo) - @status.description.map{ desc => — @desc } -
- } - } else { - @defining(statuses.groupBy(_.state)){ stateMap => - @defining(CommitState.combine(stateMap.keySet)){ state => + @defining(statuses.groupBy(_.state)){ stateMap => + @defining(CommitState.combine(stateMap.keySet)){ state => +
+ + @commitStateIcon(state) + @commitStateText(state, pullreq.commitIdTo) + — @{stateMap.map{ case (keyState, states) => states.size+" "+keyState.name }.mkString(", ")} checks +
+
+ @statuses.map{ status =>
- - @commitStateIcon(state) - @commitStateText(state, pullreq.commitIdTo) - — @{stateMap.map{ case (keyState, states) => states.size+" "+keyState.name }.mkString(", ")} checks -
-
- @statuses.map{ status => -
- @status.targetUrl.map{ url => Details } - @commitStateIcon(status.state) - @status.context - @status.description.map{ desc => — @desc } +
+ @branchProtection.contexts.find(_==status.context).map{ url => Required } + @status.targetUrl.map{ url => Details }
- } + @commitStateIcon(status.state) + @status.context + @status.description.map{ desc => — @desc }
- } + } +
} }
@@ -52,67 +46,74 @@
@if(hasConflict){ - We can’t automatically merge this pull request. +
+ This branch has conflicts that must be resolved +
+ Use the command line to resolve conflicts before continuing. +
+ } else { @if(hasRequiredStatusProblem) { +
+ Required statuses must pass before merging. +
+ All required status checks on this pull request must run successfully to enable automatic merging. +
} else { - @if(hasProblem){ - Merge with caution! +
+ Merging can be performed automatically. +
+ Merging can be performed automatically. +
+ } } +
+
+ + You can also merge branches on the command line. + -
- @if(hasConflict){ - Use the command line to resolve conflicts before continuing. - } else { - You can also merge branches on the command line. - } -
- @@ -134,8 +135,8 @@