Edgewall Software
Modify

Opened 11 years ago

Closed 10 years ago

#7512 closed defect (fixed)

report 8 (active tickets, mine first) sorts owner wrong

Reported by: mailmaix@… Owned by: Remy Blank
Priority: low Milestone: 0.11.7
Component: report system Version:
Severity: normal Keywords:
Cc: mailmaix@…, rrosauro@… Branch:
Release Notes:
API Changes:

Description

Report 8 (active tickets, mine first) is sorting wrong. It first shows my tickets, then tickets owned by others, and then those with no owner. This might be useful, but it is not the intended behavior.

(And, additionally, some of the tickets have '' as owner, some have None, though they both have no owner. So some of the no-owner tickets are within the tickets with owner.)

This is caused by this line:

ORDER BY (owner = $USER) DESC, … 

If owner is $USER, this returns 1, if it is something else, it returns 0. But if owner is NULL, it returns NULL.

It must be replaced by the following:

ORDER BY (CASE (owner = $USER) WHEN 1 THEN 1 ELSE 0 END) DESC,

Attachments (3)

7512-report8-r7613.patch (462 bytes ) - added by Remy Blank 11 years ago.
Patch against 0.11-stable fixing sorting by owner in report 8
7512-r9194.patch (591 bytes ) - added by blynch1@… 10 years ago.
Patch against trunk to allow the numerical 0 to be displayed in SQL Reports
7512-r9194.2.patch (595 bytes ) - added by blynch1@… 10 years ago.
Patch against trunk to allow the numerical 0 to be displayed in SQL Reports

Download all attachments as: .zip

Change History (18)

comment:1 by mailmaix@…, 11 years ago

Cc: mailmaix@… added

comment:2 by rrosauro@…, 11 years ago

Cc: rrosauro@… added

in reply to:  description comment:3 by Remy Blank, 11 years ago

Milestone: 0.11.3
Owner: set to Remy Blank

Replying to mailmaix@…:

Report 8 (active tickets, mine first) is sorting wrong. It first shows my tickets, then tickets owned by others, and then those with no owner. This might be useful, but it is not the intended behavior.

I assume that you mean the sorting of the second group "Active tickets" is wrong.

(And, additionally, some of the tickets have as owner, some have None, though they both have no owner. So some of the no-owner tickets are within the tickets with owner.)

Yes, that's no good either. I'll add this to my to-do list.

by Remy Blank, 11 years ago

Attachment: 7512-report8-r7613.patch added

Patch against 0.11-stable fixing sorting by owner in report 8

comment:4 by Remy Blank, 11 years ago

The patch above fixes report 8 so that NULL values don't mess up the sorting. It uses the function COALESCE() instead of the proposed CASE, though.

That some unassigned owners display as "None", whereas others display as the empty string is due to the data in the database: a NULL value translates to "None", and an empty string is returned as-is. I agree this is not intuitive, and the query module does it differently (NULL → "—" except for grouping, where NULL → "None"), but I believe this shouldn't be changed, as it risks breaking lots of reports that are out there.

If you would rather have an empty string for NULL values, replace owner with COALESCE(owner, '') as owner in the query.

comment:5 by W. Martin Borgert <debacle@…>, 11 years ago

Maybe it would be wise, if trac would leave an unset milestone field completely empty in the reports, no matter whether this is internally stored as a NULL value or as an empty string. (Or component or version.) "None" might look like "something" to the non-Pythonist.

This works for me:

--- report.py.orig      2008-11-14 13:25:14.000000000 +0100
+++ report.py   2008-11-14 13:32:55.000000000 +0100
@@ -406,7 +406,10 @@
             for header_group in header_groups:
                 cell_group = []
                 for header in header_group:
-                    value = unicode(result[col_idx])
+                    if result[col_idx]:
+                        value = unicode(result[col_idx])
+                    else:
+                        value = ""
                     cell = {'value': value, 'header': header, 'index': col_idx}
                     col = header['col']
                     col_idx += 1

comment:6 by W. Martin Borgert <debacle@…>, 11 years ago

This is shorter than the previous patch and handles CSV/TSV as well:

--- report.py.orig      2008-11-14 13:25:14.000000000 +0100
+++ report.py   2008-11-14 17:12:01.000000000 +0100
@@ -406,7 +406,7 @@
             for header_group in header_groups:
                 cell_group = []
                 for header in header_group:
-                    value = unicode(result[col_idx])
+                    value = unicode(result[col_idx] or "")
                     cell = {'value': value, 'header': header, 'index': col_idx}
                     col = header['col']
                     col_idx += 1
@@ -666,7 +666,7 @@
         for row in rows:
             row = list(row)
             for i in xrange(len(row)):
-                row[i] = converters[i](row[i]).encode('utf-8')
+                row[i] = converters[i](row[i] or "").encode('utf-8')
             writer.writerow(row)

         raise RequestDone

comment:7 by Remy Blank, 11 years ago

Resolution: fixed
Status: newclosed

The report 8 has been fixed in [7765]. This will only affect newly created environments. For existing environments, the following change has to be done by hand:

(owner = $USER) -> (COALESCE(owner, '') = $USER)

I'll also have a look at comment:6, stay tuned.

comment:8 by Remy Blank, 11 years ago

[7766] implements a slight variation of the patch in comment:6.

comment:9 by blynch1@…, 10 years ago

Not to bring up something so old, but it looks like the fix (in r7766 - specifically value = unicode(result[col_idx] or "")) actually causes another issue where the number 0 is not displayed in reports. For example:

select 0 as 'Number' from ticket will not display 0

but

select 1 as 'Number' from ticket will display 1

This seems to only be an issue with the value 0, no other value.

I came across it trying to sum values from tables and discovered that anything that summed to 0 would not display.

comment:10 by Christian Boos, 10 years ago

Milestone: 0.11.30.11.7
Resolution: fixed
Status: closedreopened

Good catch. A patch would be welcomed!

(hint: s/value or ''/value is 0 and '0' or value or ''/)

in reply to:  6 comment:11 by blynch1@…, 10 years ago

Here's a patch against trunk, and will attach the file in a moment.

--- report.py   2010-02-17 13:16:36.000000000 -0500
+++ 7512-r9194.py       2010-02-17 13:19:25.000000000 -0500
@@ -453,7 +453,8 @@
             for header_group in header_groups:
                 cell_group = []
                 for header in header_group:
-                    value = unicode(result[col_idx] or '')
+                    value = unicode(result[col_idx] is None and
+                                    result[col_idx] or '')
                     cell = {'value': value, 'header': header, 'index': col_idx}
                     col = header['col']
                     col_idx += 1

by blynch1@…, 10 years ago

Attachment: 7512-r9194.patch added

Patch against trunk to allow the numerical 0 to be displayed in SQL Reports

comment:12 by Christian Boos, 10 years ago

I doubt this will work…

>>> 0 is None and 0 or ''
''
>>> 1 is None and 1 or ''
''
>>> None is None and None or ''
''

comment:13 by blynch1@…, 10 years ago

Ah. Sorry, I had working code under non-working code when testing, and submitted the wrong one. This I think is more in line with what you were suggesting.

--- report.py   2010-02-17 13:16:36.000000000 -0500
+++ 7512-r9194.py       2010-02-17 14:41:09.000000000 -0500
@@ -453,7 +453,8 @@
             for header_group in header_groups:
                 cell_group = []
                 for header in header_group:
-                    value = unicode(result[col_idx] or '')
+                    value = unicode(result[col_idx] is 0 and '0' or
+                                    result[col_idx] or '')
                     cell = {'value': value, 'header': header, 'index': col_idx}
                     col = header['col']
                     col_idx += 1

by blynch1@…, 10 years ago

Attachment: 7512-r9194.2.patch added

Patch against trunk to allow the numerical 0 to be displayed in SQL Reports

comment:14 by Christian Boos, 10 years ago

Thanks for the patch, which was the starting point for the final fix r9202 (r9204 on trunk).

comment:15 by Christian Boos, 10 years ago

Resolution: fixed
Status: reopenedclosed

(forgot to close)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.