From b6420e0f0ad8f0c14d018880700d0481d3d4f39c Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 3 Sep 2019 08:56:24 -0700 Subject: [PATCH 01/44] Allow repository service lookups to return an ordered list of service refs Summary: Ref T13286. To support request retries, allow the service lookup method to return an ordered list of structured service references. Existing callsites continue to immediately discard all but the first reference and pull a URI out of it. Test Plan: Ran `git pull` in a clustered repository with an "up" node and a "down" node, saw 50% serivce failures and 50% clean pulls. Maniphest Tasks: T13286 Differential Revision: https://secure.phabricator.com/D20775 --- src/__phutil_library_map__.php | 2 + .../diffusion/ref/DiffusionServiceRef.php | 48 ++++++++++++++ .../diffusion/ssh/DiffusionSSHWorkflow.php | 22 +++++-- .../storage/PhabricatorRepository.php | 62 +++++++++++++------ 4 files changed, 112 insertions(+), 22 deletions(-) create mode 100644 src/applications/diffusion/ref/DiffusionServiceRef.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 96d3713b48..212a7b1607 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1021,6 +1021,7 @@ phutil_register_library_map(array( 'DiffusionSSHWorkflow' => 'applications/diffusion/ssh/DiffusionSSHWorkflow.php', 'DiffusionSearchQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php', 'DiffusionServeController' => 'applications/diffusion/controller/DiffusionServeController.php', + 'DiffusionServiceRef' => 'applications/diffusion/ref/DiffusionServiceRef.php', 'DiffusionSetPasswordSettingsPanel' => 'applications/diffusion/panel/DiffusionSetPasswordSettingsPanel.php', 'DiffusionSetupException' => 'applications/diffusion/exception/DiffusionSetupException.php', 'DiffusionSourceHyperlinkEngineExtension' => 'applications/diffusion/engineextension/DiffusionSourceHyperlinkEngineExtension.php', @@ -6967,6 +6968,7 @@ phutil_register_library_map(array( 'DiffusionSSHWorkflow' => 'PhabricatorSSHWorkflow', 'DiffusionSearchQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionServeController' => 'DiffusionController', + 'DiffusionServiceRef' => 'Phobject', 'DiffusionSetPasswordSettingsPanel' => 'PhabricatorSettingsPanel', 'DiffusionSetupException' => 'Exception', 'DiffusionSourceHyperlinkEngineExtension' => 'PhabricatorRemarkupHyperlinkEngineExtension', diff --git a/src/applications/diffusion/ref/DiffusionServiceRef.php b/src/applications/diffusion/ref/DiffusionServiceRef.php new file mode 100644 index 0000000000..d6e6948e5d --- /dev/null +++ b/src/applications/diffusion/ref/DiffusionServiceRef.php @@ -0,0 +1,48 @@ +uri = $map['uri']; + $ref->isWritable = $map['writable']; + $ref->devicePHID = $map['devicePHID']; + $ref->protocol = $map['protocol']; + $ref->deviceName = $map['device']; + + return $ref; + } + + public function isWritable() { + return $this->isWritable; + } + + public function getDevicePHID() { + return $this->devicePHID; + } + + public function getURI() { + return $this->uri; + } + + public function getProtocol() { + return $this->protocol; + } + + public function getDeviceName() { + return $this->deviceName; + } + +} diff --git a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php index 08144eb0c9..358418f44c 100644 --- a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php @@ -73,13 +73,13 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { return $this->shouldProxy; } - protected function getProxyCommand($for_write) { + final protected function getAlmanacServiceRefs($for_write) { $viewer = $this->getSSHUser(); $repository = $this->getRepository(); $is_cluster_request = $this->getIsClusterRequest(); - $uri = $repository->getAlmanacServiceURI( + $refs = $repository->getAlmanacServiceRefs( $viewer, array( 'neverProxy' => $is_cluster_request, @@ -89,14 +89,28 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { 'writable' => $for_write, )); - if (!$uri) { + if (!$refs) { throw new Exception( pht( 'Failed to generate an intracluster proxy URI even though this '. 'request was routed as a proxy request.')); } - $uri = new PhutilURI($uri); + return $refs; + } + + final protected function getProxyCommand($for_write) { + $refs = $this->getAlmanacServiceRefs($for_write); + + $ref = head($refs); + + return $this->getProxyCommandForServiceRef($ref); + } + + final protected function getProxyCommandForServiceRef( + DiffusionServiceRef $ref) { + + $uri = new PhutilURI($ref->getURI()); $username = AlmanacKeys::getClusterSSHUser(); if ($username === null) { diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index bd15e3e8de..fdc9a695c4 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1842,6 +1842,20 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO PhabricatorUser $viewer, array $options) { + $refs = $this->getAlmanacServiceRefs($viewer, $options); + + if (!$refs) { + return null; + } + + $ref = head($refs); + return $ref->getURI(); + } + + public function getAlmanacServiceRefs( + PhabricatorUser $viewer, + array $options) { + PhutilTypeSpec::checkMap( $options, array( @@ -1856,7 +1870,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $cache_key = $this->getAlmanacServiceCacheKey(); if (!$cache_key) { - return null; + return array(); } $cache = PhabricatorCaches::getMutableStructureCache(); @@ -1869,7 +1883,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } if ($uris === null) { - return null; + return array(); } $local_device = AlmanacKeys::getDeviceID(); @@ -1893,7 +1907,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO if ($local_device && $never_proxy) { if ($uri['device'] == $local_device) { - return null; + return array(); } } @@ -1954,15 +1968,20 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } } + $refs = array(); + foreach ($results as $result) { + $refs[] = DiffusionServiceRef::newFromDictionary($result); + } + // If we require a writable device, remove URIs which aren't writable. if ($writable) { - foreach ($results as $key => $uri) { - if (!$uri['writable']) { + foreach ($refs as $key => $ref) { + if (!$ref->isWritable()) { unset($results[$key]); } } - if (!$results) { + if (!$refs) { throw new Exception( pht( 'This repository ("%s") is not writable with the given '. @@ -1974,23 +1993,30 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } if ($writable) { - $results = $this->sortWritableAlmanacServiceURIs($results); + $refs = $this->sortWritableAlmanacServiceRefs($refs); } else { - shuffle($results); + $refs = $this->sortReadableAlmanacServiceRefs($refs); } - $result = head($results); - return $result['uri']; + return array_values($refs); } - private function sortWritableAlmanacServiceURIs(array $results) { + private function sortReadableAlmanacServiceRefs(array $refs) { + assert_instances_of($refs, 'DiffusionServiceRef'); + shuffle($refs); + return $refs; + } + + private function sortWritableAlmanacServiceRefs(array $refs) { + assert_instances_of($refs, 'DiffusionServiceRef'); + // See T13109 for discussion of how this method routes requests. // In the absence of other rules, we'll send traffic to devices randomly. // We also want to select randomly among nodes which are equally good // candidates to receive the write, and accomplish that by shuffling the // list up front. - shuffle($results); + shuffle($refs); $order = array(); @@ -2002,8 +2028,8 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $this->getPHID()); if ($writer) { $device_phid = $writer->getWriteProperty('devicePHID'); - foreach ($results as $key => $result) { - if ($result['devicePHID'] === $device_phid) { + foreach ($refs as $key => $ref) { + if ($ref->getDevicePHID() === $device_phid) { $order[] = $key; } } @@ -2025,8 +2051,8 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } $max_devices = array_fuse($max_devices); - foreach ($results as $key => $result) { - if (isset($max_devices[$result['devicePHID']])) { + foreach ($refs as $key => $ref) { + if (isset($max_devices[$ref->getDevicePHID()])) { $order[] = $key; } } @@ -2034,9 +2060,9 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO // Reorder the results, putting any we've selected as preferred targets for // the write at the head of the list. - $results = array_select_keys($results, $order) + $results; + $refs = array_select_keys($refs, $order) + $refs; - return $results; + return $refs; } public function supportsSynchronization() { From 95fb237ab393989b09a6354d22da622e1e6640c2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 3 Sep 2019 09:33:04 -0700 Subject: [PATCH 02/44] On Git cluster read failure, retry safe requests Summary: Depends on D20775. Ref T13286. When a Git read request fails against a cluster and there are other nodes we could safely try, try more nodes. We DO NOT retry the request if: - the client read anything; - the client wrote anything; - or we've already retried several times. Although //some// requests where bytes went over the wire in either direction may be safe to retry, they're rare in practice under Git, and we'd need to puzzle out what state we can safely emit. Since most types of failure result in an outright connection failure and this catches all of them, it's likely to almost always be sufficient in practice. Test Plan: - Started a cluster with one up node and one down node, pulled it. - Half the time, hit the up node and got a clean pull. - Half the time, hit the down node and got a connection failure followed by a retry and a clean pull. - Forced `$err = 1` so even successful attempts would retry. - On hitting the up node, got a "failure" and a decline to retry (bytes already written). - On hitting the down node, got a failure and a real retry. - (Note that, in both cases, "git pull" exits "0" after the valid wire transaction takes place, even though the remote exited non-zero. If the server gave Git everything it asked for, it doesn't seem to care if the server then exited with an error code.) Maniphest Tasks: T13286 Differential Revision: https://secure.phabricator.com/D20776 --- .../diffusion/ssh/DiffusionGitSSHWorkflow.php | 18 ++ .../ssh/DiffusionGitUploadPackSSHWorkflow.php | 183 ++++++++++++++---- 2 files changed, 163 insertions(+), 38 deletions(-) diff --git a/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php index d9cc8063d5..d8d0116017 100644 --- a/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php @@ -8,6 +8,8 @@ abstract class DiffusionGitSSHWorkflow private $protocolLog; private $wireProtocol; + private $ioBytesRead = 0; + private $ioBytesWritten = 0; protected function writeError($message) { // Git assumes we'll add our own newlines. @@ -98,6 +100,8 @@ abstract class DiffusionGitSSHWorkflow PhabricatorSSHPassthruCommand $command, $message) { + $this->ioBytesWritten += strlen($message); + $log = $this->getProtocolLog(); if ($log) { $log->didWriteBytes($message); @@ -125,7 +129,21 @@ abstract class DiffusionGitSSHWorkflow $message = $protocol->willReadBytes($message); } + // Note that bytes aren't counted until they're emittted by the protocol + // layer. This means the underlying command might emit bytes, but if they + // are buffered by the protocol layer they won't count as read bytes yet. + + $this->ioBytesRead += strlen($message); + return $message; } + final protected function getIOBytesRead() { + return $this->ioBytesRead; + } + + final protected function getIOBytesWritten() { + return $this->ioBytesWritten; + } + } diff --git a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php index 7e1f4a4f33..3e8186190a 100644 --- a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php @@ -1,6 +1,10 @@ setName('git-upload-pack'); @@ -14,39 +18,33 @@ final class DiffusionGitUploadPackSSHWorkflow extends DiffusionGitSSHWorkflow { } protected function executeRepositoryOperations() { - $repository = $this->getRepository(); + $is_proxy = $this->shouldProxy(); + if ($is_proxy) { + return $this->executeRepositoryProxyOperations(); + } + $viewer = $this->getSSHUser(); + $repository = $this->getRepository(); $device = AlmanacKeys::getLiveDevice(); $skip_sync = $this->shouldSkipReadSynchronization(); - $is_proxy = $this->shouldProxy(); - if ($is_proxy) { - $command = $this->getProxyCommand(false); + $command = csprintf('git-upload-pack -- %s', $repository->getLocalPath()); + if (!$skip_sync) { + $cluster_engine = id(new DiffusionRepositoryClusterEngine()) + ->setViewer($viewer) + ->setRepository($repository) + ->setLog($this) + ->synchronizeWorkingCopyBeforeRead(); if ($device) { $this->writeClusterEngineLogMessage( pht( - "# Fetch received by \"%s\", forwarding to cluster host.\n", + "# Cleared to fetch on cluster host \"%s\".\n", $device->getName())); } - } else { - $command = csprintf('git-upload-pack -- %s', $repository->getLocalPath()); - if (!$skip_sync) { - $cluster_engine = id(new DiffusionRepositoryClusterEngine()) - ->setViewer($viewer) - ->setRepository($repository) - ->setLog($this) - ->synchronizeWorkingCopyBeforeRead(); - - if ($device) { - $this->writeClusterEngineLogMessage( - pht( - "# Cleared to fetch on cluster host \"%s\".\n", - $device->getName())); - } - } } + $command = PhabricatorDaemon::sudoCommandAsDaemonUser($command); $pull_event = $this->newPullEvent(); @@ -60,14 +58,12 @@ final class DiffusionGitUploadPackSSHWorkflow extends DiffusionGitSSHWorkflow { $log->didStartSession($command); } - if (!$is_proxy) { - if (PhabricatorEnv::getEnvConfig('phabricator.show-prototypes')) { - $protocol = new DiffusionGitUploadPackWireProtocol(); - if ($log) { - $protocol->setProtocolLog($log); - } - $this->setWireProtocol($protocol); + if (PhabricatorEnv::getEnvConfig('phabricator.show-prototypes')) { + $protocol = new DiffusionGitUploadPackWireProtocol(); + if ($log) { + $protocol->setProtocolLog($log); } + $this->setWireProtocol($protocol); } $err = $this->newPassthruCommand() @@ -89,15 +85,7 @@ final class DiffusionGitUploadPackSSHWorkflow extends DiffusionGitSSHWorkflow { ->setResultCode(0); } - // TODO: Currently, when proxying, we do not write a log on the proxy. - // Perhaps we should write a "proxy log". This is not very useful for - // statistics or auditing, but could be useful for diagnostics. Marking - // the proxy logs as proxied (and recording devicePHID on all logs) would - // make differentiating between these use cases easier. - - if (!$is_proxy) { - $pull_event->save(); - } + $pull_event->save(); if (!$err) { $this->waitForGitClient(); @@ -106,4 +94,123 @@ final class DiffusionGitUploadPackSSHWorkflow extends DiffusionGitSSHWorkflow { return $err; } + private function executeRepositoryProxyOperations() { + $device = AlmanacKeys::getLiveDevice(); + $for_write = false; + + $refs = $this->getAlmanacServiceRefs($for_write); + $err = 1; + + while (true) { + $ref = head($refs); + + $command = $this->getProxyCommandForServiceRef($ref); + + if ($device) { + $this->writeClusterEngineLogMessage( + pht( + "# Fetch received by \"%s\", forwarding to cluster host \"%s\".\n", + $device->getName(), + $ref->getDeviceName())); + } + + $command = PhabricatorDaemon::sudoCommandAsDaemonUser($command); + + $future = id(new ExecFuture('%C', $command)) + ->setEnv($this->getEnvironment()); + + $this->didBeginRequest(); + + $err = $this->newPassthruCommand() + ->setIOChannel($this->getIOChannel()) + ->setCommandChannelFromExecFuture($future) + ->execute(); + + $err = 1; + + // TODO: Currently, when proxying, we do not write an event log on the + // proxy. Perhaps we should write a "proxy log". This is not very useful + // for statistics or auditing, but could be useful for diagnostics. + // Marking the proxy logs as proxied (and recording devicePHID on all + // logs) would make differentiating between these use cases easier. + + if (!$err) { + $this->waitForGitClient(); + return $err; + } + + // Throw away this service: the request failed and we're treating the + // failure as persistent, so we don't want to retry another request to + // the same host. + array_shift($refs); + + // Check if we have more services we can try. If we do, we'll make an + // effort to fall back to them below. If not, we can't do anything to + // recover so just bail out. + if (!$refs) { + return $err; + } + + $should_retry = $this->shouldRetryRequest(); + if (!$should_retry) { + return $err; + } + + // If we haven't bailed out yet, we'll retry the request with the next + // service. + } + + throw new Exception(pht('Reached an unreachable place.')); + } + + private function didBeginRequest() { + $this->requestAttempts++; + return $this; + } + + private function shouldRetryRequest() { + $this->requestFailures++; + + if ($this->requestFailures > $this->requestAttempts) { + throw new Exception( + pht( + "Workflow has recorded more failures than attempts; there is a ". + "missing call to \"didBeginRequest()\".\n")); + } + + $max_failures = 3; + if ($this->requestFailures >= $max_failures) { + $this->writeClusterEngineLogMessage( + pht( + "# Reached maximum number of retry attempts, giving up.\n")); + return false; + } + + $read_len = $this->getIOBytesRead(); + if ($read_len) { + $this->writeClusterEngineLogMessage( + pht( + "# Client already read from service (%s bytes), unable to retry.\n", + new PhutilNumber($read_len))); + return false; + } + + $write_len = $this->getIOBytesWritten(); + if ($write_len) { + $this->writeClusterEngineLogMessage( + pht( + "# Client already wrote to service (%s bytes), unable to retry.\n", + new PhutilNumber($write_len))); + return false; + } + + $this->writeClusterEngineLogMessage( + pht( + "# Service request failed, retrying (making attempt %s of %s).\n", + new PhutilNumber($this->requestAttempts + 1), + new PhutilNumber($max_failures))); + + return true; + } + } From ff3d1769b47538630de7bf180a360f8cf835ca86 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 3 Sep 2019 10:38:41 -0700 Subject: [PATCH 03/44] Instead of retrying safe reads 3 times, retry each eligible service once Summary: Ref T13286. When retrying a read request, keep retrying as long as we have canididate services. Since we consume a service with each attempt, there's no real reason to abort early, and trying every service allows reads to always succeed even if (for example) 8 nodes of a 16-node cluster are dead because of a severed network link between datacenters. Test Plan: Ran `git pull` in a clustered repository with an up node and a down node; saw retry count dynamically adjust to available node count. Maniphest Tasks: T13286 Differential Revision: https://secure.phabricator.com/D20777 --- .../ssh/DiffusionGitUploadPackSSHWorkflow.php | 21 ++++++------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php index 3e8186190a..5c0e2588b7 100644 --- a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php @@ -126,8 +126,6 @@ final class DiffusionGitUploadPackSSHWorkflow ->setCommandChannelFromExecFuture($future) ->execute(); - $err = 1; - // TODO: Currently, when proxying, we do not write an event log on the // proxy. Perhaps we should write a "proxy log". This is not very useful // for statistics or auditing, but could be useful for diagnostics. @@ -144,14 +142,7 @@ final class DiffusionGitUploadPackSSHWorkflow // the same host. array_shift($refs); - // Check if we have more services we can try. If we do, we'll make an - // effort to fall back to them below. If not, we can't do anything to - // recover so just bail out. - if (!$refs) { - return $err; - } - - $should_retry = $this->shouldRetryRequest(); + $should_retry = $this->shouldRetryRequest($refs); if (!$should_retry) { return $err; } @@ -168,7 +159,7 @@ final class DiffusionGitUploadPackSSHWorkflow return $this; } - private function shouldRetryRequest() { + private function shouldRetryRequest(array $remaining_refs) { $this->requestFailures++; if ($this->requestFailures > $this->requestAttempts) { @@ -178,11 +169,11 @@ final class DiffusionGitUploadPackSSHWorkflow "missing call to \"didBeginRequest()\".\n")); } - $max_failures = 3; - if ($this->requestFailures >= $max_failures) { + if (!$remaining_refs) { $this->writeClusterEngineLogMessage( pht( - "# Reached maximum number of retry attempts, giving up.\n")); + "# All available services failed to serve the request, ". + "giving up.\n")); return false; } @@ -208,7 +199,7 @@ final class DiffusionGitUploadPackSSHWorkflow pht( "# Service request failed, retrying (making attempt %s of %s).\n", new PhutilNumber($this->requestAttempts + 1), - new PhutilNumber($max_failures))); + new PhutilNumber($this->requestAttempts + count($remaining_refs)))); return true; } From 8ff3a133c4d7ffc02a47d6fc5fb4dae52b2cd0ab Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 3 Sep 2019 11:26:20 -0700 Subject: [PATCH 04/44] Generalize repository proxy retry logic to writes Summary: Ref T13286. The current (very safe / conservative) rules for retrying git reads generalize to git writes, so we can use the same ruleset in both cases. Normally, writes converge rapidly to only having good nodes at the head of the list, so this has less impact than the similar change to reads, but it generally improves consistency and allows us to assert that writes which can be served will be served. Test Plan: - In a cluster with an up node and a down node, pushed changes. - Saw a push to the down node fail, retry, and succeed. - Did some pulls, saw appropriate retries and success. - Note that once one write goes through, the node which received the write always ends up at the head of the writable list, so nodes need to be explicitly thawed to reproduce the failure/retry behavior. Maniphest Tasks: T13286 Differential Revision: https://secure.phabricator.com/D20778 --- .../DiffusionGitReceivePackSSHWorkflow.php | 61 ++++------ .../diffusion/ssh/DiffusionGitSSHWorkflow.php | 112 +++++++++++++++++ .../ssh/DiffusionGitUploadPackSSHWorkflow.php | 115 +----------------- 3 files changed, 137 insertions(+), 151 deletions(-) diff --git a/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php index abf2a4323e..f59a9b58b4 100644 --- a/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php @@ -14,42 +14,33 @@ final class DiffusionGitReceivePackSSHWorkflow extends DiffusionGitSSHWorkflow { } protected function executeRepositoryOperations() { + // This is a write, and must have write access. + $this->requireWriteAccess(); + + $is_proxy = $this->shouldProxy(); + if ($is_proxy) { + return $this->executeRepositoryProxyOperations($for_write = true); + } + $host_wait_start = microtime(true); $repository = $this->getRepository(); $viewer = $this->getSSHUser(); $device = AlmanacKeys::getLiveDevice(); - // This is a write, and must have write access. - $this->requireWriteAccess(); - $cluster_engine = id(new DiffusionRepositoryClusterEngine()) ->setViewer($viewer) ->setRepository($repository) ->setLog($this); - $is_proxy = $this->shouldProxy(); - if ($is_proxy) { - $command = $this->getProxyCommand(true); - $did_write = false; + $command = csprintf('git-receive-pack %s', $repository->getLocalPath()); + $cluster_engine->synchronizeWorkingCopyBeforeWrite(); - if ($device) { - $this->writeClusterEngineLogMessage( - pht( - "# Push received by \"%s\", forwarding to cluster host.\n", - $device->getName())); - } - } else { - $command = csprintf('git-receive-pack %s', $repository->getLocalPath()); - $did_write = true; - $cluster_engine->synchronizeWorkingCopyBeforeWrite(); - - if ($device) { - $this->writeClusterEngineLogMessage( - pht( - "# Ready to receive on cluster host \"%s\".\n", - $device->getName())); - } + if ($device) { + $this->writeClusterEngineLogMessage( + pht( + "# Ready to receive on cluster host \"%s\".\n", + $device->getName())); } $log = $this->newProtocolLog($is_proxy); @@ -71,9 +62,7 @@ final class DiffusionGitReceivePackSSHWorkflow extends DiffusionGitSSHWorkflow { // We've committed the write (or rejected it), so we can release the lock // without waiting for the client to receive the acknowledgement. - if ($did_write) { - $cluster_engine->synchronizeWorkingCopyAfterWrite(); - } + $cluster_engine->synchronizeWorkingCopyAfterWrite(); if ($caught) { throw $caught; @@ -85,18 +74,16 @@ final class DiffusionGitReceivePackSSHWorkflow extends DiffusionGitSSHWorkflow { // When a repository is clustered, we reach this cleanup code on both // the proxy and the actual final endpoint node. Don't do more cleanup // or logging than we need to. - if ($did_write) { - $repository->writeStatusMessage( - PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE, - PhabricatorRepositoryStatusMessage::CODE_OKAY); + $repository->writeStatusMessage( + PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE, + PhabricatorRepositoryStatusMessage::CODE_OKAY); - $host_wait_end = microtime(true); + $host_wait_end = microtime(true); - $this->updatePushLogWithTimingInformation( - $this->getClusterEngineLogProperty('writeWait'), - $this->getClusterEngineLogProperty('readWait'), - ($host_wait_end - $host_wait_start)); - } + $this->updatePushLogWithTimingInformation( + $this->getClusterEngineLogProperty('writeWait'), + $this->getClusterEngineLogProperty('readWait'), + ($host_wait_end - $host_wait_start)); } return $err; diff --git a/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php index d8d0116017..292741e34d 100644 --- a/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php @@ -10,6 +10,8 @@ abstract class DiffusionGitSSHWorkflow private $wireProtocol; private $ioBytesRead = 0; private $ioBytesWritten = 0; + private $requestAttempts = 0; + private $requestFailures = 0; protected function writeError($message) { // Git assumes we'll add our own newlines. @@ -146,4 +148,114 @@ abstract class DiffusionGitSSHWorkflow return $this->ioBytesWritten; } + final protected function executeRepositoryProxyOperations($for_write) { + $device = AlmanacKeys::getLiveDevice(); + + $refs = $this->getAlmanacServiceRefs($for_write); + $err = 1; + + while (true) { + $ref = head($refs); + + $command = $this->getProxyCommandForServiceRef($ref); + + if ($device) { + $this->writeClusterEngineLogMessage( + pht( + "# Request received by \"%s\", forwarding to cluster ". + "host \"%s\".\n", + $device->getName(), + $ref->getDeviceName())); + } + + $command = PhabricatorDaemon::sudoCommandAsDaemonUser($command); + + $future = id(new ExecFuture('%C', $command)) + ->setEnv($this->getEnvironment()); + + $this->didBeginRequest(); + + $err = $this->newPassthruCommand() + ->setIOChannel($this->getIOChannel()) + ->setCommandChannelFromExecFuture($future) + ->execute(); + + // TODO: Currently, when proxying, we do not write an event log on the + // proxy. Perhaps we should write a "proxy log". This is not very useful + // for statistics or auditing, but could be useful for diagnostics. + // Marking the proxy logs as proxied (and recording devicePHID on all + // logs) would make differentiating between these use cases easier. + + if (!$err) { + $this->waitForGitClient(); + return $err; + } + + // Throw away this service: the request failed and we're treating the + // failure as persistent, so we don't want to retry another request to + // the same host. + array_shift($refs); + + $should_retry = $this->shouldRetryRequest($refs); + if (!$should_retry) { + return $err; + } + + // If we haven't bailed out yet, we'll retry the request with the next + // service. + } + + throw new Exception(pht('Reached an unreachable place.')); + } + + private function didBeginRequest() { + $this->requestAttempts++; + return $this; + } + + private function shouldRetryRequest(array $remaining_refs) { + $this->requestFailures++; + + if ($this->requestFailures > $this->requestAttempts) { + throw new Exception( + pht( + "Workflow has recorded more failures than attempts; there is a ". + "missing call to \"didBeginRequest()\".\n")); + } + + if (!$remaining_refs) { + $this->writeClusterEngineLogMessage( + pht( + "# All available services failed to serve the request, ". + "giving up.\n")); + return false; + } + + $read_len = $this->getIOBytesRead(); + if ($read_len) { + $this->writeClusterEngineLogMessage( + pht( + "# Client already read from service (%s bytes), unable to retry.\n", + new PhutilNumber($read_len))); + return false; + } + + $write_len = $this->getIOBytesWritten(); + if ($write_len) { + $this->writeClusterEngineLogMessage( + pht( + "# Client already wrote to service (%s bytes), unable to retry.\n", + new PhutilNumber($write_len))); + return false; + } + + $this->writeClusterEngineLogMessage( + pht( + "# Service request failed, retrying (making attempt %s of %s).\n", + new PhutilNumber($this->requestAttempts + 1), + new PhutilNumber($this->requestAttempts + count($remaining_refs)))); + + return true; + } + } diff --git a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php index 5c0e2588b7..57c43b5a12 100644 --- a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php @@ -3,9 +3,6 @@ final class DiffusionGitUploadPackSSHWorkflow extends DiffusionGitSSHWorkflow { - private $requestAttempts = 0; - private $requestFailures = 0; - protected function didConstruct() { $this->setName('git-upload-pack'); $this->setArguments( @@ -20,7 +17,7 @@ final class DiffusionGitUploadPackSSHWorkflow protected function executeRepositoryOperations() { $is_proxy = $this->shouldProxy(); if ($is_proxy) { - return $this->executeRepositoryProxyOperations(); + return $this->executeRepositoryProxyOperations($for_write = false); } $viewer = $this->getSSHUser(); @@ -94,114 +91,4 @@ final class DiffusionGitUploadPackSSHWorkflow return $err; } - private function executeRepositoryProxyOperations() { - $device = AlmanacKeys::getLiveDevice(); - $for_write = false; - - $refs = $this->getAlmanacServiceRefs($for_write); - $err = 1; - - while (true) { - $ref = head($refs); - - $command = $this->getProxyCommandForServiceRef($ref); - - if ($device) { - $this->writeClusterEngineLogMessage( - pht( - "# Fetch received by \"%s\", forwarding to cluster host \"%s\".\n", - $device->getName(), - $ref->getDeviceName())); - } - - $command = PhabricatorDaemon::sudoCommandAsDaemonUser($command); - - $future = id(new ExecFuture('%C', $command)) - ->setEnv($this->getEnvironment()); - - $this->didBeginRequest(); - - $err = $this->newPassthruCommand() - ->setIOChannel($this->getIOChannel()) - ->setCommandChannelFromExecFuture($future) - ->execute(); - - // TODO: Currently, when proxying, we do not write an event log on the - // proxy. Perhaps we should write a "proxy log". This is not very useful - // for statistics or auditing, but could be useful for diagnostics. - // Marking the proxy logs as proxied (and recording devicePHID on all - // logs) would make differentiating between these use cases easier. - - if (!$err) { - $this->waitForGitClient(); - return $err; - } - - // Throw away this service: the request failed and we're treating the - // failure as persistent, so we don't want to retry another request to - // the same host. - array_shift($refs); - - $should_retry = $this->shouldRetryRequest($refs); - if (!$should_retry) { - return $err; - } - - // If we haven't bailed out yet, we'll retry the request with the next - // service. - } - - throw new Exception(pht('Reached an unreachable place.')); - } - - private function didBeginRequest() { - $this->requestAttempts++; - return $this; - } - - private function shouldRetryRequest(array $remaining_refs) { - $this->requestFailures++; - - if ($this->requestFailures > $this->requestAttempts) { - throw new Exception( - pht( - "Workflow has recorded more failures than attempts; there is a ". - "missing call to \"didBeginRequest()\".\n")); - } - - if (!$remaining_refs) { - $this->writeClusterEngineLogMessage( - pht( - "# All available services failed to serve the request, ". - "giving up.\n")); - return false; - } - - $read_len = $this->getIOBytesRead(); - if ($read_len) { - $this->writeClusterEngineLogMessage( - pht( - "# Client already read from service (%s bytes), unable to retry.\n", - new PhutilNumber($read_len))); - return false; - } - - $write_len = $this->getIOBytesWritten(); - if ($write_len) { - $this->writeClusterEngineLogMessage( - pht( - "# Client already wrote to service (%s bytes), unable to retry.\n", - new PhutilNumber($write_len))); - return false; - } - - $this->writeClusterEngineLogMessage( - pht( - "# Service request failed, retrying (making attempt %s of %s).\n", - new PhutilNumber($this->requestAttempts + 1), - new PhutilNumber($this->requestAttempts + count($remaining_refs)))); - - return true; - } - } From d9badba14786e786d6c76e11a5860e81151ce708 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 3 Sep 2019 12:06:17 -0700 Subject: [PATCH 05/44] Give "bin/config" a friendlier error message if "local.json" is not writable Summary: Ref T13403. We currently emit a useful error message, but it's not tailored and has a stack trace. Since this is a relatively routine error and on the first-time-setup path, tailor it so it's a bit nicer. Test Plan: - Ran `bin/config set ...` with an unwritable "local.json". - Ran `bin/config set ...` normally. Maniphest Tasks: T13403 Differential Revision: https://secure.phabricator.com/D20779 --- .../PhabricatorConfigManagementSetWorkflow.php | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php b/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php index 9eb83bd61e..6ad2db4471 100644 --- a/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php +++ b/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php @@ -140,11 +140,22 @@ final class PhabricatorConfigManagementSetWorkflow 'Wrote configuration key "%s" to database storage.', $key); } else { - $config_source = id(new PhabricatorConfigLocalSource()) - ->setKeys(array($key => $value)); + $config_source = new PhabricatorConfigLocalSource(); $local_path = $config_source->getReadablePath(); + try { + Filesystem::assertWritable($local_path); + } catch (FilesystemException $ex) { + throw new PhutilArgumentUsageException( + pht( + 'Local path "%s" is not writable. This file must be writable '. + 'so that "bin/config" can store configuration.', + Filesystem::readablePath($local_path))); + } + + $config_source->setKeys(array($key => $value)); + $write_message = pht( 'Wrote configuration key "%s" to local storage (in file "%s").', $key, From f8eec38c941954b870940e99ffb4860e4a16eb8d Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 3 Sep 2019 12:09:20 -0700 Subject: [PATCH 06/44] When "mysqli->real_connect()" fails without setting an error code, recover more gracefully Summary: Depends on D20779. Ref T13403. Bad parameters may cause this call to fail without setting an error code; if it does, catch the issue and go down the normal connection error pathway. Test Plan: - With "mysql.port" set to "quack", ran `bin/storage probe`. - Before: wild mess of warnings as the code continued below and failed when trying to interact with the connection. - After: clean connection failure with a useful error message. Maniphest Tasks: T13403 Differential Revision: https://secure.phabricator.com/D20780 --- .../AphrontBaseMySQLDatabaseConnection.php | 3 ++ .../mysql/AphrontMySQLiDatabaseConnection.php | 32 +++++++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/infrastructure/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php b/src/infrastructure/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php index 0f9201b02d..313ea5a3b0 100644 --- a/src/infrastructure/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php +++ b/src/infrastructure/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php @@ -10,6 +10,9 @@ abstract class AphrontBaseMySQLDatabaseConnection private $nextError; + const CALLERROR_QUERY = 777777; + const CALLERROR_CONNECT = 777778; + abstract protected function connect(); abstract protected function rawQuery($raw_query); abstract protected function rawQueries(array $raw_queries); diff --git a/src/infrastructure/storage/connection/mysql/AphrontMySQLiDatabaseConnection.php b/src/infrastructure/storage/connection/mysql/AphrontMySQLiDatabaseConnection.php index 7a4b5193d5..6a0bc759a7 100644 --- a/src/infrastructure/storage/connection/mysql/AphrontMySQLiDatabaseConnection.php +++ b/src/infrastructure/storage/connection/mysql/AphrontMySQLiDatabaseConnection.php @@ -68,19 +68,47 @@ final class AphrontMySQLiDatabaseConnection $host = 'p:'.$host; } - @$conn->real_connect( + $trap = new PhutilErrorTrap(); + + $ok = @$conn->real_connect( $host, $user, $pass, $database, $port); + $call_error = $trap->getErrorsAsString(); + $trap->destroy(); + $errno = $conn->connect_errno; if ($errno) { $error = $conn->connect_error; $this->throwConnectionException($errno, $error, $user, $host); } + // See T13403. If the parameters to "real_connect()" are wrong, it may + // fail without setting an error code. In this case, raise a generic + // exception. (One way to reproduce this is to pass a string to the + // "port" parameter.) + + if (!$ok) { + if (strlen($call_error)) { + $message = pht( + 'mysqli->real_connect() failed: %s', + $call_error); + } else { + $message = pht( + 'mysqli->real_connect() failed, but did not set an error code '. + 'or emit a message.'); + } + + $this->throwConnectionException( + self::CALLERROR_CONNECT, + $message, + $user, + $host); + } + // See T13238. Attempt to prevent "LOAD DATA LOCAL INFILE", which allows a // malicious server to ask the client for any file. At time of writing, // this option MUST be set after "real_connect()" on all PHP versions. @@ -152,7 +180,7 @@ final class AphrontMySQLiDatabaseConnection 'Call to "mysqli->query()" failed, but did not set an error '. 'code or emit an error message.'); } - $this->throwQueryCodeException(777777, $message); + $this->throwQueryCodeException(self::CALLERROR_QUERY, $message); } } From e0d6994adb1ce5b961a67808d53c924741da4850 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 3 Sep 2019 12:16:17 -0700 Subject: [PATCH 07/44] Use the "@" operator to silence connection retry messages if initializing the stack with database config optional Summary: Depends on D20780. Ref T13403. During initial setup, it's routine to run "bin/config" with a bad database config. We start the stack in "config optional" mode to anticipate this. However, even in this mode, we may emit warnings if the connection fails in certain ways. These warnings aren't useful; suppress them with "@". (Possibly this message should move from "phlog()" to "--trace" at some point, but it has a certain amount of context/history around it.) Test Plan: - Configured MySQL to fail with a retryable error, e.g. good host but bad port. - Ran `bin/config set ...`. - Before: saw retry warnings on stderr. - After: no retry warnings on stderr. - (Turned off suppression code artificially and verified warnings still appear under normal startup.) Maniphest Tasks: T13403 Differential Revision: https://secure.phabricator.com/D20781 --- src/infrastructure/env/PhabricatorEnv.php | 14 +++++++++++--- .../mysql/AphrontBaseMySQLDatabaseConnection.php | 9 ++++++++- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php index 24fb940c9a..d5f990065d 100644 --- a/src/infrastructure/env/PhabricatorEnv.php +++ b/src/infrastructure/env/PhabricatorEnv.php @@ -249,9 +249,17 @@ final class PhabricatorEnv extends Phobject { } try { - $stack->pushSource( - id(new PhabricatorConfigDatabaseSource('default')) - ->setName(pht('Database'))); + // See T13403. If we're starting up in "config optional" mode, suppress + // messages about connection retries. + if ($config_optional) { + $database_source = @new PhabricatorConfigDatabaseSource('default'); + } else { + $database_source = new PhabricatorConfigDatabaseSource('default'); + } + + $database_source->setName(pht('Database')); + + $stack->pushSource($database_source); } catch (AphrontSchemaQueryException $exception) { // If the database is not available, just skip this configuration // source. This happens during `bin/storage upgrade`, `bin/conf` before diff --git a/src/infrastructure/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php b/src/infrastructure/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php index 313ea5a3b0..6faf10e2c6 100644 --- a/src/infrastructure/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php +++ b/src/infrastructure/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php @@ -126,7 +126,14 @@ abstract class AphrontBaseMySQLDatabaseConnection $code, $ex->getMessage()); - phlog($message); + // See T13403. If we're silenced with the "@" operator, don't log + // this connection attempt. This keeps things quiet if we're + // running a setup workflow like "bin/config" and expect that the + // database credentials will often be incorrect. + + if (error_reporting()) { + phlog($message); + } } else { $profiler->endServiceCall($call_id, array()); throw $ex; From 22b075df97168e0812205482346638f53c62da14 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 3 Sep 2019 16:33:07 -0700 Subject: [PATCH 08/44] Fix "ONLY_FULL_GROUP_BY" issue in SystemAction queries Summary: Ref T13404. This query is invalid under "sql_mode=ONLY_FULL_GROUP_BY". Rewrite it to avoid interacting with `actorIdentity` at all; this is a little more robust in the presence of weird data and not really more complicated. Test Plan: - Enabled "ONLY_FULL_GROUP_BY". - Hit system actions (e.g., login). - Before: error. - After: clean login. - Tried to login with a bad password many times in a row, got properly limited by the system action rate limiter. Maniphest Tasks: T13404 Differential Revision: https://secure.phabricator.com/D20782 --- .../engine/PhabricatorSystemActionEngine.php | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/applications/system/engine/PhabricatorSystemActionEngine.php b/src/applications/system/engine/PhabricatorSystemActionEngine.php index c097fa04a4..6d6f9eacfd 100644 --- a/src/applications/system/engine/PhabricatorSystemActionEngine.php +++ b/src/applications/system/engine/PhabricatorSystemActionEngine.php @@ -100,32 +100,34 @@ final class PhabricatorSystemActionEngine extends Phobject { $actor_hashes = array(); foreach ($actors as $actor) { - $actor_hashes[] = PhabricatorHash::digestForIndex($actor); + $digest = PhabricatorHash::digestForIndex($actor); + $actor_hashes[$digest] = $actor; } $log = new PhabricatorSystemActionLog(); $window = self::getWindow(); - $conn_r = $log->establishConnection('r'); - $scores = queryfx_all( - $conn_r, - 'SELECT actorIdentity, SUM(score) totalScore FROM %T + $conn = $log->establishConnection('r'); + + $rows = queryfx_all( + $conn, + 'SELECT actorHash, SUM(score) totalScore FROM %T WHERE action = %s AND actorHash IN (%Ls) AND epoch >= %d GROUP BY actorHash', $log->getTableName(), $action->getActionConstant(), - $actor_hashes, - (time() - $window)); + array_keys($actor_hashes), + (PhabricatorTime::getNow() - $window)); - $scores = ipull($scores, 'totalScore', 'actorIdentity'); + $rows = ipull($rows, 'totalScore', 'actorHash'); - foreach ($scores as $key => $score) { - $scores[$key] = $score / $window; + $scores = array(); + foreach ($actor_hashes as $digest => $actor) { + $score = idx($rows, $digest, 0); + $scores[$actor] = ($score / $window); } - $scores = $scores + array_fill_keys($actors, 0); - return $scores; } From f7290bbbf220a362e7b81ab82bd04e93cacdbcf4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 4 Sep 2019 07:09:39 -0700 Subject: [PATCH 09/44] Update a straggling "getAuthorities()" call in Fund Summary: Ref T13366. The "authorities" mechanism was replaced, but I missed this callsite. Update it to use the request cache mechanism. Test Plan: As a user without permission to view some initiatives, viewed a list of initiatives. Maniphest Tasks: T13366 Differential Revision: https://secure.phabricator.com/D20783 --- src/applications/fund/storage/FundInitiative.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/applications/fund/storage/FundInitiative.php b/src/applications/fund/storage/FundInitiative.php index 5e4dd48026..1ebbb35ef1 100644 --- a/src/applications/fund/storage/FundInitiative.php +++ b/src/applications/fund/storage/FundInitiative.php @@ -136,12 +136,12 @@ final class FundInitiative extends FundDAO } if ($capability == PhabricatorPolicyCapability::CAN_VIEW) { - foreach ($viewer->getAuthorities() as $authority) { - if ($authority instanceof PhortuneMerchant) { - if ($authority->getPHID() == $this->getMerchantPHID()) { - return true; - } - } + $can_merchant = PhortuneMerchantQuery::canViewersEditMerchants( + array($viewer->getPHID()), + array($this->getMerchantPHID())); + + if ($can_merchant) { + return true; } } From 764db4869cb05b5fc7894414a7cb915a274ba476 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 4 Sep 2019 10:02:51 -0700 Subject: [PATCH 10/44] Make "bin/storage destroy" target individual hosts in database cluster mode Summary: Ref T13336. Currently, "bin/storage destroy" destroys every master. This is wonderfully destructive, but if replication fails it's useful to be able to destroy only a replica. Operate on a single host, and require "--host" to target the operation in cluster mode, so `bin/storage destroy --host dbreplica001` is a useful operation. Test Plan: Ran `bin/storage destroy` with various flags locally. Will destroy `secure002` and refresh replication. Maniphest Tasks: T13336 Differential Revision: https://secure.phabricator.com/D20784 --- .../cluster/PhabricatorDatabaseRef.php | 3 + .../PhabricatorStorageManagementAPI.php | 4 + ...icatorStorageManagementDestroyWorkflow.php | 134 +++++++++++------- 3 files changed, 87 insertions(+), 54 deletions(-) diff --git a/src/infrastructure/cluster/PhabricatorDatabaseRef.php b/src/infrastructure/cluster/PhabricatorDatabaseRef.php index 89435b5869..478f95750b 100644 --- a/src/infrastructure/cluster/PhabricatorDatabaseRef.php +++ b/src/infrastructure/cluster/PhabricatorDatabaseRef.php @@ -221,6 +221,9 @@ final class PhabricatorDatabaseRef return $this->replicaRefs; } + public function getDisplayName() { + return $this->getRefKey(); + } public function getRefKey() { $host = $this->getHost(); diff --git a/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php b/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php index b838c8a5d9..a6a0d74593 100644 --- a/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php +++ b/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php @@ -89,6 +89,10 @@ final class PhabricatorStorageManagementAPI extends Phobject { return $this->namespace.'_'.$fragment; } + public function getDisplayName() { + return $this->getRef()->getDisplayName(); + } + public function getDatabaseList(array $patches, $only_living = false) { assert_instances_of($patches, 'PhabricatorStoragePatch'); diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDestroyWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDestroyWorkflow.php index 7d0946c8c3..9b718e231d 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDestroyWorkflow.php +++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDestroyWorkflow.php @@ -21,86 +21,112 @@ final class PhabricatorStorageManagementDestroyWorkflow } public function didExecute(PhutilArgumentParser $args) { - $console = PhutilConsole::getConsole(); + $api = $this->getSingleAPI(); + + $host_display = $api->getDisplayName(); if (!$this->isDryRun() && !$this->isForce()) { if ($args->getArg('unittest-fixtures')) { - $console->writeOut( - phutil_console_wrap( - pht( - 'Are you completely sure you really want to destroy all unit '. - 'test fixure data? This operation can not be undone.'))); + $warning = pht( + 'Are you completely sure you really want to destroy all unit '. + 'test fixure data on host "%s"? This operation can not be undone.', + $host_display); + + echo tsprintf( + '%B', + id(new PhutilConsoleBlock()) + ->addParagraph($warning) + ->drawConsoleString()); + if (!phutil_console_confirm(pht('Destroy all unit test data?'))) { - $console->writeOut("%s\n", pht('Cancelled.')); + $this->logFail( + pht('CANCELLED'), + pht('User cancelled operation.')); exit(1); } } else { - $console->writeOut( - phutil_console_wrap( - pht( - 'Are you completely sure you really want to permanently destroy '. - 'all storage for Phabricator data? This operation can not be '. - 'undone and your data will not be recoverable if you proceed.'))); + $warning = pht( + 'Are you completely sure you really want to permanently destroy '. + 'all storage for Phabricator data on host "%s"? This operation '. + 'can not be undone and your data will not be recoverable if '. + 'you proceed.', + $host_display); + + echo tsprintf( + '%B', + id(new PhutilConsoleBlock()) + ->addParagraph($warning) + ->drawConsoleString()); if (!phutil_console_confirm(pht('Permanently destroy all data?'))) { - $console->writeOut("%s\n", pht('Cancelled.')); + $this->logFail( + pht('CANCELLED'), + pht('User cancelled operation.')); exit(1); } if (!phutil_console_confirm(pht('Really destroy all data forever?'))) { - $console->writeOut("%s\n", pht('Cancelled.')); + $this->logFail( + pht('CANCELLED'), + pht('User cancelled operation.')); exit(1); } } } - $apis = $this->getMasterAPIs(); - foreach ($apis as $api) { - $patches = $this->getPatches(); + $patches = $this->getPatches(); - if ($args->getArg('unittest-fixtures')) { - $conn = $api->getConn(null); - $databases = queryfx_all( - $conn, - 'SELECT DISTINCT(TABLE_SCHEMA) AS db '. - 'FROM INFORMATION_SCHEMA.TABLES '. - 'WHERE TABLE_SCHEMA LIKE %>', - PhabricatorTestCase::NAMESPACE_PREFIX); - $databases = ipull($databases, 'db'); - } else { - $databases = $api->getDatabaseList($patches); - $databases[] = $api->getDatabaseName('meta_data'); + if ($args->getArg('unittest-fixtures')) { + $conn = $api->getConn(null); + $databases = queryfx_all( + $conn, + 'SELECT DISTINCT(TABLE_SCHEMA) AS db '. + 'FROM INFORMATION_SCHEMA.TABLES '. + 'WHERE TABLE_SCHEMA LIKE %>', + PhabricatorTestCase::NAMESPACE_PREFIX); + $databases = ipull($databases, 'db'); + } else { + $databases = $api->getDatabaseList($patches); + $databases[] = $api->getDatabaseName('meta_data'); - // These are legacy databases that were dropped long ago. See T2237. - $databases[] = $api->getDatabaseName('phid'); - $databases[] = $api->getDatabaseName('directory'); - } + // These are legacy databases that were dropped long ago. See T2237. + $databases[] = $api->getDatabaseName('phid'); + $databases[] = $api->getDatabaseName('directory'); + } - foreach ($databases as $database) { - if ($this->isDryRun()) { - $console->writeOut( - "%s\n", - pht("DRYRUN: Would drop database '%s'.", $database)); - } else { - $console->writeOut( - "%s\n", - pht("Dropping database '%s'...", $database)); - queryfx( - $api->getConn(null), - 'DROP DATABASE IF EXISTS %T', - $database); - } - } + asort($databases); - if (!$this->isDryRun()) { - $console->writeOut( - "%s\n", + foreach ($databases as $database) { + if ($this->isDryRun()) { + $this->logInfo( + pht('DRY RUN'), pht( - 'Storage on "%s" was destroyed.', - $api->getRef()->getRefKey())); + 'Would drop database "%s" on host "%s".', + $database, + $host_display)); + } else { + $this->logWarn( + pht('DESTROY'), + pht( + 'Dropping database "%s" on host "%s"...', + $database, + $host_display)); + + queryfx( + $api->getConn(null), + 'DROP DATABASE IF EXISTS %T', + $database); } } + if (!$this->isDryRun()) { + $this->logOkay( + pht('DONE'), + pht( + 'Storage on "%s" was destroyed.', + $host_display)); + } + return 0; } From adc2002d2870f3ca277723a2e81fff1b6922cd81 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 5 Sep 2019 03:43:22 -0700 Subject: [PATCH 11/44] Make it easier to parse "X-Forwarded-For" with one or more load balancers Summary: Fixes T13392. If you have 17 load balancers in sequence, Phabricator will receive requests with at least 17 "X-Forwarded-For" components in the header. We want to select the 17th-from-last element, since prior elements are not trustworthy. This currently isn't very easy/obvious, and you have to add a kind of sketchy piece of custom code to `preamble.php` to do any "X-Forwarded-For" parsing. Make handling this correctly easier. Test Plan: - Ran unit tests. - Configured my local `preamble.php` to call `preamble_trust_x_forwarded_for_header(4)`, then made `/debug/` dump the header and the final value of `REMOTE_ADDR`. ``` $ curl http://local.phacility.com/debug/ 

