85600 - Target Milestones misordered in Bugzilla query form (JavaScript ignores sortkey)

Status: ASSIGNED

(Bugzilla :: Query/Bug List, defect, P3)

Description

[Brian Ryner (not reading)]

The target milestones on the query page are in a semi-random order after M16.  
dmose says this is because of the collation keys not matching up between 
products.

Comment 1

[Asa Dotzler [:asa]]

I've looked in Mozilla, Navigator 4.x and IE6 so this isn't a Mozilla bug (they
all display the list out of order). I've done all kinds of playing with the sort
order keys and nothing makes this any better. If you select just the Browser
Product in query.cgi, the list is all messed up so it's not likely a conflict
with sortkeys from other Products.  I think this must be a Bugzilla bug. I did a
couple quick queries and didn't see it reported there so I'm sending this bug over.

Comment 2

[Dave Miller [:justdave]]

Just for comparison, can you give me a list of the milestones and sort keys for
the Browser product?

Comment 3

[Asa Dotzler [:asa]]

I was playing around and noticed that when I reset my query to the default with
http://bugzilla.mozilla.org/query.cgi?nukedefaultquery=1 that all my Target
Milestones seemed to line up just fine. Is is possible that by having only
several of Products selected in my default that it could have messed up the sort
order? It's working fine for me now.

Comment 4

[Asa Dotzler [:asa]]

...or maybe it was fixed by something we picked up in our sort of 2.14 update a
few days ago. 

Comment 5

[Matthew Tuck [:CodeMachine]]

Hmm, looks like 2.14 or something done at the same time fixed it.  Would be nice
to know what was wrong before resolving though.

Comment 6

[Matthew Tuck [:CodeMachine]]

Moving to new Bugzilla product ...

Comment 7

[Matthew Tuck [:CodeMachine]]

I saw this come back the other day ... but hopefully with the landing of bug
#96534 this will be fixed due to the JS code being rewritten, except for bug
#97736.

Comment 8

[Matthew Tuck [:CodeMachine]]

OK, here's the deal.

