mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-19 21:32:43 +01:00
Improve documentation and tooling around autoclose
Summary: Fixes T4767. I believe 80% of this was actually caused by the author issue fixed in T5771, but this should help make the other 20% debuggable. - Record why we didn't autoclose a commit when we process it. - Show branch autoclose status in the main branch table. - Show commit autoclose status on the edit screen. - Add documentation about how to find these statuses and what they mean. Test Plan: - Read documentation. - Viewed branches and hovered over the various states. - Viewed commits in various states and checked the "Autoclose?" field. - Pushed some commits and saw autoclose activate. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4767 Differential Revision: https://secure.phabricator.com/D10348
This commit is contained in:
parent
124fd20654
commit
53a678c568
6 changed files with 287 additions and 61 deletions
|
@ -8,13 +8,13 @@ final class DiffusionCommitEditController extends DiffusionController {
|
|||
}
|
||||
|
||||
public function processRequest() {
|
||||
|
||||
$request = $this->getRequest();
|
||||
$user = $request->getUser();
|
||||
$drequest = $this->getDiffusionRequest();
|
||||
$callsign = $drequest->getRepository()->getCallsign();
|
||||
$repository = $drequest->getRepository();
|
||||
$commit = $drequest->loadCommit();
|
||||
$data = $commit->loadCommitData();
|
||||
$page_title = pht('Edit Diffusion Commit');
|
||||
|
||||
if (!$commit) {
|
||||
|
@ -72,6 +72,42 @@ final class DiffusionCommitEditController extends DiffusionController {
|
|||
pht('Create New Project')))
|
||||
->setDatasource(new PhabricatorProjectDatasource()));
|
||||
|
||||
$reason = $data->getCommitDetail('autocloseReason', false);
|
||||
if ($reason !== false) {
|
||||
switch ($reason) {
|
||||
case PhabricatorRepository::BECAUSE_REPOSITORY_IMPORTING:
|
||||
$desc = pht('No, Repository Importing');
|
||||
break;
|
||||
case PhabricatorRepository::BECAUSE_AUTOCLOSE_DISABLED:
|
||||
$desc = pht('No, Autoclose Disabled');
|
||||
break;
|
||||
case PhabricatorRepository::BECAUSE_NOT_ON_AUTOCLOSE_BRANCH:
|
||||
$desc = pht('No, Not On Autoclose Branch');
|
||||
break;
|
||||
case null:
|
||||
$desc = pht('Yes');
|
||||
break;
|
||||
default:
|
||||
$desc = pht('Unknown');
|
||||
break;
|
||||
}
|
||||
|
||||
$doc_href = PhabricatorEnv::getDoclink('Diffusion User Guide: Autoclose');
|
||||
$doc_link = phutil_tag(
|
||||
'a',
|
||||
array(
|
||||
'href' => $doc_href,
|
||||
'target' => '_blank',
|
||||
),
|
||||
pht('Learn More'));
|
||||
|
||||
$form->appendChild(
|
||||
id(new AphrontFormMarkupControl())
|
||||
->setLabel(pht('Autoclose?'))
|
||||
->setValue(array($desc, " \xC2\xB7 ", $doc_link)));
|
||||
}
|
||||
|
||||
|
||||
Javelin::initBehavior('project-create', array(
|
||||
'tokenizerID' => $tokenizer_id,
|
||||
));
|
||||
|
@ -81,12 +117,18 @@ final class DiffusionCommitEditController extends DiffusionController {
|
|||
->addCancelButton('/r'.$callsign.$commit->getCommitIdentifier());
|
||||
$form->appendChild($submit);
|
||||
|
||||
$crumbs = $this->buildCrumbs(array(
|
||||
'commit' => true,
|
||||
));
|
||||
$crumbs->addTextCrumb(pht('Edit'));
|
||||
|
||||
$form_box = id(new PHUIObjectBoxView())
|
||||
->setHeaderText($page_title)
|
||||
->setForm($form);
|
||||
|
||||
return $this->buildApplicationPage(
|
||||
array(
|
||||
$crumbs,
|
||||
$form_box,
|
||||
),
|
||||
array(
|
||||
|
|
|
@ -20,6 +20,11 @@ final class DiffusionBranchTableView extends DiffusionView {
|
|||
public function render() {
|
||||
$drequest = $this->getDiffusionRequest();
|
||||
$current_branch = $drequest->getBranch();
|
||||
$repository = $drequest->getRepository();
|
||||
|
||||
Javelin::initBehavior('phabricator-tooltips');
|
||||
|
||||
$doc_href = PhabricatorEnv::getDoclink('Diffusion User Guide: Autoclose');
|
||||
|
||||
$rows = array();
|
||||
$rowc = array();
|
||||
|
@ -33,6 +38,43 @@ final class DiffusionBranchTableView extends DiffusionView {
|
|||
$details = null;
|
||||
}
|
||||
|
||||
switch ($repository->shouldSkipAutocloseBranch($branch->getShortName())) {
|
||||
case PhabricatorRepository::BECAUSE_REPOSITORY_IMPORTING:
|
||||
$icon = 'fa-times bluegrey';
|
||||
$tip = pht('Repository Importing');
|
||||
break;
|
||||
case PhabricatorRepository::BECAUSE_AUTOCLOSE_DISABLED:
|
||||
$icon = 'fa-times bluegrey';
|
||||
$tip = pht('Repository Autoclose Disabled');
|
||||
break;
|
||||
case PhabricatorRepository::BECAUSE_BRANCH_UNTRACKED:
|
||||
$icon = 'fa-times bluegrey';
|
||||
$tip = pht('Branch Untracked');
|
||||
break;
|
||||
case PhabricatorRepository::BECAUSE_BRANCH_NOT_AUTOCLOSE:
|
||||
$icon = 'fa-times bluegrey';
|
||||
$tip = pht('Branch Autoclose Disabled');
|
||||
break;
|
||||
case null:
|
||||
$icon = 'fa-check bluegrey';
|
||||
$tip = pht('Autoclose Enabled');
|
||||
break;
|
||||
default:
|
||||
$icon = 'fa-question';
|
||||
$tip = pht('Status Unknown');
|
||||
break;
|
||||
}
|
||||
|
||||
$status_icon = id(new PHUIIconView())
|
||||
->setIconFont($icon)
|
||||
->addSigil('has-tooltip')
|
||||
->setHref($doc_href)
|
||||
->setMetadata(
|
||||
array(
|
||||
'tip' => $tip,
|
||||
'size' => 200,
|
||||
));
|
||||
|
||||
$rows[] = array(
|
||||
phutil_tag(
|
||||
'a',
|
||||
|
@ -57,6 +99,7 @@ final class DiffusionBranchTableView extends DiffusionView {
|
|||
self::linkCommit(
|
||||
$drequest->getRepository(),
|
||||
$branch->getCommitIdentifier()),
|
||||
$status_icon,
|
||||
$datetime,
|
||||
AphrontTableView::renderSingleDisplayLine($details),
|
||||
);
|
||||
|
@ -73,6 +116,7 @@ final class DiffusionBranchTableView extends DiffusionView {
|
|||
pht('History'),
|
||||
pht('Branch'),
|
||||
pht('Head'),
|
||||
pht(''),
|
||||
pht('Modified'),
|
||||
pht('Details'),
|
||||
));
|
||||
|
@ -82,6 +126,7 @@ final class DiffusionBranchTableView extends DiffusionView {
|
|||
'pri',
|
||||
'',
|
||||
'',
|
||||
'',
|
||||
'wide',
|
||||
));
|
||||
$view->setRowClasses($rowc);
|
||||
|
|
|
@ -475,9 +475,7 @@ final class HeraldCommitAdapter extends HeraldAdapter {
|
|||
$refs = DiffusionRepositoryRef::loadAllFromDictionaries($result);
|
||||
return mpull($refs, 'getShortName');
|
||||
case self::FIELD_REPOSITORY_AUTOCLOSE_BRANCH:
|
||||
return $this->repository->shouldAutocloseCommit(
|
||||
$this->commit,
|
||||
$this->commitData);
|
||||
return $this->repository->shouldAutocloseCommit($this->commit);
|
||||
}
|
||||
|
||||
return parent::getHeraldField($field);
|
||||
|
|
|
@ -2,6 +2,7 @@
|
|||
|
||||
/**
|
||||
* @task uri Repository URI Management
|
||||
* @task autoclose Autoclose
|
||||
*/
|
||||
final class PhabricatorRepository extends PhabricatorRepositoryDAO
|
||||
implements
|
||||
|
@ -33,6 +34,12 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
|
|||
const SERVE_READONLY = 'readonly';
|
||||
const SERVE_READWRITE = 'readwrite';
|
||||
|
||||
const BECAUSE_REPOSITORY_IMPORTING = 'auto/importing';
|
||||
const BECAUSE_AUTOCLOSE_DISABLED = 'auto/disabled';
|
||||
const BECAUSE_NOT_ON_AUTOCLOSE_BRANCH = 'auto/nobranch';
|
||||
const BECAUSE_BRANCH_UNTRACKED = 'auto/notrack';
|
||||
const BECAUSE_BRANCH_NOT_AUTOCLOSE = 'auto/noclose';
|
||||
|
||||
protected $name;
|
||||
protected $callsign;
|
||||
protected $uuid;
|
||||
|
@ -586,59 +593,6 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
|
|||
return $this->isBranchInFilter($branch, 'branch-filter');
|
||||
}
|
||||
|
||||
public function shouldAutocloseBranch($branch) {
|
||||
if ($this->isImporting()) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if ($this->getDetail('disable-autoclose', false)) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if (!$this->shouldTrackBranch($branch)) {
|
||||
return false;
|
||||
}
|
||||
|
||||
return $this->isBranchInFilter($branch, 'close-commits-filter');
|
||||
}
|
||||
|
||||
public function shouldAutocloseCommit(
|
||||
PhabricatorRepositoryCommit $commit,
|
||||
PhabricatorRepositoryCommitData $data) {
|
||||
|
||||
if ($this->getDetail('disable-autoclose', false)) {
|
||||
return false;
|
||||
}
|
||||
|
||||
switch ($this->getVersionControlSystem()) {
|
||||
case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
|
||||
return true;
|
||||
case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
|
||||
break;
|
||||
case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
|
||||
return true;
|
||||
default:
|
||||
throw new Exception('Unrecognized version control system.');
|
||||
}
|
||||
|
||||
$closeable_flag = PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE;
|
||||
if ($commit->isPartiallyImported($closeable_flag)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
// TODO: Remove this eventually, it's no longer written to by the import
|
||||
// pipeline (however, old tasks may still be queued which don't reflect
|
||||
// the new data format).
|
||||
$branches = $data->getCommitDetail('seenOnBranches', array());
|
||||
foreach ($branches as $branch) {
|
||||
if ($this->shouldAutocloseBranch($branch)) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
public function formatCommitName($commit_identifier) {
|
||||
$vcs = $this->getVersionControlSystem();
|
||||
|
||||
|
@ -661,6 +615,125 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
|
|||
}
|
||||
|
||||
|
||||
/* -( Autoclose )---------------------------------------------------------- */
|
||||
|
||||
|
||||
/**
|
||||
* Determine if autoclose is active for a branch.
|
||||
*
|
||||
* For more details about why, use @{method:shouldSkipAutocloseBranch}.
|
||||
*
|
||||
* @param string Branch name to check.
|
||||
* @return bool True if autoclose is active for the branch.
|
||||
* @task autoclose
|
||||
*/
|
||||
public function shouldAutocloseBranch($branch) {
|
||||
return ($this->shouldSkipAutocloseBranch($branch) === null);
|
||||
}
|
||||
|
||||
/**
|
||||
* Determine if autoclose is active for a commit.
|
||||
*
|
||||
* For more details about why, use @{method:shouldSkipAutocloseCommit}.
|
||||
*
|
||||
* @param PhabricatorRepositoryCommit Commit to check.
|
||||
* @return bool True if autoclose is active for the commit.
|
||||
* @task autoclose
|
||||
*/
|
||||
public function shouldAutocloseCommit(PhabricatorRepositoryCommit $commit) {
|
||||
return ($this->shouldSkipAutocloseCommit($commit) === null);
|
||||
}
|
||||
|
||||
|
||||
/**
|
||||
* Determine why autoclose should be skipped for a branch.
|
||||
*
|
||||
* This method gives a detailed reason why autoclose will be skipped. To
|
||||
* perform a simple test, use @{method:shouldAutocloseBranch}.
|
||||
*
|
||||
* @param string Branch name to check.
|
||||
* @return const|null Constant identifying reason to skip this branch, or null
|
||||
* if autoclose is active.
|
||||
* @task autoclose
|
||||
*/
|
||||
public function shouldSkipAutocloseBranch($branch) {
|
||||
$all_reason = $this->shouldSkipAllAutoclose();
|
||||
if ($all_reason) {
|
||||
return $all_reason;
|
||||
}
|
||||
|
||||
if (!$this->shouldTrackBranch($branch)) {
|
||||
return self::BECAUSE_BRANCH_UNTRACKED;
|
||||
}
|
||||
|
||||
if (!$this->isBranchInFilter($branch, 'close-commits-filter')) {
|
||||
return self::BECAUSE_BRANCH_NOT_AUTOCLOSE;
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
|
||||
/**
|
||||
* Determine why autoclose should be skipped for a commit.
|
||||
*
|
||||
* This method gives a detailed reason why autoclose will be skipped. To
|
||||
* perform a simple test, use @{method:shouldAutocloseCommit}.
|
||||
*
|
||||
* @param PhabricatorRepositoryCommit Commit to check.
|
||||
* @return const|null Constant identifying reason to skip this commit, or null
|
||||
* if autoclose is active.
|
||||
* @task autoclose
|
||||
*/
|
||||
public function shouldSkipAutocloseCommit(
|
||||
PhabricatorRepositoryCommit $commit) {
|
||||
|
||||
$all_reason = $this->shouldSkipAllAutoclose();
|
||||
if ($all_reason) {
|
||||
return $all_reason;
|
||||
}
|
||||
|
||||
switch ($this->getVersionControlSystem()) {
|
||||
case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
|
||||
case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
|
||||
return null;
|
||||
case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
|
||||
break;
|
||||
default:
|
||||
throw new Exception('Unrecognized version control system.');
|
||||
}
|
||||
|
||||
$closeable_flag = PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE;
|
||||
if (!$commit->isPartiallyImported($closeable_flag)) {
|
||||
return self::BECAUSE_NOT_ON_AUTOCLOSE_BRANCH;
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
|
||||
/**
|
||||
* Determine why all autoclose operations should be skipped for this
|
||||
* repository.
|
||||
*
|
||||
* @return const|null Constant identifying reason to skip all autoclose
|
||||
* operations, or null if autoclose operations are not blocked at the
|
||||
* repository level.
|
||||
* @task autoclose
|
||||
*/
|
||||
private function shouldSkipAllAutoclose() {
|
||||
if ($this->isImporting()) {
|
||||
return self::BECAUSE_REPOSITORY_IMPORTING;
|
||||
}
|
||||
|
||||
if ($this->getDetail('disable-autoclose', false)) {
|
||||
return self::BECAUSE_AUTOCLOSE_DISABLED;
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
|
||||
/* -( Repository URI Management )------------------------------------------ */
|
||||
|
||||
|
||||
|
|
|
@ -68,6 +68,16 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
|
|||
$commit->setSummary($data->getSummary());
|
||||
$commit->save();
|
||||
|
||||
// Figure out if we're going to try to "autoclose" related objects (e.g.,
|
||||
// close linked tasks and related revisions) and, if not, record why we
|
||||
// aren't. Autoclose can be disabled for various reasons at the repository
|
||||
// or commit levels.
|
||||
|
||||
$autoclose_reason = $repository->shouldSkipAutocloseCommit($commit);
|
||||
$data->setCommitDetail('autocloseReason', $autoclose_reason);
|
||||
$should_autoclose = $repository->shouldAutocloseCommit($commit);
|
||||
|
||||
|
||||
// When updating related objects, we'll act under an omnipotent user to
|
||||
// ensure we can see them, but take actions as either the committer or
|
||||
// author (if we recognize their accounts) or the Diffusion application
|
||||
|
@ -90,8 +100,6 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
|
|||
// someone probably did something very silly, though.)
|
||||
|
||||
$revision = null;
|
||||
$should_autoclose = $repository->shouldAutocloseCommit($commit, $data);
|
||||
|
||||
if ($revision_id) {
|
||||
$revision_query = id(new DifferentialRevisionQuery())
|
||||
->withIDs(array($revision_id))
|
||||
|
|
60
src/docs/user/userguide/diffusion_autoclose.diviner
Normal file
60
src/docs/user/userguide/diffusion_autoclose.diviner
Normal file
|
@ -0,0 +1,60 @@
|
|||
@title Diffusion User Guide: Autoclose
|
||||
@group userguide
|
||||
|
||||
Explains when Diffusion will close tasks and revisions upon discovery of related
|
||||
commits.
|
||||
|
||||
Overview
|
||||
========
|
||||
|
||||
Diffusion can close tasks and revisions when related commits appear in a
|
||||
repository. For example, if you make a commit with `Fixes T123` in the commit
|
||||
message, Diffusion will close the task `T123`.
|
||||
|
||||
This document explains how autoclose works, how to configure it, and how to
|
||||
troubleshoot it.
|
||||
|
||||
Troubleshooting Autoclose
|
||||
=========================
|
||||
|
||||
You can check if a branch is currently configured to autoclose on the main
|
||||
repository view, or in the branches list view. Hover over the {icon check} or
|
||||
{icon times} icon and you should see one of these messages:
|
||||
|
||||
- {icon check} **Autoclose Enabled** Autoclose is active for this branch.
|
||||
- {icon times} **Repository Importing** This repository is still importing.
|
||||
Autoclose does not activate until a repository finishes importing for the
|
||||
first time. This prevents situations where you import a repository and
|
||||
accidentally close hundreds of related objects during import. Autoclose
|
||||
will activate for new commits after the initial import completes.
|
||||
- {icon times} **Repository Autoclose Disabled** Autoclose is disabled for
|
||||
this entire repository. You can enable it in **Edit Repository**.
|
||||
- {icon times} **Branch Untracked** This branch is not tracked. Because it
|
||||
is not tracked, commits on it won't be seen and won't be discovered.
|
||||
- {icon times} **Branch Autoclose Disabled** Autoclose is not enabled for
|
||||
this branch. You can adjust which branches autoclose in **Edit Repository**.
|
||||
This option is only available in Git.
|
||||
|
||||
If a branch is in good shape, you can check a specific commit by viewing it
|
||||
in the web UI and clicking **Edit Commit**. There should be an **Autoclose?**
|
||||
field visible in the form, with possible values listed below.
|
||||
|
||||
Note that this field records the state of the world at the time the commit was
|
||||
processed, and does not necessarily reflect the current state of the world.
|
||||
For example, if a commit did not trigger autoclose because it was processed
|
||||
during initial import, the field will still show **No, Repository Importing**
|
||||
even after import completes. This means that the commit did not trigger
|
||||
autoclose because the repository was importing at the time it was processed,
|
||||
not necessarily that the repository is still importing.
|
||||
|
||||
- **Yes** At the time the commit was imported, autoclose triggered and
|
||||
Phabricator attempted to close related objects.
|
||||
- **No, Repository Importing** At the time the commit was processed, the
|
||||
repository was still importing. Autoclose does not activate until a
|
||||
repository fully imports for the first time.
|
||||
- **No, Autoclose Disabled** At the time the commit was processed, the
|
||||
repository had autoclose disabled.
|
||||
- **No, Not On Autoclose Branch** At the time the commit was processed,
|
||||
no containing branch was configured to autoclose.
|
||||
- //Field Not Present// This commit was processed before we implemented
|
||||
this diagnostic feature, and no information is available.
|
Loading…
Reference in a new issue