HTTP_X_FORWARDED_FOR =
   FINAL REMOTE_ADDR = 127.0.0.1
``` ``` $ curl -H 'X-Forwarded-For: 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5, 6.6.6.6' http://local.phacility.com/debug/ 

HTTP_X_FORWARDED_FOR = 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5, 6.6.6.6
   FINAL REMOTE_ADDR = 3.3.3.3
``` ``` $ curl -H 'X-Forwarded-For: 5.5.5.5, 6.6.6.6' http://local.phacility.com/debug/ 

HTTP_X_FORWARDED_FOR = 5.5.5.5, 6.6.6.6
   FINAL REMOTE_ADDR = 5.5.5.5
``` Maniphest Tasks: T13392 Differential Revision: https://secure.phabricator.com/D20785 --- src/__phutil_library_map__.php | 2 + .../configuring_preamble.diviner | 51 ++++++------ src/infrastructure/env/PhabricatorEnv.php | 5 ++ .../__tests__/PhabricatorPreambleTestCase.php | 74 ++++++++++++++++++ support/startup/preamble-utils.php | 77 +++++++++++++++++++ webroot/index.php | 1 + 6 files changed, 187 insertions(+), 23 deletions(-) create mode 100644 src/infrastructure/util/__tests__/PhabricatorPreambleTestCase.php create mode 100644 support/startup/preamble-utils.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 212a7b1607..fa9a5ce903 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4201,6 +4201,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyTestObject' => 'applications/policy/__tests__/PhabricatorPolicyTestObject.php', 'PhabricatorPolicyType' => 'applications/policy/constants/PhabricatorPolicyType.php', 'PhabricatorPonderApplication' => 'applications/ponder/application/PhabricatorPonderApplication.php', + 'PhabricatorPreambleTestCase' => 'infrastructure/util/__tests__/PhabricatorPreambleTestCase.php', 'PhabricatorPrimaryEmailUserLogType' => 'applications/people/userlog/PhabricatorPrimaryEmailUserLogType.php', 'PhabricatorProfileMenuEditEngine' => 'applications/search/editor/PhabricatorProfileMenuEditEngine.php', 'PhabricatorProfileMenuEditor' => 'applications/search/editor/PhabricatorProfileMenuEditor.php', @@ -10681,6 +10682,7 @@ phutil_register_library_map(array( ), 'PhabricatorPolicyType' => 'PhabricatorPolicyConstants', 'PhabricatorPonderApplication' => 'PhabricatorApplication', + 'PhabricatorPreambleTestCase' => 'PhabricatorTestCase', 'PhabricatorPrimaryEmailUserLogType' => 'PhabricatorUserLogType', 'PhabricatorProfileMenuEditEngine' => 'PhabricatorEditEngine', 'PhabricatorProfileMenuEditor' => 'PhabricatorApplicationTransactionEditor', diff --git a/src/docs/user/configuration/configuring_preamble.diviner b/src/docs/user/configuration/configuring_preamble.diviner index fc804e9072..5299afa27d 100644 --- a/src/docs/user/configuration/configuring_preamble.diviner +++ b/src/docs/user/configuration/configuring_preamble.diviner @@ -15,10 +15,9 @@ You can use a special preamble script to make arbitrary adjustments to the environment and some parts of Phabricator's configuration in order to fix these problems and set up the environment which Phabricator expects. -NOTE: This is an advanced feature. Most installs should not need to configure -a preamble script. -= Creating a Preamble Script = +Creating a Preamble Script +========================== To create a preamble script, write a file to: @@ -37,6 +36,7 @@ If present, this script will be executed at the very beginning of each web request, allowing you to adjust the environment. For common adjustments and examples, see the next sections. + Adjusting Client IPs ==================== @@ -44,9 +44,15 @@ If your install is behind a load balancer, Phabricator may incorrectly detect all requests as originating from the load balancer, rather than from the correct client IPs. -If this is the case and some other header (like `X-Forwarded-For`) is known to -be trustworthy, you can read the header and overwrite the `REMOTE_ADDR` value -so Phabricator can figure out the client IP correctly. +In common cases where networks are configured like this, the `X-Forwarded-For` +header will have trustworthy information about the real client IP. You +can use the function `preamble_trust_x_forwarded_for_header()` in your +preamble to tell Phabricator that you expect to receive requests from a +load balancer or proxy which modifies this header: + +```name="Trust X-Forwarded-For Header", lang=php +preamble_trust_x_forwarded_for_header(); +``` You should do this //only// if the `X-Forwarded-For` header is known to be trustworthy. In particular, if users can make requests to the web server @@ -54,30 +60,29 @@ directly, they can provide an arbitrary `X-Forwarded-For` header, and thereby spoof an arbitrary client IP. The `X-Forwarded-For` header may also contain a list of addresses if a request -has been forwarded through multiple loadbalancers. Using a snippet like this -will usually handle most situations correctly: +has been forwarded through multiple load balancers. If you know that requests +on your network are routed through `N` trustworthy devices, you can specify +that `N` to tell the function how many layers of `X-Forwarded-For` to discard: +```name="Trust X-Forwarded-For Header, Multiple Layers", lang=php +preamble_trust_x_forwarded_for_header(3); ``` -name=Overwrite REMOTE_ADDR with X-Forwarded-For - '1.2.3.4', + 'layers' => 1, + 'expect' => '1.2.3.4', + ), + + // In this case, the LB received a request which already had an + // "X-Forwarded-For" header. This might be legitimate (in the case of + // a CDN request) or illegitimate (in the case of a client making + // things up). We don't want to trust it. + array( + 'header' => '9.9.9.9, 1.2.3.4', + 'layers' => 1, + 'expect' => '1.2.3.4', + ), + + // Multiple layers of load balancers. + array( + 'header' => '9.9.9.9, 1.2.3.4', + 'layers' => 2, + 'expect' => '9.9.9.9', + ), + + // Multiple layers of load balancers, plus a client-supplied value. + array( + 'header' => '8.8.8.8, 9.9.9.9, 1.2.3.4', + 'layers' => 2, + 'expect' => '9.9.9.9', + ), + + // Multiple layers of load balancers, but this request came from + // somewhere inside the network. + array( + 'header' => '1.2.3.4', + 'layers' => 2, + 'expect' => '1.2.3.4', + ), + + array( + 'header' => 'A, B, C, D, E, F, G, H, I', + 'layers' => 7, + 'expect' => 'C', + ), + ); + + foreach ($tests as $test) { + $header = $test['header']; + $layers = $test['layers']; + $expect = $test['expect']; + + $actual = preamble_get_x_forwarded_for_address($header, $layers); + + $this->assertEqual( + $expect, + $actual, + pht( + 'Address after stripping %d layers from: %s', + $layers, + $header)); + } + } + +} diff --git a/support/startup/preamble-utils.php b/support/startup/preamble-utils.php new file mode 100644 index 0000000000..8dd3b502d6 --- /dev/null +++ b/support/startup/preamble-utils.php @@ -0,0 +1,77 @@ +): '. + '"layers" parameter must an integer larger than 0.'."\n"; + echo "\n"; + exit(1); + } + + if (!isset($_SERVER['HTTP_X_FORWARDED_FOR'])) { + return; + } + + $forwarded_for = $_SERVER['HTTP_X_FORWARDED_FOR']; + if (!strlen($forwarded_for)) { + return; + } + + $address = preamble_get_x_forwarded_for_address($forwarded_for, $layers); + + $_SERVER['REMOTE_ADDR'] = $address; +} + +function preamble_get_x_forwarded_for_address($raw_header, $layers) { + // The raw header may be a list of IPs, like "1.2.3.4, 4.5.6.7", if the + // request the load balancer received also had this header. In particular, + // this happens routinely with requests received through a CDN, but can also + // happen illegitimately if the client just makes up an "X-Forwarded-For" + // header full of lies. + + // We can only trust the N elements at the end of the list which correspond + // to network-adjacent devices we control. Usually, we're behind a single + // load balancer and "N" is 1, so we want to take the last element in the + // list. + + // In some cases, "N" may be more than 1, if the network is configured so + // that that requests are routed through multiple layers of load balancers + // and proxies. In this case, we want to take the Nth-to-last element of + // the list. + + $addresses = explode(',', $raw_header); + + // If we have more than one trustworthy device on the network path, discard + // corresponding elements from the list. For example, if we have 7 devices, + // we want to discard the last 6 elements of the list. + + // The final device address does not appear in the list, since devices do + // not append their own addresses to "X-Forwarded-For". + + $discard_addresses = ($layers - 1); + + // However, we don't want to throw away all of the addresses. Some requests + // may originate from within the network, and may thus not have as many + // addresses as we expect. If we have fewer addresses than trustworthy + // devices, discard all but one address. + + $max_discard = (count($addresses) - 1); + + $discard_count = min($discard_addresses, $max_discard); + if ($discard_count) { + $addresses = array_slice($addresses, 0, -$discard_count); + } + + $original_address = end($addresses); + $original_address = trim($original_address); + + return $original_address; +} diff --git a/webroot/index.php b/webroot/index.php index 0014edfa2c..38c5c77809 100644 --- a/webroot/index.php +++ b/webroot/index.php @@ -85,6 +85,7 @@ function phabricator_startup() { require_once $root.'/support/startup/PhabricatorClientLimit.php'; require_once $root.'/support/startup/PhabricatorClientRateLimit.php'; require_once $root.'/support/startup/PhabricatorClientConnectionLimit.php'; + require_once $root.'/support/startup/preamble-utils.php'; // If the preamble script exists, load it. $t_preamble = microtime(true); From 7e2bec92807d2a6c432fd7f690e9cb7dda0b1fc3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 6 Sep 2019 08:18:28 -0700 Subject: [PATCH 12/44] Add a global setting for controlling the default main menu search scope Summary: Fixes T13405. The default behavior of the global search bar isn't currently configurable, but can be made configurable fairly easily. Test Plan: Changed setting as an administrator, saw setting reflected as a user with no previous preference. As a user with an existing preference, saw preference retained. Maniphest Tasks: T13405 Differential Revision: https://secure.phabricator.com/D20787 --- src/__phutil_library_map__.php | 4 ++- .../panel/PhabricatorSearchSettingsPanel.php | 28 +++++++++++++++++++ .../setting/PhabricatorSearchScopeSetting.php | 27 +++++++++++++++++- .../menu/PhabricatorMainMenuSearchView.php | 21 ++++++++++++-- 4 files changed, 75 insertions(+), 5 deletions(-) create mode 100644 src/applications/settings/panel/PhabricatorSearchSettingsPanel.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index fa9a5ce903..e6e9063453 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4655,6 +4655,7 @@ phutil_register_library_map(array( 'PhabricatorSearchScopeSetting' => 'applications/settings/setting/PhabricatorSearchScopeSetting.php', 'PhabricatorSearchSelectField' => 'applications/search/field/PhabricatorSearchSelectField.php', 'PhabricatorSearchService' => 'infrastructure/cluster/search/PhabricatorSearchService.php', + 'PhabricatorSearchSettingsPanel' => 'applications/settings/panel/PhabricatorSearchSettingsPanel.php', 'PhabricatorSearchStringListField' => 'applications/search/field/PhabricatorSearchStringListField.php', 'PhabricatorSearchSubscribersField' => 'applications/search/field/PhabricatorSearchSubscribersField.php', 'PhabricatorSearchTextField' => 'applications/search/field/PhabricatorSearchTextField.php', @@ -11248,9 +11249,10 @@ phutil_register_library_map(array( 'PhabricatorSearchResultBucketGroup' => 'Phobject', 'PhabricatorSearchResultView' => 'AphrontView', 'PhabricatorSearchSchemaSpec' => 'PhabricatorConfigSchemaSpec', - 'PhabricatorSearchScopeSetting' => 'PhabricatorInternalSetting', + 'PhabricatorSearchScopeSetting' => 'PhabricatorSelectSetting', 'PhabricatorSearchSelectField' => 'PhabricatorSearchField', 'PhabricatorSearchService' => 'Phobject', + 'PhabricatorSearchSettingsPanel' => 'PhabricatorEditEngineSettingsPanel', 'PhabricatorSearchStringListField' => 'PhabricatorSearchField', 'PhabricatorSearchSubscribersField' => 'PhabricatorSearchTokenizerField', 'PhabricatorSearchTextField' => 'PhabricatorSearchField', diff --git a/src/applications/settings/panel/PhabricatorSearchSettingsPanel.php b/src/applications/settings/panel/PhabricatorSearchSettingsPanel.php new file mode 100644 index 0000000000..37b0ea919d --- /dev/null +++ b/src/applications/settings/panel/PhabricatorSearchSettingsPanel.php @@ -0,0 +1,28 @@ +getViewer(), + new PhabricatorSettingsApplication()); + + $scope_map = array(); + foreach ($scopes as $scope) { + if (!isset($scope['value'])) { + continue; + } + $scope_map[$scope['value']] = $scope['name']; + } + + return $scope_map; + } + } diff --git a/src/view/page/menu/PhabricatorMainMenuSearchView.php b/src/view/page/menu/PhabricatorMainMenuSearchView.php index 15319a357e..aaa7c5a160 100644 --- a/src/view/page/menu/PhabricatorMainMenuSearchView.php +++ b/src/view/page/menu/PhabricatorMainMenuSearchView.php @@ -116,8 +116,9 @@ final class PhabricatorMainMenuSearchView extends AphrontView { return $form; } - private function buildModeSelector($selector_id, $application_id) { - $viewer = $this->getViewer(); + public static function getGlobalSearchScopeItems( + PhabricatorUser $viewer, + PhabricatorApplication $application) { $items = array(); $items[] = array( @@ -132,7 +133,6 @@ final class PhabricatorMainMenuSearchView extends AphrontView { $application_value = null; $application_icon = self::DEFAULT_APPLICATION_ICON; - $application = $this->getApplication(); if ($application) { $application_value = get_class($application); if ($application->getApplicationSearchDocumentTypes()) { @@ -185,6 +185,14 @@ final class PhabricatorMainMenuSearchView extends AphrontView { 'href' => PhabricatorEnv::getDoclink('Search User Guide'), ); + return $items; + } + + private function buildModeSelector($selector_id, $application_id) { + $viewer = $this->getViewer(); + + $items = self::getGlobalSearchScopeItems($viewer, $this->getApplication()); + $scope_key = PhabricatorSearchScopeSetting::SETTINGKEY; $current_value = $viewer->getUserSetting($scope_key); @@ -196,6 +204,13 @@ final class PhabricatorMainMenuSearchView extends AphrontView { } } + $application = $this->getApplication(); + + $application_value = null; + if ($application) { + $application_value = get_class($application); + } + $selector = id(new PHUIButtonView()) ->setID($selector_id) ->addClass('phabricator-main-menu-search-dropdown') From 318e8ebdac9511d1529bbfa22de08a0c5fd81b80 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Sun, 8 Sep 2019 00:16:19 +0000 Subject: [PATCH 13/44] Allow bin/config to create config file Summary: See D20779, https://discourse.phabricator-community.org/t/3089. `bin/config set` complains about missing config file as if it's un-writable. Test Plan: run `bin/config set` with missing, writable, unwritable conf.json and parent dir. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D20788 --- .../management/PhabricatorConfigManagementSetWorkflow.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php b/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php index 6ad2db4471..d69e903bcc 100644 --- a/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php +++ b/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php @@ -145,7 +145,7 @@ final class PhabricatorConfigManagementSetWorkflow $local_path = $config_source->getReadablePath(); try { - Filesystem::assertWritable($local_path); + $config_source->setKeys(array($key => $value)); } catch (FilesystemException $ex) { throw new PhutilArgumentUsageException( pht( @@ -154,8 +154,6 @@ final class PhabricatorConfigManagementSetWorkflow Filesystem::readablePath($local_path))); } - $config_source->setKeys(array($key => $value)); - $write_message = pht( 'Wrote configuration key "%s" to local storage (in file "%s").', $key, From caccbb69d20bc61d3fc2e328bc7fe2d2789071a1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 8 Sep 2019 09:45:53 -0700 Subject: [PATCH 14/44] When users try to log out with no providers configured, warn them of the consequences Summary: Fixes T13406. On the logout screen, test for no configured providers and warn users they may be getting into more trouble than they expect. Test Plan: - Logged out of a normal install and a fresh (unconfigured) install. {F6847659} Maniphest Tasks: T13406 Differential Revision: https://secure.phabricator.com/D20789 --- .../PhabricatorLogoutController.php | 36 +++++++++++++++++-- .../config/PhabricatorAuthListController.php | 2 +- src/view/AphrontDialogView.php | 14 ++++++++ 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/applications/auth/controller/PhabricatorLogoutController.php b/src/applications/auth/controller/PhabricatorLogoutController.php index dccf6bb45b..d71d080cbd 100644 --- a/src/applications/auth/controller/PhabricatorLogoutController.php +++ b/src/applications/auth/controller/PhabricatorLogoutController.php @@ -68,12 +68,42 @@ final class PhabricatorLogoutController ->setURI('/auth/loggedout/'); } + if ($viewer->getPHID()) { - return $this->newDialog() + $dialog = $this->newDialog() ->setTitle(pht('Log Out?')) - ->appendChild(pht('Are you sure you want to log out?')) - ->addSubmitButton(pht('Log Out')) + ->appendParagraph(pht('Are you sure you want to log out?')) ->addCancelButton('/'); + + $configs = id(new PhabricatorAuthProviderConfigQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->execute(); + if (!$configs) { + $dialog + ->appendRemarkup( + pht( + 'WARNING: You have not configured any authentication providers '. + 'yet, so your account has no login credentials. If you log out '. + 'now, you will not be able to log back in normally.')) + ->appendParagraph( + pht( + 'To enable the login flow, follow setup guidance and configure '. + 'at least one authentication provider, then associate '. + 'credentials with your account. After completing these steps, '. + 'you will be able to log out and log back in normally.')) + ->appendParagraph( + pht( + 'If you log out now, you can still regain access to your '. + 'account later by using the account recovery workflow. The '. + 'login screen will prompt you with recovery instructions.')); + + $button = pht('Log Out Anyway'); + } else { + $button = pht('Log Out'); + } + + $dialog->addSubmitButton($button); + return $dialog; } return id(new AphrontRedirectResponse())->setURI('/'); diff --git a/src/applications/auth/controller/config/PhabricatorAuthListController.php b/src/applications/auth/controller/config/PhabricatorAuthListController.php index 5d1d85cca6..b25c791e27 100644 --- a/src/applications/auth/controller/config/PhabricatorAuthListController.php +++ b/src/applications/auth/controller/config/PhabricatorAuthListController.php @@ -64,7 +64,7 @@ final class PhabricatorAuthListController array( 'href' => $this->getApplicationURI('config/new/'), ), - pht('Add Authentication Provider')))); + pht('Add Provider')))); $crumbs = $this->buildApplicationCrumbs(); $crumbs->addTextCrumb(pht('Login and Registration')); diff --git a/src/view/AphrontDialogView.php b/src/view/AphrontDialogView.php index 09fc8e7a16..b8b00a6b3e 100644 --- a/src/view/AphrontDialogView.php +++ b/src/view/AphrontDialogView.php @@ -160,6 +160,20 @@ final class AphrontDialogView return $this->appendChild($box); } + public function appendRemarkup($remarkup) { + $viewer = $this->getViewer(); + $view = new PHUIRemarkupView($viewer, $remarkup); + + $view_tag = phutil_tag( + 'div', + array( + 'class' => 'aphront-dialog-view-paragraph', + ), + $view); + + return $this->appendChild($view_tag); + } + public function appendParagraph($paragraph) { return $this->appendParagraphTag($paragraph); } From f16365ed0778e2513f28166b9e4a84c78780ccab Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 8 Sep 2019 12:56:48 -0700 Subject: [PATCH 15/44] Weaken the guidance recommending that installs enable "STRICT_ALL_TABLES" Summary: Ref T13404. Enabling "STRICT_ALL_TABLES" is good, but if you don't want to bother it doesn't matter too much. All upstream development has been on "STRICT_ALL_TABLES" for a long time. Test Plan: {F6847839} Maniphest Tasks: T13404 Differential Revision: https://secure.phabricator.com/D20790 --- .../check/PhabricatorMySQLSetupCheck.php | 57 ++++++++++++------- 1 file changed, 37 insertions(+), 20 deletions(-) diff --git a/src/applications/config/check/PhabricatorMySQLSetupCheck.php b/src/applications/config/check/PhabricatorMySQLSetupCheck.php index a4048cbc33..5a0d50e425 100644 --- a/src/applications/config/check/PhabricatorMySQLSetupCheck.php +++ b/src/applications/config/check/PhabricatorMySQLSetupCheck.php @@ -50,30 +50,47 @@ final class PhabricatorMySQLSetupCheck extends PhabricatorSetupCheck { if (!in_array('STRICT_ALL_TABLES', $modes)) { $summary = pht( 'MySQL is not in strict mode (on host "%s"), but using strict mode '. - 'is strongly encouraged.', + 'is recommended.', $host_name); $message = pht( - "On database host \"%s\", the global %s is not set to %s. ". - "It is strongly encouraged that you enable this mode when running ". - "Phabricator.\n\n". - "By default MySQL will silently ignore some types of errors, which ". - "can cause data loss and raise security concerns. Enabling strict ". - "mode makes MySQL raise an explicit error instead, and prevents this ". - "entire class of problems from doing any damage.\n\n". - "You can find more information about this mode (and how to configure ". - "it) in the MySQL manual. Usually, it is sufficient to add this to ". - "your %s file (in the %s section) and then restart %s:\n\n". - "%s\n". - "(Note that if you run other applications against the same database, ". - "they may not work in strict mode. Be careful about enabling it in ". - "these cases.)", + 'On database host "%s", the global "sql_mode" setting does not '. + 'include the "STRICT_ALL_TABLES" mode. Enabling this mode is '. + 'recommended to generally improve how MySQL handles certain errors.'. + "\n\n". + 'Without this mode enabled, MySQL will silently ignore some error '. + 'conditions, including inserts which attempt to store more data in '. + 'a column than actually fits. This behavior is usually undesirable '. + 'and can lead to data corruption (by truncating multibyte characters '. + 'in the middle), data loss (by discarding the data which does not '. + 'fit into the column), or security concerns (for example, by '. + 'truncating keys or credentials).'. + "\n\n". + 'Phabricator is developed and tested in "STRICT_ALL_TABLES" mode so '. + 'you should normally never encounter these situations, but may run '. + 'into them if you interact with the database directly, run '. + 'third-party code, develop extensions, or just encounter a bug in '. + 'the software.'. + "\n\n". + 'Enabling "STRICT_ALL_TABLES" makes MySQL raise an explicit error '. + 'if one of these unusual situations does occur. This is a safer '. + 'behavior and prevents these situations from causing secret, subtle, '. + 'and potentially serious issues later on.'. + "\n\n". + 'You can find more information about this mode (and how to configure '. + 'it) in the MySQL manual. Usually, it is sufficient to add this to '. + 'your "my.cnf" file (in the "[mysqld]" section) and then '. + 'restart "mysqld":'. + "\n\n". + '%s'. + "\n". + 'Note that if you run other applications against the same database, '. + 'they may not work in strict mode.'. + "\n\n". + 'If you can not or do not want to enable "STRICT_ALL_TABLES", you '. + 'can safely ignore this warning. Phabricator will work correctly '. + 'with this mode enabled or disabled.', $host_name, - phutil_tag('tt', array(), 'sql_mode'), - phutil_tag('tt', array(), 'STRICT_ALL_TABLES'), - phutil_tag('tt', array(), 'my.cnf'), - phutil_tag('tt', array(), '[mysqld]'), - phutil_tag('tt', array(), 'mysqld'), phutil_tag('pre', array(), 'sql_mode=STRICT_ALL_TABLES')); $this->newIssue('mysql.mode') From 535c8e6bdc1e9db145c599e6c47a9c2370b5ad67 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 8 Sep 2019 12:59:59 -0700 Subject: [PATCH 16/44] Remove the "ONLY_FULL_GROUP_BY" SQL mode setup warning and change the setup key for "STRICT_ALL_TABLES" Summary: Ref T13404. Except for one known issue in Multimeter, Phabricator appears to function properly in this mode. It is broadly desirable that we run in this mode; it's good on its own, and enabled by default in at least some recent MySQL. Additionally, "ONLY_FULL_GROUP_BY" and "STRICT_ALL_TABLES" shared a setup key, so ignoring one would ignore both. Change the key so that existing ignores on "ONLY_FULL_GROUP_BY" do not mask "STRICT_ALL_TABLES" warnings. Test Plan: Grepped for `ONLY_FULL_GROUP_BY`. Maniphest Tasks: T13404 Differential Revision: https://secure.phabricator.com/D20791 --- .../check/PhabricatorMySQLSetupCheck.php | 45 +------------------ 1 file changed, 1 insertion(+), 44 deletions(-) diff --git a/src/applications/config/check/PhabricatorMySQLSetupCheck.php b/src/applications/config/check/PhabricatorMySQLSetupCheck.php index 5a0d50e425..de9a9e8b54 100644 --- a/src/applications/config/check/PhabricatorMySQLSetupCheck.php +++ b/src/applications/config/check/PhabricatorMySQLSetupCheck.php @@ -93,7 +93,7 @@ final class PhabricatorMySQLSetupCheck extends PhabricatorSetupCheck { $host_name, phutil_tag('pre', array(), 'sql_mode=STRICT_ALL_TABLES')); - $this->newIssue('mysql.mode') + $this->newIssue('sql_mode.strict') ->setName(pht('MySQL %s Mode Not Set', 'STRICT_ALL_TABLES')) ->setSummary($summary) ->setMessage($message) @@ -101,49 +101,6 @@ final class PhabricatorMySQLSetupCheck extends PhabricatorSetupCheck { ->addMySQLConfig('sql_mode'); } - if (in_array('ONLY_FULL_GROUP_BY', $modes)) { - $summary = pht( - 'MySQL is in ONLY_FULL_GROUP_BY mode (on host "%s"), but using this '. - 'mode is strongly discouraged.', - $host_name); - - $message = pht( - "On database host \"%s\", the global %s is set to %s. ". - "It is strongly encouraged that you disable this mode when running ". - "Phabricator.\n\n". - "With %s enabled, MySQL rejects queries for which the select list ". - "or (as of MySQL 5.0.23) %s list refer to nonaggregated columns ". - "that are not named in the %s clause. More importantly, Phabricator ". - "does not work properly with this mode enabled.\n\n". - "You can find more information about this mode (and how to configure ". - "it) in the MySQL manual. Usually, it is sufficient to change the %s ". - "in your %s file (in the %s section) and then restart %s:\n\n". - "%s\n". - "(Note that if you run other applications against the same database, ". - "they may not work with %s. Be careful about enabling ". - "it in these cases and consider migrating Phabricator to a different ". - "database.)", - $host_name, - phutil_tag('tt', array(), 'sql_mode'), - phutil_tag('tt', array(), 'ONLY_FULL_GROUP_BY'), - phutil_tag('tt', array(), 'ONLY_FULL_GROUP_BY'), - phutil_tag('tt', array(), 'HAVING'), - phutil_tag('tt', array(), 'GROUP BY'), - phutil_tag('tt', array(), 'sql_mode'), - phutil_tag('tt', array(), 'my.cnf'), - phutil_tag('tt', array(), '[mysqld]'), - phutil_tag('tt', array(), 'mysqld'), - phutil_tag('pre', array(), 'sql_mode=STRICT_ALL_TABLES'), - phutil_tag('tt', array(), 'ONLY_FULL_GROUP_BY')); - - $this->newIssue('mysql.mode') - ->setName(pht('MySQL %s Mode Set', 'ONLY_FULL_GROUP_BY')) - ->setSummary($summary) - ->setMessage($message) - ->setDatabaseRef($ref) - ->addMySQLConfig('sql_mode'); - } - $is_innodb_fulltext = false; $is_myisam_fulltext = false; if ($this->shouldUseMySQLSearchEngine()) { From 63c7302af1309eb7f3874166042953491a092e30 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Sep 2019 12:10:21 -0700 Subject: [PATCH 17/44] Fix global search scope fatal on 404 page (or other pages with no Application) Summary: Ref T13405. Some pages don't have a contextual application. Test Plan: Viewed 404 page, no more fatal. Maniphest Tasks: T13405 Differential Revision: https://secure.phabricator.com/D20792 --- src/view/page/menu/PhabricatorMainMenuSearchView.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/view/page/menu/PhabricatorMainMenuSearchView.php b/src/view/page/menu/PhabricatorMainMenuSearchView.php index aaa7c5a160..60e7233e96 100644 --- a/src/view/page/menu/PhabricatorMainMenuSearchView.php +++ b/src/view/page/menu/PhabricatorMainMenuSearchView.php @@ -118,7 +118,7 @@ final class PhabricatorMainMenuSearchView extends AphrontView { public static function getGlobalSearchScopeItems( PhabricatorUser $viewer, - PhabricatorApplication $application) { + PhabricatorApplication $application = null) { $items = array(); $items[] = array( From 278092974f2f4115cc97172e099ec7fe3941eeb1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Sep 2019 12:15:00 -0700 Subject: [PATCH 18/44] Don't offer personal saved queries in global "Search Scope" settings dropdown Summary: Fixes T13405. We currently offer non-global custom saved queries here, but this doesn't make sense as a global default setting. Test Plan: Saved a global search query, edited global search settings, no longer saw the non-global query as an option. Maniphest Tasks: T13405 Differential Revision: https://secure.phabricator.com/D20793 --- .../setting/PhabricatorSearchScopeSetting.php | 3 ++- .../page/menu/PhabricatorMainMenuSearchView.php | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/applications/settings/setting/PhabricatorSearchScopeSetting.php b/src/applications/settings/setting/PhabricatorSearchScopeSetting.php index 54fa12a84c..c4b1941540 100644 --- a/src/applications/settings/setting/PhabricatorSearchScopeSetting.php +++ b/src/applications/settings/setting/PhabricatorSearchScopeSetting.php @@ -25,7 +25,8 @@ final class PhabricatorSearchScopeSetting protected function getSelectOptions() { $scopes = PhabricatorMainMenuSearchView::getGlobalSearchScopeItems( $this->getViewer(), - new PhabricatorSettingsApplication()); + new PhabricatorSettingsApplication(), + $only_global = true); $scope_map = array(); foreach ($scopes as $scope) { diff --git a/src/view/page/menu/PhabricatorMainMenuSearchView.php b/src/view/page/menu/PhabricatorMainMenuSearchView.php index 60e7233e96..e038eec6c8 100644 --- a/src/view/page/menu/PhabricatorMainMenuSearchView.php +++ b/src/view/page/menu/PhabricatorMainMenuSearchView.php @@ -118,7 +118,8 @@ final class PhabricatorMainMenuSearchView extends AphrontView { public static function getGlobalSearchScopeItems( PhabricatorUser $viewer, - PhabricatorApplication $application = null) { + PhabricatorApplication $application = null, + $global_only = false) { $items = array(); $items[] = array( @@ -154,14 +155,24 @@ final class PhabricatorMainMenuSearchView extends AphrontView { $engine = id(new PhabricatorSearchApplicationSearchEngine()) ->setViewer($viewer); $engine_queries = $engine->loadEnabledNamedQueries(); - $query_map = mpull($engine_queries, 'getQueryName', 'getQueryKey'); - foreach ($query_map as $query_key => $query_name) { + foreach ($engine_queries as $query) { + $query_key = $query->getQueryKey(); if ($query_key == 'all') { // Skip the builtin "All" query since it's redundant with the default // setting. continue; } + // In the global "Settings" panel, we don't want to offer personal + // queries the viewer may have saved. + if ($global_only) { + if (!$query->isGlobal()) { + continue; + } + } + + $query_name = $query->getQueryName(); + $items[] = array( 'icon' => 'fa-certificate', 'name' => $query_name, From aaaea5759133c1690733e4cecbe0ef9351b47e4e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Sep 2019 12:28:17 -0700 Subject: [PATCH 19/44] Fix fatal during redirection safety check for searching for Phabricator base-uri with no trailing slash Summary: Fixes T13412. If you search for "https://phabricator.example.com" with no trailing slash, we currently redirect you to "", which is fouled by a safety check in the redirection flow. Test Plan: - Searched for "https://local.phacility.com"; before: fatal in redirection; after: clean redirect. - Searched for other self-URIs, got normal redirects. Maniphest Tasks: T13412 Differential Revision: https://secure.phabricator.com/D20794 --- .../PhabricatorDatasourceURIEngineExtension.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/applications/meta/engineextension/PhabricatorDatasourceURIEngineExtension.php b/src/applications/meta/engineextension/PhabricatorDatasourceURIEngineExtension.php index 147aa9635f..16d8dd9315 100644 --- a/src/applications/meta/engineextension/PhabricatorDatasourceURIEngineExtension.php +++ b/src/applications/meta/engineextension/PhabricatorDatasourceURIEngineExtension.php @@ -26,7 +26,17 @@ final class PhabricatorDatasourceURIEngineExtension ->setProtocol(null) ->setPort(null); - return phutil_string_cast($uri); + $uri = phutil_string_cast($uri); + + // See T13412. If the URI was in the form "http://dev.example.com" with + // no trailing slash, there may be no path. Redirecting to the empty + // string is considered an error by safety checks during redirection, + // so treat this like the user entered the URI with a trailing slash. + if (!strlen($uri)) { + $uri = '/'; + } + + return $uri; } return null; From d965d9a669b58ed8968cda51d1467d3aeb570fc9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Sep 2019 12:38:36 -0700 Subject: [PATCH 20/44] Index Herald fields, not just actions, when identifying objects related to a particular Herald rule Summary: Fixes T13408. Currently, when a package (or other object) appears in a field (rather than an action), it is not indexed. Instead: index fields too, not just actions. Test Plan: - Wrote a rule like "[ Affected packages include ] ...". - Updated the search index. - Saw rule appear on "Affected By Herald Rules" on the package detail page. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13408 Differential Revision: https://secure.phabricator.com/D20795 --- .../20190909.herald.01.rebuild.php | 3 +++ .../HeraldRuleIndexEngineExtension.php | 14 ++++++++++++ src/applications/herald/field/HeraldField.php | 22 +++++++++++++++++++ 3 files changed, 39 insertions(+) create mode 100644 resources/sql/autopatches/20190909.herald.01.rebuild.php diff --git a/resources/sql/autopatches/20190909.herald.01.rebuild.php b/resources/sql/autopatches/20190909.herald.01.rebuild.php new file mode 100644 index 0000000000..a29b7d2f45 --- /dev/null +++ b/resources/sql/autopatches/20190909.herald.01.rebuild.php @@ -0,0 +1,3 @@ +getConditions() as $condition_record) { + $field = idx($fields, $condition_record->getFieldName()); + + if (!$field) { + continue; + } + + $affected_phids = $field->getPHIDsAffectedByCondition($condition_record); + foreach ($affected_phids as $phid) { + $phids[] = $phid; + } + } + $actions = HeraldAction::getAllActions(); foreach ($rule->getActions() as $action_record) { $action = idx($actions, $action_record->getAction()); diff --git a/src/applications/herald/field/HeraldField.php b/src/applications/herald/field/HeraldField.php index 611ea2cdb3..3df242712e 100644 --- a/src/applications/herald/field/HeraldField.php +++ b/src/applications/herald/field/HeraldField.php @@ -176,6 +176,28 @@ abstract class HeraldField extends Phobject { return $value_type->renderEditorValue($value); } + public function getPHIDsAffectedByCondition(HeraldCondition $condition) { + $phids = array(); + + $standard_type = $this->getHeraldFieldStandardType(); + switch ($standard_type) { + case self::STANDARD_PHID: + case self::STANDARD_PHID_NULLABLE: + $phid = $condition->getValue(); + if ($phid) { + $phids[] = $phid; + } + break; + case self::STANDARD_PHID_LIST: + foreach ($condition->getValue() as $phid) { + $phids[] = $phid; + } + break; + } + + return $phids; + } + final public function setAdapter(HeraldAdapter $adapter) { $this->adapter = $adapter; return $this; From 454771446306f213a3ee68f3d89c8ad1e5e6bbf6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Sep 2019 13:00:38 -0700 Subject: [PATCH 21/44] Add a "Remove flag" action to Herald Summary: Fixes T13409. This is a companion to the existing "Mark with flag" rule. Test Plan: Used a "remove flag" rule on an object with no flag (not removed), the right type of flag (removed), and a different type of flag (not removed). Maniphest Tasks: T13409 Differential Revision: https://secure.phabricator.com/D20796 --- src/__phutil_library_map__.php | 6 +- .../PhabricatorFlagAddFlagHeraldAction.php | 15 +--- .../herald/PhabricatorFlagHeraldAction.php | 18 +++++ .../PhabricatorFlagRemoveFlagHeraldAction.php | 79 +++++++++++++++++++ 4 files changed, 104 insertions(+), 14 deletions(-) create mode 100644 src/applications/flag/herald/PhabricatorFlagHeraldAction.php create mode 100644 src/applications/flag/herald/PhabricatorFlagRemoveFlagHeraldAction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e6e9063453..fad919f4dd 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3438,8 +3438,10 @@ phutil_register_library_map(array( 'PhabricatorFlagDeleteController' => 'applications/flag/controller/PhabricatorFlagDeleteController.php', 'PhabricatorFlagDestructionEngineExtension' => 'applications/flag/engineextension/PhabricatorFlagDestructionEngineExtension.php', 'PhabricatorFlagEditController' => 'applications/flag/controller/PhabricatorFlagEditController.php', + 'PhabricatorFlagHeraldAction' => 'applications/flag/herald/PhabricatorFlagHeraldAction.php', 'PhabricatorFlagListController' => 'applications/flag/controller/PhabricatorFlagListController.php', 'PhabricatorFlagQuery' => 'applications/flag/query/PhabricatorFlagQuery.php', + 'PhabricatorFlagRemoveFlagHeraldAction' => 'applications/flag/herald/PhabricatorFlagRemoveFlagHeraldAction.php', 'PhabricatorFlagSearchEngine' => 'applications/flag/query/PhabricatorFlagSearchEngine.php', 'PhabricatorFlagSelectControl' => 'applications/flag/view/PhabricatorFlagSelectControl.php', 'PhabricatorFlaggableInterface' => 'applications/flag/interface/PhabricatorFlaggableInterface.php', @@ -9799,7 +9801,7 @@ phutil_register_library_map(array( 'PhabricatorFlagDAO', 'PhabricatorPolicyInterface', ), - 'PhabricatorFlagAddFlagHeraldAction' => 'HeraldAction', + 'PhabricatorFlagAddFlagHeraldAction' => 'PhabricatorFlagHeraldAction', 'PhabricatorFlagColor' => 'PhabricatorFlagConstants', 'PhabricatorFlagConstants' => 'Phobject', 'PhabricatorFlagController' => 'PhabricatorController', @@ -9807,8 +9809,10 @@ phutil_register_library_map(array( 'PhabricatorFlagDeleteController' => 'PhabricatorFlagController', 'PhabricatorFlagDestructionEngineExtension' => 'PhabricatorDestructionEngineExtension', 'PhabricatorFlagEditController' => 'PhabricatorFlagController', + 'PhabricatorFlagHeraldAction' => 'HeraldAction', 'PhabricatorFlagListController' => 'PhabricatorFlagController', 'PhabricatorFlagQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorFlagRemoveFlagHeraldAction' => 'PhabricatorFlagHeraldAction', 'PhabricatorFlagSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorFlagSelectControl' => 'AphrontFormControl', 'PhabricatorFlaggableInterface' => 'PhabricatorPHIDInterface', diff --git a/src/applications/flag/herald/PhabricatorFlagAddFlagHeraldAction.php b/src/applications/flag/herald/PhabricatorFlagAddFlagHeraldAction.php index e4ae03915f..57772ecb5f 100644 --- a/src/applications/flag/herald/PhabricatorFlagAddFlagHeraldAction.php +++ b/src/applications/flag/herald/PhabricatorFlagAddFlagHeraldAction.php @@ -1,6 +1,7 @@ getAdapter()->getPHID(); $rule = $effect->getRule(); diff --git a/src/applications/flag/herald/PhabricatorFlagHeraldAction.php b/src/applications/flag/herald/PhabricatorFlagHeraldAction.php new file mode 100644 index 0000000000..6f87a4ee6c --- /dev/null +++ b/src/applications/flag/herald/PhabricatorFlagHeraldAction.php @@ -0,0 +1,18 @@ +getAdapter()->getPHID(); + $rule = $effect->getRule(); + $author = $rule->getAuthor(); + + $flag = PhabricatorFlagQuery::loadUserFlag($author, $phid); + if (!$flag) { + $this->logEffect(self::DO_IGNORE_UNFLAG, null); + return; + } + + if ($flag->getColor() !== $effect->getTarget()) { + $this->logEffect(self::DO_IGNORE_UNFLAG, $flag->getColor()); + return; + } + + $flag->delete(); + + $this->logEffect(self::DO_UNFLAG, $flag->getColor()); + } + + public function getHeraldActionValueType() { + return id(new HeraldSelectFieldValue()) + ->setKey('flag.color') + ->setOptions(PhabricatorFlagColor::getColorNameMap()) + ->setDefault(PhabricatorFlagColor::COLOR_BLUE); + } + + protected function getActionEffectMap() { + return array( + self::DO_IGNORE_UNFLAG => array( + 'icon' => 'fa-times', + 'color' => 'grey', + 'name' => pht('Did Not Remove Flag'), + ), + self::DO_UNFLAG => array( + 'icon' => 'fa-flag', + 'name' => pht('Removed Flag'), + ), + ); + } + + public function renderActionDescription($value) { + $color = PhabricatorFlagColor::getColorName($value); + return pht('Remove %s flag.', $color); + } + + protected function renderActionEffectDescription($type, $data) { + switch ($type) { + case self::DO_IGNORE_UNFLAG: + if (!$data) { + return pht('Not marked with any flag.'); + } else { + return pht( + 'Marked with flag of the wrong color ("%s").', + PhabricatorFlagColor::getColorName($data)); + } + case self::DO_UNFLAG: + return pht( + 'Removed "%s" flag.', + PhabricatorFlagColor::getColorName($data)); + } + } + +} From 7593a265d5931877d38756282ddd66daf6ec99b8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Sep 2019 13:09:46 -0700 Subject: [PATCH 22/44] When Herald changes object subscribers, always hide the feed story Summary: Fixes T8952. These feed stories are not interesting and tend to be generated as collateral damage when a non-story update is made to an old task and someone has a "subscribe me" Herald rule. Also clean up some of the Herald field/condition indexing behavior slightly. Test Plan: Wrote a "Subscribe X" herald rule, made a trivial update to a task. Before: low-value feed story; after: no feed story. Maniphest Tasks: T8952 Differential Revision: https://secure.phabricator.com/D20797 --- src/applications/herald/field/HeraldField.php | 23 ++++++++++--------- .../PhabricatorApplicationTransaction.php | 13 ++++++----- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/src/applications/herald/field/HeraldField.php b/src/applications/herald/field/HeraldField.php index 3df242712e..6f14091aff 100644 --- a/src/applications/herald/field/HeraldField.php +++ b/src/applications/herald/field/HeraldField.php @@ -177,25 +177,26 @@ abstract class HeraldField extends Phobject { } public function getPHIDsAffectedByCondition(HeraldCondition $condition) { - $phids = array(); + try { + $standard_type = $this->getHeraldFieldStandardType(); + } catch (PhutilMethodNotImplementedException $ex) { + $standard_type = null; + } - $standard_type = $this->getHeraldFieldStandardType(); switch ($standard_type) { case self::STANDARD_PHID: case self::STANDARD_PHID_NULLABLE: - $phid = $condition->getValue(); - if ($phid) { - $phids[] = $phid; - } - break; case self::STANDARD_PHID_LIST: - foreach ($condition->getValue() as $phid) { - $phids[] = $phid; + $phids = $condition->getValue(); + + if (!is_array($phids)) { + $phids = array(); } - break; + + return $phids; } - return $phids; + return array(); } final public function setAdapter(HeraldAdapter $adapter) { diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 7c327393f8..5fa4562164 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -775,6 +775,13 @@ abstract class PhabricatorApplicationTransaction case PhabricatorTransactions::TYPE_TOKEN: case PhabricatorTransactions::TYPE_MFA: return true; + case PhabricatorTransactions::TYPE_SUBSCRIBERS: + // See T8952. When an application (usually Herald) modifies + // subscribers, this tends to be very uninteresting. + if ($this->isApplicationAuthor()) { + return true; + } + break; case PhabricatorTransactions::TYPE_EDGE: $edge_type = $this->getMetadataValue('edge:type'); switch ($edge_type) { @@ -1387,12 +1394,6 @@ abstract class PhabricatorApplicationTransaction return 25; } - if ($this->isApplicationAuthor()) { - // When applications (most often: Herald) change subscriptions it - // is very uninteresting. - return 1; - } - // In other cases, subscriptions are more interesting than comments // (which are shown anyway) but less interesting than any other type of // transaction. From 1d1a60fdda88b6b2ebea09e26fa788b89a9a0c18 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Sep 2019 13:24:27 -0700 Subject: [PATCH 23/44] Improve rendering of Herald rules in "Another Herald rule..." field Summary: Fixes T9136. - Fix a bug where the name is rendered improperly. - Put disabled rules at the bottom. - Always show the rule monogram so you can distingiush between rules with the same name. Test Plan: {F6849915} Maniphest Tasks: T9136 Differential Revision: https://secure.phabricator.com/D20798 --- .../herald/controller/HeraldRuleController.php | 13 ++----------- src/applications/herald/storage/HeraldRule.php | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index d05ed2d525..73c1e8c39c 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/src/applications/herald/controller/HeraldRuleController.php @@ -404,8 +404,8 @@ final class HeraldRuleController extends HeraldController { HeraldAdapter $adapter) { $all_rules = $this->loadRulesThisRuleMayDependUpon($rule); - $all_rules = mpull($all_rules, 'getName', 'getPHID'); - asort($all_rules); + $all_rules = msortv($all_rules, 'getEditorSortVector'); + $all_rules = mpull($all_rules, 'getEditorDisplayName', 'getPHID'); $all_fields = $adapter->getFieldNameMap(); $all_conditions = $adapter->getConditionNameMap(); @@ -674,15 +674,6 @@ final class HeraldRuleController extends HeraldController { ->execute(); } - // mark disabled rules as disabled since they are not useful as such; - // don't filter though to keep edit cases sane / expected - foreach ($all_rules as $current_rule) { - if ($current_rule->getIsDisabled()) { - $current_rule->makeEphemeral(); - $current_rule->setName($rule->getName().' '.pht('(Disabled)')); - } - } - // A rule can not depend upon itself. unset($all_rules[$rule->getID()]); diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index a9c131e717..07be991bda 100644 --- a/src/applications/herald/storage/HeraldRule.php +++ b/src/applications/herald/storage/HeraldRule.php @@ -259,6 +259,22 @@ final class HeraldRule extends HeraldDAO return '/'.$this->getMonogram(); } + public function getEditorSortVector() { + return id(new PhutilSortVector()) + ->addInt($this->getIsDisabled() ? 1 : 0) + ->addString($this->getName()); + } + + public function getEditorDisplayName() { + $name = pht('%s %s', $this->getMonogram(), $this->getName()); + + if ($this->getIsDisabled()) { + $name = pht('%s (Disabled)', $name); + } + + return $name; + } + /* -( Repetition Policies )------------------------------------------------ */ From d2e1c4163a3b4695d67d91b292c3e1ac7ec5e4d3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Sep 2019 13:33:53 -0700 Subject: [PATCH 24/44] When a project has a custom icon, use that icon to label the project policy in the policy dropown Summary: Fixes T8808. Currently, all project use the default ("Briefcase") project icon when they appear in a policy dropdown. Since project policies are separated out into a "Members of Projects" section of the dropdown anyway, there is no reason not to use the actual project icon, which is often more clear. Test Plan: {F6849927} Maniphest Tasks: T8808 Differential Revision: https://secure.phabricator.com/D20799 --- src/applications/policy/storage/PhabricatorPolicy.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/applications/policy/storage/PhabricatorPolicy.php b/src/applications/policy/storage/PhabricatorPolicy.php index 2df8fdf6a0..3d6b4ca609 100644 --- a/src/applications/policy/storage/PhabricatorPolicy.php +++ b/src/applications/policy/storage/PhabricatorPolicy.php @@ -85,8 +85,10 @@ final class PhabricatorPolicy $phid_type = phid_get_type($policy_identifier); switch ($phid_type) { case PhabricatorProjectProjectPHIDType::TYPECONST: - $policy->setType(PhabricatorPolicyType::TYPE_PROJECT); - $policy->setName($handle->getName()); + $policy + ->setType(PhabricatorPolicyType::TYPE_PROJECT) + ->setName($handle->getName()) + ->setIcon($handle->getIcon()); break; case PhabricatorPeopleUserPHIDType::TYPECONST: $policy->setType(PhabricatorPolicyType::TYPE_USER); From a0ade503e1663d2cb60ce433833843952305ef61 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 11 Sep 2019 21:01:56 -0700 Subject: [PATCH 25/44] Remove "Moved Document from ..." notice in Phriction Summary: Ref T13410. See PHI1431. Currently, when you move a document in Phriction, the target shows a "This document was moved from ..." banner until it is edited. This banner isn't particularly useful, and it's distracting and it isn't obvious how to dismiss it, and making a trivial edit to dismiss it is awkward. This information is also already available in the transaction log. Just remove this banner since it doesn't really serve any clear purpose. Test Plan: - Moved a page in Phriction, then loaded the destination page. Before change: header banner. After change: nothing. - Viewed a normal (non-moved) page, saw normal behavior. - Reviewed transactions, saw "Moved from ..." in the timeline. Maniphest Tasks: T13410 Differential Revision: https://secure.phabricator.com/D20800 --- .../PhrictionDocumentController.php | 31 ------------------- .../PhrictionDocumentMoveToTransaction.php | 2 +- 2 files changed, 1 insertion(+), 32 deletions(-) diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php index bf7282b35a..5d30b5a90b 100644 --- a/src/applications/phriction/controller/PhrictionDocumentController.php +++ b/src/applications/phriction/controller/PhrictionDocumentController.php @@ -293,37 +293,6 @@ final class PhrictionDocumentController } else { throw new Exception(pht("Unknown document status '%s'!", $doc_status)); } - - $move_notice = null; - if ($current_status == PhrictionChangeType::CHANGE_MOVE_HERE) { - $from_doc_id = $content->getChangeRef(); - - $slug_uri = null; - - // If the old document exists and is visible, provide a link to it. - $from_docs = id(new PhrictionDocumentQuery()) - ->setViewer($viewer) - ->withIDs(array($from_doc_id)) - ->execute(); - if ($from_docs) { - $from_doc = head($from_docs); - $slug_uri = PhrictionDocument::getSlugURI($from_doc->getSlug()); - } - - $move_notice = id(new PHUIInfoView()) - ->setSeverity(PHUIInfoView::SEVERITY_NOTICE); - - if ($slug_uri) { - $move_notice->appendChild( - pht( - 'This document was moved from %s.', - phutil_tag('a', array('href' => $slug_uri), $slug_uri))); - } else { - // Render this for consistency, even though it's a bit silly. - $move_notice->appendChild( - pht('This document was moved from elsewhere.')); - } - } } $children = $this->renderDocumentChildren($slug); diff --git a/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php index d18c436cd0..078e9a0130 100644 --- a/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php +++ b/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php @@ -49,7 +49,7 @@ final class PhrictionDocumentMoveToTransaction $new = $this->getNewValue(); return pht( - '%s moved this document from %s', + '%s moved this document from %s.', $this->renderAuthor(), $this->renderHandle($new['phid'])); } From a35d7c3c218a4f053ad35849a972eee513161dc0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 11 Sep 2019 21:50:25 -0700 Subject: [PATCH 26/44] Update rendering of policy edit transactions in Applications Summary: Ref T13411. Since circa D19829, transactions have rendered policy changes in a modern way, notably making "Custom Policy" clickable to show the policy rules. Edit transactions in Applications still use a separate, older approach to render policies. This produces policy renderings which don't use modern quoting rules and don't link in a modern way. Make Applications use the same rendering code that other transactions (like normal edit/view edits) use. Test Plan: Edited policies in Applications, saw more useful transactions in the log. Clicked "Custom Policy" in the transaction log and got a useful explanation of the policy. Maniphest Tasks: T13411 Differential Revision: https://secure.phabricator.com/D20801 --- ...atorApplicationPolicyChangeTransaction.php | 46 ++++--------------- ...rApplicationTransactionValueController.php | 1 + 2 files changed, 9 insertions(+), 38 deletions(-) diff --git a/src/applications/meta/xactions/PhabricatorApplicationPolicyChangeTransaction.php b/src/applications/meta/xactions/PhabricatorApplicationPolicyChangeTransaction.php index 5364a3d4fa..3c58b42ab5 100644 --- a/src/applications/meta/xactions/PhabricatorApplicationPolicyChangeTransaction.php +++ b/src/applications/meta/xactions/PhabricatorApplicationPolicyChangeTransaction.php @@ -52,15 +52,12 @@ final class PhabricatorApplicationPolicyChangeTransaction } public function getTitle() { - $old = $this->renderApplicationPolicy($this->getOldValue()); - $new = $this->renderApplicationPolicy($this->getNewValue()); - return pht( - '%s changed the "%s" policy from "%s" to "%s".', + '%s changed the %s policy from %s to %s.', $this->renderAuthor(), $this->renderCapability(), - $old, - $new); + $this->renderOldPolicy(), + $this->renderNewPolicy()); } public function getTitleForFeed() { @@ -68,12 +65,12 @@ final class PhabricatorApplicationPolicyChangeTransaction $new = $this->renderApplicationPolicy($this->getNewValue()); return pht( - '%s changed the "%s" policy for application %s from "%s" to "%s".', + '%s changed the %s policy for application %s from %s to %s.', $this->renderAuthor(), $this->renderCapability(), $this->renderObject(), - $old, - $new); + $this->renderOldPolicy(), + $this->renderNewPolicy()); } public function validateTransactions($object, array $xactions) { @@ -165,38 +162,11 @@ final class PhabricatorApplicationPolicyChangeTransaction return $errors; } - private function renderApplicationPolicy($name) { - $policies = $this->getAllPolicies(); - if (empty($policies[$name])) { - // Not a standard policy, check for a custom policy. - $policy = id(new PhabricatorPolicyQuery()) - ->setViewer($this->getViewer()) - ->withPHIDs(array($name)) - ->executeOne(); - $policies[$name] = $policy; - } - - $policy = idx($policies, $name); - return $this->renderValue($policy->getFullName()); - } - - private function getAllPolicies() { - if (!$this->policies) { - $viewer = $this->getViewer(); - $application = $this->getObject(); - $this->policies = id(new PhabricatorPolicyQuery()) - ->setViewer($viewer) - ->setObject($application) - ->execute(); - } - - return $this->policies; - } - private function renderCapability() { $application = $this->getObject(); $capability = $this->getCapabilityName(); - return $application->getCapabilityLabel($capability); + $label = $application->getCapabilityLabel($capability); + return $this->renderValue($label); } private function getCapabilityName() { diff --git a/src/applications/transactions/controller/PhabricatorApplicationTransactionValueController.php b/src/applications/transactions/controller/PhabricatorApplicationTransactionValueController.php index bef6fef5a8..9c82ff99c8 100644 --- a/src/applications/transactions/controller/PhabricatorApplicationTransactionValueController.php +++ b/src/applications/transactions/controller/PhabricatorApplicationTransactionValueController.php @@ -33,6 +33,7 @@ final class PhabricatorApplicationTransactionValueController case PhabricatorTransactions::TYPE_EDIT_POLICY: case PhabricatorTransactions::TYPE_JOIN_POLICY: case PhabricatorRepositoryPushPolicyTransaction::TRANSACTIONTYPE: + case PhabricatorApplicationPolicyChangeTransaction::TRANSACTIONTYPE: break; default: return new Aphront404Response(); From 9c6969e810227aed3b75de293b43a4002e22b495 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 11 Sep 2019 21:51:27 -0700 Subject: [PATCH 27/44] Remove "Editable By" description fields in Passphrase, Phame, and Spaces Summary: Ref T13411. These three applications render an "Editable By: " field in their descriptions. The pages that these appear on all have "Edit " actions which either tell you the policy or allow you to discover the policy, and this field is unusual (the vast majority of objects don't have it). I think it largely got copy/pasted or used as space-filler and doesn't offer much of value. Remove it to simplify/standardize these pages and make changes to how this field works simpler to implement. Test Plan: Viewed a Credential, Blog, and Space; no longer saw the "Editable By" field. Maniphest Tasks: T13411 Differential Revision: https://secure.phabricator.com/D20802 --- .../controller/PassphraseCredentialViewController.php | 8 -------- .../phame/controller/blog/PhameBlogManageController.php | 8 -------- .../spaces/controller/PhabricatorSpacesViewController.php | 8 -------- 3 files changed, 24 deletions(-) diff --git a/src/applications/passphrase/controller/PassphraseCredentialViewController.php b/src/applications/passphrase/controller/PassphraseCredentialViewController.php index aabb3821e0..6688bef285 100644 --- a/src/applications/passphrase/controller/PassphraseCredentialViewController.php +++ b/src/applications/passphrase/controller/PassphraseCredentialViewController.php @@ -190,14 +190,6 @@ final class PassphraseCredentialViewController extends PassphraseController { pht('Credential Type'), $type->getCredentialTypeName()); - $descriptions = PhabricatorPolicyQuery::renderPolicyDescriptions( - $viewer, - $credential); - - $properties->addProperty( - pht('Editable By'), - $descriptions[PhabricatorPolicyCapability::CAN_EDIT]); - if ($type->shouldRequireUsername()) { $properties->addProperty( pht('Username'), diff --git a/src/applications/phame/controller/blog/PhameBlogManageController.php b/src/applications/phame/controller/blog/PhameBlogManageController.php index 2bdcbf7635..65378d91cb 100644 --- a/src/applications/phame/controller/blog/PhameBlogManageController.php +++ b/src/applications/phame/controller/blog/PhameBlogManageController.php @@ -143,14 +143,6 @@ final class PhameBlogManageController extends PhameBlogController { ), $feed_uri)); - $descriptions = PhabricatorPolicyQuery::renderPolicyDescriptions( - $viewer, - $blog); - - $properties->addProperty( - pht('Editable By'), - $descriptions[PhabricatorPolicyCapability::CAN_EDIT]); - $engine = id(new PhabricatorMarkupEngine()) ->setViewer($viewer) ->addObject($blog, PhameBlog::MARKUP_FIELD_DESCRIPTION) diff --git a/src/applications/spaces/controller/PhabricatorSpacesViewController.php b/src/applications/spaces/controller/PhabricatorSpacesViewController.php index 5fa1c01143..ba55ba90a7 100644 --- a/src/applications/spaces/controller/PhabricatorSpacesViewController.php +++ b/src/applications/spaces/controller/PhabricatorSpacesViewController.php @@ -80,14 +80,6 @@ final class PhabricatorSpacesViewController ? pht('Yes') : pht('No')); - $descriptions = PhabricatorPolicyQuery::renderPolicyDescriptions( - $viewer, - $space); - - $list->addProperty( - pht('Editable By'), - $descriptions[PhabricatorPolicyCapability::CAN_EDIT]); - $description = $space->getDescription(); if (strlen($description)) { $description = new PHUIRemarkupView($viewer, $description); From c9b0d107f079583e72b273e4df572b0f2dde1447 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 11 Sep 2019 22:01:20 -0700 Subject: [PATCH 28/44] Remove unused "icon" parameter from policy name rendering Summary: Ref T13411. This pathway has an unused "icon" parameter with no callsites. Throw it away to ease refactoring. Test Plan: Grepped for callsites, found none using this parameter. Maniphest Tasks: T13411 Differential Revision: https://secure.phabricator.com/D20803 --- .../PhabricatorPolicyExplainController.php | 4 ++-- .../policy/query/PhabricatorPolicyQuery.php | 5 ++--- .../policy/storage/PhabricatorPolicy.php | 19 +++---------------- 3 files changed, 7 insertions(+), 21 deletions(-) diff --git a/src/applications/policy/controller/PhabricatorPolicyExplainController.php b/src/applications/policy/controller/PhabricatorPolicyExplainController.php index a4ba355b16..8b41ef7357 100644 --- a/src/applications/policy/controller/PhabricatorPolicyExplainController.php +++ b/src/applications/policy/controller/PhabricatorPolicyExplainController.php @@ -333,8 +333,8 @@ final class PhabricatorPolicyExplainController ->appendList( array( PhabricatorPolicy::getPolicyExplanation( - $viewer, - $policy->getPHID()), + $viewer, + $policy->getPHID()), )); $strength = $this->getStrengthInformation($object, $policy, $capability); diff --git a/src/applications/policy/query/PhabricatorPolicyQuery.php b/src/applications/policy/query/PhabricatorPolicyQuery.php index e51b2ca401..3337a912b0 100644 --- a/src/applications/policy/query/PhabricatorPolicyQuery.php +++ b/src/applications/policy/query/PhabricatorPolicyQuery.php @@ -43,13 +43,12 @@ final class PhabricatorPolicyQuery public static function renderPolicyDescriptions( PhabricatorUser $viewer, - PhabricatorPolicyInterface $object, - $icon = false) { + PhabricatorPolicyInterface $object) { $policies = self::loadPolicies($viewer, $object); foreach ($policies as $capability => $policy) { - $policies[$capability] = $policy->renderDescription($icon); + $policies[$capability] = $policy->renderDescription(); } return $policies; diff --git a/src/applications/policy/storage/PhabricatorPolicy.php b/src/applications/policy/storage/PhabricatorPolicy.php index 3d6b4ca609..834ca7a798 100644 --- a/src/applications/policy/storage/PhabricatorPolicy.php +++ b/src/applications/policy/storage/PhabricatorPolicy.php @@ -276,13 +276,7 @@ final class PhabricatorPolicy } } - public function renderDescription($icon = false) { - $img = null; - if ($icon) { - $img = id(new PHUIIconView()) - ->setIcon($this->getIcon()); - } - + public function renderDescription() { if ($this->getHref()) { $desc = javelin_tag( 'a', @@ -291,16 +285,9 @@ final class PhabricatorPolicy 'class' => 'policy-link', 'sigil' => $this->getWorkflow() ? 'workflow' : null, ), - array( - $img, - $this->getName(), - )); + $this->getName()); } else { - if ($img) { - $desc = array($img, $this->getName()); - } else { - $desc = $this->getName(); - } + $desc = $this->getName(); } switch ($this->getType()) { From 506f93b4a39007b50b2daa5ded3223b19c85f490 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Sep 2019 07:23:43 -0700 Subject: [PATCH 29/44] Give policy name rendering explicit "text name", "capability link", and "transaction link" pathways Summary: Ref T13411. This cleans up policy name rendering. We ultimately render into three contexts: - Plain text contexts, like `bin/policy show`. - Transaction contexts, where we're showing a policy change. In these cases, we link some policies (like project policies and custom policies) but the links go directly to the relevant object or a minimal explanation of the change. We don't link policies like "All Users". - Capability contexts, where we're describing a capability, like "Can Push" or cases in Applicaitons. In these cases, we link all policies to the full policy explanation flow. Test Plan: - Used `bin/policy show` to examine the policy of an object with a project policy, no longer saw HTML. - Viewed the transaction logs of Applications (ModularTransactions) and Tasks (not ModularTransactions) with policy edits, including project and custom policies. - Clicked "Custom Policy" in both logs, got consistent dialogs. - Viewed application detail pages, saw all capabities linked to explanatory capability dialogs. The value of having this dialog is that the user can get a full explanation of special rules even if the policy is something mundane like "All Users". Maniphest Tasks: T13411 Differential Revision: https://secure.phabricator.com/D20804 --- src/__phutil_library_map__.php | 2 + ...habricatorPolicyManagementShowWorkflow.php | 4 +- .../policy/query/PhabricatorPolicyQuery.php | 3 +- .../policy/storage/PhabricatorPolicy.php | 40 +++----- .../policy/view/PhabricatorPolicyRef.php | 99 +++++++++++++++++++ .../PhabricatorApplicationTransaction.php | 16 ++- .../PhabricatorModularTransactionType.php | 15 ++- src/docs/user/userguide/unlocking.diviner | 17 +++- 8 files changed, 146 insertions(+), 50 deletions(-) create mode 100644 src/applications/policy/view/PhabricatorPolicyRef.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index fad919f4dd..d99427dcab 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4195,6 +4195,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyManagementWorkflow' => 'applications/policy/management/PhabricatorPolicyManagementWorkflow.php', 'PhabricatorPolicyPHIDTypePolicy' => 'applications/policy/phid/PhabricatorPolicyPHIDTypePolicy.php', 'PhabricatorPolicyQuery' => 'applications/policy/query/PhabricatorPolicyQuery.php', + 'PhabricatorPolicyRef' => 'applications/policy/view/PhabricatorPolicyRef.php', 'PhabricatorPolicyRequestExceptionHandler' => 'aphront/handler/PhabricatorPolicyRequestExceptionHandler.php', 'PhabricatorPolicyRule' => 'applications/policy/rule/PhabricatorPolicyRule.php', 'PhabricatorPolicySearchEngineExtension' => 'applications/policy/engineextension/PhabricatorPolicySearchEngineExtension.php', @@ -10675,6 +10676,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyManagementWorkflow' => 'PhabricatorManagementWorkflow', 'PhabricatorPolicyPHIDTypePolicy' => 'PhabricatorPHIDType', 'PhabricatorPolicyQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorPolicyRef' => 'Phobject', 'PhabricatorPolicyRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler', 'PhabricatorPolicyRule' => 'Phobject', 'PhabricatorPolicySearchEngineExtension' => 'PhabricatorSearchEngineExtension', diff --git a/src/applications/policy/management/PhabricatorPolicyManagementShowWorkflow.php b/src/applications/policy/management/PhabricatorPolicyManagementShowWorkflow.php index 208f1ae964..a378ecc076 100644 --- a/src/applications/policy/management/PhabricatorPolicyManagementShowWorkflow.php +++ b/src/applications/policy/management/PhabricatorPolicyManagementShowWorkflow.php @@ -60,8 +60,10 @@ final class PhabricatorPolicyManagementShowWorkflow $console->writeOut("__%s__\n\n", pht('CAPABILITIES')); foreach ($policies as $capability => $policy) { + $ref = $policy->newRef($viewer); + $console->writeOut(" **%s**\n", $capability); - $console->writeOut(" %s\n", $policy->renderDescription()); + $console->writeOut(" %s\n", $ref->getPolicyDisplayName()); $console->writeOut(" %s\n", PhabricatorPolicy::getPolicyExplanation($viewer, $policy->getPHID())); $console->writeOut("\n"); diff --git a/src/applications/policy/query/PhabricatorPolicyQuery.php b/src/applications/policy/query/PhabricatorPolicyQuery.php index 3337a912b0..018007db28 100644 --- a/src/applications/policy/query/PhabricatorPolicyQuery.php +++ b/src/applications/policy/query/PhabricatorPolicyQuery.php @@ -48,7 +48,8 @@ final class PhabricatorPolicyQuery $policies = self::loadPolicies($viewer, $object); foreach ($policies as $capability => $policy) { - $policies[$capability] = $policy->renderDescription(); + $policies[$capability] = $policy->newRef($viewer) + ->newCapabilityLink($object, $capability); } return $policies; diff --git a/src/applications/policy/storage/PhabricatorPolicy.php b/src/applications/policy/storage/PhabricatorPolicy.php index 834ca7a798..82d4f355bc 100644 --- a/src/applications/policy/storage/PhabricatorPolicy.php +++ b/src/applications/policy/storage/PhabricatorPolicy.php @@ -276,32 +276,22 @@ final class PhabricatorPolicy } } - public function renderDescription() { - if ($this->getHref()) { - $desc = javelin_tag( - 'a', - array( - 'href' => $this->getHref(), - 'class' => 'policy-link', - 'sigil' => $this->getWorkflow() ? 'workflow' : null, - ), - $this->getName()); - } else { - $desc = $this->getName(); - } + public function newRef(PhabricatorUser $viewer) { + return id(new PhabricatorPolicyRef()) + ->setViewer($viewer) + ->setPolicy($this); + } - switch ($this->getType()) { - case PhabricatorPolicyType::TYPE_PROJECT: - return pht('%s (Project)', $desc); - case PhabricatorPolicyType::TYPE_CUSTOM: - return $desc; - case PhabricatorPolicyType::TYPE_MASKED: - return pht( - '%s (You do not have permission to view policy details.)', - $desc); - default: - return $desc; - } + public function isProjectPolicy() { + return ($this->getType() === PhabricatorPolicyType::TYPE_PROJECT); + } + + public function isCustomPolicy() { + return ($this->getType() === PhabricatorPolicyType::TYPE_CUSTOM); + } + + public function isMaskedPolicy() { + return ($this->getType() === PhabricatorPolicyType::TYPE_MASKED); } /** diff --git a/src/applications/policy/view/PhabricatorPolicyRef.php b/src/applications/policy/view/PhabricatorPolicyRef.php new file mode 100644 index 0000000000..605d59ee45 --- /dev/null +++ b/src/applications/policy/view/PhabricatorPolicyRef.php @@ -0,0 +1,99 @@ +viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + + public function setPolicy(PhabricatorPolicy $policy) { + $this->policy = $policy; + return $this; + } + + public function getPolicy() { + return $this->policy; + } + + public function getPolicyDisplayName() { + $policy = $this->getPolicy(); + return $policy->getFullName(); + } + + public function newTransactionLink( + $mode, + PhabricatorApplicationTransaction $xaction) { + + $policy = $this->getPolicy(); + + if ($policy->isCustomPolicy()) { + $uri = urisprintf( + '/transactions/%s/%s/', + $mode, + $xaction->getPHID()); + $workflow = true; + } else { + $uri = $policy->getHref(); + $workflow = false; + } + + return $this->newLink($uri, $workflow); + } + + public function newCapabilityLink($object, $capability) { + $policy = $this->getPolicy(); + + $uri = urisprintf( + '/policy/explain/%s/%s/', + $object->getPHID(), + $capability); + + return $this->newLink($uri, true); + } + + private function newLink($uri, $workflow) { + $policy = $this->getPolicy(); + $name = $policy->getName(); + + if ($uri !== null) { + $name = javelin_tag( + 'a', + array( + 'href' => $uri, + 'sigil' => ($workflow ? 'workflow' : null), + ), + $name); + } + + $hint = $this->getPolicyTypeHint(); + if ($hint !== null) { + $name = pht('%s (%s)', $name, $hint); + } + + return $name; + } + + private function getPolicyTypeHint() { + $policy = $this->getPolicy(); + + if ($policy->isProjectPolicy()) { + return pht('Project'); + } + + if ($policy->isMaskedPolicy()) { + return pht('You do not have permission to view policy details.'); + } + + return null; + } + +} diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 5fa4562164..7e65ac0a09 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -445,19 +445,15 @@ abstract class PhabricatorApplicationTransaction $policy = PhabricatorPolicy::newFromPolicyAndHandle( $phid, $this->getHandleIfExists($phid)); + + $ref = $policy->newRef($this->getViewer()); + if ($this->renderingTarget == self::TARGET_HTML) { - switch ($policy->getType()) { - case PhabricatorPolicyType::TYPE_CUSTOM: - $policy->setHref('/transactions/'.$state.'/'.$this->getPHID().'/'); - $policy->setWorkflow(true); - break; - default: - break; - } - $output = $policy->renderDescription(); + $output = $ref->newTransactionLink($state, $this); } else { - $output = hsprintf('%s', $policy->getFullName()); + $output = $ref->getPolicyDisplayName(); } + return $output; } diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php index 2d0cb8e7c1..7d5e3c533e 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php +++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php @@ -215,17 +215,16 @@ abstract class PhabricatorModularTransactionType $phid, $handles[$phid]); + $ref = $policy->newRef($viewer); + if ($this->isTextMode()) { - return $this->renderValue($policy->getFullName()); + $name = $ref->getPolicyDisplayName(); + } else { + $storage = $this->getStorage(); + $name = $ref->newTransactionLink($mode, $storage); } - $storage = $this->getStorage(); - if ($policy->getType() == PhabricatorPolicyType::TYPE_CUSTOM) { - $policy->setHref('/transactions/'.$mode.'/'.$storage->getPHID().'/'); - $policy->setWorkflow(true); - } - - return $this->renderValue($policy->renderDescription()); + return $this->renderValue($name); } final protected function renderHandleList(array $phids) { diff --git a/src/docs/user/userguide/unlocking.diviner b/src/docs/user/userguide/unlocking.diviner index 7dc29f69bd..456655a393 100644 --- a/src/docs/user/userguide/unlocking.diviner +++ b/src/docs/user/userguide/unlocking.diviner @@ -63,16 +63,23 @@ For detailed help on managing and stripping MFA, see the instructions in Unlocking Objects ================= -If you aren't sure who owns an object, or no user account has access to an -object, you can directly change object policies from the CLI: +If you aren't sure who owns an object, you can inspect the policies from the +CLI: + +``` +$ ./bin/policy show +``` + +To identify the object you want to examine, you can specify an object +name (like `T123`) or a PHID as the `` parameter. + +If examining the policy isn't helpful, or no user account has access to an +object, you can then directly change object policies from the CLI: ``` $ ./bin/policy unlock [--view ...] [--edit ...] [--owner ...] ``` -To identify the object you want to unlock, you can specify an object name (like -`T123`) or a PHID as the `` parameter. - Use the `--view` and `--edit` flags (and, for some objects, the `--owner` flag) to specify new policies for the object. From 9a36e6931cf737e94ceec70492ef435eec7ec0fd Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Sep 2019 08:20:04 -0700 Subject: [PATCH 30/44] Inline custom policy rules inside policy capability explanation dialogs Summary: Ref T13411. When users click a link to explain a capability (like the policy header on many objects, or the link next to specific capabilities in "Applications", "Diffusion", etc), inline the full ruleset for the custom policy into the dialog if the object has a custom policy. Test Plan: {F6856365} Maniphest Tasks: T13411 Differential Revision: https://secure.phabricator.com/D20805 --- resources/celerity/map.php | 8 +- src/__phutil_library_map__.php | 2 + .../PhabricatorPolicyExplainController.php | 9 +- .../policy/view/PHUIPolicySectionView.php | 12 ++- .../view/PhabricatorPolicyRulesView.php | 84 +++++++++++++++++++ ...rApplicationTransactionValueController.php | 81 +----------------- webroot/rsrc/css/phui/phui-header-view.css | 42 ---------- .../css/phui/phui-policy-section-view.css | 62 ++++++++++++++ 8 files changed, 176 insertions(+), 124 deletions(-) create mode 100644 src/applications/policy/view/PhabricatorPolicyRulesView.php create mode 100644 webroot/rsrc/css/phui/phui-policy-section-view.css diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 37794cfb81..651714b117 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => '5a4a5010', + 'core.pkg.css' => 'c69171e6', 'core.pkg.js' => '73a06a9f', 'differential.pkg.css' => '8d8360fb', 'differential.pkg.js' => '0b037a4f', @@ -155,7 +155,7 @@ return array( 'rsrc/css/phui/phui-form-view.css' => '01b796c0', 'rsrc/css/phui/phui-form.css' => '159e2d9c', 'rsrc/css/phui/phui-head-thing.css' => 'd7f293df', - 'rsrc/css/phui/phui-header-view.css' => '285c9139', + 'rsrc/css/phui/phui-header-view.css' => 'b500eeea', 'rsrc/css/phui/phui-hovercard.css' => '6ca90fa0', 'rsrc/css/phui/phui-icon-set-selector.css' => '7aa5f3ec', 'rsrc/css/phui/phui-icon.css' => '4cbc684a', @@ -168,6 +168,7 @@ return array( 'rsrc/css/phui/phui-object-box.css' => 'f434b6be', 'rsrc/css/phui/phui-pager.css' => 'd022c7ad', 'rsrc/css/phui/phui-pinboard-view.css' => '1f08f5d8', + 'rsrc/css/phui/phui-policy-section-view.css' => '139fdc64', 'rsrc/css/phui/phui-property-list-view.css' => 'cad62236', 'rsrc/css/phui/phui-remarkup-preview.css' => '91767007', 'rsrc/css/phui/phui-segment-bar-view.css' => '5166b370', @@ -842,7 +843,7 @@ return array( 'phui-form-css' => '159e2d9c', 'phui-form-view-css' => '01b796c0', 'phui-head-thing-view-css' => 'd7f293df', - 'phui-header-view-css' => '285c9139', + 'phui-header-view-css' => 'b500eeea', 'phui-hovercard' => '074f0783', 'phui-hovercard-view-css' => '6ca90fa0', 'phui-icon-set-selector-css' => '7aa5f3ec', @@ -863,6 +864,7 @@ return array( 'phui-oi-simple-ui-css' => '6a30fa46', 'phui-pager-css' => 'd022c7ad', 'phui-pinboard-view-css' => '1f08f5d8', + 'phui-policy-section-view-css' => '139fdc64', 'phui-property-list-view-css' => 'cad62236', 'phui-remarkup-preview-css' => '91767007', 'phui-segment-bar-view-css' => '5166b370', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d99427dcab..1f844f9695 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4198,6 +4198,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyRef' => 'applications/policy/view/PhabricatorPolicyRef.php', 'PhabricatorPolicyRequestExceptionHandler' => 'aphront/handler/PhabricatorPolicyRequestExceptionHandler.php', 'PhabricatorPolicyRule' => 'applications/policy/rule/PhabricatorPolicyRule.php', + 'PhabricatorPolicyRulesView' => 'applications/policy/view/PhabricatorPolicyRulesView.php', 'PhabricatorPolicySearchEngineExtension' => 'applications/policy/engineextension/PhabricatorPolicySearchEngineExtension.php', 'PhabricatorPolicyStrengthConstants' => 'applications/policy/constants/PhabricatorPolicyStrengthConstants.php', 'PhabricatorPolicyTestCase' => 'applications/policy/__tests__/PhabricatorPolicyTestCase.php', @@ -10679,6 +10680,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyRef' => 'Phobject', 'PhabricatorPolicyRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler', 'PhabricatorPolicyRule' => 'Phobject', + 'PhabricatorPolicyRulesView' => 'AphrontView', 'PhabricatorPolicySearchEngineExtension' => 'PhabricatorSearchEngineExtension', 'PhabricatorPolicyStrengthConstants' => 'PhabricatorPolicyConstants', 'PhabricatorPolicyTestCase' => 'PhabricatorTestCase', diff --git a/src/applications/policy/controller/PhabricatorPolicyExplainController.php b/src/applications/policy/controller/PhabricatorPolicyExplainController.php index 8b41ef7357..cb9bd20088 100644 --- a/src/applications/policy/controller/PhabricatorPolicyExplainController.php +++ b/src/applications/policy/controller/PhabricatorPolicyExplainController.php @@ -318,7 +318,7 @@ final class PhabricatorPolicyExplainController ->setViewer($viewer) ->setIcon($handle->getIcon().' bluegrey') ->setHeader(pht('Object Policy')) - ->appendList( + ->appendParagraph( array( array( phutil_tag('strong', array(), pht('%s:', $capability_name)), @@ -337,6 +337,13 @@ final class PhabricatorPolicyExplainController $policy->getPHID()), )); + if ($policy->isCustomPolicy()) { + $rules_view = id(new PhabricatorPolicyRulesView()) + ->setViewer($viewer) + ->setPolicy($policy); + $object_section->appendRulesView($rules_view); + } + $strength = $this->getStrengthInformation($object, $policy, $capability); if ($strength) { $object_section->appendHint($strength); diff --git a/src/applications/policy/view/PHUIPolicySectionView.php b/src/applications/policy/view/PHUIPolicySectionView.php index 471e5035fb..14d97fb17e 100644 --- a/src/applications/policy/view/PHUIPolicySectionView.php +++ b/src/applications/policy/view/PHUIPolicySectionView.php @@ -93,6 +93,16 @@ final class PHUIPolicySectionView return $this->appendChild(phutil_tag('p', array(), $content)); } + public function appendRulesView(PhabricatorPolicyRulesView $rules_view) { + return $this->appendChild( + phutil_tag( + 'div', + array( + 'class' => 'phui-policy-section-view-rules', + ), + $rules_view)); + } + protected function getTagAttributes() { return array( 'class' => 'phui-policy-section-view', @@ -100,7 +110,7 @@ final class PHUIPolicySectionView } protected function getTagContent() { - require_celerity_resource('phui-header-view-css'); + require_celerity_resource('phui-policy-section-view-css'); $icon_view = null; $icon = $this->getIcon(); diff --git a/src/applications/policy/view/PhabricatorPolicyRulesView.php b/src/applications/policy/view/PhabricatorPolicyRulesView.php new file mode 100644 index 0000000000..657b612f16 --- /dev/null +++ b/src/applications/policy/view/PhabricatorPolicyRulesView.php @@ -0,0 +1,84 @@ +policy = $policy; + return $this; + } + + public function getPolicy() { + return $this->policy; + } + + public function render() { + $policy = $this->getPolicy(); + + require_celerity_resource('policy-transaction-detail-css'); + + $rule_objects = array(); + foreach ($policy->getCustomRuleClasses() as $class) { + $rule_objects[$class] = newv($class, array()); + } + + $policy = clone $policy; + $policy->attachRuleObjects($rule_objects); + + $details = array(); + $details[] = phutil_tag( + 'p', + array( + 'class' => 'policy-transaction-detail-intro', + ), + pht('These rules are processed in order:')); + + foreach ($policy->getRules() as $index => $rule) { + $rule_object = $rule_objects[$rule['rule']]; + if ($rule['action'] == 'allow') { + $icon = 'fa-check-circle green'; + } else { + $icon = 'fa-minus-circle red'; + } + $icon = id(new PHUIIconView()) + ->setIcon($icon) + ->setText( + ucfirst($rule['action']).' '.$rule_object->getRuleDescription()); + + $handle_phids = $rule_object->getRequiredHandlePHIDsForSummary( + $rule['value']); + if ($handle_phids) { + $value = $this->getViewer() + ->renderHandleList($handle_phids) + ->setAsInline(true); + } else { + $value = $rule['value']; + } + + $details[] = phutil_tag('div', + array( + 'class' => 'policy-transaction-detail-row', + ), + array( + $icon, + $value, + )); + } + + $details[] = phutil_tag( + 'p', + array( + 'class' => 'policy-transaction-detail-end', + ), + pht( + 'If no rules match, %s all other users.', + phutil_tag('b', + array(), + $policy->getDefaultAction()))); + + return $details; + } + +} diff --git a/src/applications/transactions/controller/PhabricatorApplicationTransactionValueController.php b/src/applications/transactions/controller/PhabricatorApplicationTransactionValueController.php index 9c82ff99c8..ef5e168898 100644 --- a/src/applications/transactions/controller/PhabricatorApplicationTransactionValueController.php +++ b/src/applications/transactions/controller/PhabricatorApplicationTransactionValueController.php @@ -58,89 +58,16 @@ final class PhabricatorApplicationTransactionValueController return new Aphront404Response(); } - $rule_objects = array(); - foreach ($policy->getCustomRuleClasses() as $class) { - $rule_objects[$class] = newv($class, array()); - } - $policy->attachRuleObjects($rule_objects); + $rules_view = id(new PhabricatorPolicyRulesView()) + ->setViewer($viewer) + ->setPolicy($policy); - $this->requireResource('policy-transaction-detail-css'); $cancel_uri = $this->guessCancelURI($viewer, $xaction); return $this->newDialog() ->setTitle($policy->getFullName()) ->setWidth(AphrontDialogView::WIDTH_FORM) - ->appendChild($this->renderPolicyDetails($policy, $rule_objects)) + ->appendChild($rules_view) ->addCancelButton($cancel_uri, pht('Close')); } - - private function extractPHIDs( - PhabricatorPolicy $policy, - array $rule_objects) { - - $phids = array(); - foreach ($policy->getRules() as $rule) { - $rule_object = $rule_objects[$rule['rule']]; - $phids[] = - $rule_object->getRequiredHandlePHIDsForSummary($rule['value']); - } - return array_filter(array_mergev($phids)); - } - - private function renderPolicyDetails( - PhabricatorPolicy $policy, - array $rule_objects) { - $details = array(); - $details[] = phutil_tag( - 'p', - array( - 'class' => 'policy-transaction-detail-intro', - ), - pht('These rules are processed in order:')); - - foreach ($policy->getRules() as $index => $rule) { - $rule_object = $rule_objects[$rule['rule']]; - if ($rule['action'] == 'allow') { - $icon = 'fa-check-circle green'; - } else { - $icon = 'fa-minus-circle red'; - } - $icon = id(new PHUIIconView()) - ->setIcon($icon) - ->setText( - ucfirst($rule['action']).' '.$rule_object->getRuleDescription()); - - $handle_phids = $rule_object->getRequiredHandlePHIDsForSummary( - $rule['value']); - if ($handle_phids) { - $value = $this->getViewer() - ->renderHandleList($handle_phids) - ->setAsInline(true); - } else { - $value = $rule['value']; - } - - $details[] = phutil_tag('div', - array( - 'class' => 'policy-transaction-detail-row', - ), - array( - $icon, - $value, - )); - } - - $details[] = phutil_tag( - 'p', - array( - 'class' => 'policy-transaction-detail-end', - ), - pht( - 'If no rules match, %s all other users.', - phutil_tag('b', - array(), - $policy->getDefaultAction()))); - return $details; - } - } diff --git a/webroot/rsrc/css/phui/phui-header-view.css b/webroot/rsrc/css/phui/phui-header-view.css index 6a096af76d..1d851f04ee 100644 --- a/webroot/rsrc/css/phui/phui-header-view.css +++ b/webroot/rsrc/css/phui/phui-header-view.css @@ -354,45 +354,3 @@ body .phui-header-shell.phui-bleed-header .phui-header-view .phui-tag-indigo a { color: {$sh-indigotext}; } - -.phui-policy-section-view { - margin-bottom: 24px; -} - -.phui-policy-section-view-header { - background: {$bluebackground}; - border-bottom: 1px solid {$lightblueborder}; - padding: 4px 8px; - color: {$darkbluetext}; - margin-bottom: 8px; -} - -.phui-policy-section-view-header-text { - font-weight: bold; -} - -.phui-policy-section-view-header .phui-icon-view { - margin-right: 8px; -} - -.phui-policy-section-view-link { - float: right; -} - -.phui-policy-section-view-link .phui-icon-view { - color: {$bluetext}; -} - -.phui-policy-section-view-hint { - color: {$greytext}; - background: {$lightbluebackground}; - padding: 8px; -} - -.phui-policy-section-view-body { - padding: 0 12px; -} - -.phui-policy-section-view-inactive-rule { - color: {$greytext}; -} diff --git a/webroot/rsrc/css/phui/phui-policy-section-view.css b/webroot/rsrc/css/phui/phui-policy-section-view.css new file mode 100644 index 0000000000..4325b34867 --- /dev/null +++ b/webroot/rsrc/css/phui/phui-policy-section-view.css @@ -0,0 +1,62 @@ +/** + * @provides phui-policy-section-view-css + */ + +.phui-policy-section-view { + margin-bottom: 24px; +} + +.phui-policy-section-view-header { + background: {$bluebackground}; + border-bottom: 1px solid {$lightblueborder}; + padding: 4px 8px; + color: {$darkbluetext}; + margin-bottom: 8px; +} + +.phui-policy-section-view-header-text { + font-weight: bold; +} + +.phui-policy-section-view-header .phui-icon-view { + margin-right: 8px; +} + +.phui-policy-section-view-link { + float: right; +} + +.phui-policy-section-view-link .phui-icon-view { + color: {$bluetext}; +} + +.phui-policy-section-view-hint { + color: {$greytext}; + background: {$lightbluebackground}; + padding: 8px; +} + +.phui-policy-section-view-body { + padding: 0 12px; +} + +.phui-policy-section-view-inactive-rule { + color: {$greytext}; +} + +.phui-policy-section-view-rules { + margin: 8px 0; + padding: 8px; + background: {$lightbluebackground}; + border: 1px solid {$lightblueborder}; +} + +.phui-policy-section-view .phui-policy-section-view-body ul { + margin: 8px 0; + padding: 0 16px 0 24px; + list-style: disc; +} + +.phui-policy-section-view .phui-policy-section-view-body p + p { + margin-top: 8px; +} From 0c7ea8c94215eadae8d6082384709bf1988039e2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Sep 2019 08:53:14 -0700 Subject: [PATCH 31/44] When users fail a "CAN_SEE" check, give them an "opaque" policy explanation Summary: Ref T13411. Currently, if you hit a policy exception because you can't view an object, we disclose details about the view policy of the object, particularly which project's members can see the object for project policies. Although there's a large amount of grey area here, this feels like a more substantial disclosure than we offer in other contexts. Instead, if you encounter a policy exception while testing "CAN_VIEW" or don't have "CAN_VIEW", present an "opaque" explanation which omits details that viewers who can't view the object shouldn't have access to. Today, this is the name of "Project" policies (and, implicitly, the rulesets of custom policies, which we now disclose in other similar contexts). Test Plan: - Hit policy exceptions for "CAN_VIEW" on an object with a project view policy, saw an opaque explanation. - Hit policy exceptions for "CAN_EDIT" on an object with a project edit policy and a view policy I satisfied, saw a more detailed explanation. Maniphest Tasks: T13411 Differential Revision: https://secure.phabricator.com/D20806 --- .../policy/filter/PhabricatorPolicyFilter.php | 28 +++++++++++++++++-- .../policy/storage/PhabricatorPolicy.php | 23 ++++++++++++++- 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index 4ea7ce1549..63f3f98cec 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -602,12 +602,13 @@ final class PhabricatorPolicyFilter extends Phobject { PhabricatorPolicyInterface $object, $policy, $capability) { + $viewer = $this->viewer; if (!$this->raisePolicyExceptions) { return; } - if ($this->viewer->isOmnipotent()) { + if ($viewer->isOmnipotent()) { // Never raise policy exceptions for the omnipotent viewer. Although we // will never normally issue a policy rejection for the omnipotent // viewer, we can end up here when queries blanket reject objects that @@ -634,7 +635,30 @@ final class PhabricatorPolicyFilter extends Phobject { $capability); } - $more = PhabricatorPolicy::getPolicyExplanation($this->viewer, $policy); + // See T13411. If you receive a policy exception because you can't view + // an object, we also want to avoid disclosing too many details about the + // actual policy (for example, the names of projects in the policy). + + // If you failed a "CAN_VIEW" check, or failed some other check and don't + // have "CAN_VIEW" on the object, we give you an "opaque" explanation. + // Otherwise, we give you a more detailed explanation. + + $view_capability = PhabricatorPolicyCapability::CAN_VIEW; + if ($capability === $view_capability) { + $show_details = false; + } else { + $show_details = self::hasCapability( + $viewer, + $object, + $view_capability); + } + + if ($show_details) { + $more = PhabricatorPolicy::getPolicyExplanation($viewer, $policy); + } else { + $more = PhabricatorPolicy::getOpaquePolicyExplanation($viewer, $policy); + } + $more = (array)$more; $more = array_filter($more); diff --git a/src/applications/policy/storage/PhabricatorPolicy.php b/src/applications/policy/storage/PhabricatorPolicy.php index 82d4f355bc..66a7d9e3be 100644 --- a/src/applications/policy/storage/PhabricatorPolicy.php +++ b/src/applications/policy/storage/PhabricatorPolicy.php @@ -220,6 +220,25 @@ final class PhabricatorPolicy PhabricatorUser $viewer, $policy) { + $type = phid_get_type($policy); + if ($type === PhabricatorProjectProjectPHIDType::TYPECONST) { + $handle = id(new PhabricatorHandleQuery()) + ->setViewer($viewer) + ->withPHIDs(array($policy)) + ->executeOne(); + + return pht( + 'Members of the project "%s" can take this action.', + $handle->getFullName()); + } + + return self::getOpaquePolicyExplanation($viewer, $policy); + } + + public static function getOpaquePolicyExplanation( + PhabricatorUser $viewer, + $policy) { + $rule = PhabricatorPolicyQuery::getObjectPolicyRule($policy); if ($rule) { return $rule->getPolicyExplanation(); @@ -245,7 +264,9 @@ final class PhabricatorPolicy $type = phid_get_type($policy); if ($type == PhabricatorProjectProjectPHIDType::TYPECONST) { return pht( - 'Members of the project "%s" can take this action.', + 'Members of a particular project can take this action. (You '. + 'can not see this object, so the name of this project is '. + 'restricted.)', $handle->getFullName()); } else if ($type == PhabricatorPeopleUserPHIDType::TYPECONST) { return pht( From 4f845d8f8c7749b536f32c2f26f1fdb7c32e2766 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Sep 2019 09:28:56 -0700 Subject: [PATCH 32/44] When users encounter a policy exception for a non-view capability with a custom policy, inline the policy rules Summary: Fixes T13411. This looks like the last case where you hit a policy explanation and have permission to see the policy, but we don't currently show you the policy rules. This implementation is slightly clumsy, but likely harmless. Test Plan: {F6856421} Maniphest Tasks: T13411 Differential Revision: https://secure.phabricator.com/D20807 --- .../policy/filter/PhabricatorPolicyFilter.php | 45 ++++++++++++++++--- 1 file changed, 38 insertions(+), 7 deletions(-) diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index 63f3f98cec..d8f239e51a 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -653,14 +653,42 @@ final class PhabricatorPolicyFilter extends Phobject { $view_capability); } + // TODO: This is a bit clumsy. We're producing HTML and text versions of + // this message, but can't render the full policy rules in text today. + // Users almost never get a text-only version of this exception anyway. + + $head = null; + $more = null; + if ($show_details) { - $more = PhabricatorPolicy::getPolicyExplanation($viewer, $policy); + $head = PhabricatorPolicy::getPolicyExplanation($viewer, $policy); + + $policy_type = PhabricatorPolicyPHIDTypePolicy::TYPECONST; + $is_custom = (phid_get_type($policy) === $policy_type); + if ($is_custom) { + $policy_map = PhabricatorPolicyQuery::loadPolicies( + $viewer, + $object); + if (isset($policy_map[$capability])) { + require_celerity_resource('phui-policy-section-view-css'); + + $more = id(new PhabricatorPolicyRulesView()) + ->setViewer($viewer) + ->setPolicy($policy_map[$capability]); + + $more = phutil_tag( + 'div', + array( + 'class' => 'phui-policy-section-view-rules', + ), + $more); + } + } } else { - $more = PhabricatorPolicy::getOpaquePolicyExplanation($viewer, $policy); + $head = PhabricatorPolicy::getOpaquePolicyExplanation($viewer, $policy); } - $more = (array)$more; - $more = array_filter($more); + $head = (array)$head; $exceptions = PhabricatorPolicy::getSpecialRules( $object, @@ -668,7 +696,10 @@ final class PhabricatorPolicyFilter extends Phobject { $capability, true); - $details = array_filter(array_merge($more, $exceptions)); + $text_details = array_filter(array_merge($head, $exceptions)); + $text_details = implode(' ', $text_details); + + $html_details = array($head, $more, $exceptions); $access_denied = $this->renderAccessDenied($object); @@ -677,7 +708,7 @@ final class PhabricatorPolicyFilter extends Phobject { $access_denied, $capability_name, $rejection, - implode(' ', $details)); + $text_details); $exception = id(new PhabricatorPolicyException($full_message)) ->setTitle($access_denied) @@ -685,7 +716,7 @@ final class PhabricatorPolicyFilter extends Phobject { ->setRejection($rejection) ->setCapability($capability) ->setCapabilityName($capability_name) - ->setMoreInfo($details); + ->setMoreInfo($html_details); throw $exception; } From d60d4e6a05212f5e31c93c36ee39c775eafba56a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Sep 2019 12:54:08 -0700 Subject: [PATCH 33/44] Don't present users with Herald fields/actions for uninstalled applications, unless the rule already uses them Summary: Fixes T7961. Currently, we present Herald users with actions like "Require legalpad signatures" and "Run build plans" even if Legalpad and Harbormaster are not installed. Instead, allow fields and actions to be made "unavailable", which means that we won't present them as options when adding to new or existing rules. If you edit a rule which already uses one of these fields or actions, it isn't affected. Test Plan: - Created a rule with a legalpad action, uninstalled legalpad, edited the rule. Action remained untouched. - Created a new rule, wasn't offered the legalpad action. - Reinstalled the application, saw the action again. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T7961 Differential Revision: https://secure.phabricator.com/D20808 --- resources/celerity/map.php | 6 +-- .../HarbormasterRunBuildPlansHeraldAction.php | 4 ++ .../herald/action/HeraldAction.php | 4 ++ .../herald/adapter/HeraldAdapter.php | 20 ++++++++++ .../controller/HeraldRuleController.php | 10 ++++- src/applications/herald/field/HeraldField.php | 4 ++ .../LegalpadRequireSignatureHeraldAction.php | 5 +++ .../js/application/herald/HeraldRuleEditor.js | 40 +++++++++++++++---- 8 files changed, 80 insertions(+), 13 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 651714b117..f5ff8827ae 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -397,7 +397,7 @@ return array( 'rsrc/js/application/files/behavior-icon-composer.js' => '38a6cedb', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => 'a17b84f1', 'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => 'b347a301', - 'rsrc/js/application/herald/HeraldRuleEditor.js' => '27daef73', + 'rsrc/js/application/herald/HeraldRuleEditor.js' => '2633bef7', 'rsrc/js/application/herald/PathTypeahead.js' => 'ad486db3', 'rsrc/js/application/herald/herald-rule-editor.js' => '0922e81d', 'rsrc/js/application/maniphest/behavior-batch-selector.js' => '139ef688', @@ -571,7 +571,7 @@ return array( 'global-drag-and-drop-css' => '1d2713a4', 'harbormaster-css' => '8dfe16b2', 'herald-css' => '648d39e2', - 'herald-rule-editor' => '27daef73', + 'herald-rule-editor' => '2633bef7', 'herald-test-css' => 'e004176f', 'inline-comment-summary-css' => '81eb368d', 'javelin-aphlict' => '022516b4', @@ -1117,7 +1117,7 @@ return array( 'javelin-json', 'phabricator-draggable-list', ), - '27daef73' => array( + '2633bef7' => array( 'multirow-row-manager', 'javelin-install', 'javelin-util', diff --git a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php index 9fc053e8ae..6934a2e3d9 100644 --- a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php +++ b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php @@ -96,4 +96,8 @@ final class HarbormasterRunBuildPlansHeraldAction return $record->getTarget(); } + public function isActionAvailable() { + return id(new PhabricatorHarbormasterApplication())->isInstalled(); + } + } diff --git a/src/applications/herald/action/HeraldAction.php b/src/applications/herald/action/HeraldAction.php index a9740d1736..d914c97a1c 100644 --- a/src/applications/herald/action/HeraldAction.php +++ b/src/applications/herald/action/HeraldAction.php @@ -405,4 +405,8 @@ abstract class HeraldAction extends Phobject { return array(); } + public function isActionAvailable() { + return true; + } + } diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index cb8545b71d..2832c6d9f4 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -373,6 +373,16 @@ abstract class HeraldAdapter extends Phobject { return $field->getFieldGroupKey(); } + public function isFieldAvailable($field_key) { + $field = $this->getFieldImplementation($field_key); + + if (!$field) { + return null; + } + + return $field->isFieldAvailable(); + } + /* -( Conditions )--------------------------------------------------------- */ @@ -765,6 +775,16 @@ abstract class HeraldAdapter extends Phobject { return $action->getActionGroupKey(); } + public function isActionAvailable($action_key) { + $action = $this->getActionImplementation($action_key); + + if (!$action) { + return null; + } + + return $action->isActionAvailable(); + } + public function getActions($rule_type) { $actions = array(); foreach ($this->getActionsForRuleType($rule_type) as $key => $action) { diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index 73c1e8c39c..5e13522b6e 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/src/applications/herald/controller/HeraldRuleController.php @@ -684,7 +684,10 @@ final class HeraldRuleController extends HeraldController { $group_map = array(); foreach ($field_map as $field_key => $field_name) { $group_key = $adapter->getFieldGroupKey($field_key); - $group_map[$group_key][$field_key] = $field_name; + $group_map[$group_key][$field_key] = array( + 'name' => $field_name, + 'available' => $adapter->isFieldAvailable($field_key), + ); } return $this->getGroups( @@ -696,7 +699,10 @@ final class HeraldRuleController extends HeraldController { $group_map = array(); foreach ($action_map as $action_key => $action_name) { $group_key = $adapter->getActionGroupKey($action_key); - $group_map[$group_key][$action_key] = $action_name; + $group_map[$group_key][$action_key] = array( + 'name' => $action_name, + 'available' => $adapter->isActionAvailable($action_key), + ); } return $this->getGroups( diff --git a/src/applications/herald/field/HeraldField.php b/src/applications/herald/field/HeraldField.php index 6f14091aff..2a1e89d558 100644 --- a/src/applications/herald/field/HeraldField.php +++ b/src/applications/herald/field/HeraldField.php @@ -241,4 +241,8 @@ abstract class HeraldField extends Phobject { return false; } + public function isFieldAvailable() { + return true; + } + } diff --git a/src/applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php b/src/applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php index 7ff69d37d5..b477c2ce35 100644 --- a/src/applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php +++ b/src/applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php @@ -130,4 +130,9 @@ final class LegalpadRequireSignatureHeraldAction 'Require document signatures: %s.', $this->renderHandleList($value)); } + + public function isActionAvailable() { + return id(new PhabricatorLegalpadApplication())->isInstalled(); + } + } diff --git a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js b/webroot/rsrc/js/application/herald/HeraldRuleEditor.js index 443fe9811e..254061533a 100644 --- a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js +++ b/webroot/rsrc/js/application/herald/HeraldRuleEditor.js @@ -350,8 +350,10 @@ JX.install('HeraldRuleEditor', { sigil: 'field-select' }; - var field_select = this._renderGroupSelect(groups, attrs); - field_select.value = this._config.conditions[row_id][0]; + var field_select = this._renderGroupSelect( + groups, + attrs, + this._config.conditions[row_id][0]); var field_cell = JX.$N('td', {sigil: 'field-cell'}, field_select); @@ -367,18 +369,38 @@ JX.install('HeraldRuleEditor', { } }, - _renderGroupSelect: function(groups, attrs) { + _renderGroupSelect: function(groups, attrs, value) { var optgroups = []; for (var ii = 0; ii < groups.length; ii++) { var group = groups[ii]; var options = []; for (var k in group.options) { - options.push(JX.$N('option', {value: k}, group.options[k])); + var option = group.options[k]; + + var name = option.name; + var available = option.available; + + // See T7961. If the option is not marked as "available", we only + // include it in the dropdown if the dropdown already has it as a + // value. We want to hide options provided by applications which are + // not installed, but do not want to break existing rules. + + if (available || (k === value)) { + options.push(JX.$N('option', {value: k}, name)); + } + } + if (options.length) { + optgroups.push(JX.$N('optgroup', {label: group.label}, options)); } - optgroups.push(JX.$N('optgroup', {label: group.label}, options)); } - return JX.$N('select', attrs, optgroups); + var select = JX.$N('select', attrs, optgroups); + + if (value !== undefined) { + select.value = value; + } + + return select; }, _newAction : function(data) { @@ -402,8 +424,10 @@ JX.install('HeraldRuleEditor', { sigil: 'action-select' }; - var action_select = this._renderGroupSelect(groups, attrs); - action_select.value = action[0]; + var action_select = this._renderGroupSelect( + groups, + attrs, + action[0]); var action_cell = JX.$N('td', {sigil: 'action-cell'}, action_select); From 3e60128037252fb62067315eaeff67f517967119 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Sep 2019 13:11:18 -0700 Subject: [PATCH 34/44] Support "Subtype" in Herald Summary: See PHI1434. For objects that support subtypes and have subtypes configured, allow Herald rules to act on subtypes. Test Plan: - Configured task and project subtypes, wrote Herald rules, saw "Subtypes" as an option, saw appropriate typeahead values and detail page rendering. - Unconfigured project subtypes, saw field vanish from UI for new rules. - Wrote a "subtype"-depenent rule that added a comment, interacted with tasks of that subtype and a different subtype. Saw Herald act only on tasks with the correct subtype. Differential Revision: https://secure.phabricator.com/D20809 --- src/__phutil_library_map__.php | 2 + .../maniphest/storage/ManiphestTask.php | 3 +- .../project/storage/PhabricatorProject.php | 3 +- .../PhabricatorEditEngineSubtypeMap.php | 14 +++++ ...habricatorEditEngineSubtypeHeraldField.php | 52 +++++++++++++++++++ 5 files changed, 72 insertions(+), 2 deletions(-) create mode 100644 src/applications/transactions/herald/PhabricatorEditEngineSubtypeHeraldField.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 1f844f9695..95c5059d06 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3217,6 +3217,7 @@ phutil_register_library_map(array( 'PhabricatorEditEngineSettingsPanel' => 'applications/settings/panel/PhabricatorEditEngineSettingsPanel.php', 'PhabricatorEditEngineStaticCommentAction' => 'applications/transactions/commentaction/PhabricatorEditEngineStaticCommentAction.php', 'PhabricatorEditEngineSubtype' => 'applications/transactions/editengine/PhabricatorEditEngineSubtype.php', + 'PhabricatorEditEngineSubtypeHeraldField' => 'applications/transactions/herald/PhabricatorEditEngineSubtypeHeraldField.php', 'PhabricatorEditEngineSubtypeInterface' => 'applications/transactions/editengine/PhabricatorEditEngineSubtypeInterface.php', 'PhabricatorEditEngineSubtypeMap' => 'applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php', 'PhabricatorEditEngineSubtypeTestCase' => 'applications/transactions/editengine/__tests__/PhabricatorEditEngineSubtypeTestCase.php', @@ -9550,6 +9551,7 @@ phutil_register_library_map(array( 'PhabricatorEditEngineSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorEditEngineStaticCommentAction' => 'PhabricatorEditEngineCommentAction', 'PhabricatorEditEngineSubtype' => 'Phobject', + 'PhabricatorEditEngineSubtypeHeraldField' => 'HeraldField', 'PhabricatorEditEngineSubtypeMap' => 'Phobject', 'PhabricatorEditEngineSubtypeTestCase' => 'PhabricatorTestCase', 'PhabricatorEditEngineSubtypeTransaction' => 'PhabricatorEditEngineTransactionType', diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index d2700895ce..c56d8fe57a 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -564,7 +564,8 @@ final class ManiphestTask extends ManiphestDAO public function newEditEngineSubtypeMap() { $config = PhabricatorEnv::getEnvConfig('maniphest.subtypes'); - return PhabricatorEditEngineSubtype::newSubtypeMap($config); + return PhabricatorEditEngineSubtype::newSubtypeMap($config) + ->setDatasource(new ManiphestTaskSubtypeDatasource()); } diff --git a/src/applications/project/storage/PhabricatorProject.php b/src/applications/project/storage/PhabricatorProject.php index 54267829d3..1d234d4285 100644 --- a/src/applications/project/storage/PhabricatorProject.php +++ b/src/applications/project/storage/PhabricatorProject.php @@ -904,7 +904,8 @@ final class PhabricatorProject extends PhabricatorProjectDAO public function newEditEngineSubtypeMap() { $config = PhabricatorEnv::getEnvConfig('projects.subtypes'); - return PhabricatorEditEngineSubtype::newSubtypeMap($config); + return PhabricatorEditEngineSubtype::newSubtypeMap($config) + ->setDatasource(new PhabricatorProjectSubtypeDatasource()); } public function newSubtypeObject() { diff --git a/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php b/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php index 638b665184..dc1ee2842a 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php @@ -5,6 +5,7 @@ final class PhabricatorEditEngineSubtypeMap extends Phobject { private $subtypes; + private $datasource; public function __construct(array $subtypes) { assert_instances_of($subtypes, 'PhabricatorEditEngineSubtype'); @@ -39,6 +40,19 @@ final class PhabricatorEditEngineSubtypeMap return $this->subtypes[$subtype_key]; } + public function setDatasource(PhabricatorTypeaheadDatasource $datasource) { + $this->datasource = $datasource; + return $this; + } + + public function newDatasource() { + if (!$this->datasource) { + throw new PhutilInvalidStateException('setDatasource'); + } + + return clone($this->datasource); + } + public function getCreateFormsForSubtype( PhabricatorEditEngine $edit_engine, PhabricatorEditEngineSubtypeInterface $object) { diff --git a/src/applications/transactions/herald/PhabricatorEditEngineSubtypeHeraldField.php b/src/applications/transactions/herald/PhabricatorEditEngineSubtypeHeraldField.php new file mode 100644 index 0000000000..be15540fc9 --- /dev/null +++ b/src/applications/transactions/herald/PhabricatorEditEngineSubtypeHeraldField.php @@ -0,0 +1,52 @@ +getEditEngineSubtype(); + } + + protected function getHeraldFieldStandardType() { + return self::STANDARD_PHID; + } + + protected function getDatasource() { + $object = $this->getAdapter()->getObject(); + $map = $object->newEditEngineSubtypeMap(); + return $map->newDatasource(); + } + + protected function getDatasourceValueMap() { + $object = $this->getAdapter()->getObject(); + $map = $object->newEditEngineSubtypeMap(); + + $result = array(); + foreach ($map->getSubtypes() as $subtype) { + $result[$subtype->getKey()] = $subtype->getName(); + } + + return $result; + } + + public function isFieldAvailable() { + $object = $this->getAdapter()->getObject(); + $map = $object->newEditEngineSubtypeMap(); + return ($map->getCount() > 1); + } + +} From 41f0b8b0a3a10b713017ed1356d5d0a89432c29d Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Sep 2019 16:05:40 -0700 Subject: [PATCH 35/44] Allow subtypes to specify "mutations", to control the behavior of the "Change Subtype" action Summary: Fixes T13415. Provide a way for subtypes to customize the behavior of "Change Subtype" actions that appear above comment areas. Subtypes may disable this action by specifying `"mutations": []`, or provide a list of subtypes. The bulk editor and API can still perform any change. Test Plan: - Tried to define an invalid "mutations" list with a bad subtype, got a sensible error. - Specified a limited mutations list and an empty mutations list, verified that corresponding tasks got corresponding actions. - Used the bulk editor to perform a freeform mutation. - Verified that tasks of a subtype with no "mutations" still work the same way they used to (allow mutation into any subtype). Maniphest Tasks: T13415 Differential Revision: https://secure.phabricator.com/D20810 --- .../PhabricatorManiphestConfigOptions.php | 41 +++++++++++++++---- .../PhabricatorEditEngineSubtype.php | 35 ++++++++++++++++ .../PhabricatorEditEngineSubtypeMap.php | 38 +++++++++++++++++ .../PhabricatorSubtypeEditEngineExtension.php | 18 +++++--- 4 files changed, 120 insertions(+), 12 deletions(-) diff --git a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php index 077fc511ce..60a41a26ca 100644 --- a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php +++ b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php @@ -344,6 +344,8 @@ dictionary with these keys: - `children` //Optional map.// Configure options shown to the user when they "Create Subtask". See below. - `fields` //Optional map.// Configure field behaviors. See below. + - `mutations` //Optional list.// Configure which subtypes this subtype + can easily be converted to by using the "Change Subtype" action. See below. Each subtype must have a unique key, and you must define a subtype with the key "%s", which is used as a default subtype. @@ -404,15 +406,15 @@ The `fields` key can configure the behavior of custom fields on specific task subtypes. For example: ``` -{ - ... - "fields": { - "custom.some-field": { - "disabled": true + { + ... + "fields": { + "custom.some-field": { + "disabled": true + } } + ... } - ... -} ``` Each field supports these options: @@ -421,6 +423,31 @@ Each field supports these options: subtypes. - `name` //Optional string.// Custom name of this field for the subtype. + +The `mutations` key allows you to control the behavior of the "Change Subtype" +action above the comment area. By default, this action allows users to change +the task subtype into any other subtype. + +If you'd prefer to make it more difficult to change subtypes or offer only a +subset of subtypes, you can specify the list of subtypes that "Change Subtypes" +offers. For example, if you have several similar subtypes and want to allow +tasks to be converted between them but not easily converted to other types, +you can make the "Change Subtypes" control show only these options like this: + +``` + { + ... + "mutations": ["bug", "issue", "defect"] + ... + } +``` + +If you specify an empty list, the "Change Subtypes" action will be completely +hidden. + +This mutation list is advisory and only configures the UI. Tasks may still be +converted across subtypes freely by using the Bulk Editor or API. + EOTEXT , $subtype_default_key)); diff --git a/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php b/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php index 6e1d1de115..f471fcd92f 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php @@ -14,6 +14,7 @@ final class PhabricatorEditEngineSubtype private $childSubtypes = array(); private $childIdentifiers = array(); private $fieldConfiguration = array(); + private $mutations; public function setKey($key) { $this->key = $key; @@ -78,6 +79,15 @@ final class PhabricatorEditEngineSubtype return $this->childIdentifiers; } + public function setMutations($mutations) { + $this->mutations = $mutations; + return $this; + } + + public function getMutations() { + return $this->mutations; + } + public function hasTagView() { return (bool)strlen($this->getTagText()); } @@ -152,6 +162,7 @@ final class PhabricatorEditEngineSubtype 'icon' => 'optional string', 'children' => 'optional map', 'fields' => 'optional map', + 'mutations' => 'optional list', )); $key = $value['key']; @@ -217,6 +228,28 @@ final class PhabricatorEditEngineSubtype 'with key "%s". This subtype is required and must be defined.', self::SUBTYPE_DEFAULT)); } + + foreach ($config as $value) { + $key = idx($value, 'key'); + + $mutations = idx($value, 'mutations'); + if (!$mutations) { + continue; + } + + foreach ($mutations as $mutation) { + if (!isset($map[$mutation])) { + throw new Exception( + pht( + 'Subtype configuration is invalid: subtype with key "%s" '. + 'specifies that it can mutate into subtype "%s", but that is '. + 'not a valid subtype.', + $key, + $mutation)); + } + } + } + } public static function newSubtypeMap(array $config) { @@ -267,6 +300,8 @@ final class PhabricatorEditEngineSubtype } } + $subtype->setMutations(idx($entry, 'mutations')); + $map[$key] = $subtype; } diff --git a/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php b/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php index dc1ee2842a..edf2d2045a 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php @@ -53,6 +53,44 @@ final class PhabricatorEditEngineSubtypeMap return clone($this->datasource); } + public function getMutationMap($source_key) { + return mpull($this->getMutations($source_key), 'getName'); + } + + public function getMutations($source_key) { + $mutations = $this->subtypes; + + $subtype = idx($this->subtypes, $source_key); + if ($subtype) { + $map = $subtype->getMutations(); + if ($map !== null) { + $map = array_fuse($map); + foreach ($mutations as $key => $mutation) { + if ($key === $source_key) { + // This is the current subtype, so we always want to show it. + continue; + } + + if (isset($map[$key])) { + // This is an allowed mutation, so keep it. + continue; + } + + // Discard other subtypes as mutation options. + unset($mutations[$key]); + } + } + } + + // If the only available mutation is the current subtype, treat this like + // no mutations are available. + if (array_keys($mutations) === array($source_key)) { + $mutations = array(); + } + + return $mutations; + } + public function getCreateFormsForSubtype( PhabricatorEditEngine $edit_engine, PhabricatorEditEngineSubtypeInterface $object) { diff --git a/src/applications/transactions/engineextension/PhabricatorSubtypeEditEngineExtension.php b/src/applications/transactions/engineextension/PhabricatorSubtypeEditEngineExtension.php index d0b6d017f3..e73d476d74 100644 --- a/src/applications/transactions/engineextension/PhabricatorSubtypeEditEngineExtension.php +++ b/src/applications/transactions/engineextension/PhabricatorSubtypeEditEngineExtension.php @@ -29,9 +29,17 @@ final class PhabricatorSubtypeEditEngineExtension PhabricatorApplicationTransactionInterface $object) { $subtype_type = PhabricatorTransactions::TYPE_SUBTYPE; + $subtype_value = $object->getEditEngineSubtype(); $map = $object->newEditEngineSubtypeMap(); - $options = $map->getDisplayMap(); + + if ($object->getID()) { + $options = $map->getMutationMap($subtype_value); + } else { + // NOTE: This is a crude proxy for "are we in the bulk edit workflow". + // We want to allow any mutation. + $options = $map->getDisplayMap(); + } $subtype_field = id(new PhabricatorSelectEditField()) ->setKey(self::EDITKEY) @@ -40,12 +48,12 @@ final class PhabricatorSubtypeEditEngineExtension ->setTransactionType($subtype_type) ->setConduitDescription(pht('Change the object subtype.')) ->setConduitTypeDescription(pht('New object subtype key.')) - ->setValue($object->getEditEngineSubtype()) + ->setValue($subtype_value) ->setOptions($options); - // If subtypes are configured, enable changing them from the bulk editor - // and comment action stack. - if ($map->getCount() > 1) { + // If subtypes are configured, enable changing them from the bulk editor. + // Bulk editor + if ($options) { $subtype_field ->setBulkEditLabel(pht('Change subtype to')) ->setCommentActionLabel(pht('Change Subtype')) From 3f6620336216840028b1c44cd8c5171e4914fb58 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Sep 2019 16:20:33 -0700 Subject: [PATCH 36/44] Fix a straggling callsite to "renderApplicationPolicy()" Summary: Ref T13411. This is a leftover from recent policy rendering changes. Test Plan: Viewed feed with application policy stories, no more fatal. Maniphest Tasks: T13411 Differential Revision: https://secure.phabricator.com/D20811 --- .../xactions/PhabricatorApplicationPolicyChangeTransaction.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/applications/meta/xactions/PhabricatorApplicationPolicyChangeTransaction.php b/src/applications/meta/xactions/PhabricatorApplicationPolicyChangeTransaction.php index 3c58b42ab5..94e1cc495d 100644 --- a/src/applications/meta/xactions/PhabricatorApplicationPolicyChangeTransaction.php +++ b/src/applications/meta/xactions/PhabricatorApplicationPolicyChangeTransaction.php @@ -61,9 +61,6 @@ final class PhabricatorApplicationPolicyChangeTransaction } public function getTitleForFeed() { - $old = $this->renderApplicationPolicy($this->getOldValue()); - $new = $this->renderApplicationPolicy($this->getNewValue()); - return pht( '%s changed the %s policy for application %s from %s to %s.', $this->renderAuthor(), From 3dcb4a7b5036f627548a9a42ae3ef916130ccfea Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Sep 2019 18:55:18 -0700 Subject: [PATCH 37/44] Work around rendering engine freeze in Chrome 77 affecting workboards Summary: Ref T13413. In Chrome 77, workboard cards with titles that must break in the middle of words cause the browser to completely lock up. Work around the major known instance of this by overriding the "break-word" behavior. This gives us worse rendering for tasks with very long "words" in their titles (they are truncated instead of broken) but fixes the freezing. Once Chrome is fixed, this can be reverted. Test Plan: - Created a task named "MMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMM" on a workboard. - Loaded the board in Chrome 77. - Before: entire page locks up. - After: smooth sailing, except the "MMMMMM..." is truncated. Maniphest Tasks: T13413 Differential Revision: https://secure.phabricator.com/D20812 --- resources/celerity/map.php | 4 ++-- webroot/rsrc/css/phui/workboards/phui-workcard.css | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index f5ff8827ae..d1233b3684 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -179,7 +179,7 @@ return array( 'rsrc/css/phui/phui-two-column-view.css' => '01e6991e', 'rsrc/css/phui/workboards/phui-workboard-color.css' => 'e86de308', 'rsrc/css/phui/workboards/phui-workboard.css' => '74fc9d98', - 'rsrc/css/phui/workboards/phui-workcard.css' => '9e9eb0df', + 'rsrc/css/phui/workboards/phui-workcard.css' => '913441b6', 'rsrc/css/phui/workboards/phui-workpanel.css' => '3ae89b20', 'rsrc/css/sprite-login.css' => '18b368a6', 'rsrc/css/sprite-tokens.css' => 'f1896dc5', @@ -876,7 +876,7 @@ return array( 'phui-two-column-view-css' => '01e6991e', 'phui-workboard-color-css' => 'e86de308', 'phui-workboard-view-css' => '74fc9d98', - 'phui-workcard-view-css' => '9e9eb0df', + 'phui-workcard-view-css' => '913441b6', 'phui-workpanel-view-css' => '3ae89b20', 'phuix-action-list-view' => 'c68f183f', 'phuix-action-view' => 'aaa08f3b', diff --git a/webroot/rsrc/css/phui/workboards/phui-workcard.css b/webroot/rsrc/css/phui/workboards/phui-workcard.css index 3c6a798fc8..d9acef12ef 100644 --- a/webroot/rsrc/css/phui/workboards/phui-workcard.css +++ b/webroot/rsrc/css/phui/workboards/phui-workcard.css @@ -36,6 +36,10 @@ .phui-workcard .phui-oi-link { white-space: normal; + + /* See T13413. This works around a Chrome 77 rendering engine freeze. */ + word-wrap: normal; + font-weight: normal; color: {$blacktext}; margin-left: 2px; From c5a4dea8cf7c15026352164b00c3d92eab072dc9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 15 Sep 2019 06:59:14 -0700 Subject: [PATCH 38/44] Fix a typo in preamble X-Forwarded-For psuedocode Summary: This psueudocode should use the result of computation at the end. Test Plan: Read carefully. Differential Revision: https://secure.phabricator.com/D20813 --- src/docs/user/configuration/configuring_preamble.diviner | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/docs/user/configuration/configuring_preamble.diviner b/src/docs/user/configuration/configuring_preamble.diviner index 5299afa27d..6b6b9da149 100644 --- a/src/docs/user/configuration/configuring_preamble.diviner +++ b/src/docs/user/configuration/configuring_preamble.diviner @@ -82,7 +82,7 @@ if (isset($_SERVER['HTTP_X_FORWARDED_FOR'])) { $real_address = your_custom_parsing_function($raw_header); - $_SERVER['REMOTE_ADDR'] = $raw_header; + $_SERVER['REMOTE_ADDR'] = $real_address; } ``` From f529abf90065888f9139e4b2b123fd1dfd00c01b Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 9 May 2019 08:58:11 -0700 Subject: [PATCH 39/44] In stacked area charts, group nearby points so they don't overlap Summary: Ref T13279. We currently draw a point on the chart for each datapoint, but this leads to many overlapping circles. Instead, aggregate the raw points into display points ("events") at the end. Test Plan: Viewed a stacked area chart with many points, saw a more palatable number of drawn dots. Subscribers: yelirekim Maniphest Tasks: T13279 Differential Revision: https://secure.phabricator.com/D20814 --- resources/celerity/map.php | 20 +++--- .../PhabricatorChartStackedAreaDataset.php | 61 ++++++++++++++++++- webroot/rsrc/css/phui/phui-chart.css | 7 ++- webroot/rsrc/js/application/fact/Chart.js | 22 +++++-- 4 files changed, 90 insertions(+), 20 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index d1233b3684..9d18ebfcb8 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -141,7 +141,7 @@ return array( 'rsrc/css/phui/phui-big-info-view.css' => '362ad37b', 'rsrc/css/phui/phui-box.css' => '5ed3b8cb', 'rsrc/css/phui/phui-bulk-editor.css' => '374d5e30', - 'rsrc/css/phui/phui-chart.css' => '10135a9d', + 'rsrc/css/phui/phui-chart.css' => '14df9ae3', 'rsrc/css/phui/phui-cms.css' => '8c05c41e', 'rsrc/css/phui/phui-comment-form.css' => '68a2d99a', 'rsrc/css/phui/phui-comment-panel.css' => 'ec4e31c0', @@ -390,7 +390,7 @@ return array( 'rsrc/js/application/diffusion/behavior-pull-lastmodified.js' => 'c715c123', 'rsrc/js/application/doorkeeper/behavior-doorkeeper-tag.js' => '6a85bc5a', 'rsrc/js/application/drydock/drydock-live-operation-status.js' => '47a0728b', - 'rsrc/js/application/fact/Chart.js' => 'eec96de0', + 'rsrc/js/application/fact/Chart.js' => 'ddb9dd1f', 'rsrc/js/application/fact/ChartCurtainView.js' => '86954222', 'rsrc/js/application/fact/ChartFunctionLabel.js' => '81de1dab', 'rsrc/js/application/files/behavior-document-engine.js' => '243d6c22', @@ -699,7 +699,7 @@ return array( 'javelin-behavior-user-menu' => '60cd9241', 'javelin-behavior-view-placeholder' => 'a9942052', 'javelin-behavior-workflow' => '9623adc1', - 'javelin-chart' => 'eec96de0', + 'javelin-chart' => 'ddb9dd1f', 'javelin-chart-curtain-view' => '86954222', 'javelin-chart-function-label' => '81de1dab', 'javelin-color' => '78f811c9', @@ -828,7 +828,7 @@ return array( 'phui-calendar-day-css' => '9597d706', 'phui-calendar-list-css' => 'ccd7e4e2', 'phui-calendar-month-css' => 'cb758c42', - 'phui-chart-css' => '10135a9d', + 'phui-chart-css' => '14df9ae3', 'phui-cms-css' => '8c05c41e', 'phui-comment-form-css' => '68a2d99a', 'phui-comment-panel-css' => 'ec4e31c0', @@ -2066,6 +2066,12 @@ return array( 'javelin-uri', 'phabricator-notification', ), + 'ddb9dd1f' => array( + 'phui-chart-css', + 'd3', + 'javelin-chart-curtain-view', + 'javelin-chart-function-label', + ), 'dfa1d313' => array( 'javelin-behavior', 'javelin-dom', @@ -2127,12 +2133,6 @@ return array( 'phabricator-keyboard-shortcut', 'javelin-stratcom', ), - 'eec96de0' => array( - 'phui-chart-css', - 'd3', - 'javelin-chart-curtain-view', - 'javelin-chart-function-label', - ), 'ef836bf2' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php b/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php index 8bf4445984..4d30cd767b 100644 --- a/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php +++ b/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php @@ -17,8 +17,8 @@ final class PhabricatorChartStackedAreaDataset $datapoints = $function->newDatapoints($data_query); foreach ($datapoints as $point) { - $x = $point['x']; - $function_points[$function_idx][$x] = $point; + $x_value = $point['x']; + $function_points[$function_idx][$x_value] = $point; } } @@ -140,12 +140,67 @@ final class PhabricatorChartStackedAreaDataset $series = array_reverse($series); + // We're going to group multiple events into a single point if they have + // X values that are very close to one another. + // + // If the Y values are also close to one another (these points are near + // one another in a horizontal line), it can be hard to select any + // individual point with the mouse. + // + // Even if the Y values are not close together (the points are on a + // fairly steep slope up or down), it's usually better to be able to + // mouse over a single point at the top or bottom of the slope and get + // a summary of what's going on. + + $domain_max = $data_query->getMaximumValue(); + $domain_min = $data_query->getMinimumValue(); + $resolution = ($domain_max - $domain_min) / 100; + $events = array(); foreach ($raw_points as $function_idx => $points) { $event_list = array(); + + $event_group = array(); + $head_event = null; foreach ($points as $point) { - $event_list[] = $point; + $x = $point['x']; + + if ($head_event === null) { + // We don't have any points yet, so start a new group. + $head_event = $x; + $event_group[] = $point; + } else if (($x - $head_event) <= $resolution) { + // This point is close to the first point in this group, so + // add it to the existing group. + $event_group[] = $point; + } else { + // This point is not close to the first point in the group, + // so create a new group. + $event_list[] = $event_group; + $head_event = $x; + $event_group = array($point); + } } + + if ($event_group) { + $event_list[] = $event_group; + } + + $event_spec = array(); + foreach ($event_list as $key => $event_points) { + // NOTE: We're using the last point as the representative point so + // that you can learn about a section of a chart by hovering over + // the point to right of the section, which is more intuitive than + // other points. + $event = last($event_points); + + $event = $event + array( + 'n' => count($event_points), + ); + + $event_list[$key] = $event; + } + $events[] = $event_list; } diff --git a/webroot/rsrc/css/phui/phui-chart.css b/webroot/rsrc/css/phui/phui-chart.css index 350d86014a..646ed52581 100644 --- a/webroot/rsrc/css/phui/phui-chart.css +++ b/webroot/rsrc/css/phui/phui-chart.css @@ -36,16 +36,17 @@ } .chart .point { - fill: {$lightblue}; + fill: #ffffff; stroke: {$blue}; - stroke-width: 1px; + stroke-width: 2px; + position: relative; + cursor: pointer; } .chart-tooltip { position: absolute; text-align: center; width: 120px; - height: 16px; overflow: hidden; padding: 2px; background: {$lightbluebackground}; diff --git a/webroot/rsrc/js/application/fact/Chart.js b/webroot/rsrc/js/application/fact/Chart.js index 9ce50822ee..a57e5459da 100644 --- a/webroot/rsrc/js/application/fact/Chart.js +++ b/webroot/rsrc/js/application/fact/Chart.js @@ -133,6 +133,8 @@ JX.install('Chart', { }, _newStackedArea: function(g, dataset, x, y, div, curtain) { + var ii; + var to_date = JX.bind(this, this._newDate); var area = d3.area() @@ -144,7 +146,7 @@ JX.install('Chart', { .x(function(d) { return x(to_date(d.x)); }) .y(function(d) { return y(d.y1); }); - for (var ii = 0; ii < dataset.data.length; ii++) { + for (ii = 0; ii < dataset.data.length; ii++) { var label = new JX.ChartFunctionLabel(dataset.labels[ii]); var fill_color = label.getFillColor() || label.getColor(); @@ -160,6 +162,11 @@ JX.install('Chart', { .style('stroke', stroke_color) .attr('d', line(dataset.data[ii])); + curtain.addFunctionLabel(label); + } + + // Now that we've drawn all the areas and lines, draw the dots. + for (ii = 0; ii < dataset.data.length; ii++) { g.selectAll('dot') .data(dataset.events[ii]) .enter() @@ -178,8 +185,16 @@ JX.install('Chart', { var d_d = dd.getDate(); + var y = parseInt(d.y1); + + var label = d.n + ' Points'; + + var view = + d_y + '-' + d_m + '-' + d_d + ': ' + y + '
' + + label; + div - .html(d_y + '-' + d_m + '-' + d_d + ': ' + d.y1) + .html(view) .style('opacity', 0.9) .style('left', (d3.event.pageX - 60) + 'px') .style('top', (d3.event.pageY - 38) + 'px'); @@ -187,9 +202,8 @@ JX.install('Chart', { .on('mouseout', function() { div.style('opacity', 0); }); - - curtain.addFunctionLabel(label); } + }, _newDate: function(epoch) { From 769e745a3f2750365d4e684e669a31ebc06bf8bb Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 16 Sep 2019 11:34:25 -0700 Subject: [PATCH 40/44] In charts, make "min" and "max" into pure functions and formally mark pure functions as pure Summary: Depends on D20814. Currently, "min()" and "max()" are still "min(f, n)". This is no longer consistent with the construction of functions a function-generators that are composed at top level. Turn them into "min(n)" and "max(n)" (i.e., not higher-order functions). Then, mark all the functions which are pure mathematical functions and not higher-order as "pure". These functions have no function parameters and do not reference external data. For now, this distinction has no immediate implications, but it will simplify the next change (which tracks where data came from when it originated from an external source -- these pure functions never have any source information, since they only apply pure mathematical transformations to data). Test Plan: Loaded a burnup chart, nothing seemed obviously broken. Subscribers: yelirekim Differential Revision: https://secure.phabricator.com/D20815 --- src/__phutil_library_map__.php | 16 +++++++------ .../PhabricatorConstantChartFunction.php | 2 +- .../chart/PhabricatorCosChartFunction.php | 2 +- .../chart/PhabricatorMaxChartFunction.php | 23 ++++++------------ .../chart/PhabricatorMinChartFunction.php | 23 ++++++------------ .../chart/PhabricatorPureChartFunction.php | 4 ++++ .../chart/PhabricatorScaleChartFunction.php | 2 +- .../chart/PhabricatorShiftChartFunction.php | 2 +- .../chart/PhabricatorSinChartFunction.php | 2 +- .../PhabricatorProjectBurndownChartEngine.php | 24 ++++++++++++------- 10 files changed, 48 insertions(+), 52 deletions(-) create mode 100644 src/applications/fact/chart/PhabricatorPureChartFunction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 95c5059d06..e8c0392427 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4422,6 +4422,7 @@ phutil_register_library_map(array( 'PhabricatorProjectsWatchersSearchEngineAttachment' => 'applications/project/engineextension/PhabricatorProjectsWatchersSearchEngineAttachment.php', 'PhabricatorPronounSetting' => 'applications/settings/setting/PhabricatorPronounSetting.php', 'PhabricatorProtocolLog' => 'infrastructure/log/PhabricatorProtocolLog.php', + 'PhabricatorPureChartFunction' => 'applications/fact/chart/PhabricatorPureChartFunction.php', 'PhabricatorPygmentSetupCheck' => 'applications/config/check/PhabricatorPygmentSetupCheck.php', 'PhabricatorQuery' => 'infrastructure/query/PhabricatorQuery.php', 'PhabricatorQueryConstraint' => 'infrastructure/query/constraint/PhabricatorQueryConstraint.php', @@ -9155,7 +9156,7 @@ phutil_register_library_map(array( 'PhabricatorConpherenceWidgetVisibleSetting' => 'PhabricatorInternalSetting', 'PhabricatorConsoleApplication' => 'PhabricatorApplication', 'PhabricatorConsoleContentSource' => 'PhabricatorContentSource', - 'PhabricatorConstantChartFunction' => 'PhabricatorChartFunction', + 'PhabricatorConstantChartFunction' => 'PhabricatorPureChartFunction', 'PhabricatorContactNumbersSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorContentSource' => 'Phobject', 'PhabricatorContentSourceModule' => 'PhabricatorConfigModule', @@ -9167,7 +9168,7 @@ phutil_register_library_map(array( 'PhabricatorCoreCreateTransaction' => 'PhabricatorCoreTransactionType', 'PhabricatorCoreTransactionType' => 'PhabricatorModularTransactionType', 'PhabricatorCoreVoidTransaction' => 'PhabricatorModularTransactionType', - 'PhabricatorCosChartFunction' => 'PhabricatorChartFunction', + 'PhabricatorCosChartFunction' => 'PhabricatorPureChartFunction', 'PhabricatorCountFact' => 'PhabricatorFact', 'PhabricatorCountdown' => array( 'PhabricatorCountdownDAO', @@ -10068,7 +10069,7 @@ phutil_register_library_map(array( 'PhabricatorMarkupInterface', ), 'PhabricatorMarkupPreviewController' => 'PhabricatorController', - 'PhabricatorMaxChartFunction' => 'PhabricatorChartFunction', + 'PhabricatorMaxChartFunction' => 'PhabricatorPureChartFunction', 'PhabricatorMemeEngine' => 'Phobject', 'PhabricatorMemeRemarkupRule' => 'PhutilRemarkupRule', 'PhabricatorMentionRemarkupRule' => 'PhutilRemarkupRule', @@ -10135,7 +10136,7 @@ phutil_register_library_map(array( 'PhabricatorMetronome' => 'Phobject', 'PhabricatorMetronomeTestCase' => 'PhabricatorTestCase', 'PhabricatorMetronomicTriggerClock' => 'PhabricatorTriggerClock', - 'PhabricatorMinChartFunction' => 'PhabricatorChartFunction', + 'PhabricatorMinChartFunction' => 'PhabricatorPureChartFunction', 'PhabricatorModularTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorModularTransactionType' => 'Phobject', 'PhabricatorMonogramDatasourceEngineExtension' => 'PhabricatorDatasourceEngineExtension', @@ -10950,6 +10951,7 @@ phutil_register_library_map(array( 'PhabricatorProjectsWatchersSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', 'PhabricatorPronounSetting' => 'PhabricatorSelectSetting', 'PhabricatorProtocolLog' => 'Phobject', + 'PhabricatorPureChartFunction' => 'PhabricatorChartFunction', 'PhabricatorPygmentSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorQuery' => 'Phobject', 'PhabricatorQueryConstraint' => 'Phobject', @@ -11208,7 +11210,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyInterface', ), 'PhabricatorSavedQueryQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', - 'PhabricatorScaleChartFunction' => 'PhabricatorChartFunction', + 'PhabricatorScaleChartFunction' => 'PhabricatorPureChartFunction', 'PhabricatorScheduleTaskTriggerAction' => 'PhabricatorTriggerAction', 'PhabricatorScopedEnv' => 'Phobject', 'PhabricatorSearchAbstractDocument' => 'Phobject', @@ -11299,12 +11301,12 @@ phutil_register_library_map(array( 'PhabricatorSetupIssue' => 'Phobject', 'PhabricatorSetupIssueUIExample' => 'PhabricatorUIExample', 'PhabricatorSetupIssueView' => 'AphrontView', - 'PhabricatorShiftChartFunction' => 'PhabricatorChartFunction', + 'PhabricatorShiftChartFunction' => 'PhabricatorPureChartFunction', 'PhabricatorShortSite' => 'PhabricatorSite', 'PhabricatorShowFiletreeSetting' => 'PhabricatorSelectSetting', 'PhabricatorSignDocumentsUserLogType' => 'PhabricatorUserLogType', 'PhabricatorSimpleEditType' => 'PhabricatorEditType', - 'PhabricatorSinChartFunction' => 'PhabricatorChartFunction', + 'PhabricatorSinChartFunction' => 'PhabricatorPureChartFunction', 'PhabricatorSite' => 'AphrontSite', 'PhabricatorSlackAuthProvider' => 'PhabricatorOAuth2AuthProvider', 'PhabricatorSlowvoteApplication' => 'PhabricatorApplication', diff --git a/src/applications/fact/chart/PhabricatorConstantChartFunction.php b/src/applications/fact/chart/PhabricatorConstantChartFunction.php index cdc6c9494a..e65c577827 100644 --- a/src/applications/fact/chart/PhabricatorConstantChartFunction.php +++ b/src/applications/fact/chart/PhabricatorConstantChartFunction.php @@ -1,7 +1,7 @@ newArgument() - ->setName('x') - ->setType('function'), $this->newArgument() ->setName('max') ->setType('number'), ); } - public function getDomain() { - return $this->getArgument('x')->getDomain(); - } - - public function newInputValues(PhabricatorChartDataQuery $query) { - return $this->getArgument('x')->newInputValues($query); - } - public function evaluateFunction(array $xv) { - $yv = $this->getArgument('x')->evaluateFunction($xv); $max = $this->getArgument('max'); - foreach ($yv as $k => $y) { - if ($y > $max) { - $yv[$k] = null; + $yv = array(); + foreach ($xv as $x) { + if ($x > $max) { + $yv[] = null; + } else { + $yv[] = $x; } } diff --git a/src/applications/fact/chart/PhabricatorMinChartFunction.php b/src/applications/fact/chart/PhabricatorMinChartFunction.php index db1a003811..e6dcec06a4 100644 --- a/src/applications/fact/chart/PhabricatorMinChartFunction.php +++ b/src/applications/fact/chart/PhabricatorMinChartFunction.php @@ -1,36 +1,27 @@ newArgument() - ->setName('x') - ->setType('function'), $this->newArgument() ->setName('min') ->setType('number'), ); } - public function getDomain() { - return $this->getArgument('x')->getDomain(); - } - - public function newInputValues(PhabricatorChartDataQuery $query) { - return $this->getArgument('x')->newInputValues($query); - } - public function evaluateFunction(array $xv) { - $yv = $this->getArgument('x')->evaluateFunction($xv); $min = $this->getArgument('min'); - foreach ($yv as $k => $y) { - if ($y < $min) { - $yv[$k] = null; + $yv = array(); + foreach ($xv as $x) { + if ($x < $min) { + $yv[] = null; + } else { + $yv[] = $x; } } diff --git a/src/applications/fact/chart/PhabricatorPureChartFunction.php b/src/applications/fact/chart/PhabricatorPureChartFunction.php new file mode 100644 index 0000000000..aa1213f8c4 --- /dev/null +++ b/src/applications/fact/chart/PhabricatorPureChartFunction.php @@ -0,0 +1,4 @@ +newFunction( - 'min', array( 'accumulate', array('fact', 'tasks.open-count.assign.project', $project_phid), ), - 0); + array( + 'min', + 0, + )); $function->getFunctionLabel() ->setName(pht('Tasks Moved Into Project')) @@ -47,12 +49,14 @@ final class PhabricatorProjectBurndownChartEngine $functions[] = $function; $function = $this->newFunction( - 'min', array( 'accumulate', array('fact', 'tasks.open-count.status.project', $project_phid), ), - 0); + array( + 'min', + 0, + )); $function->getFunctionLabel() ->setName(pht('Tasks Reopened')) @@ -68,20 +72,24 @@ final class PhabricatorProjectBurndownChartEngine array('fact', 'tasks.open-count.create.project', $project_phid), ), array( - 'max', array( 'accumulate', array('fact', 'tasks.open-count.status.project', $project_phid), ), - 0, + array( + 'max', + 0, + ), ), array( - 'max', array( 'accumulate', array('fact', 'tasks.open-count.assign.project', $project_phid), ), - 0, + array( + 'max', + 0, + ), )); $function->getFunctionLabel() From 080e132aa740a4f36582ff7adeb1d1550d6a3bbf Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 16 Sep 2019 11:19:24 -0700 Subject: [PATCH 41/44] Track chart datapoints from their sources and provide a tabular view of chart data Summary: Depends on D20815. Ref T13279. Give datapoints "refs", which allow us to figure out where particular datapoints came from even after the point is transformed by functions. For now, show the raw points in a table below the chart. Test Plan: Viewed chart data, saw reasonable-looking numbers. Subscribers: yelirekim Maniphest Tasks: T13279 Differential Revision: https://secure.phabricator.com/D20816 --- src/__phutil_library_map__.php | 2 +- .../PhabricatorAccumulateChartFunction.php | 10 +- .../fact/chart/PhabricatorChartDataset.php | 31 ++++ .../fact/chart/PhabricatorChartFunction.php | 2 + .../chart/PhabricatorComposeChartFunction.php | 18 +++ .../chart/PhabricatorFactChartFunction.php | 68 ++++++++- .../PhabricatorHigherOrderChartFunction.php | 34 +++++ .../chart/PhabricatorPureChartFunction.php | 12 +- .../PhabricatorFactChartController.php | 12 +- .../PhabricatorChartRenderingEngine.php | 142 +++++++++++++++++- 10 files changed, 315 insertions(+), 16 deletions(-) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e8c0392427..470a6c69fe 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -8301,7 +8301,7 @@ phutil_register_library_map(array( 'PhabricatorAccessLog' => 'Phobject', 'PhabricatorAccessLogConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorAccessibilitySetting' => 'PhabricatorSelectSetting', - 'PhabricatorAccumulateChartFunction' => 'PhabricatorChartFunction', + 'PhabricatorAccumulateChartFunction' => 'PhabricatorHigherOrderChartFunction', 'PhabricatorActionListView' => 'AphrontTagView', 'PhabricatorActionView' => 'AphrontView', 'PhabricatorActivitySettingsPanel' => 'PhabricatorSettingsPanel', diff --git a/src/applications/fact/chart/PhabricatorAccumulateChartFunction.php b/src/applications/fact/chart/PhabricatorAccumulateChartFunction.php index 6ffbb85da9..63570ef234 100644 --- a/src/applications/fact/chart/PhabricatorAccumulateChartFunction.php +++ b/src/applications/fact/chart/PhabricatorAccumulateChartFunction.php @@ -1,7 +1,7 @@ getArgument('x')->getDomain(); - } - - public function newInputValues(PhabricatorChartDataQuery $query) { - return $this->getArgument('x')->newInputValues($query); - } - public function evaluateFunction(array $xv) { // First, we're going to accumulate the underlying function. Then // we'll map the inputs through the accumulation. diff --git a/src/applications/fact/chart/PhabricatorChartDataset.php b/src/applications/fact/chart/PhabricatorChartDataset.php index 9faf02b740..a794834229 100644 --- a/src/applications/fact/chart/PhabricatorChartDataset.php +++ b/src/applications/fact/chart/PhabricatorChartDataset.php @@ -75,4 +75,35 @@ abstract class PhabricatorChartDataset PhabricatorChartDataQuery $data_query); + final public function getTabularDisplayData( + PhabricatorChartDataQuery $data_query) { + $results = array(); + + $functions = $this->getFunctions(); + foreach ($functions as $function) { + $datapoints = $function->newDatapoints($data_query); + + $refs = $function->getDataRefs(ipull($datapoints, 'x')); + + foreach ($datapoints as $key => $point) { + $x = $point['x']; + + if (isset($refs[$x])) { + $xrefs = $refs[$x]; + } else { + $xrefs = array(); + } + + $datapoints[$key]['refs'] = $xrefs; + } + + $results[] = array( + 'data' => $datapoints, + ); + } + + return id(new PhabricatorChartDisplayData()) + ->setWireData($results); + } + } diff --git a/src/applications/fact/chart/PhabricatorChartFunction.php b/src/applications/fact/chart/PhabricatorChartFunction.php index ac7ab64650..ef5c5641bf 100644 --- a/src/applications/fact/chart/PhabricatorChartFunction.php +++ b/src/applications/fact/chart/PhabricatorChartFunction.php @@ -180,6 +180,8 @@ abstract class PhabricatorChartFunction } abstract public function evaluateFunction(array $xv); + abstract public function getDataRefs(array $xv); + abstract public function loadRefs(array $refs); public function getDomain() { return null; diff --git a/src/applications/fact/chart/PhabricatorComposeChartFunction.php b/src/applications/fact/chart/PhabricatorComposeChartFunction.php index f6148ceae9..e69455b3ff 100644 --- a/src/applications/fact/chart/PhabricatorComposeChartFunction.php +++ b/src/applications/fact/chart/PhabricatorComposeChartFunction.php @@ -70,4 +70,22 @@ final class PhabricatorComposeChartFunction return $yv; } + public function getDataRefs(array $xv) { + // TODO: This is not entirely correct. The correct implementation would + // map "x -> y" at each stage of composition and pull and aggregate all + // the datapoint refs. In practice, we currently never compose functions + // with a data function somewhere in the middle, so just grabbing the first + // result is close enough. + + // In the future, we may: notably, "x -> shift(-1 month) -> ..." to + // generate a month-over-month overlay is a sensible operation which will + // source data from the middle of a function composition. + + foreach ($this->getFunctionArguments() as $function) { + return $function->getDataRefs($xv); + } + + return array(); + } + } diff --git a/src/applications/fact/chart/PhabricatorFactChartFunction.php b/src/applications/fact/chart/PhabricatorFactChartFunction.php index dbb886dd3e..0e940d644d 100644 --- a/src/applications/fact/chart/PhabricatorFactChartFunction.php +++ b/src/applications/fact/chart/PhabricatorFactChartFunction.php @@ -7,6 +7,7 @@ final class PhabricatorFactChartFunction private $fact; private $map; + private $refs; protected function newArguments() { $key_argument = $this->newArgument() @@ -51,13 +52,15 @@ final class PhabricatorFactChartFunction $data = queryfx_all( $conn, - 'SELECT value, epoch FROM %T WHERE %LA ORDER BY epoch ASC', + 'SELECT id, value, epoch FROM %T WHERE %LA ORDER BY epoch ASC', $table_name, $where); $map = array(); + $refs = array(); if ($data) { foreach ($data as $row) { + $ref = (string)$row['id']; $value = (int)$row['value']; $epoch = (int)$row['epoch']; @@ -66,10 +69,17 @@ final class PhabricatorFactChartFunction } $map[$epoch] += $value; + + if (!isset($refs[$epoch])) { + $refs[$epoch] = array(); + } + + $refs[$epoch][] = $ref; } } $this->map = $map; + $this->refs = $refs; } public function getDomain() { @@ -99,4 +109,60 @@ final class PhabricatorFactChartFunction return $yv; } + public function getDataRefs(array $xv) { + return array_select_keys($this->refs, $xv); + } + + public function loadRefs(array $refs) { + $fact = $this->fact; + + $datapoint_table = $fact->newDatapoint(); + $conn = $datapoint_table->establishConnection('r'); + + $dimension_table = new PhabricatorFactObjectDimension(); + + $where = array(); + + $where[] = qsprintf( + $conn, + 'p.id IN (%Ld)', + $refs); + + + $rows = queryfx_all( + $conn, + 'SELECT + p.id id, + p.value, + od.objectPHID objectPHID, + dd.objectPHID dimensionPHID + FROM %R p + LEFT JOIN %R od ON od.id = p.objectID + LEFT JOIN %R dd ON dd.id = p.dimensionID + WHERE %LA', + $datapoint_table, + $dimension_table, + $dimension_table, + $where); + $rows = ipull($rows, null, 'id'); + + $results = array(); + + foreach ($refs as $ref) { + if (!isset($rows[$ref])) { + continue; + } + + $row = $rows[$ref]; + + $results[$ref] = array( + 'objectPHID' => $row['objectPHID'], + 'dimensionPHID' => $row['dimensionPHID'], + 'value' => (float)$row['value'], + ); + } + + return $results; + } + } diff --git a/src/applications/fact/chart/PhabricatorHigherOrderChartFunction.php b/src/applications/fact/chart/PhabricatorHigherOrderChartFunction.php index ab160bd10f..7124603388 100644 --- a/src/applications/fact/chart/PhabricatorHigherOrderChartFunction.php +++ b/src/applications/fact/chart/PhabricatorHigherOrderChartFunction.php @@ -32,4 +32,38 @@ abstract class PhabricatorHigherOrderChartFunction return array_keys($map); } + public function getDataRefs(array $xv) { + $refs = array(); + + foreach ($this->getFunctionArguments() as $function) { + $function_refs = $function->getDataRefs($xv); + + $function_refs = array_select_keys($function_refs, $xv); + if (!$function_refs) { + continue; + } + + foreach ($function_refs as $x => $ref_list) { + if (!isset($refs[$x])) { + $refs[$x] = array(); + } + foreach ($ref_list as $ref) { + $refs[$x][] = $ref; + } + } + } + + return $refs; + } + + public function loadRefs(array $refs) { + $results = array(); + + foreach ($this->getFunctionArguments() as $function) { + $results += $function->loadRefs($refs); + } + + return $results; + } + } diff --git a/src/applications/fact/chart/PhabricatorPureChartFunction.php b/src/applications/fact/chart/PhabricatorPureChartFunction.php index aa1213f8c4..74c748c274 100644 --- a/src/applications/fact/chart/PhabricatorPureChartFunction.php +++ b/src/applications/fact/chart/PhabricatorPureChartFunction.php @@ -1,4 +1,14 @@ newChartView(); - return $this->newChartResponse($chart_view); + $tabular_view = $engine->newTabularView(); + + return $this->newChartResponse($chart_view, $tabular_view); } - private function newChartResponse($chart_view) { + private function newChartResponse($chart_view, $tabular_view) { $box = id(new PHUIObjectBoxView()) ->setHeaderText(pht('Chart')) ->appendChild($chart_view); @@ -50,7 +52,11 @@ final class PhabricatorFactChartController extends PhabricatorFactController { return $this->newPage() ->setTitle($title) ->setCrumbs($crumbs) - ->appendChild($box); + ->appendChild( + array( + $box, + $tabular_view, + )); } private function newDemoChart() { diff --git a/src/applications/fact/engine/PhabricatorChartRenderingEngine.php b/src/applications/fact/engine/PhabricatorChartRenderingEngine.php index b328241ea6..c691fe1abf 100644 --- a/src/applications/fact/engine/PhabricatorChartRenderingEngine.php +++ b/src/applications/fact/engine/PhabricatorChartRenderingEngine.php @@ -109,7 +109,143 @@ final class PhabricatorChartRenderingEngine return $chart_view; } + public function newTabularView() { + $viewer = $this->getViewer(); + $tabular_data = $this->newTabularData(); + + $ref_keys = array(); + foreach ($tabular_data['datasets'] as $tabular_dataset) { + foreach ($tabular_dataset as $function) { + foreach ($function['data'] as $point) { + foreach ($point['refs'] as $ref) { + $ref_keys[$ref] = $ref; + } + } + } + } + + $chart = $this->getStoredChart(); + + $ref_map = array(); + foreach ($chart->getDatasets() as $dataset) { + foreach ($dataset->getFunctions() as $function) { + // If we aren't looking for anything else, bail out. + if (!$ref_keys) { + break 2; + } + + $function_refs = $function->loadRefs($ref_keys); + + $ref_map += $function_refs; + + // Remove the ref keys that we found data for from the list of keys + // we are looking for. If any function gives us data for a given ref, + // that's satisfactory. + foreach ($function_refs as $ref_key => $ref_data) { + unset($ref_keys[$ref_key]); + } + } + } + + $phids = array(); + foreach ($ref_map as $ref => $ref_data) { + if (isset($ref_data['objectPHID'])) { + $phids[] = $ref_data['objectPHID']; + } + } + + $handles = $viewer->loadHandles($phids); + + $tabular_view = array(); + foreach ($tabular_data['datasets'] as $tabular_data) { + foreach ($tabular_data as $function) { + $rows = array(); + foreach ($function['data'] as $point) { + $ref_views = array(); + + $xv = date('Y-m-d h:i:s', $point['x']); + $yv = $point['y']; + + $point_refs = array(); + foreach ($point['refs'] as $ref) { + if (!isset($ref_map[$ref])) { + continue; + } + $point_refs[$ref] = $ref_map[$ref]; + } + + if (!$point_refs) { + $rows[] = array( + $xv, + $yv, + ); + } else { + foreach ($point_refs as $ref => $ref_data) { + $ref_value = $ref_data['value']; + $ref_link = $handles[$ref_data['objectPHID']] + ->renderLink(); + + $view_uri = urisprintf( + '/fact/object/%s/', + $ref_data['objectPHID']); + + $ref_button = id(new PHUIButtonView()) + ->setIcon('fa-table') + ->setTag('a') + ->setColor('grey') + ->setHref($view_uri) + ->setText(pht('View Data')); + + $rows[] = array( + $xv, + $yv, + $ref_value, + $ref_link, + $ref_button, + ); + + $xv = null; + $yv = null; + } + } + } + + $table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + pht('X'), + pht('Y'), + pht('Raw'), + pht('Refs'), + null, + )) + ->setColumnClasses( + array( + 'n', + 'n', + 'n', + 'wide', + null, + )); + + $tabular_view[] = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Function')) + ->setTable($table); + } + } + + return $tabular_view; + } + public function newChartData() { + return $this->newWireData(false); + } + + public function newTabularData() { + return $this->newWireData(true); + } + + private function newWireData($is_tabular) { $chart = $this->getStoredChart(); $chart_key = $chart->getChartKey(); @@ -151,7 +287,11 @@ final class PhabricatorChartRenderingEngine $wire_datasets = array(); $ranges = array(); foreach ($datasets as $dataset) { - $display_data = $dataset->getChartDisplayData($data_query); + if ($is_tabular) { + $display_data = $dataset->getTabularDisplayData($data_query); + } else { + $display_data = $dataset->getChartDisplayData($data_query); + } $ranges[] = $display_data->getRange(); $wire_datasets[] = $display_data->getWireData(); From d4ed5d0428ec4b0e6fa23fba83da7ff02cea868e Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 17 Sep 2019 08:58:32 -0700 Subject: [PATCH 42/44] Make various UX improvements to charts so they're closer to making visual sense Summary: Ref T13279. Fix some tabular stuff, draw areas better, make the "compose()" API more consistent, unfatal the demo chart, unfatal the project burndown, make the project chart do something roughly physical. Test Plan: Looked at charts, saw fewer obvious horrors. Subscribers: yelirekim Maniphest Tasks: T13279 Differential Revision: https://secure.phabricator.com/D20817 --- resources/celerity/map.php | 16 +-- src/__phutil_library_map__.php | 2 + .../PhabricatorFactChartController.php | 45 +------- .../fact/engine/PhabricatorChartEngine.php | 10 +- .../PhabricatorChartRenderingEngine.php | 3 + .../engine/PhabricatorDemoChartEngine.php | 44 ++++++++ .../PhabricatorProjectBurndownChartEngine.php | 104 +++++++++--------- webroot/rsrc/js/application/fact/Chart.js | 15 ++- 8 files changed, 130 insertions(+), 109 deletions(-) create mode 100644 src/applications/fact/engine/PhabricatorDemoChartEngine.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 9d18ebfcb8..73e7c19144 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -390,7 +390,7 @@ return array( 'rsrc/js/application/diffusion/behavior-pull-lastmodified.js' => 'c715c123', 'rsrc/js/application/doorkeeper/behavior-doorkeeper-tag.js' => '6a85bc5a', 'rsrc/js/application/drydock/drydock-live-operation-status.js' => '47a0728b', - 'rsrc/js/application/fact/Chart.js' => 'ddb9dd1f', + 'rsrc/js/application/fact/Chart.js' => '52e3ff03', 'rsrc/js/application/fact/ChartCurtainView.js' => '86954222', 'rsrc/js/application/fact/ChartFunctionLabel.js' => '81de1dab', 'rsrc/js/application/files/behavior-document-engine.js' => '243d6c22', @@ -699,7 +699,7 @@ return array( 'javelin-behavior-user-menu' => '60cd9241', 'javelin-behavior-view-placeholder' => 'a9942052', 'javelin-behavior-workflow' => '9623adc1', - 'javelin-chart' => 'ddb9dd1f', + 'javelin-chart' => '52e3ff03', 'javelin-chart-curtain-view' => '86954222', 'javelin-chart-function-label' => '81de1dab', 'javelin-color' => '78f811c9', @@ -1369,6 +1369,12 @@ return array( 'javelin-dom', 'javelin-fx', ), + '52e3ff03' => array( + 'phui-chart-css', + 'd3', + 'javelin-chart-curtain-view', + 'javelin-chart-function-label', + ), '541f81c3' => array( 'javelin-install', ), @@ -2066,12 +2072,6 @@ return array( 'javelin-uri', 'phabricator-notification', ), - 'ddb9dd1f' => array( - 'phui-chart-css', - 'd3', - 'javelin-chart-curtain-view', - 'javelin-chart-function-label', - ), 'dfa1d313' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 470a6c69fe..2a649cd96e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3104,6 +3104,7 @@ phutil_register_library_map(array( 'PhabricatorDefaultRequestExceptionHandler' => 'aphront/handler/PhabricatorDefaultRequestExceptionHandler.php', 'PhabricatorDefaultSyntaxStyle' => 'infrastructure/syntax/PhabricatorDefaultSyntaxStyle.php', 'PhabricatorDefaultUnlockEngine' => 'applications/system/engine/PhabricatorDefaultUnlockEngine.php', + 'PhabricatorDemoChartEngine' => 'applications/fact/engine/PhabricatorDemoChartEngine.php', 'PhabricatorDestructibleCodex' => 'applications/system/codex/PhabricatorDestructibleCodex.php', 'PhabricatorDestructibleCodexInterface' => 'applications/system/interface/PhabricatorDestructibleCodexInterface.php', 'PhabricatorDestructibleInterface' => 'applications/system/interface/PhabricatorDestructibleInterface.php', @@ -9434,6 +9435,7 @@ phutil_register_library_map(array( 'PhabricatorDefaultRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler', 'PhabricatorDefaultSyntaxStyle' => 'PhabricatorSyntaxStyle', 'PhabricatorDefaultUnlockEngine' => 'PhabricatorUnlockEngine', + 'PhabricatorDemoChartEngine' => 'PhabricatorChartEngine', 'PhabricatorDestructibleCodex' => 'Phobject', 'PhabricatorDestructionEngine' => 'Phobject', 'PhabricatorDestructionEngineExtension' => 'Phobject', diff --git a/src/applications/fact/controller/PhabricatorFactChartController.php b/src/applications/fact/controller/PhabricatorFactChartController.php index 1c9e1dd32e..dc8c436dfe 100644 --- a/src/applications/fact/controller/PhabricatorFactChartController.php +++ b/src/applications/fact/controller/PhabricatorFactChartController.php @@ -62,50 +62,9 @@ final class PhabricatorFactChartController extends PhabricatorFactController { private function newDemoChart() { $viewer = $this->getViewer(); - $argvs = array(); - - $argvs[] = array('fact', 'tasks.count.create'); - - $argvs[] = array('constant', 360); - - $argvs[] = array('fact', 'tasks.open-count.create'); - - $argvs[] = array( - 'sum', - array( - 'accumulate', - array('fact', 'tasks.count.create'), - ), - array( - 'accumulate', - array('fact', 'tasks.open-count.create'), - ), - ); - - $argvs[] = array( - 'compose', - array('scale', 0.001), - array('cos'), - array('scale', 100), - array('shift', 800), - ); - - $datasets = array(); - foreach ($argvs as $argv) { - $datasets[] = PhabricatorChartDataset::newFromDictionary( - array( - 'function' => $argv, - )); - } - - $chart = id(new PhabricatorFactChart()) - ->setDatasets($datasets); - - $engine = id(new PhabricatorChartRenderingEngine()) + $chart = id(new PhabricatorDemoChartEngine()) ->setViewer($viewer) - ->setChart($chart); - - $chart = $engine->getStoredChart(); + ->newStoredChart(); return id(new AphrontRedirectResponse())->setURI($chart->getURI()); } diff --git a/src/applications/fact/engine/PhabricatorChartEngine.php b/src/applications/fact/engine/PhabricatorChartEngine.php index f723633d6a..918817d475 100644 --- a/src/applications/fact/engine/PhabricatorChartEngine.php +++ b/src/applications/fact/engine/PhabricatorChartEngine.php @@ -63,7 +63,7 @@ abstract class PhabricatorChartEngine abstract protected function newChart(PhabricatorFactChart $chart, array $map); - final public function buildChartPanel() { + final public function newStoredChart() { $viewer = $this->getViewer(); $parameters = $this->getEngineParameters(); @@ -76,7 +76,11 @@ abstract class PhabricatorChartEngine ->setViewer($viewer) ->setChart($chart); - $chart = $rendering_engine->getStoredChart(); + return $rendering_engine->getStoredChart(); + } + + final public function buildChartPanel() { + $chart = $this->newStoredChart(); $panel_type = id(new PhabricatorDashboardChartPanelType()) ->getPanelTypeKey(); @@ -91,7 +95,7 @@ abstract class PhabricatorChartEngine final protected function newFunction($name /* , ... */) { $argv = func_get_args(); return id(new PhabricatorComposeChartFunction()) - ->setArguments(array($argv)); + ->setArguments($argv); } } diff --git a/src/applications/fact/engine/PhabricatorChartRenderingEngine.php b/src/applications/fact/engine/PhabricatorChartRenderingEngine.php index c691fe1abf..bf487f0052 100644 --- a/src/applications/fact/engine/PhabricatorChartRenderingEngine.php +++ b/src/applications/fact/engine/PhabricatorChartRenderingEngine.php @@ -178,6 +178,9 @@ final class PhabricatorChartRenderingEngine $rows[] = array( $xv, $yv, + null, + null, + null, ); } else { foreach ($point_refs as $ref => $ref_data) { diff --git a/src/applications/fact/engine/PhabricatorDemoChartEngine.php b/src/applications/fact/engine/PhabricatorDemoChartEngine.php new file mode 100644 index 0000000000..71fec03309 --- /dev/null +++ b/src/applications/fact/engine/PhabricatorDemoChartEngine.php @@ -0,0 +1,44 @@ +getViewer(); + + $functions = array(); + + $function = $this->newFunction( + array('scale', 0.0001), + array('cos'), + array('scale', 128), + array('shift', 256)); + + $function->getFunctionLabel() + ->setName(pht('cos(x)')) + ->setColor('rgba(0, 200, 0, 1)') + ->setFillColor('rgba(0, 200, 0, 0.15)'); + + $functions[] = $function; + + $function = $this->newFunction( + array('constant', 345)); + + $function->getFunctionLabel() + ->setName(pht('constant(345)')) + ->setColor('rgba(0, 0, 200, 1)') + ->setFillColor('rgba(0, 0, 200, 0.15)'); + + $functions[] = $function; + + $datasets = array(); + + $datasets[] = id(new PhabricatorChartStackedAreaDataset()) + ->setFunctions($functions); + + $chart->attachDatasets($datasets); + } + +} diff --git a/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php b/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php index e5b360837a..2dbb2a6a16 100644 --- a/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php +++ b/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php @@ -34,62 +34,24 @@ final class PhabricatorProjectBurndownChartEngine $function = $this->newFunction( array( 'accumulate', - array('fact', 'tasks.open-count.assign.project', $project_phid), - ), - array( - 'min', - 0, + array( + 'compose', + array('fact', 'tasks.open-count.assign.project', $project_phid), + array('min', 0), + ), )); $function->getFunctionLabel() ->setName(pht('Tasks Moved Into Project')) - ->setColor('rgba(0, 200, 200, 1)') - ->setFillColor('rgba(0, 200, 200, 0.15)'); + ->setColor('rgba(128, 128, 200, 1)') + ->setFillColor('rgba(128, 128, 200, 0.15)'); $functions[] = $function; $function = $this->newFunction( - array( - 'accumulate', - array('fact', 'tasks.open-count.status.project', $project_phid), - ), - array( - 'min', - 0, - )); - - $function->getFunctionLabel() - ->setName(pht('Tasks Reopened')) - ->setColor('rgba(200, 0, 200, 1)') - ->setFillColor('rgba(200, 0, 200, 0.15)'); - - $functions[] = $function; - - $function = $this->newFunction( - 'sum', array( 'accumulate', array('fact', 'tasks.open-count.create.project', $project_phid), - ), - array( - array( - 'accumulate', - array('fact', 'tasks.open-count.status.project', $project_phid), - ), - array( - 'max', - 0, - ), - ), - array( - array( - 'accumulate', - array('fact', 'tasks.open-count.assign.project', $project_phid), - ), - array( - 'max', - 0, - ), )); $function->getFunctionLabel() @@ -98,27 +60,61 @@ final class PhabricatorProjectBurndownChartEngine ->setFillColor('rgba(0, 0, 200, 0.15)'); $functions[] = $function; + + $function = $this->newFunction( + array( + 'accumulate', + array( + 'compose', + array('fact', 'tasks.open-count.assign.project', $project_phid), + array('max', 0), + ), + )); + + $function->getFunctionLabel() + ->setName(pht('Tasks Moved Out of Project')) + ->setColor('rgba(128, 200, 128, 1)') + ->setFillColor('rgba(128, 200, 128, 0.15)'); + + $functions[] = $function; + + $function = $this->newFunction( + array( + 'accumulate', + array('fact', 'tasks.open-count.status.project', $project_phid), + )); + + $function->getFunctionLabel() + ->setName(pht('Tasks Closed')) + ->setColor('rgba(0, 200, 0, 1)') + ->setFillColor('rgba(0, 200, 0, 0.15)'); + + $functions[] = $function; } } else { $function = $this->newFunction( - 'accumulate', - array('fact', 'tasks.open-count.create')); + array( + 'accumulate', + array('fact', 'tasks.open-count.create'), + )); $function->getFunctionLabel() ->setName(pht('Tasks Created')) - ->setColor('rgba(0, 200, 200, 1)') - ->setFillColor('rgba(0, 200, 200, 0.15)'); + ->setColor('rgba(0, 0, 200, 1)') + ->setFillColor('rgba(0, 0, 200, 0.15)'); $functions[] = $function; $function = $this->newFunction( - 'accumulate', - array('fact', 'tasks.open-count.status')); + array( + 'accumulate', + array('fact', 'tasks.open-count.status'), + )); $function->getFunctionLabel() - ->setName(pht('Tasks Closed / Reopened')) - ->setColor('rgba(200, 0, 200, 1)') - ->setFillColor('rgba(200, 0, 200, 0.15)'); + ->setName(pht('Tasks Closed')) + ->setColor('rgba(0, 200, 0, 1)') + ->setFillColor('rgba(0, 200, 0, 0.15)'); $functions[] = $function; } diff --git a/webroot/rsrc/js/application/fact/Chart.js b/webroot/rsrc/js/application/fact/Chart.js index a57e5459da..473feedcea 100644 --- a/webroot/rsrc/js/application/fact/Chart.js +++ b/webroot/rsrc/js/application/fact/Chart.js @@ -139,7 +139,20 @@ JX.install('Chart', { var area = d3.area() .x(function(d) { return x(to_date(d.x)); }) - .y0(function(d) { return y(d.y0); }) + .y0(function(d) { + // When the area is positive, draw it above the X axis. When the area + // is negative, draw it below the X axis. We currently avoid having + // functions which cross the X axis by clever construction. + if (d.y0 >= 0 && d.y1 >= 0) { + return y(d.y0); + } + + if (d.y0 <= 0 && d.y1 <= 0) { + return y(d.y0); + } + + return y(0); + }) .y1(function(d) { return y(d.y1); }); var line = d3.line() From c06dd4818b53e1bf6f4872c00d7e643beff57178 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 17 Sep 2019 12:04:44 -0700 Subject: [PATCH 43/44] Support explicit stacking configuration in stacked area charts Summary: Ref T13279. Allow engines to choose how areas in a stacked area chart stack on top of one another. This could also be accomplished by using multiple stacked area datasets, but datasets would still need to know if they're stacking "up" or "down" so it's probably about the same at the end of the day. Test Plan: {F6865165} Subscribers: yelirekim Maniphest Tasks: T13279 Differential Revision: https://secure.phabricator.com/D20818 --- .../fact/chart/PhabricatorChartDataset.php | 7 - .../fact/chart/PhabricatorChartFunction.php | 11 +- .../chart/PhabricatorChartFunctionLabel.php | 11 + .../PhabricatorChartStackedAreaDataset.php | 284 +++++++++++------- .../PhabricatorProjectBurndownChartEngine.php | 68 ++++- 5 files changed, 247 insertions(+), 134 deletions(-) diff --git a/src/applications/fact/chart/PhabricatorChartDataset.php b/src/applications/fact/chart/PhabricatorChartDataset.php index a794834229..093a742077 100644 --- a/src/applications/fact/chart/PhabricatorChartDataset.php +++ b/src/applications/fact/chart/PhabricatorChartDataset.php @@ -59,13 +59,6 @@ abstract class PhabricatorChartDataset return $dataset; } - final public function toDictionary() { - return array( - 'type' => $this->getDatasetTypeKey(), - 'functions' => mpull($this->getFunctions(), 'toDictionary'), - ); - } - final public function getChartDisplayData( PhabricatorChartDataQuery $data_query) { return $this->newChartDisplayData($data_query); diff --git a/src/applications/fact/chart/PhabricatorChartFunction.php b/src/applications/fact/chart/PhabricatorChartFunction.php index ef5c5641bf..3ddcd6aec0 100644 --- a/src/applications/fact/chart/PhabricatorChartFunction.php +++ b/src/applications/fact/chart/PhabricatorChartFunction.php @@ -60,6 +60,10 @@ abstract class PhabricatorChartFunction return $this->functionLabel; } + final public function getKey() { + return $this->getFunctionLabel()->getKey(); + } + final public static function newFromDictionary(array $map) { PhutilTypeSpec::checkMap( $map, @@ -86,13 +90,6 @@ abstract class PhabricatorChartFunction return $function; } - public function toDictionary() { - return array( - 'function' => $this->getFunctionKey(), - 'arguments' => $this->getArgumentParser()->getRawArguments(), - ); - } - public function getSubfunctions() { $result = array(); $result[] = $this; diff --git a/src/applications/fact/chart/PhabricatorChartFunctionLabel.php b/src/applications/fact/chart/PhabricatorChartFunctionLabel.php index ad85c49b71..fa3f65aa67 100644 --- a/src/applications/fact/chart/PhabricatorChartFunctionLabel.php +++ b/src/applications/fact/chart/PhabricatorChartFunctionLabel.php @@ -3,11 +3,21 @@ final class PhabricatorChartFunctionLabel extends Phobject { + private $key; private $name; private $color; private $icon; private $fillColor; + public function setKey($key) { + $this->key = $key; + return $this; + } + + public function getKey() { + return $this->key; + } + public function setName($name) { $this->name = $name; return $this; @@ -46,6 +56,7 @@ final class PhabricatorChartFunctionLabel public function toWireFormat() { return array( + 'key' => $this->getKey(), 'name' => $this->getName(), 'color' => $this->getColor(), 'icon' => $this->getIcon(), diff --git a/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php b/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php index 4d30cd767b..2ba08ea1c9 100644 --- a/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php +++ b/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php @@ -5,124 +5,89 @@ final class PhabricatorChartStackedAreaDataset const DATASETKEY = 'stacked-area'; + private $stacks; + + public function setStacks(array $stacks) { + $this->stacks = $stacks; + return $this; + } + + public function getStacks() { + return $this->stacks; + } + protected function newChartDisplayData( PhabricatorChartDataQuery $data_query) { + $functions = $this->getFunctions(); + $functions = mpull($functions, null, 'getKey'); - $reversed_functions = array_reverse($functions, true); + $stacks = $this->getStacks(); - $function_points = array(); - foreach ($reversed_functions as $function_idx => $function) { - $function_points[$function_idx] = array(); - - $datapoints = $function->newDatapoints($data_query); - foreach ($datapoints as $point) { - $x_value = $point['x']; - $function_points[$function_idx][$x_value] = $point; - } + if (!$stacks) { + $stacks = array( + array_reverse(array_keys($functions), true), + ); } - $raw_points = $function_points; + $series = array(); + $raw_points = array(); - // We need to define every function we're drawing at every point where - // any of the functions we're drawing are defined. If we don't, we'll - // end up with weird gaps or overlaps between adjacent areas, and won't - // know how much we need to lift each point above the baseline when - // stacking the functions on top of one another. + foreach ($stacks as $stack) { + $stack_functions = array_select_keys($functions, $stack); - $must_define = array(); - foreach ($function_points as $function_idx => $points) { - foreach ($points as $x => $point) { - $must_define[$x] = $x; + $function_points = $this->getFunctionDatapoints( + $data_query, + $stack_functions); + + $stack_points = $function_points; + + $function_points = $this->getGeometry( + $data_query, + $function_points); + + $baseline = array(); + foreach ($function_points as $function_idx => $points) { + $bounds = array(); + foreach ($points as $x => $point) { + if (!isset($baseline[$x])) { + $baseline[$x] = 0; + } + + $y0 = $baseline[$x]; + $baseline[$x] += $point['y']; + $y1 = $baseline[$x]; + + $bounds[] = array( + 'x' => $x, + 'y0' => $y0, + 'y1' => $y1, + ); + + if (isset($stack_points[$function_idx][$x])) { + $stack_points[$function_idx][$x]['y1'] = $y1; + } + } + + $series[$function_idx] = $bounds; } + + $raw_points += $stack_points; } - ksort($must_define); - foreach ($reversed_functions as $function_idx => $function) { - $missing = array(); - foreach ($must_define as $x) { - if (!isset($function_points[$function_idx][$x])) { - $missing[$x] = true; - } - } + $series = array_select_keys($series, array_keys($functions)); + $series = array_values($series); - if (!$missing) { - continue; - } - - $points = $function_points[$function_idx]; - - $values = array_keys($points); - $cursor = -1; - $length = count($values); - - foreach ($missing as $x => $ignored) { - // Move the cursor forward until we find the last point before "x" - // which is defined. - while ($cursor + 1 < $length && $values[$cursor + 1] < $x) { - $cursor++; - } - - // If this new point is to the left of all defined points, we'll - // assume the value is 0. If the point is to the right of all defined - // points, we assume the value is the same as the last known value. - - // If it's between two defined points, we average them. - - if ($cursor < 0) { - $y = 0; - } else if ($cursor + 1 < $length) { - $xmin = $values[$cursor]; - $xmax = $values[$cursor + 1]; - - $ymin = $points[$xmin]['y']; - $ymax = $points[$xmax]['y']; - - // Fill in the missing point by creating a linear interpolation - // between the two adjacent points. - $distance = ($x - $xmin) / ($xmax - $xmin); - $y = $ymin + (($ymax - $ymin) * $distance); - } else { - $xmin = $values[$cursor]; - $y = $function_points[$function_idx][$xmin]['y']; - } - - $function_points[$function_idx][$x] = array( - 'x' => $x, - 'y' => $y, - ); - } - - ksort($function_points[$function_idx]); - } + $raw_points = array_select_keys($raw_points, array_keys($functions)); + $raw_points = array_values($raw_points); $range_min = null; $range_max = null; - $series = array(); - $baseline = array(); - foreach ($function_points as $function_idx => $points) { - $below = idx($function_points, $function_idx - 1); - - $bounds = array(); - foreach ($points as $x => $point) { - if (!isset($baseline[$x])) { - $baseline[$x] = 0; - } - - $y0 = $baseline[$x]; - $baseline[$x] += $point['y']; - $y1 = $baseline[$x]; - - $bounds[] = array( - 'x' => $x, - 'y0' => $y0, - 'y1' => $y1, - ); - - if (isset($raw_points[$function_idx][$x])) { - $raw_points[$function_idx][$x]['y1'] = $y1; - } + foreach ($series as $geometry_list) { + foreach ($geometry_list as $geometry_item) { + $y0 = $geometry_item['y0']; + $y1 = $geometry_item['y1']; if ($range_min === null) { $range_min = $y0; @@ -134,12 +99,8 @@ final class PhabricatorChartStackedAreaDataset } $range_max = max($range_max, $y0, $y1); } - - $series[] = $bounds; } - $series = array_reverse($series); - // We're going to group multiple events into a single point if they have // X values that are very close to one another. // @@ -222,5 +183,118 @@ final class PhabricatorChartStackedAreaDataset ->setRange(new PhabricatorChartInterval($range_min, $range_max)); } + private function getAllXValuesAsMap( + PhabricatorChartDataQuery $data_query, + array $point_lists) { + + // We need to define every function we're drawing at every point where + // any of the functions we're drawing are defined. If we don't, we'll + // end up with weird gaps or overlaps between adjacent areas, and won't + // know how much we need to lift each point above the baseline when + // stacking the functions on top of one another. + + $must_define = array(); + + $min = $data_query->getMinimumValue(); + $max = $data_query->getMaximumValue(); + $must_define[$max] = $max; + $must_define[$min] = $min; + + foreach ($point_lists as $point_list) { + foreach ($point_list as $x => $point) { + $must_define[$x] = $x; + } + } + + ksort($must_define); + + return $must_define; + } + + private function getFunctionDatapoints( + PhabricatorChartDataQuery $data_query, + array $functions) { + + assert_instances_of($functions, 'PhabricatorChartFunction'); + + $points = array(); + foreach ($functions as $idx => $function) { + $points[$idx] = array(); + + $datapoints = $function->newDatapoints($data_query); + foreach ($datapoints as $point) { + $x_value = $point['x']; + $points[$idx][$x_value] = $point; + } + } + + return $points; + } + + private function getGeometry( + PhabricatorChartDataQuery $data_query, + array $point_lists) { + + $must_define = $this->getAllXValuesAsMap($data_query, $point_lists); + + foreach ($point_lists as $idx => $points) { + + $missing = array(); + foreach ($must_define as $x) { + if (!isset($points[$x])) { + $missing[$x] = true; + } + } + + if (!$missing) { + continue; + } + + $values = array_keys($points); + $cursor = -1; + $length = count($values); + + foreach ($missing as $x => $ignored) { + // Move the cursor forward until we find the last point before "x" + // which is defined. + while ($cursor + 1 < $length && $values[$cursor + 1] < $x) { + $cursor++; + } + + // If this new point is to the left of all defined points, we'll + // assume the value is 0. If the point is to the right of all defined + // points, we assume the value is the same as the last known value. + + // If it's between two defined points, we average them. + + if ($cursor < 0) { + $y = 0; + } else if ($cursor + 1 < $length) { + $xmin = $values[$cursor]; + $xmax = $values[$cursor + 1]; + + $ymin = $points[$xmin]['y']; + $ymax = $points[$xmax]['y']; + + // Fill in the missing point by creating a linear interpolation + // between the two adjacent points. + $distance = ($x - $xmin) / ($xmax - $xmin); + $y = $ymin + (($ymax - $ymin) * $distance); + } else { + $xmin = $values[$cursor]; + $y = $points[$xmin]['y']; + } + + $point_lists[$idx][$x] = array( + 'x' => $x, + 'y' => $y, + ); + } + + ksort($point_lists[$idx]); + } + + return $point_lists; + } } diff --git a/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php b/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php index 2dbb2a6a16..8dda7cde16 100644 --- a/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php +++ b/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php @@ -29,6 +29,8 @@ final class PhabricatorProjectBurndownChartEngine } $functions = array(); + $stacks = array(); + if ($project_phids) { foreach ($project_phids as $project_phid) { $function = $this->newFunction( @@ -42,12 +44,31 @@ final class PhabricatorProjectBurndownChartEngine )); $function->getFunctionLabel() + ->setKey('moved-in') ->setName(pht('Tasks Moved Into Project')) ->setColor('rgba(128, 128, 200, 1)') ->setFillColor('rgba(128, 128, 200, 0.15)'); $functions[] = $function; + $function = $this->newFunction( + array( + 'accumulate', + array( + 'compose', + array('fact', 'tasks.open-count.status.project', $project_phid), + array('min', 0), + ), + )); + + $function->getFunctionLabel() + ->setKey('reopened') + ->setName(pht('Tasks Reopened')) + ->setColor('rgba(128, 128, 200, 1)') + ->setFillColor('rgba(128, 128, 200, 0.15)'); + + $functions[] = $function; + $function = $this->newFunction( array( 'accumulate', @@ -55,12 +76,31 @@ final class PhabricatorProjectBurndownChartEngine )); $function->getFunctionLabel() + ->setKey('created') ->setName(pht('Tasks Created')) ->setColor('rgba(0, 0, 200, 1)') ->setFillColor('rgba(0, 0, 200, 0.15)'); $functions[] = $function; + $function = $this->newFunction( + array( + 'accumulate', + array( + 'compose', + array('fact', 'tasks.open-count.status.project', $project_phid), + array('max', 0), + ), + )); + + $function->getFunctionLabel() + ->setKey('closed') + ->setName(pht('Tasks Closed')) + ->setColor('rgba(0, 200, 0, 1)') + ->setFillColor('rgba(0, 200, 0, 0.15)'); + + $functions[] = $function; + $function = $this->newFunction( array( 'accumulate', @@ -72,24 +112,15 @@ final class PhabricatorProjectBurndownChartEngine )); $function->getFunctionLabel() + ->setKey('moved-out') ->setName(pht('Tasks Moved Out of Project')) ->setColor('rgba(128, 200, 128, 1)') ->setFillColor('rgba(128, 200, 128, 0.15)'); $functions[] = $function; - $function = $this->newFunction( - array( - 'accumulate', - array('fact', 'tasks.open-count.status.project', $project_phid), - )); - - $function->getFunctionLabel() - ->setName(pht('Tasks Closed')) - ->setColor('rgba(0, 200, 0, 1)') - ->setFillColor('rgba(0, 200, 0, 0.15)'); - - $functions[] = $function; + $stacks[] = array('created', 'reopened', 'moved-in'); + $stacks[] = array('closed', 'moved-out'); } } else { $function = $this->newFunction( @@ -99,7 +130,8 @@ final class PhabricatorProjectBurndownChartEngine )); $function->getFunctionLabel() - ->setName(pht('Tasks Created')) + ->setKey('open') + ->setName(pht('Open Tasks')) ->setColor('rgba(0, 0, 200, 1)') ->setFillColor('rgba(0, 0, 200, 0.15)'); @@ -112,7 +144,8 @@ final class PhabricatorProjectBurndownChartEngine )); $function->getFunctionLabel() - ->setName(pht('Tasks Closed')) + ->setKey('closed') + ->setName(pht('Closed Tasks')) ->setColor('rgba(0, 200, 0, 1)') ->setFillColor('rgba(0, 200, 0, 0.15)'); @@ -121,9 +154,14 @@ final class PhabricatorProjectBurndownChartEngine $datasets = array(); - $datasets[] = id(new PhabricatorChartStackedAreaDataset()) + $dataset = id(new PhabricatorChartStackedAreaDataset()) ->setFunctions($functions); + if ($stacks) { + $dataset->setStacks($stacks); + } + + $datasets[] = $dataset; $chart->attachDatasets($datasets); } From 16de9151c7c163e2651820ed6c893f09b7e9282b Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 17 Sep 2019 12:54:29 -0700 Subject: [PATCH 44/44] Give "Burndown" charts a more straightforward definition and move all the event stuff into "Activity" charts Summary: Depends on D20818. Ref T13279. The behavior of the "burndown" chart has wandered fairly far afield; make it look more like a burndown. Move the other thing into an "Activity" chart. Test Plan: {F6865207} Maniphest Tasks: T13279 Differential Revision: https://secure.phabricator.com/D20819 --- src/__phutil_library_map__.php | 2 + .../PhabricatorProjectActivityChartEngine.php | 135 ++++++++++++++ .../PhabricatorProjectBurndownChartEngine.php | 165 ++++++------------ .../PhabricatorProjectReportsController.php | 14 ++ 4 files changed, 203 insertions(+), 113 deletions(-) create mode 100644 src/applications/project/chart/PhabricatorProjectActivityChartEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2a649cd96e..f062b577c3 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4224,6 +4224,7 @@ phutil_register_library_map(array( 'PhabricatorProfileMenuItemView' => 'applications/search/engine/PhabricatorProfileMenuItemView.php', 'PhabricatorProfileMenuItemViewList' => 'applications/search/engine/PhabricatorProfileMenuItemViewList.php', 'PhabricatorProject' => 'applications/project/storage/PhabricatorProject.php', + 'PhabricatorProjectActivityChartEngine' => 'applications/project/chart/PhabricatorProjectActivityChartEngine.php', 'PhabricatorProjectAddHeraldAction' => 'applications/project/herald/PhabricatorProjectAddHeraldAction.php', 'PhabricatorProjectApplication' => 'applications/project/application/PhabricatorProjectApplication.php', 'PhabricatorProjectArchiveController' => 'applications/project/controller/PhabricatorProjectArchiveController.php', @@ -10733,6 +10734,7 @@ phutil_register_library_map(array( 'PhabricatorSpacesInterface', 'PhabricatorEditEngineSubtypeInterface', ), + 'PhabricatorProjectActivityChartEngine' => 'PhabricatorChartEngine', 'PhabricatorProjectAddHeraldAction' => 'PhabricatorProjectHeraldAction', 'PhabricatorProjectApplication' => 'PhabricatorApplication', 'PhabricatorProjectArchiveController' => 'PhabricatorProjectController', diff --git a/src/applications/project/chart/PhabricatorProjectActivityChartEngine.php b/src/applications/project/chart/PhabricatorProjectActivityChartEngine.php new file mode 100644 index 0000000000..7fc599317f --- /dev/null +++ b/src/applications/project/chart/PhabricatorProjectActivityChartEngine.php @@ -0,0 +1,135 @@ +setEngineParameter('projectPHIDs', $project_phids); + } + + protected function newChart(PhabricatorFactChart $chart, array $map) { + $viewer = $this->getViewer(); + + $map = $map + array( + 'projectPHIDs' => array(), + ); + + if ($map['projectPHIDs']) { + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($viewer) + ->withPHIDs($map['projectPHIDs']) + ->execute(); + $project_phids = mpull($projects, 'getPHID'); + } else { + $project_phids = array(); + } + + $project_phid = head($project_phids); + + $functions = array(); + $stacks = array(); + + $function = $this->newFunction( + array( + 'accumulate', + array( + 'compose', + array('fact', 'tasks.open-count.assign.project', $project_phid), + array('min', 0), + ), + )); + + $function->getFunctionLabel() + ->setKey('moved-in') + ->setName(pht('Tasks Moved Into Project')) + ->setColor('rgba(128, 128, 200, 1)') + ->setFillColor('rgba(128, 128, 200, 0.15)'); + + $functions[] = $function; + + $function = $this->newFunction( + array( + 'accumulate', + array( + 'compose', + array('fact', 'tasks.open-count.status.project', $project_phid), + array('min', 0), + ), + )); + + $function->getFunctionLabel() + ->setKey('reopened') + ->setName(pht('Tasks Reopened')) + ->setColor('rgba(128, 128, 200, 1)') + ->setFillColor('rgba(128, 128, 200, 0.15)'); + + $functions[] = $function; + + $function = $this->newFunction( + array( + 'accumulate', + array('fact', 'tasks.open-count.create.project', $project_phid), + )); + + $function->getFunctionLabel() + ->setKey('created') + ->setName(pht('Tasks Created')) + ->setColor('rgba(0, 0, 200, 1)') + ->setFillColor('rgba(0, 0, 200, 0.15)'); + + $functions[] = $function; + + $function = $this->newFunction( + array( + 'accumulate', + array( + 'compose', + array('fact', 'tasks.open-count.status.project', $project_phid), + array('max', 0), + ), + )); + + $function->getFunctionLabel() + ->setKey('closed') + ->setName(pht('Tasks Closed')) + ->setColor('rgba(0, 200, 0, 1)') + ->setFillColor('rgba(0, 200, 0, 0.15)'); + + $functions[] = $function; + + $function = $this->newFunction( + array( + 'accumulate', + array( + 'compose', + array('fact', 'tasks.open-count.assign.project', $project_phid), + array('max', 0), + ), + )); + + $function->getFunctionLabel() + ->setKey('moved-out') + ->setName(pht('Tasks Moved Out of Project')) + ->setColor('rgba(128, 200, 128, 1)') + ->setFillColor('rgba(128, 200, 128, 0.15)'); + + $functions[] = $function; + + $stacks[] = array('created', 'reopened', 'moved-in'); + $stacks[] = array('closed', 'moved-out'); + + $datasets = array(); + + $dataset = id(new PhabricatorChartStackedAreaDataset()) + ->setFunctions($functions) + ->setStacks($stacks); + + $datasets[] = $dataset; + $chart->attachDatasets($datasets); + } + +} diff --git a/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php b/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php index 8dda7cde16..1296f2eec8 100644 --- a/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php +++ b/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php @@ -29,140 +29,79 @@ final class PhabricatorProjectBurndownChartEngine } $functions = array(); - $stacks = array(); - if ($project_phids) { - foreach ($project_phids as $project_phid) { - $function = $this->newFunction( + $open_function = $this->newFunction( + array( + 'accumulate', array( - 'accumulate', - array( - 'compose', - array('fact', 'tasks.open-count.assign.project', $project_phid), - array('min', 0), - ), - )); + 'sum', + $this->newFactSum( + 'tasks.open-count.create.project', $project_phids), + $this->newFactSum( + 'tasks.open-count.status.project', $project_phids), + $this->newFactSum( + 'tasks.open-count.assign.project', $project_phids), + ), + )); - $function->getFunctionLabel() - ->setKey('moved-in') - ->setName(pht('Tasks Moved Into Project')) - ->setColor('rgba(128, 128, 200, 1)') - ->setFillColor('rgba(128, 128, 200, 0.15)'); - - $functions[] = $function; - - $function = $this->newFunction( - array( - 'accumulate', - array( - 'compose', - array('fact', 'tasks.open-count.status.project', $project_phid), - array('min', 0), - ), - )); - - $function->getFunctionLabel() - ->setKey('reopened') - ->setName(pht('Tasks Reopened')) - ->setColor('rgba(128, 128, 200, 1)') - ->setFillColor('rgba(128, 128, 200, 0.15)'); - - $functions[] = $function; - - $function = $this->newFunction( - array( - 'accumulate', - array('fact', 'tasks.open-count.create.project', $project_phid), - )); - - $function->getFunctionLabel() - ->setKey('created') - ->setName(pht('Tasks Created')) - ->setColor('rgba(0, 0, 200, 1)') - ->setFillColor('rgba(0, 0, 200, 0.15)'); - - $functions[] = $function; - - $function = $this->newFunction( - array( - 'accumulate', - array( - 'compose', - array('fact', 'tasks.open-count.status.project', $project_phid), - array('max', 0), - ), - )); - - $function->getFunctionLabel() - ->setKey('closed') - ->setName(pht('Tasks Closed')) - ->setColor('rgba(0, 200, 0, 1)') - ->setFillColor('rgba(0, 200, 0, 0.15)'); - - $functions[] = $function; - - $function = $this->newFunction( - array( - 'accumulate', - array( - 'compose', - array('fact', 'tasks.open-count.assign.project', $project_phid), - array('max', 0), - ), - )); - - $function->getFunctionLabel() - ->setKey('moved-out') - ->setName(pht('Tasks Moved Out of Project')) - ->setColor('rgba(128, 200, 128, 1)') - ->setFillColor('rgba(128, 200, 128, 0.15)'); - - $functions[] = $function; - - $stacks[] = array('created', 'reopened', 'moved-in'); - $stacks[] = array('closed', 'moved-out'); - } + $closed_function = $this->newFunction( + array( + 'accumulate', + $this->newFactSum('tasks.open-count.status.project', $project_phids), + )); } else { - $function = $this->newFunction( + $open_function = $this->newFunction( array( 'accumulate', array('fact', 'tasks.open-count.create'), )); - $function->getFunctionLabel() - ->setKey('open') - ->setName(pht('Open Tasks')) - ->setColor('rgba(0, 0, 200, 1)') - ->setFillColor('rgba(0, 0, 200, 0.15)'); - - $functions[] = $function; - - $function = $this->newFunction( + $closed_function = $this->newFunction( array( 'accumulate', array('fact', 'tasks.open-count.status'), )); - - $function->getFunctionLabel() - ->setKey('closed') - ->setName(pht('Closed Tasks')) - ->setColor('rgba(0, 200, 0, 1)') - ->setFillColor('rgba(0, 200, 0, 0.15)'); - - $functions[] = $function; } + $open_function->getFunctionLabel() + ->setKey('open') + ->setName(pht('Open Tasks')) + ->setColor('rgba(0, 0, 200, 1)') + ->setFillColor('rgba(0, 0, 200, 0.15)'); + + $closed_function->getFunctionLabel() + ->setKey('closed') + ->setName(pht('Closed Tasks')) + ->setColor('rgba(0, 200, 0, 1)') + ->setFillColor('rgba(0, 200, 0, 0.15)'); + $datasets = array(); $dataset = id(new PhabricatorChartStackedAreaDataset()) - ->setFunctions($functions); - - if ($stacks) { - $dataset->setStacks($stacks); - } + ->setFunctions( + array( + $open_function, + $closed_function, + )) + ->setStacks( + array( + array('open'), + array('closed'), + )); $datasets[] = $dataset; $chart->attachDatasets($datasets); } + private function newFactSum($fact_key, array $phids) { + $result = array(); + $result[] = 'sum'; + + foreach ($phids as $phid) { + $result[] = array('fact', $fact_key, $phid); + } + + return $result; + } + } diff --git a/src/applications/project/controller/PhabricatorProjectReportsController.php b/src/applications/project/controller/PhabricatorProjectReportsController.php index bee114917b..cfd9ee3253 100644 --- a/src/applications/project/controller/PhabricatorProjectReportsController.php +++ b/src/applications/project/controller/PhabricatorProjectReportsController.php @@ -44,10 +44,24 @@ final class PhabricatorProjectReportsController ->setParentPanelPHIDs(array()) ->renderPanel(); + $activity_panel = id(new PhabricatorProjectActivityChartEngine()) + ->setViewer($viewer) + ->setProjects(array($project)) + ->buildChartPanel(); + + $activity_panel->setName(pht('%s: Activity', $project->getName())); + + $activity_view = id(new PhabricatorDashboardPanelRenderingEngine()) + ->setViewer($viewer) + ->setPanel($activity_panel) + ->setParentPanelPHIDs(array()) + ->renderPanel(); + $view = id(new PHUITwoColumnView()) ->setFooter( array( $chart_view, + $activity_view, )); return $this->newPage()