Page MenuHomePhabricator

"Resolved" should be the first (default) option in "Change Status" action
Closed, ResolvedPublic

Description

When Wikimedia used Bugzilla, the default option for "RESOLVED" was "FIXED".

Also here in https://secure.phabricator.org, when a users "Change Status" in a comment, the default option is "Resolved".

In contrast, in http://phabricator.wikimedia.org the default option offered is "Declined", which doesn't send a positive message.

This is probably due to the sorting of the options, which is alphabetical. It would be great to be able to configure a different sorting. For instance, we would prefer to have

  • Open
  • Resolved
  • Needs Info
  • Declined
  • Invalid

Original report: https://phabricator.wikimedia.org/T548

Event Timeline

qgil raised the priority of this task from to Needs Triage.
qgil updated the task description. (Show Details)
qgil added a project: Maniphest.
qgil added a subscriber: qgil.
chad claimed this task.
chad added a subscriber: chad.

I think it just goes in the order it's listed in maniphest.statuses

pasted_file (104×180 px, 13 KB)

whoops, didnt mean to close, haha

Yeah, I moved "Spite" on my install to just after "Open", and it's now the default when the field displays.

pasted_file (228×318 px, 13 KB)

Hm. I believe @aklapper and @chasemp found that the sorting of custom statuses was alphabetical regardless of their position in maniphest.statuses. See https://phabricator.wikimedia.org/T548

I updated the documentation. :D

Thanks guys. To give some background we generate the json file via puppet and so a custom ruby function. I bet that function is sorting these by key which means this is probably a local install particular. We are not crazy I am almost mostly sure.

How can it be considered upstream bug exactly? Sounds like an upstream feature request to support ordering by some other method. Or we just fix our puppet setup to somehow force the correct ordering.

I consider it a bug that the configuration relies on the order of a JSON object, while JSON objects are explicitly unordered. http://json.org/

@matmarex has a point here. @epriestley: Would you be receptive to us pushing a patch upstream that adds an ordering property to the status elements?
A simple int property that would serve as a sort field seems most straightforward and intuitive.

tldr;
We are faced with writing some custom hack to output the json in the determined order, due to the fact that we are currently stuck with ruby 1.8 on our puppetmasters.
It seems more productive to spend the time writing a patch for maniphest if it would be acceptable in phabricator upstream rather than writing yet another puppet parser function specialized for this one use that will have to change when we finally migrate to ruby 1.9+ (but, afaik, ruby 1.9 is also nondeterministic with it's json encoding).

@epriestley: as a form of bribery, I offer this random meme I googled for just now. it took a lot of effort, obviously.

https://s-media-cache-ak0.pinimg.com/236x/c4/e6/6b/c4e66bcb215e6300df1af51cb74dc246.jpg

PS: Don't worry, I haven't forgotten that I still owe you guys several of $your_choice_beverage the next time I'm in SF. ;)

I don't want to add an explicit "order" parameter because then it becomes possible to get wrong (e.g., you can specify meaningless orders, specify the same order twice, etc). It has to fall back to object order anyway, or we'd break every existing install. So this would introduce a new option that's way worse for humans which almost no install would ever use.

Although we're technically in the wrong by treating JSON as ordered, I think it's a lot better for humans than an "order" field, and this config is almost always edited by humans.

The real, authoritative config isn't JSON, either -- it's PHP, which does have ordered dictionaries. This is probably not much comfort because I'd guess that Ruby isn't great at writing out PHP data structures either, but I think it leaves us mostly out of the technical grey area.

Is there a reason the deploy process needs to regenerate the JSON every time from scratch? Our deploy process for Phacility has config files checked-in like this (written in PHP, and using PHP to compose different pieces of shared/external configuration):

admin.conf.php
<?php

$core = require_once dirname(__FILE__).'/core.conf.php';
$root = dirname(__FILE__).'/../../';
$keys = require_once $root.'/conf/keystore/phabricator/admin.conf.php';

return $core + $keys + array(
  'load-libraries' => array(
    'core/src/',
    'instances/src/',
    'services/src/',
  ),
  'phabricator.show-prototypes' => true,
  'ui.header-color' => 'indigo',
  'ui.footer-items' => array(
    array(
      'name' => pht('Phacility, Inc'),
      'href' => 'https://www.phacility.com/',
    ),
...

Then the deploy script links them up by symlinking them into phabricator/conf/custom/ and selecting the correct file by writing to phabricator/conf/local/ENVIRONMENT:

CoreHostUpgradeWorkflow.php
...
    foreach ($phconf as $conf) {
      execx(
        'ln -sf %s %s',
        $phconf_dir.'/'.$conf,
        $this->getPath('lib/phabricator/conf/custom/'.$conf));
    }

    execx(
      'echo %s > %s',
      'custom/'.$this->getHostAttribute('role'),
      $this->getPath('lib/phabricator/conf/local/ENVIRONMENT'));
...

There are some specifics in the documentation here:

https://secure.phabricator.com/book/phabricator/article/advanced_configuration/

That might not be terribly helpful if you're regenerating the value because the statuses are dynamic, but maybe it is?

We can avoid all the issues with "order" parameter by using an entire
separate setting like `'maniphest.statuses.order' => ['open', 'resolved',
'declined']`, and indeed falling back to object order. If the set of
values specified in it would not match the set of keys of
maniphest.statuses, it could just be ignored.

Every option other than the status quo is worse for us and human users (more complex, more error prone, requires more code, harder to debug or support, etc). We aren't interested in upstreaming a workaround which benefits installs emitting JSON from Puppet scripts to the detriment of all other installs.

@epriestley: This seems entirely reasonable and as usual you've gone out of your way to be helpful. Thanks! Sorry for beating the dead horse on this one. I will look into doing the config with php instead of yml -> puppet (ruby hash) -> json

Sorry I can't offer more help, I'm sure it's a pain on your end and that was a very nice meme.