Currently our JS does sorting without regard to sortkey (bug #97736).  The old
code did sorting of sortkeys properly, but it may have done it within products
when you've selected a project.

But when you load query.cgi with no products selected it loads from the
versioncache rather than constructing a list in JS, and this leads to misordered
milestones, because it loads from the versioncache which sorts on sortkey rather
than sortkey within product.

So either:

- we should sort versioncache on just sortkey, mozilla.org needs to fix their
sortkeys to segregate the milestones, and the query JS needs to sort purely on
sortkey
- we should sort versioncache on just sortkey within products, and the query JS
needs to do the same

Comment 9

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 10

[Bradley Baetz (:bbaetz)]

*** Bug 143660 has been marked as a duplicate of this bug. ***

Comment 11

[Joel Peshkin]

[e-mail] seems to be working on getting this into 2.18, so we'll
shoot for 2.20

Comment 12

[Max Kanat-Alexander]

Yeah, this still happens on the trunk, too.

Comment 13

[Max Kanat-Alexander]

*** Bug 97736 has been marked as a duplicate of this bug. ***

Comment 14

[Frédéric Buclin]

*** Bug 314504 has been marked as a duplicate of this bug. ***

Comment 15

[Paul]

It comes from the server already sorted.  Why JavaScript at all?

    <label for="target_milestone" accesskey="t">
      <select name="target_milestone" id="target_milestone">
          <option value="Bugzilla old">Bugzilla old
          </option>
          <option value="Bugzilla 2.12">Bugzilla 2.12
          </option>
          <option value="Bugzilla 2.14">Bugzilla 2.14
          </option>
          <option value="Bugzilla 2.16">Bugzilla 2.16
          </option>
          <option value="Bugzilla 2.18">Bugzilla 2.18
          </option>
          <option value="---">---
          </option>
          <option value="Bugzilla 2.20">Bugzilla 2.20
          </option>
          <option value="Bugzilla 2.22" selected>Bugzilla 2.22
          </option>
          <option value="Bugzilla 2.24">Bugzilla 2.24
          </option>
          <option value="Bugzilla 3.0">Bugzilla 3.0
          </option>
          <option value="Bugzilla 3.2">Bugzilla 3.2
          </option>
          <option value="Future">Future
          </option>
      </select>
    </label>

Comment 16

[Paul]

Sorry, now I see the problem is that we need to merge milestones for a possible cross product search.... Hmmm.

Comment 17

[Paul]

I was looking into making a patch for this, but there are 4 different ways that are not so easily handled.

1. If one product is selected
    (easy! show that product's milestones in proper order)
2. If two products are selected
    (How do we mix the milestones? P1's first, then P2's? What if there are duplicate milestone names?)
3. If nothing is selected, is a milestone relevant?  Won't we choose a product when we choose a milestone?  Of course unless it's a duplicate)
4. On initial page load (do the same as #3, I would guess)

Should we just use case #1, and in all other cases just alphabetize?

Comment 18

[Max Kanat-Alexander]

(In reply to comment #17)
They should always be ordered first by sortkey and then alphabetically, in every case.

Comment 19

[Paul]

(In reply to comment #18)
> (In reply to comment #17)
> They should always be ordered first by sortkey and then alphabetically, in
> every case.
> 

So if we have 2 products R1 & R2.
R1 has milestones AAA = 1, BBB = 2, CCC = 3
R2 has milestones ZZZ = 1, XXX = 2, YYY = 3

You want to see
AAA
ZZZ
BBB
XXX
CCC
YYY

is that right?

Comment 20

[Max Kanat-Alexander]

(In reply to comment #19)
> is that right?

  Exactly! :-)

Comment 21

[Max Kanat-Alexander]

Looks like Firefox did some strange selecting of the group radio boxes on the mass-change page when I clicked refresh in my browser before that last mass-change. This is not really a security bug. :-)

Comment 22

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 23

[Max Kanat-Alexander]

*** Bug 360321 has been marked as a duplicate of this bug. ***

Comment 24

[Scott Nance]

I'm not sure of the procedure to make a patch, but I've fixed this bug in BZ 3.0.

I made the fix in query.cgi.  Instead of saving a "1" in the hash used to remove duplicates, I changed it to save the sortkey value.  Then I sorted the milestones hash to build a sorted milestones array.

It works as before.  The one issue with this fix is if there are duplicate milestones with different sortkeys, the key for the last milestone encountered is used.  I keep my milestones pretty well synced so this isn't a problem for me but it may affect some other installations.

Details for the fix in query.cgi:

Change Line 194 to read:

    $milestones{$_->name} = $_->sortkey foreach (@{$product->milestones});

Replace Line 199 with:

my @milestones = ();
foreach my $m (sort { $milestones{$a} <=> $milestones{$b} } keys %milestones)
{
    push @milestones, $m;
}


I'm sure there's probably a way to get the second chunk all into one line, but I was just happy to have it fixed.  :)

   -Scott

Comment 25

[Scott Nance]

Attachment: Fixed query.cgi for Milestone Sort Bug

Comment 26

[Max Kanat-Alexander]

Hey Brian! Thanks for the fix! :-) I think the Contributor's Guide explains how to make a patch:

  http://www.bugzilla.org/docs/contributor.html

And if it doesn't, then you should come into IRC and we'll help you out. Details on how to get into IRC are here:

  http://wiki.mozilla.org/Bugzilla:Communicate

Comment 27

[Scott Nance]

Attachment: Patch of above fix to query.cgi

Comment 28

[Scott Nance]

Patch checked in and older attachment of query.cgi has been obsoleted.

This has been tested on my BZ 3.0 server.

   -Scott

Comment 29

[Frédéric Buclin]

Comment on attachment 265993 [details] [diff] [review]
Patch of above fix to query.cgi

>+my @milestones = ();
>+foreach my $m (sort { $milestones{$a} <=> $milestones{$b} } keys %milestones)
>+{
>+    push @milestones, $m;
>+}

Why not writing:

my @milestones = sort { $milestones{$a} <=> $milestones{$b} } keys %milestones;

It's cleaner.

Comment 30

[Scott Nance]

Even better.  Like I said, I was sure there was a tighter way.

Comment 31

[Frédéric Buclin]

Comment on attachment 265993 [details] [diff] [review]
Patch of above fix to query.cgi

per my previous comment. Use the shorter form.

Comment 32

[Scott Nance]

Your suggested change does not work.  I actually tried it before I submitted the longer form.

Just re-tried your suggestion as-written and have confirmed that it does not work.  The search page never completes loading.

Resetting Review flag on original working patch for reconsideration.

Comment 33

[Scott Nance]

Comment on attachment 265993 [details] [diff] [review]
Patch of above fix to query.cgi

Suggested change by reviewer (and the reason for denial) does not work.

Comment 34

[Frédéric Buclin]

Comment on attachment 265993 [details] [diff] [review]
Patch of above fix to query.cgi

My suggestion works perfectly; I tested it.

Comment 35

[Scott Nance]

Not on my system.  Hey...  I'm not going to argue about this.  The patch I submitted works fine on my system.  The line you posted, copied exactly, does not.  If you REALLY decide you want it in the shorter form, then YOU fix it and submit the patch.

I was trying to be a good guy and share the fix of an annoying bug with the Bugzilla community at-large.  But I'm not interested in playing little power games for 3 lines of code.

My fix works for me.  Yours doesn't.  Take which ever one you like.  Or take neither.  It's no skin off my back.  MY SYSTEM is working properly.  Either way, I'm done with this bug.

Comment 37

[miketosh]

Attachment: Patch for query.cgi on 3.0.2

Seems like some bad comments here between people, I'm uploading the patch as requested by LpSolit to use the single line code.  This does work for me and loads quickly.

If someone can PLEASE PLEASE PLEASE review, approve, and check in this code, I'd be appreciative.

Comment 38

[Frédéric Buclin]

Comment on attachment 291476 [details] [diff] [review]
Patch for query.cgi on 3.0.2

>-my @milestones = sort(keys %milestones);
>+my @milestones = sort { $milestones{$a} <=> $milestones{$b} } keys %milestones;

Nit: shouldn't you also sort them alphabetically when the sortkey is identical? Something like:

 sort { $milestones{$a} <=> $milestones{$b} || $a cmp $b }

Comment 39

[miketosh]

Attachment: UPDATED Patch for query.cgi on 3.0.2

Ok, I agree, here is the patch.

Comment 40

[miketosh]

 or wouldn't the entries come in alphabetical order anyway?  we don't need to resort them if they are the same.  Sounds redundant.

Comment 41

[Frédéric Buclin]

(In reply to comment #40)
>  or wouldn't the entries come in alphabetical order anyway?  we don't need to
> resort them if they are the same.  Sounds redundant.

They are not sorted alphabetically as they come from a hash.

Comment 42

[Jeff]

This also seems to be an issue with the report.cgi. I'm not perl programmer so I wouldn't know where to start looking but I am getting strange results when using version like milestones even with or without sortkeys defined.

My report results are being sorted as follows even when using the sortkeys shown.

Milestone        Sortkey
2.0.10 release   2010
2.0.11 release   2011
2.0.9 release    2009

Comment 43

[miketosh]

Funny thing, I just found that today myself.  Tabular reports don't sort the milestones correctly.

Comment 44

[Jeff]

And just as a note, the patch (attachment#291663 [details] [diff] [review]) doesn't seem to correct this issue at least in my tests. 

Comment 45

[miketosh]

Sorting Milestones in report.cgi would be a new issue.  you mean the tabular report, right?  When you do a search it displays the table alphabetically.

For me, the query.cgi displays the sort correctly, even when editing or creating the query values.

Comment 46

[Jeff]

Bug#407762 has been created to track issues associated with comment#42 through comment#45.

Comment 47

[Frédéric Buclin]

Something is wrong with this patch. When you load query.cgi, the list is sorted based on the sortkey, as passed from query.cgi itself. But when you select a product and then unselect all products, you get the old behavior where all milestones are sorted alphabetically. It looks like you also have to fix the JS script to use milestones's sortkey instead of their name. Michael, could you investigate?

Comment 48

[Frédéric Buclin]

Comment on attachment 291663 [details] [diff] [review]
UPDATED Patch for query.cgi on 3.0.2

r- for further investigation.

Comment 49

[Brian H]

I was going to submit a new bug, but this seems related enough.

I just upgraded from bugzilla 2.1.8 to 3.0.3, and noticed strange behavior with the target milestone list on the query page:

Initially, with no product selected, it is sorted alphabetically.
Once you choose a product, the milestones for that product are sorted by sortkey.

Now, the fun part...
De-select the product (using CTRL), and the milestone list now has multiple copies of several milestones.  It should be able to filter out duplicate milestones.

(I should mention that we create the same milestones for multiple products, so that is probably a necessary prerequisite to seeing this bug)

I see above people are discussing version 3.2.x already... I apologize if this is something fixed long ago, but it seemed that 3.0.3 was recommended as the latest stable release.

Comment 50

[Frédéric Buclin]

Bugzilla 3.2 is restricted to security bugs only. Mass-retargetting to 3.6.

Comment 51

[Armin Bube]

I upgrade from version 2.22 to 3.6. In 2.22 I found a workaround for me by editing globals.pl:253:

# reading target milestones in from the database - [e-mail]
        SendSQL("SELECT milestones.value, products.name " .
                "FROM milestones, products " .
                "WHERE products.id = milestones.product_id " .
                "ORDER BY milestones.value DESC");

globals.pl doesn't exist any more in version 3.6. Is there any workaround to fix the unsorted list? Problem still exists by selecting only a classification (no product).

Comment 52

[Frédéric Buclin]

Bugzilla 3.6 is now restricted to security fixes only. We will retarget this bug once it has a patch ready for checkin.

Comment 53

[Pierre Ossman]

Many years later and this still seems to be an issue. Anyone giving this some love? :)