1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 00:42:41 +01:00

Remove "days fresh" / "days stale" indictator in Differential revision list

Summary:
Ref T10939. I'm not //totally// opposed to the existence of this element, but I think it's the kind of thing that would never make it upstream today. I think this should just be a T418 custom sort of thing in the long run, not a mainline upstream feature.

Overall, I think this thing is nearly useless and just adds visual clutter. My dashboard is about 100% red. This also sort of teaches users that it's fine to let revisions sit for a couple of days, which isn't what I'd like the UI to teach. Finally, removing it helps the UI feel a little less cluttered after the visually busy changes in D15926.

Test Plan: Grepped for removed config. Viewed revision list.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10939

Differential Revision: https://secure.phabricator.com/D15927
This commit is contained in:
epriestley 2016-05-16 09:43:01 -07:00
parent d46378df20
commit 8a98868bfb
5 changed files with 8 additions and 89 deletions

View file

@ -21,22 +21,4 @@ final class PhabricatorCalendarHoliday extends PhabricatorCalendarDAO {
) + parent::getConfiguration();
}
public static function getNthBusinessDay($epoch, $n) {
// Sadly, there are not many holidays. So we can load all of them.
$holidays = id(new PhabricatorCalendarHoliday())->loadAll();
$holidays = mpull($holidays, null, 'getDay');
$interval = ($n > 0 ? 1 : -1) * 24 * 60 * 60;
$return = $epoch;
for ($i = abs($n); $i > 0; ) {
$return += $interval;
$weekday = date('w', $return);
if ($weekday != 0 && $weekday != 6 && // Sunday and Saturday
!isset($holidays[date('Y-m-d', $return)])) {
$i--;
}
}
return $return;
}
}

View file

@ -16,24 +16,4 @@ final class PhabricatorCalendarHolidayTestCase extends PhabricatorTestCase {
->save();
}
public function testNthBusinessDay() {
$map = array(
array('2011-12-30', 1, '2012-01-03'),
array('2012-01-01', 1, '2012-01-03'),
array('2012-01-01', 0, '2012-01-01'),
array('2012-01-01', -1, '2011-12-30'),
array('2012-01-04', -1, '2012-01-03'),
);
foreach ($map as $val) {
list($date, $n, $expect) = $val;
$actual = PhabricatorCalendarHoliday::getNthBusinessDay(
strtotime($date),
$n);
$this->assertEqual(
$expect,
date('Y-m-d', $actual),
pht("%d business days since '%s'", $n, $date));
}
}
}

View file

@ -186,6 +186,10 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
'Configuration of the notification server has changed substantially. '.
'For discussion, see T10794.');
$stale_reason = pht(
'The Differential revision list view age UI elements have been removed '.
'to simplify the interface.');
$ancient_config += array(
'phid.external-loaders' =>
pht(
@ -314,6 +318,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
'metamta.differential.unified-comment-context' => pht(
'Inline comments are now always rendered with a limited amount '.
'of context.'),
'differential.days-fresh' => $stale_reason,
'differential.days-stale' => $stale_reason,
);
return $ancient_config;

View file

@ -229,25 +229,6 @@ final class PhabricatorDifferentialConfigOptions
"\n\n".
'This sort of workflow is very unusual. Very few installs should '.
'need to change this option.')),
$this->newOption('differential.days-fresh', 'int', 1)
->setSummary(
pht(
"For how many business days should a revision be considered ".
"'fresh'?"))
->setDescription(
pht(
'Revisions newer than this number of days are marked as fresh in '.
'Action Required and Revisions Waiting on You views. Only work '.
'days (not weekends and holidays) are included. Set to 0 to '.
'disable this feature.')),
$this->newOption('differential.days-stale', 'int', 3)
->setSummary(
pht("After this many days, a revision will be considered 'stale'."))
->setDescription(
pht(
"Similar to `%s` but marks stale revisions. ".
"If the revision is even older than it is when marked as 'old'.",
'differential.days-fresh')),
$this->newOption(
'metamta.differential.subject-prefix',
'string',

View file

@ -65,20 +65,6 @@ final class DifferentialRevisionListView extends AphrontView {
public function render() {
$viewer = $this->getViewer();
$fresh = PhabricatorEnv::getEnvConfig('differential.days-fresh');
if ($fresh) {
$fresh = PhabricatorCalendarHoliday::getNthBusinessDay(
time(),
-$fresh);
}
$stale = PhabricatorEnv::getEnvConfig('differential.days-stale');
if ($stale) {
$stale = PhabricatorCalendarHoliday::getNthBusinessDay(
time(),
-$stale);
}
$this->initBehavior('phabricator-tooltips', array());
$this->requireResource('aphront-tooltip-css');
@ -109,18 +95,6 @@ final class DifferentialRevisionListView extends AphrontView {
$modified = $revision->getDateModified();
$status = $revision->getStatus();
$show_age = ($fresh || $stale) &&
$this->highlightAge &&
!$revision->isClosed();
if ($stale && $modified < $stale) {
$object_age = PHUIObjectItemView::AGE_OLD;
} else if ($fresh && $modified < $fresh) {
$object_age = PHUIObjectItemView::AGE_STALE;
} else {
$object_age = PHUIObjectItemView::AGE_FRESH;
}
$status_name =
ArcanistDifferentialRevisionStatus::getNameForRevisionStatus($status);
@ -143,11 +117,6 @@ final class DifferentialRevisionListView extends AphrontView {
$item->addAttribute($draft);
}
/* Most things 'Need Review', so accept it's the default */
if ($status != ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) {
$item->addAttribute($status_name);
}
// Author
$author_handle = $this->handles[$revision->getAuthorPHID()];
$item->addByline(pht('Author: %s', $author_handle->renderLink()));
@ -164,7 +133,7 @@ final class DifferentialRevisionListView extends AphrontView {
}
$item->addAttribute(pht('Reviewers: %s', $reviewers));
$item->setEpoch($revision->getDateModified(), $object_age);
$item->setEpoch($revision->getDateModified());
switch ($status) {
case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: