142394 - Tabular reports should be sortable

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, enhancement, P2)

Description

[Jouni Heikniemi]

The bug counts by engineer view should be sortable through clickable headers or
a similar mechanism.

Comment 1

[Gervase Markham [:gerv]]

The new reporting architecture is now in place, and sorting is explicitly not
supported - for architectural and UI reasons. If you need to sort, export the
data as CSV and import it into a spreadsheet.

Gerv

Comment 2

[Gervase Markham [:gerv]]

So WONTFIX, then.

Gerv

Comment 3

[Jouni Heikniemi]

Architectural and UI reasons? What on Earth are you talking about?

I have no problem with you wontfixing this (since it's such a minor issue), but 
we should have more than a obscure reference to "architectural and UI reasons" 
to defend this decision against possible future duplicate rfes.

Comment 4

[Bradley Baetz (:bbaetz)]

REOPEN

You can sort in the template as a worst case option, but I'm reasonlay certain
that you can do so in the db, too.

Comment 5

[Bradley Baetz (:bbaetz)]

Obviously, you can only sort on something you have selected for one of the axis,
though.

Comment 6

[Gervase Markham [:gerv]]

The current architecture of report.cgi makes it very difficult to implement
sorting in a sane manner. The axes are alphabetically sorted, and that's what
you get. Sorting in the template is likewise somewhere between very hard and
impossible due to the data structures used. I am actively saying that I will not
fix this bug because it would require a(nother) complete rearchitecting of
report.cgi. I suggest that people who want advanced data processing capabilities
use a dedicated application such as a spreadsheet.

If this bug is to remain open, then it needs a new assignee. :-)

Gerv

Comment 7

[Bradley Baetz (:bbaetz)]

Well, its not that tricky. All you need to do is:

a) move the addition into perl, from the template (which is probably better
anyway, since that would be marginally faster)

b) make names.{tbl,col,row} an array, not a hash (so that ordering is preserved)

c) foreach table, foreach column we're sorting on, do:

sort { $a->{$col} <=> $b->{$col} } @rows

Yes, this only allows sorting on coluns, but then we avoid the issues you hinted
at. Adding rows isn't hard, either; allowing both may be.

If you don't want to do this, then future + assign to nobody, but don't close it..

Comment 8

[Gervase Markham [:gerv]]

> Well, its not that tricky. All you need to do is:

Before you say how easy it is, check the hoops I'm having to jump through over
in bug 173005 to support graphical charts. Any solution needs to fit the
architecture I'm using over there. :-| 

I'll take your suggestion, and future this and assign it to nobody.

Use a spreadsheet, people :-)

Gerv

Comment 9

[Jouni Heikniemi]

>Use a spreadsheet, people :-)

I'm nobody to judge the triviality of implementing this, so I have faith in what
you say. Still, with all due respect, the architecture is seriously flawed if
_sorting a table_ is that hard. I mean, it's not that odd a need: "Who's got the
most ASSIGNED bugs at the moment" isn't something you should have a spreadsheet
app for. If I really need some extreme stats, I'll dump the MySQL db into my
OLAP application and start digging. But expecting everybody looking for people's
bug counts to do that is ridiculous.

The current solution (future+nobody) is ok with me, of course.

Comment 10

[Dave Miller [:justdave]]

.

Comment 11

[Matthew Tuck [:CodeMachine]]

Yeah, this is a pretty serious deficiency.

I don't see why you couldn't sort in multiple dimensions though, you would just
look at the total for that rowset/columnset/table.

The interesting question is whether the ordering has to be the same between
tables (3D).  I'd say yes, because you can't change the ordering of columns for
different rows, so it seems you shouldn't be able to for the third dimension,
although that could perhaps be an option.

Comment 12

[Max Kanat-Alexander]

This would actually be useful, if it hasn't already happened.

Comment 13

[Max Kanat-Alexander]

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

Comment 14

[Frédéric Buclin]

That's also something I would like to see implemented, difficult or not to implement.

Comment 15

[Frédéric Buclin]

Attachment: patch, v1 (using sorttable.js)

I don't know what pyrzak has in mind to implement this, but there is a small 17Kb JS script which does all we need, and is ultra simple to use: sorttable.js. I know b.m.o already uses it to sort columns in buglist.cgi, so I think it would be fine to add sorttable.js to our codebase.

Attached is the unaltered JS file from http://www.kryogenix.org/code/browser/sorttable/sorttable.js

Comment 16

[Frédéric Buclin]

To make things clear, what I'm implementing is only about tabular reports, not graphical reports. We shouldn't try to fix both problems in the same bug else we will go nowhere; especially because this patch does the sorting client-side which is something impossible to do with graphical reports.

Comment 17

[Gervase Markham [:gerv]]

Anywhere I can try this out?

Gerv

Comment 18

[Guy Pyrzak]

Sure thing. I was planning on also having a filter capability. But this is clean and works so go for it. Assigning it to LpSolit.

Comment 19

[Frédéric Buclin]

gerv, you can test it here:
https://landfill.bugzilla.org/qa32/query.cgi?format=report-table

Comment 20

[Max Kanat-Alexander]

Don't use the unaltered sorttable file, we fixed a few things about it for the bmo code, and you should use the fixed version that's on bugzilla.everythingsolved.com.

Comment 21

[Frédéric Buclin]

I would prefer if the initial checkin is the original JS file, so that we can more easily track what we changed in it. And without a description of what you really fixed in your customized version, it's unhelpful to me. I can do a diff (and I already did it), but this doesn't tell me why you did such or such customization. Please tell me more.

Comment 22

[Gervase Markham [:gerv]]

Looks OK. It would be nice if the initial sort were displayed, and if the headers showed some sign that they were clickable, e.g. by being styled as links.

Gerv

Comment 23

[Max Kanat-Alexander]

(In reply to comment #21)
> Please tell me more.

A list of comments from the developer of the bmo code:

* "I modified it to handle the in-table headers, so that they stay fixed."
* "I refactored the original sort code a lot, so its easier to read/modify/fix."
* "To indicate multi-column sorting, I used different shades of gray. The 
  darkest one being the most recent sort."

  It also works with the "collapsed" headers on buglist.cgi.

Comment 24

[Gervase Markham [:gerv]]

Normally, a refactoring of 3rd party code would be an "!!", but given that this code hasn't changed in years, I guess it's fairly safe.

Gerv

Comment 25

[Gervase Markham [:gerv]]

LpSolit: is the ball in your court here, or do you need something from me?

Gerv

Comment 26

[Frédéric Buclin]

(In reply to comment #25)
> LpSolit: is the ball in your court here, or do you need something from me?

gerv: all I have to do is to replace the original sorttable.js file by the one on b.m.o and to make sure it's working fine. From what I can read in comment 23, the altered JS file uses a gray background in column headers to show which column is used to sort the data. This would fix the first part of your request in comment 22, but wouldn't fix the second part (making it clear that you can click the column header to sort the data). Is that fine anyway, for now?

Comment 27

[Gervase Markham [:gerv]]

That sounds fine. My comment #22 was a WIBNI, not a requirement.

Gerv

Comment 28

[Frédéric Buclin]

Max, sorttable.js at https://bugzilla.mozilla.org/js/sorttable.js and https://bugzilla.everythingsolved.com/attachment.cgi?id=524 do not match. Which one should I take for this bug? Note that bug 438004 should be fixed first.

Comment 29

[Max Kanat-Alexander]

(In reply to comment #28)
> Which one should I take for this bug?

  Take whatever's in bzr at: bzr://bzr.everythingsolved.com/mozilla/3.0/

Comment 30

[Frédéric Buclin]

(In reply to comment #29)
>   Take whatever's in bzr at: bzr://bzr.everythingsolved.com/mozilla/3.0/

This one doesn't work as expected. I first had the following JS error:

Erreur : table.sorttable_bodies[0] is undefined
Fichier Source : https://localhost/bugzilla-pg/js/sorttable.js
Ligne : 368

To fix it, I had to add class="sorttable_body" to <tbody> (where is this documented?). But then I got the following JS error:

Erreur : node is undefined
Fichier Source : https://localhost/bugzilla-pg/js/sorttable.js
Ligne : 405

I have no idea how to fix this one. Any help is welcome!


Now, isn't your customized sorttable.js specific to buglist.cgi? Why do I see document.cookie = 'BUGLIST='+BUGLIST; in it?

Comment 31

[Max Kanat-Alexander]

(In reply to comment #30)
> Now, isn't your customized sorttable.js specific to buglist.cgi? Why do I see
> document.cookie = 'BUGLIST='+BUGLIST; in it?

  It is specific to buglist.cgi, at the moment. I'm sure that you could fix that, if you understand the code.

  You do have to modify the templates for it to work correctly. You can look through the "bzr log" and take all the changes related to it.

Comment 32

[Frédéric Buclin]

Comment on attachment 312746 [details] [diff] [review]
patch, v1 (using sorttable.js)

Someone (not me) should make sorttable.js less buglist-centric.

Comment 33

[Frédéric Buclin]

Attachment: patch, v2

The error I reported last year was easy to fix.

I will attach a diff for sorttable.js which shows changes I made in it in order to work here too.

Comment 34

[Frédéric Buclin]

Attachment: diff in sorttable.js

This shows what I changed in sorttable.js compared to what bmo has.

Comment 35

[Max Kanat-Alexander]

Comment on attachment 414079 [details] [diff] [review]
patch, v2

I'm sorry, but I don't really want to accept sorttable.js as it is. It was written a long time ago, long before we had YUI (or before any real JS library even existed). It needs to be re-architected in modern times to use our libraries.

Comment 36

[Gervase Markham [:gerv]]

And you would prefer that we had no client-side sorting rather than working and useful client-side sorting until someone finds time to do that?

Gerv

Comment 37

[Max Kanat-Alexander]

(In reply to comment #36)
> And you would prefer that we had no client-side sorting rather than working and
> useful client-side sorting until someone finds time to do that?

  Well, the issue is maintenance. On top of the fact that the architecture is somewhat ancient, in sorttable.js, it's also code that's fundamentally unfamiliar to any of us. As you saw when we put it on bmo, there were various issues, some of which never got resolved, and largely that had to do with architecture and understanding of the code.

  If we're going to have a feature, I want it to work right. The server-side sorting may be slower than client-side sorting, but it's been very thoroughly debugged for many years, and now works very nicely. I understand that any new feature won't be that stable that quickly, and that in fact new features are highly desirable, but adding code that we don't understand and that is poorly architected is a regressive action for Bugzilla to take, particularly when we've worked *so* hard for all these years to correct the architecture of the system, and when we've spent so much effort in 3.6 (and will spend further effort in 3.8) finishing and correcting Bugzilla's incomplete or broken features.

  I'm sure that there is somebody who could contribute to the Bugzilla project who has enough JavaScript expertise to refactor the script appropriately. I could, but I don't know how high on my priority list this will end up. (Really, I would have fun doing it, I just have a lot of things to do.) And if nobody has the expertise to understand the code and JavaScript itself well enough to re-architect it, then I'd also say that nobody has the expertise to maintain the code--a situation we definitely don't want to be in, particularly not with something as important as the bug list.

  So I'm all for feature improvement, I just want it to be maintainable and excellently-designed feature improvement so that Bugzilla is not only an excellent product in the present, but continues to be an excellent product well into the future.

Comment 38

[Gervase Markham [:gerv]]

It's true that it's not our code, but it is a stable bit of web plumbing used by a load of sites. Stuart Langridge is a great and responsible guy; he wouldn't let his code rot if there was maintenance required. And there hasn't been since April 2007. Why do we think that we are going to have to maintain it significantly? 

I don't recall these issues on b.m.o. of which you speak; could you point me at the bugs?

I hear your point about taking code you think is poorly architected (although again, I don't think this code is badly written) but there's a difference between taking core Perl code which is a mess (which I agree we've been working against for years) and taking a self-contained external JS library. After all, I haven't seen us audit all the Perl modules we use for code quality (although I guess we pick the most popular ones for any task, which is a vague proxy for quality). I don't think the "slippery slope" between taking sorttable.js and Bugzilla re-descending into a labyrinthine mess of unreadable Perl is actually all that slippery.

It's not just buglist sorting, of course - this bug is about tabular reports, which have no server-side sorting mechanism.

I think you might be letting the best be the enemy of the good, here...?

Gerv

Comment 39

[Frédéric Buclin]

Nevermind, it seems very easy to do with YUI 2, see http://developer.yahoo.com/yui/examples/datatable/dt_enhanced.html

Comment 40

[Frédéric Buclin]

Attachment: patch, v3

Now using YUI2. There is only one problem, which should be fixed in YUI 2.9.0: the total row at the bottom of the table cannot be displayed, because DataTable knows nothing about <tfoot>, see http://yuilibrary.com/projects/yui2/ticket/2527635. Maybe we could go with it, and re-add the total row when YUI supports it?

Comment 41

[Guy Pyrzak]

Attachment: Report table with fancier JS using YUI (no hacks!)

Comment 42

[Max Kanat-Alexander]

Comment on attachment 456376 [details] [diff] [review]
Report table with fancier JS using YUI (no hacks!)

That JS really needs to go into a separate file. And it should probably be an object like the autocompletes and dupTable are, no?

Comment 43

[Frédéric Buclin]

(In reply to comment #42)
> That JS really needs to go into a separate file. And it should probably be an
> object like the autocompletes and dupTable are, no?

Not sure you can easily move the JS code in a separate js file. It depends on the selected fields and their values. pyrzak, what do you think?

Comment 44

[Max Kanat-Alexander]

Comment on attachment 456376 [details] [diff] [review]
Report table with fancier JS using YUI (no hacks!)

Hmm. Well, the urlbase can go into the "BUGZILLA" JS constant in header.html.tmpl, and that would let you move some of it, no?

Comment 45

[Frédéric Buclin]

urlbase is not the main problem. If we can easily pass the row and column names as well as column values + some other extra data, then yes, we can move it outside the template. But I honestly wouldn't block this bug due to that.

pyrzak, your patch is working great. I'm going to remove one line, though, which doesn't work in my browser: console.log(), which is not defined (I guess you need Firebug to make it work).

Comment 46

[Frédéric Buclin]

Attachment: patch combined with pyrzak's contribution, v4

I simply removed console.log() from the previous patch. mkanat said on IRC it was fine to move the JS code to an external file in a separate bug.

Comment 47

[Frédéric Buclin]

Attachment: patch combined with pyrzak's contribution, v4.1

No code change. I simply added pyrzak and myself to the contributors list.

Comment 48

[Guy Pyrzak]

Comment on attachment 456462 [details] [diff] [review]
patch combined with pyrzak's contribution, v4.1

Test it on IE6 and if it works then i think we're good to go. If it doesn't work i think it's ok too because it would just fail and look like the current UI.

Comment 49

[Frédéric Buclin]

IE6 displays "null" instead of actual bug counts. No idea why.

Comment 50

[Frédéric Buclin]

Attachment: patch, v4.2

Fix a regexp which wasn't understood by IE6. Now works with IE6, IE8, Fx, Opera, Safari and Google Chrome. Carrying forward pyrzak's r+.

Comment 51

[Frédéric Buclin]

Updating the bug summary as we can only sort tabular reports (I don't know what "sort a graphical report" means).

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified template/en/default/reports/report-table.html.tmpl
modified template/en/default/reports/report.html.tmpl
Committed revision 7302.

Comment 52

[Frédéric Buclin]

Added to relnotes in bug 713346.