Page MenuHomePhabricator
Create Task
Maniphest T548

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

Description

Reported upstream: https://secure.phabricator.com/T7164

Currently in Bugzilla, when you mark a bug "RESOLVED", the default option offered is "FIXED".

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

In contrast, in our Phabricator the default option offered is "Declined", which doesn't send a positive message.

This is probably due to the sorting of the options, which in our case is alphabetical. Proposing this sorting instead:

  1. Open
  2. Resolved
  3. Needs Info
  4. Declined
  5. Invalid

Details

SubjectRepoBranchLines +/-
Move maniphest status settings into custom/wmf-defaults.phpoperations/puppetproduction+75 -39
Preserve order of 'maniphest.statuses' in Phabricator settingsoperations/puppetproduction+171 -2
Customize query in gerrit

Related Objects

Event Timeline

Qgil created this task.Oct 6 2014, 9:18 AM
Qgil raised the priority of this task from to Needs Triage.
Qgil updated the task description. (Show Details)
Qgil added a project: Bugzilla-Migration.
Qgil changed Security from none to None.
Qgil subscribed.
Aklapper subscribed.Oct 6 2014, 11:51 AM

Generally +1 on the request.

"Declined" is the last item in https://git.wikimedia.org/blame/operations%2Fpuppet.git/e97b10890b3d3671c7f3412b2e6e69b2b086fde1/modules%2Fphabricator%2Fdata%2Ffixed_settings.yaml but https://phabricator.wikimedia.org/config/edit/maniphest.statuses/ lists stuff alphabetically.

Not sure if this is a Phabricator upstream limitation (no custom sort order allowed?) or if this is inconsistent between the stuff in Puppet and our instance.

Aklapper renamed this task from "Resolved" should be the default option in "Change Status" action to "Resolved" should be the first (default) option in "Change Status" action.Oct 6 2014, 11:52 AM
Aklapper triaged this task as Medium priority.
Qgil added a comment.Oct 6 2014, 11:55 AM
In T548#8372, @Aklapper wrote:

Not sure if this is a Phabricator upstream limitation (no custom sort order allowed?) or if this is inconsistent between the stuff in Puppet and our instance.

No idea, but upstream's UI offers this sorting:

  1. Open
  2. Resolved
  3. Wontfix
  4. Invalid
  5. Spit
• chasemp closed this task as Declined.Oct 6 2014, 3:56 PM
• chasemp claimed this task.

It sure seems like the custom status's are displayed alphabetically. We could test this in labs by putting in a Zzzzz and an Aaaaa status and playing a bit, may come down to requesting the ability to have an explicit ordering here https://phabricator.wikimedia.org/config/edit/maniphest.statuses/

but yeah, would prefer resolved to the first option as well

Qgil reopened this task as Open.Oct 6 2014, 3:59 PM
Qgil edited projects, added Phabricator, Phabricator (Upstream); removed Bugzilla-Migration.

Ok, removing bugzilla-migration requirement, adding phabricator.org in order to file this upstream. I'll do this.

Qgil removed • chasemp as the assignee of this task.Oct 6 2014, 4:00 PM
Qgil added a subscriber: • chasemp.
Palexis mentioned this in T364: How to organize Wiki Release team monthly sprints and quarterly reviews.Oct 20 2014, 1:29 AM
Qgil lowered the priority of this task from Medium to Low.Oct 22 2014, 9:08 PM
Qgil edited projects, added Project-Management; removed Phabricator.Oct 22 2014, 9:13 PM
Umherirrender subscribed.Dec 2 2014, 7:52 PM
Jdforrester-WMF subscribed.Dec 6 2014, 11:02 PM
Qgil moved this task from Backlog to Ready To Go on the Phabricator (Upstream) board.Dec 10 2014, 12:21 PM
Qgil edited projects, added Phabricator; removed Project-Management.Dec 23 2014, 9:11 PM
Qgil updated the task description. (Show Details)Feb 4 2015, 6:59 PM
Qgil moved this task from Ready To Go to Upstreamed on the Phabricator (Upstream) board.
Qgil added a comment.Feb 5 2015, 7:43 AM

According to upstream, and according to @chasemp, this is a local problem in our instance.

In https://secure.phabricator.com/T7164#94841, @chasemp wrote:

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.

Qgil removed a project: Phabricator (Upstream).Feb 5 2015, 7:43 AM
Umherirrender unsubscribed.Feb 24 2015, 7:28 PM
Qgil merged a task: T96534: When quick-editing a task while commenting, and choosing "Change Status" action, "Status" dropdown should default to "Resolved" and not "Declined".Apr 20 2015, 7:24 AM
Qgil moved this task from To Triage to Need discussion on the Phabricator board.
Qgil added subscribers: • MZMcBride, matmarex.
matmarex added a comment.Apr 20 2015, 6:09 PM

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.

Ruby hashes, unlike PHP associative arrays, do not guarantee keeping the ordering. And JSON (and YAML) objects specifically are not ordered. Even if it's surfaced only by our magic configuration, this should still be considered an upstream bug.

• mmodell subscribed.Apr 20 2015, 6:17 PM
In T548#1221411, @matmarex wrote:

Ruby hashes, unlike PHP associative arrays, do not guarantee keeping the ordering. And JSON (and YAML) objects specifically are not ordered. Even if it's surfaced only by our magic configuration, this should still be considered an upstream bug.

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.

dduvall subscribed.Apr 20 2015, 6:47 PM
In T548#1221411, @matmarex wrote:

Ruby hashes, unlike PHP associative arrays, do not guarantee keeping the ordering. And JSON (and YAML) objects specifically are not ordered. Even if it's surfaced only by our magic configuration, this should still be considered an upstream bug.

FWIW, this is only true of hashes in Ruby 1.8. In either case, the ordered_json function explicitly sorts a hash argument before iterating over it.

gerritbot subscribed.Apr 20 2015, 6:52 PM

Change 205338 had a related patch set uploaded (by Bartosz Dziewoński):
Do no order Phabricator settings JSON lexicographically

https://gerrit.wikimedia.org/r/205338

gerritbot added a project: Patch-For-Review.Apr 20 2015, 6:52 PM
matmarex added a comment.Apr 20 2015, 7:45 PM
In T548#1221432, @mmodell wrote:

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/

• chasemp added a comment.Apr 20 2015, 10:13 PM
In T548#1222093, @matmarex wrote:
In T548#1221432, @mmodell wrote:

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/

that's probably a fair thought

• mmodell added a comment.Apr 20 2015, 10:20 PM

So we should probably submit a patch upstream that adds an ordering property (int) to the status objects. I imagine they would accept it if we make the effort.

• mmodell added a comment.Apr 21 2015, 3:02 AM

I inquired in the upstream task as to whether they would be receptive to a patch that adds explicit ordering. I suspect that they won't be too against it, especially after my cheap () but skilled bribery (lols) ...

• mmodell added a comment.Apr 21 2015, 6:19 PM

My humor was unpersuasive, but Evan suggested an entirely sane way to handle the situation. For this particular config key, we really don't have to store the value in yml/puppet as it's entirely a static value (no need for puppet to dynamically change it in any way). I think the easiest thing to do would be to set this particular phabricator config key from php instead of the dynamically generated json.

gerritbot added a comment.Apr 22 2015, 12:52 AM

Change 205797 had a related patch set uploaded (by 20after4):
Move maniphest status settings into custom/wmf-defaults.php

https://gerrit.wikimedia.org/r/205797

gerritbot added a comment.Apr 22 2015, 9:46 AM

Change 205338 abandoned by Bartosz Dziewoński:
Preserve order of 'maniphest.statuses' in Phabricator settings

Reason:
Superseded by https://gerrit.wikimedia.org/r/#/c/205797/

https://gerrit.wikimedia.org/r/205338

• mmodell changed the task status from Open to Stalled.Apr 26 2015, 8:43 AM

My change got -1'd in gerrit so I'm fixing this in the database (this is a temp fix which isn't ideal but it does address the problem for now)

• mmodell claimed this task.Apr 26 2015, 8:43 AM
Aklapper added a comment.Apr 27 2015, 9:20 PM

@mmodell: Yeah, "Database" differs now from "Local Config" on https://phabricator.wikimedia.org/config/edit/maniphest.statuses/ but it works. Thanks ("for the time being"?). :)

• mmodell closed this task as Resolved.Jun 2 2015, 12:04 PM
gerritbot added a comment.Jul 1 2015, 9:24 AM

Change 205797 abandoned by 20after4:
Move maniphest status settings into custom/wmf-defaults.php

Reason:
grumble grumble

https://gerrit.wikimedia.org/r/205797

Restricted Application added a subscriber: scfc. · View Herald TranscriptJul 1 2015, 9:24 AM
gerritbot added a comment.Jul 9 2015, 2:03 AM

Change 205797 restored by 20after4:
Move maniphest status settings into custom/wmf-defaults.php

Reason:
this is still a good idea, and I still don't understand Alexandros' -1 on this change

https://gerrit.wikimedia.org/r/205797

gerritbot added a comment.Sep 23 2015, 7:10 PM

Change 205797 abandoned by 20after4:
Move maniphest status settings into custom/wmf-defaults.php

Reason:
I still think this is the way to go, however, things have changed a bit since I wrote this and I think it needs to be reworked a bit. I'll bring this back from the dead sometime when I can give it the attention it deserves.

https://gerrit.wikimedia.org/r/205797

greg added a project: Essential-Work.Jan 13 2016, 9:39 PM
Restricted Application added a subscriber: Luke081515. · View Herald TranscriptJan 13 2016, 9:39 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits