diff --git a/resources/celerity/map.php b/resources/celerity/map.php --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ 'names' => array( 'conpherence.pkg.css' => '82aca405', 'conpherence.pkg.js' => '6249a1cf', - 'core.pkg.css' => 'c012c648', + 'core.pkg.css' => 'dc689e29', 'core.pkg.js' => '1fa7c0c5', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '90b30783', @@ -145,7 +145,7 @@ 'rsrc/css/phui/phui-document.css' => 'c32e8dec', 'rsrc/css/phui/phui-feed-story.css' => '44a9c8e9', 'rsrc/css/phui/phui-fontkit.css' => '1320ed01', - 'rsrc/css/phui/phui-form-view.css' => 'cf198e10', + 'rsrc/css/phui/phui-form-view.css' => '6175808d', 'rsrc/css/phui/phui-form.css' => 'b62c01d8', 'rsrc/css/phui/phui-head-thing.css' => 'fd311e5f', 'rsrc/css/phui/phui-header-view.css' => '9cf828ce', @@ -530,7 +530,7 @@ 'rsrc/js/phuix/PHUIXActionView.js' => 'b3465b9b', 'rsrc/js/phuix/PHUIXAutocomplete.js' => '7c492cd2', 'rsrc/js/phuix/PHUIXDropdownMenu.js' => '8018ee50', - 'rsrc/js/phuix/PHUIXFormControl.js' => 'bbece68d', + 'rsrc/js/phuix/PHUIXFormControl.js' => '83e03671', 'rsrc/js/phuix/PHUIXIconView.js' => 'bff6884b', ), 'symbols' => array( @@ -847,7 +847,7 @@ 'phui-font-icon-base-css' => '870a7360', 'phui-fontkit-css' => '1320ed01', 'phui-form-css' => 'b62c01d8', - 'phui-form-view-css' => 'cf198e10', + 'phui-form-view-css' => '6175808d', 'phui-head-thing-view-css' => 'fd311e5f', 'phui-header-view-css' => '9cf828ce', 'phui-hovercard' => '1bd28176', @@ -887,7 +887,7 @@ 'phuix-action-view' => 'b3465b9b', 'phuix-autocomplete' => '7c492cd2', 'phuix-dropdown-menu' => '8018ee50', - 'phuix-form-control-view' => 'bbece68d', + 'phuix-form-control-view' => '83e03671', 'phuix-icon-view' => 'bff6884b', 'policy-css' => '957ea14c', 'policy-edit-css' => '815c66f7', @@ -1518,6 +1518,10 @@ 'javelin-behavior', 'javelin-scrollbar', ), + '83e03671' => array( + 'javelin-install', + 'javelin-dom', + ), '8499b6ab' => array( 'javelin-behavior', 'javelin-dom', @@ -1902,10 +1906,6 @@ 'javelin-vector', 'javelin-install', ), - 'bbece68d' => array( - 'javelin-install', - 'javelin-dom', - ), 'bcaccd64' => array( 'javelin-behavior', 'javelin-behavior-device', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2602,6 +2602,7 @@ 'PhabricatorEdgesDestructionEngineExtension' => 'infrastructure/edges/engineextension/PhabricatorEdgesDestructionEngineExtension.php', 'PhabricatorEditEngine' => 'applications/transactions/editengine/PhabricatorEditEngine.php', 'PhabricatorEditEngineAPIMethod' => 'applications/transactions/editengine/PhabricatorEditEngineAPIMethod.php', + 'PhabricatorEditEngineCheckboxesCommentAction' => 'applications/transactions/commentaction/PhabricatorEditEngineCheckboxesCommentAction.php', 'PhabricatorEditEngineColumnsCommentAction' => 'applications/transactions/commentaction/PhabricatorEditEngineColumnsCommentAction.php', 'PhabricatorEditEngineCommentAction' => 'applications/transactions/commentaction/PhabricatorEditEngineCommentAction.php', 'PhabricatorEditEngineCommentActionGroup' => 'applications/transactions/commentaction/PhabricatorEditEngineCommentActionGroup.php', @@ -7686,6 +7687,7 @@ 'PhabricatorPolicyInterface', ), 'PhabricatorEditEngineAPIMethod' => 'ConduitAPIMethod', + 'PhabricatorEditEngineCheckboxesCommentAction' => 'PhabricatorEditEngineCommentAction', 'PhabricatorEditEngineColumnsCommentAction' => 'PhabricatorEditEngineCommentAction', 'PhabricatorEditEngineCommentAction' => 'Phobject', 'PhabricatorEditEngineCommentActionGroup' => 'Phobject', diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -50,4 +50,35 @@ return ($this->getReviewerStatus() == $status_resigned); } + public function isAccepted($diff_phid) { + $status_accepted = DifferentialReviewerStatus::STATUS_ACCEPTED; + + if ($this->getReviewerStatus() != $status_accepted) { + return false; + } + + if (!$diff_phid) { + return true; + } + + $action_phid = $this->getLastActionDiffPHID(); + + if (!$action_phid) { + return true; + } + + if ($action_phid == $diff_phid) { + return true; + } + + $sticky_key = 'differential.sticky-accept'; + $is_sticky = PhabricatorEnv::getEnvConfig($sticky_key); + + if ($is_sticky) { + return true; + } + + return false; + } + } diff --git a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php --- a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php @@ -48,6 +48,72 @@ return pht('Accept a revision.'); } + protected function getActionOptions( + PhabricatorUser $viewer, + DifferentialRevision $revision) { + + $reviewers = $revision->getReviewers(); + + $options = array(); + $value = array(); + + // Put the viewer's user reviewer first, if it exists, so that "Accept as + // yourself" is always at the top. + $head = array(); + $tail = array(); + foreach ($reviewers as $key => $reviewer) { + if ($reviewer->isUser()) { + $head[$key] = $reviewer; + } else { + $tail[$key] = $reviewer; + } + } + $reviewers = $head + $tail; + + $diff_phid = $this->getActiveDiffPHID($revision); + $reviewer_phids = array(); + + // If the viewer isn't a reviewer, add them to the list of options first. + // This happens when you navigate to some revision you aren't involved in: + // you can accept and become a reviewer. + + $viewer_phid = $viewer->getPHID(); + if ($viewer_phid) { + if (!isset($reviewers[$viewer_phid])) { + $reviewer_phids[$viewer_phid] = $viewer_phid; + } + } + + foreach ($reviewers as $reviewer) { + if (!$reviewer->hasAuthority($viewer)) { + // If the viewer doesn't have authority to act on behalf of a reviewer, + // don't include that reviewer as an option. + continue; + } + + if ($reviewer->isAccepted($diff_phid)) { + // If a reviewer is already in a full "accepted" state, don't + // include that reviewer as an option. + continue; + } + + $reviewer_phid = $reviewer->getReviewerPHID(); + $reviewer_phids[$reviewer_phid] = $reviewer_phid; + } + + $handles = $viewer->loadHandles($reviewer_phids); + + foreach ($reviewer_phids as $reviewer_phid) { + $options[$reviewer_phid] = pht( + 'Accept as %s', + $viewer->renderHandle($reviewer_phid)); + + $value[] = $reviewer_phid; + } + + return array($options, $value); + } + public function generateOldValue($object) { $actor = $this->getActor(); return $this->isViewerFullyAccepted($object, $actor); @@ -87,10 +153,39 @@ } } + protected function validateOptionValue($object, $actor, array $value) { + if (!$value) { + throw new Exception( + pht( + 'When accepting a revision, you must accept on behalf of at '. + 'least one reviewer.')); + } + + list($options) = $this->getActionOptions($actor, $object); + foreach ($value as $phid) { + if (!isset($options[$phid])) { + throw new Exception( + pht( + 'Reviewer "%s" is not a valid reviewer which you have authority '. + 'to accept on behalf of.', + $phid)); + } + } + } + public function getTitle() { - return pht( - '%s accepted this revision.', - $this->renderAuthor()); + $new = $this->getNewValue(); + if (is_array($new) && $new) { + return pht( + '%s accepted this revision as %s reviewer(s): %s.', + $this->renderAuthor(), + phutil_count($new), + $this->renderHandleList($new)); + } else { + return pht( + '%s accepted this revision.', + $this->renderAuthor()); + } } public function getTitleForFeed() { diff --git a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php --- a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php @@ -19,6 +19,10 @@ abstract protected function validateAction($object, PhabricatorUser $viewer); abstract protected function getRevisionActionLabel(); + protected function validateOptionValue($object, $actor, array $value) { + return null; + } + public function getCommandKeyword() { return null; } @@ -70,6 +74,15 @@ return ($viewer->getPHID() === $revision->getAuthorPHID()); } + protected function getActionOptions( + PhabricatorUser $viewer, + DifferentialRevision $revision) { + return array( + array(), + null, + ); + } + public function newEditField( DifferentialRevision $revision, PhabricatorUser $viewer) { @@ -107,6 +120,12 @@ // It's not clear that these combinations are actually useful, so just // keep things simple for now. $field->setActionConflictKey('revision.action'); + + list($options, $value) = $this->getActionOptions($viewer, $revision); + if (count($options) > 1) { + $field->setOptions($options); + $field->setValue($value); + } } } @@ -129,6 +148,20 @@ $errors[] = $this->newInvalidError( $action_exception->getMessage(), $xaction); + continue; + } + + $new = $xaction->getNewValue(); + if (!is_array($new)) { + continue; + } + + try { + $this->validateOptionValue($object, $actor, $new); + } catch (Exception $ex) { + $errors[] = $this->newInvalidError( + $ex->getMessage(), + $xaction); } } diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php --- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php @@ -7,6 +7,26 @@ return DifferentialRevisionEditEngine::ACTIONGROUP_REVIEW; } + public function generateNewValue($object, $value) { + if (!is_array($value)) { + return true; + } + + // If the list of options is the same as the default list, just treat this + // as a "take the default action" transaction. + $viewer = $this->getActor(); + list($options, $default) = $this->getActionOptions($viewer, $object); + + sort($default); + sort($value); + + if ($default === $value) { + return true; + } + + return $value; + } + protected function isViewerAnyReviewer( DifferentialRevision $revision, PhabricatorUser $viewer) { @@ -118,6 +138,12 @@ // In all cases, you affect yourself. $map[$viewer->getPHID()] = $status; + // If the user has submitted a specific list of reviewers to act as (by + // unchecking some checkboxes under "Accept"), only affect those reviewers. + if (is_array($value)) { + $map = array_select_keys($map, $value); + } + // Convert reviewer statuses into edge data. foreach ($map as $reviewer_phid => $reviewer_status) { $map[$reviewer_phid] = array( diff --git a/src/applications/transactions/commentaction/PhabricatorEditEngineCheckboxesCommentAction.php b/src/applications/transactions/commentaction/PhabricatorEditEngineCheckboxesCommentAction.php new file mode 100644 --- /dev/null +++ b/src/applications/transactions/commentaction/PhabricatorEditEngineCheckboxesCommentAction.php @@ -0,0 +1,36 @@ +options = $options; + return $this; + } + + public function getOptions() { + return $this->options; + } + + public function getPHUIXControlType() { + return 'checkboxes'; + } + + public function getPHUIXControlSpecification() { + $options = $this->getOptions(); + + $labels = array(); + foreach ($options as $key => $option) { + $labels[$key] = hsprintf('%s', $option); + } + + return array( + 'value' => $this->getValue(), + 'keys' => array_keys($options), + 'labels' => $labels, + ); + } + +} diff --git a/src/applications/transactions/editfield/PhabricatorApplyEditField.php b/src/applications/transactions/editfield/PhabricatorApplyEditField.php --- a/src/applications/transactions/editfield/PhabricatorApplyEditField.php +++ b/src/applications/transactions/editfield/PhabricatorApplyEditField.php @@ -5,6 +5,7 @@ private $actionDescription; private $actionConflictKey; + private $options; protected function newControl() { return null; @@ -28,8 +29,21 @@ return $this->actionConflictKey; } + public function setOptions(array $options) { + $this->options = $options; + return $this; + } + + public function getOptions() { + return $this->options; + } + protected function newHTTPParameterType() { - return new AphrontBoolHTTPParameterType(); + if ($this->getOptions()) { + return new AphrontPHIDListHTTPParameterType(); + } else { + return new AphrontBoolHTTPParameterType(); + } } protected function newConduitParameterType() { @@ -43,9 +57,16 @@ } protected function newCommentAction() { - return id(new PhabricatorEditEngineStaticCommentAction()) - ->setDescription($this->getActionDescription()) - ->setConflictKey($this->getActionConflictKey()); + $options = $this->getOptions(); + if ($options) { + return id(new PhabricatorEditEngineCheckboxesCommentAction()) + ->setConflictKey($this->getActionConflictKey()) + ->setOptions($options); + } else { + return id(new PhabricatorEditEngineStaticCommentAction()) + ->setConflictKey($this->getActionConflictKey()) + ->setDescription($this->getActionDescription()); + } } } diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php --- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php +++ b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php @@ -1608,6 +1608,8 @@ ), ), + '%s accepted this revision as %s reviewer(s): %s.' => + '%s accepted this revision as: %3$s.', ); } diff --git a/webroot/rsrc/css/phui/phui-form-view.css b/webroot/rsrc/css/phui/phui-form-view.css --- a/webroot/rsrc/css/phui/phui-form-view.css +++ b/webroot/rsrc/css/phui/phui-form-view.css @@ -548,3 +548,16 @@ padding: 4px; color: {$bluetext}; } + +.phuix-form-checkbox-action { + padding: 4px; + color: {$bluetext}; +} + +.phuix-form-checkbox-action input[type=checkbox] { + margin: 4px 0; +} + +.phuix-form-checkbox-label { + margin-left: 4px; +} diff --git a/webroot/rsrc/js/phuix/PHUIXFormControl.js b/webroot/rsrc/js/phuix/PHUIXFormControl.js --- a/webroot/rsrc/js/phuix/PHUIXFormControl.js +++ b/webroot/rsrc/js/phuix/PHUIXFormControl.js @@ -50,6 +50,9 @@ case 'static': input = this._newStatic(spec); break; + case 'checkboxes': + input = this._newCheckboxes(spec); + break; default: // TODO: Default or better error? JX.$E('Bad Input Type'); @@ -194,6 +197,89 @@ }; }, + _newCheckboxes: function(spec) { + var checkboxes = []; + var checkbox_list = []; + for (var ii = 0; ii < spec.keys.length; ii++) { + var key = spec.keys[ii]; + var checkbox_id = 'checkbox-' + Math.floor(Math.random() * 1000000); + + var checkbox = JX.$N( + 'input', + { + type: 'checkbox', + value: key, + id: checkbox_id + }); + + checkboxes.push(checkbox); + + var label = JX.$N( + 'label', + { + className: 'phuix-form-checkbox-label', + htmlFor: checkbox_id + }, + JX.$H(spec.labels[key] || '')); + + var display = JX.$N( + 'div', + { + className: 'phuix-form-checkbox-item' + }, + [checkbox, label]); + + checkbox_list.push(display); + } + + var node = JX.$N( + 'div', + { + className: 'phuix-form-checkbox-action' + }, + checkbox_list); + + var get_value = function() { + var list = []; + for (var ii = 0; ii < checkboxes.length; ii++) { + if (checkboxes[ii].checked) { + list.push(checkboxes[ii].value); + } + } + return list; + }; + + var set_value = function(value) { + value = value || []; + + if (!value.length) { + value = []; + } + + var map = {}; + var ii; + for (ii = 0; ii < value.length; ii++) { + map[value[ii]] = true; + } + + for (ii = 0; ii < checkboxes.length; ii++) { + if (map.hasOwnProperty(checkboxes[ii].value)) { + checkboxes[ii].checked = 'checked'; + } else { + checkboxes[ii].checked = false; + } + } + }; + + set_value(spec.value); + + return { + node: node, + get: get_value, + set: set_value + }; + }, + _newPoints: function(spec) { var attrs = { type: 'text',