206425 - Allow sorting shared / saved searches in the footer

Status: RESOLVED INCOMPLETE

(Bugzilla :: Query/Bug List, enhancement, P4)

Description

[Johan Runnedahl]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)
Build Identifier: 

At the bottom of each page in the yellow box it is possible to store or 
remember preset queries.

However, they are not sorted in any particular order other than creation order. 
It would be a nice with some possibility for the user to sort the queries.

Suggestion: This could be done, either by just sorting them alphabetically or 
give the user the possibility to sort them with a sortkey.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Jacob Steenhagen]

Looking at my stored queries they appear to be sorted in alphabetical order...

Confirming enhancement request for a sortkey.

Comment 2

[Johan Runnedahl]

Looking at my stored queries they are not sorted in alphabetical order.

A sort key sounds good. But a first step might be to have the actual alphabetic 
ordering. If such a step would not be to much sunk work.

Comment 3

[Albert Ting]

I'm seeing this problem as well.  I tried to modify the template files myself
but am not familiar enough with the syntax.

Comment 4

[GavinS]

The current order is 'My Bugs' first, followed by the rest of the queries,
alphabetically (see Bugzilla/User.pm:154)

Comment 5

[Johan Runnedahl]

I am runnig Bugzilla v 2.16.3 and my Preset Queries are not sorted 
alphabetically. This is the, non-alphabetic, order of my first 5 queries.

"My Bugs | 
To be verified by me | 
Verified or closed by me | 
CC to me | 
Really bugs "

Comment 6

[GavinS]

I was talking about the latest version in CVS - sorry!

If you had access, and wanted to make 2.16.3 sort alphabetically, I think that
the change would be at about line 498 of CGI.pl

Comment 7

[Frédéric Buclin]

That's something I would like to see fixed for 2.24. I have around 25 saved searches and having them sorted alphabetically is painful.

Comment 8

[Paul]

This works for me on Bugzilla  Version 2.20+  and CVS TIP.

Comment 9

[Frédéric Buclin]

paul, read comment 1. The goal here is to add a sortkey so that you can sort them in any order you want, not only alphabetically.

I have saved searches about different topics, and I would like to sort them by topic. And AFAIK, this doesn't work for me on tip. ;)

Comment 10

[Paul]

Attachment: patch for tip

From the discussion, I wasn't sure that was the goal of the bug.

Comment 11

[Frédéric Buclin]

Comment on attachment 206626 [details] [diff] [review]
patch for tip

>Index: userprefs.cgi

>+        my $sortord = $cgi->param("sortkey_$q->{'name'}");
>+        if ( defined($sortord) ) {
>+            # detaint it
>+            $sortord    = ($sortord =~ m/^([\-\+]{0,1}[0-9]+)$/) ? $1 : 0 ;
>+        }
>+        else {
>+            $sortord = 0;
>+        }
>+            
>+        my $qrysortkey = defined($cgi->param("sortkey_$q->{'name'}")) ? $sortord : 0;

$sortord should be an integer (no negative values allowed), so use detaint_natural() for that. And then use this value directly in the SQL query. As the default is 0 already, you don't have to care whether detaint_natural() will return undef or a number.



>Index: Bugzilla/User.pm

