385391 - csv table cell for product in zyx label is bad (missing a space)

Status: NEW

(Bugzilla :: Reporting/Charting, defect)

Description

[timeless]

Product: "mozilla.org""Version" / "Target Milestone"	---
other	76

Comment 1

[Gervase Markham [:gerv]]

The problem here is that we are applying the "csv" filter to sub-parts of a cell. The filter is designed only to correctly quote an entire cell. Applying it to sub-parts leads to incorrectly-escaped double quotes in the middle of a value, and therefore parse errors. 

In addition, if the values of any of these fields contains either a colon or a slash, it is very difficult to programmatically extract the original values. As one of the benefits of CSV, unlike HTML tables, is that it is machine-readable, this is a disadvantage.

We should change the value of this cell, instead of containing human-readable descriptions, to just contain the value to which that particular table pertains. So in the case given above, it would be simply "mozilla.org".

So a 3D query split by product, priority and severity would look like this (space added for clarity):

"mozilla.org", P1, P2, P3
blocker,        1,  5,  7
normal,        12,  3,  0
trivial,       17, 41, 76

Firefox,       P1, P2, P3
blocker,       11, 15, 27
normal,         2, 16,  5
trivial,        7, 45,  9

...

This allows programs to easily extract the "axis labels" for the three dimensions.

Gerv

Comment 2

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Here is a patch. It implements the change suggested above, and also the inconsistency bug that, for CSV output, we output totals for 3D tables but not for 2D or 1D tables. 

Gerv

Comment 3

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Sorry, bug in the previous patch.

Gerv

Comment 4

[Frédéric Buclin]

Comment on attachment 418078 [details] [diff] [review]
Patch v.2

>Index: template/en/default/reports/report-table.csv.tmpl

>-  [% tbl_field_disp FILTER csv %]: [% tbl_disp FILTER csv %]
>+  [% tbl_disp FILTER csv %]
> [% END %]
>-[% IF row_field %]
>-  [% row_field_disp FILTER csv %]
>-[% END %]
>-[% " / " IF col_field AND row_field %]
>-[% col_field_disp FILTER csv %]

IMO, you should combine all three fields into a single one, something like
  [% tbl_field_disp _ ": " _ tbl_disp _ " | " _ row_field_disp _ " | " _ col_field_disp FILTER csv %]
or
  [% tbl_field _ ": " _ tbl_disp _ " | " _ row_field _ " | " _ col_field FILTER csv %]
to avoid parsing problems.

Having no clue about what the columns are is a bad idea.



>Index: template/en/default/reports/report.csv.tmpl

>+[% IF tbl_field AND tbl_names.size > 1 %]
>+  [%# Pop totals table which we don't use %]
>+  [% discard = tbl_names.pop %]
>+  [% discard = data.pop %]
>+[% END %]

data is a hash. You cannot use pop() on it. You can simply ignore data here; we loop over tbl_names.

Comment 5

[Gervase Markham [:gerv]]

> Having no clue about what the columns are is a bad idea.

They are the columns you asked for on the query URL. I agree perhaps in an ideal world we would provide them, but I can't see a good way of providing them all in one cell, and changing the number of cells would be backwardly-incompatible if there's any code out there already which parses this table.

Can you explain how your suggestions avoid parsing problems in the case where various fields contain either a colon or the | character? b.m.o. has many component names, for example, containing a colon. Sadly, I think that the only character guaranteed not to appear in these texts is the comma. :-/ 

> data is a hash. You cannot use pop() on it.

Actually, it seems that calling pop() on it happens to "do the right thing" in TT. But you are right, I should fix this to do the really right thing.

Gerv

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
> Can you explain how your suggestions avoid parsing problems

I don't really care if the parsing is not perfect because a field contains a rarely used character. In these rare cases, the field value could be set manually. But in 99% of cases, I suppose it would just work. This is 99% better than not listing fields used at all (because once you save your file locally, you will quickly forget what the labels were).


> TT. But you are right, I should fix this to do the really right thing.

As I said, you don't need to do anything about this hash. It doesn't matter if it contains extra data; useless data will simply be ignored.

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> I don't really care if the parsing is not perfect because a field contains a
> rarely used character.

Oh, wait. We are talking about fields themselves here, not their values. We have no fields having e.g. "|" in them. So this is safe 100% of the time.

Comment 8

[Gervase Markham [:gerv]]

Attachment: Patch v.3

OK, if we use field names rather than descriptions, we can get unambiguous (if a little complex) parsing. 

Here's a patch using your second suggested combination method (although changing a | to a \ to try and demonstrate a bit better which field name is which axis).

Gerv

Comment 9

[Frédéric Buclin]

Comment on attachment 419885 [details] [diff] [review]
Patch v.3

>Index: template/en/default/reports/report-table.csv.tmpl

>+  [% tbl_field _ ": " _ tbl_disp _ " | " _ row_field _ ' \ ' _ col_field FILTER
>+csv %]

I hope it's not intentional to split this line. csv %] should be at the end of the previous line (I don't care if it exceeds some arbitrary line length, it's more readable).

Comment 10

[Gervase Markham [:gerv]]

I can certainly fix that on checkin.

Gerv

Comment 11

[Gervase Markham [:gerv]]

LpSolit: do you have further review comments on this patch, or just the one in comment #9?

Gerv

Comment 12

[Frédéric Buclin]

(In reply to comment #11)
> LpSolit: do you have further review comments on this patch, or just the one in
> comment #9?

Sorry. I wanted to review it yesterday, but I finally spent my free time helping ghendricks to track a blocker in Testopia, and then I didn't have the time to look at this patch. Doing this in a few minutes.

Comment 13

[Frédéric Buclin]

Comment on attachment 419885 [details] [diff] [review]
Patch v.3

> [% IF tbl_field %]

>+  [% tbl_field _ ": " _ tbl_disp _ " | " _ row_field _ ' \ ' _ col_field FILTER
>+csv %]

Move csv %] on its previous line (this makes t/008filter.t to fail). Also, if either row_field or col_field is undefined, "\" shouldn't be displayed.


>-[% IF row_field %]
>-  [% row_field_disp FILTER csv %]
>-[% END %]
>-[% " / " IF col_field AND row_field %]
>-[% col_field_disp FILTER csv %]

Note that if no multiple tables are displayed, we get no column descs at all, as it's now all enclosed in the first [% IF tbl_field %] block above.