>     my $sth = $dbh->prepare(q{ SELECT
>-                             DISTINCT name, query, linkinfooter, query_type,
>+                             DISTINCT name, query, linkinfooter, query_type, 

Nit: please don't add trailing whitespaces.

Moreover, User::queries() has changed a lot. This patch no longer applies cleanly on tip.



>Index: template/en/default/account/prefs/saved-searches.html.tmpl

>+      <th>
>+        Sort Order
>+      </th>

In all other places, we write "Sortkey" or "Sort Key". Use one of them here too.


>+        <td align="center">
>+          <input id="sortkey" size="20" maxlength="20" name="sortkey_[% q.name FILTER html %]" value="[% q.sortkey %]">
>+        </td>

You restrict sortkeys to small integers. I don't think a number with 20 characters will fit in a small integer!

Comment 12

[Paul]

Attachment: refreshed from bitrot, changed the size of the field,used detaint_signed

Comment 13

[Frédéric Buclin]

Comment on attachment 207935 [details] [diff] [review]
refreshed from bitrot, changed the size of the field,used detaint_signed

>Index: checksetup.pl

>+# 2005-12-22 [e-mail] - Bug 206425
>+$dbh->bz_add_column('namedqueries', 'sortkey',
>+                    {TYPE => 'INT2', NOTNULL => 1, DEFAULT => 0});
>+
> # 2005-11-04 [e-mail] - Bug 305927

Nit: please respect the chronological order and move this part right before --TABLE--.



>Index: userprefs.cgi

>+        my $sortord = $cgi->param("sortkey_$q->{'name'}");
>+        detaint_signed($sortord);

The sortkey should be positive, and so you should use detaint_natural(). The reason we allowed negative sortkeys for target milestones was because you could have miletones in the past and in the future. But I see no good reason to allow them for saved searches.



>Index: template/en/default/account/prefs/saved-searches.html.tmpl

>+        <td align="center">
>+          <input id="sortkey" size="5" maxlength="5" name="sortkey_[% q.name FILTER html %]" value="[% q.sortkey %]">
>+        </td>

q.sortkey is not filtered. This makes runtests.pl to complain:

not ok 22 - (en/default) template/en/default/account/prefs/saved-searches.html.tmpl has unfiltered directives:
#   93: q.sortkey
# --ERROR

Comment 14

[Frédéric Buclin]

(In reply to comment #13)

> >+        <td align="center">
> >+          <input id="sortkey" size="5" maxlength="5" name="sortkey_[% q.name FILTER html %]" value="[% q.sortkey %]">
> >+        </td>

Also, split this long line.

Comment 15

[Paul]

Attachment: checksetup: chronological order, detaint_natural(), template FILTER html

Comment 16

[Paul]

Attachment: detaint_natural's return needed to check for undef for inserts

Comment 17

[Frédéric Buclin]

Comment on attachment 207955 [details] [diff] [review]
detaint_natural's return needed to check for undef for inserts

works fine. Thanks for the patch. r=LpSolit

Comment 18

[Dave Miller [:justdave]]

I don't like the end-user UI on this much.  The average user is going to be confused by "sort key".  What would work better is if we just automatically assign them and have "move up" and "move down" widgets in the saved searches manager in the prefs.  Default them to alphabetical of course, we don't need to renumber them all until the first time the user tries to change them.

Comment 19

[Paul]

(In reply to comment #18)
> I don't like the end-user UI on this much.  The average user is going to be
> confused by "sort key".  What would work better is if we just automatically
> assign them and have "move up" and "move down" widgets in the saved searches
> manager in the prefs.  Default them to alphabetical of course, we don't need to
> renumber them all until the first time the user tries to change them.

This is working in the standard way all other ordering is done in Bugzilla.  A "move up" and "move down" should be a separate bug and applied to all sorting activities.

Comment 20

[Paul]

This doesn't need to bitrot.  Please check it in.

Comment 21

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #18)
> I don't like the end-user UI on this much.

Enough to give it review-?  If so, please mark as such.  Otherwise, let's approve and iteratively improve.

Comment 22

[Paul]

(In reply to comment #21)
> (In reply to comment #18)
> > I don't like the end-user UI on this much.
> Enough to give it review-?  If so, please mark as such.  Otherwise, let's
> approve and iteratively improve.

The problem is that more and more stuff is using the exact same mechanism to order things.  How can you choose this bug to stand on to say it's not good UI?  Why have the others been allowed?  and continue to be allowed?

There's no reason for my work to be wasted.  
Commit and then create a new bug if you must.

Comment 23

[Dave Miller [:justdave]]

yeah, agreed.  Consistency matters.  We need to go through and fix them all at once so people stop copying it. :)

But this patch is so bitrotted it's not funny right now... let's get it updated.

Comment 24

[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 26

[timeless]

afaict, bug 69000 massacred this patch. I think the options are hijacking the namedqueries_link_in_footer table, or creating yet another table.

I think I'd rather hijack it (which probably includes adding two fields, one being a boolean for link in footer).

I don't particularly like the idea of creating dozens of tables each time a query needs a new property.

Comment 27

[Guy Pyrzak]

removing UE keyword because it doesn't ask for changes in the UI, but asks for functionality that normally works, to also work for a 'special' case.

Comment 28

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Comment 29

[Max Kanat-Alexander]

Honestly, I'm not even sure the code complexity of such a feature is really worth it.

Comment 30

[:glob ✱]

(In reply to Max Kanat-Alexander from comment #29)
> Honestly, I'm not even sure the code complexity of such a feature is really
> worth it.

agreed.

Comment 31

[Paul]

It's worth it.

Comment 32

[Andre Klapper]

Paul: Why? And why do you think that a lot of people would ever use this?

Comment 33

[Frédéric Buclin]

(In reply to Andre Klapper from comment #32)
> Paul: Why? And why do you think that a lot of people would ever use this?

See the long list of users in the CC list + comments 0, 3, 7. Users with tens of saved searches probably want to order them (I do), as already explained in comment 7.

Comment 34

[Firefox Bug Husbandry Bot]

Removing good-first-bug keyword because team does not have bandwidth to mentor at the moment.

Comment 35

[Firefox Bug Husbandry Bot]

Removing outreachy because team does not have bandwidth to mentor this cycle.

Comment 36

[Kohei Yoshino [:kohei]]

The global footer has been removed.