176461 - Move descs strings from change-columns.html.tmpl to field-descs.html.tmpl

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Tobias Burnus]

Patch to come.

Comment 1

[Tobias Burnus]

Attachment: v1: Move descs strings from change-columns.html.tmpl to field-descs.html.tmpl

I left in change-columns.html.tmpl:
[% field_descs = { "summary" => "Summary (first 60 characters)",
		   "summaryfull" => "Full Summary" } %]

Comment 2

[Gervase Markham [:gerv]]

Two things:

- This creates duplication in the field_descs table. Ideally, change-columns
should use the standard database names for fields. Is it possible to do this?

- Did you test this? If you continue to set field-descs in
change-columns.html.tmpl, it'll overwrite the previously defined one. So most of
them won't be found. You need to set the new values explicitly:
[% field_descs.summary = "Summary (First 60 characters" %]
etc.

Gerv

Comment 3

[Tobias Burnus]

Attachment: v2: Change 3 field names in buglist and add missing to field-descs.html.tmpl

Review fixes.

Comment 4

[Tobias Burnus]

Attachment: v3: As v2 but include also globals.pl changes (default_column_list)

I missed to include globals.pl this in my diff :-(

Comment 5

[Tobias Burnus]

Attachment: v4: As v3 but additionally summary -> short_desc, id -> bug_id, + table.html.tmpl

I also changed table.html.tmpl to use field_descs.html.tmpl as well.
I hope I catched all id => bug_id changes.

Do we still need the title column in buglist.cgi's DefineColumn?

Comment 6

[Tobias Burnus]

Attachment: v5: Fixes to v4 for multi-edit.

Note this does not fix/touch bug 76526

Comment 7

[Gervase Markham [:gerv]]

OK, this looks pretty good. :-)

But the question is: will is break people's COLUMNS cookies? I suspect so. The
question then becomes: do we care? This change does make sense.

Dave? bbaetz?

Gerv

Comment 8

[Bradley Baetz (:bbaetz)]

Hmm.

Whats behind the urge to change the names?

Will this affect buglist.cgi queries/results/etc? If people have existing
COLUMNLIST cookies, won't they now get the errormessage because the cookie is
invalid?

Comment 9

[Gervase Markham [:gerv]]

We want to change the names to avoid duplication in field-descs.html.tmpl , and
for consistency. The question is whether it's worth the cost. It will affect
people's COLUMNLIST cookies, yes. I'm not sure what'd happen...

Gerv

Comment 10

[Tobias Burnus]

> It will affect people's COLUMNLIST cookies, yes. I'm not sure what'd happen...
Well only fields which match will be shown, the others will silently ignored as
I saw testing it. That is summary, severity and status might not be shown by
default if an old cookie exists.

Comment 11

[Gervase Markham [:gerv]]

Right. Can we put migration code in at the point where we read the COLUMNLIST
cookie?

Gerv

Comment 12

[Tobias Burnus]

Attachment: v6: Add migration code to buglist.cgi due to renamed cookies

> Right. Can we put migration code in at the point where we read the COLUMNLIST

> cookie?
Ok. Added COOKIE migration code.

Comment 13

[Gervase Markham [:gerv]]

Comment on attachment 104260 [details] [diff] [review]
v6: Add migration code to buglist.cgi due to renamed cookies

>Index: colchange.cgi
>===================================================================
>-my @masterlist = ("opendate", "changeddate", "severity", "priority",
>-                  "platform", "owner", "reporter", "status", "resolution",
>-                  "product", "component", "version", "os", "votes");
>+my @masterlist = ("opendate", "changeddate", "bug_severity", "priority",
>+                  "platform", "owner", "reporter", "bug_status", "resolution",
>+                  "product", "component", "version", "op_sys", "votes");

owner -> assigned_to.

>-push(@masterlist, ("summary", "summaryfull"));
>+push(@masterlist, ("short_desc", "short_descfull"));

Given we are changing the names here, we should get it right. short_descfull is
actually the real short_desc; the other, truncated one should be
short_short_desc :-) Ideally, actually, this distinction would be made in the
template, but let's leave that for the moment.

>+    # Rename column names (see bug 176461)
>+    my $columnlist = $::COOKIE{'COLUMNLIST'};
>+    $columnlist =~ s/(^| )severity($| )/$1bug_severity$2/;
>+    $columnlist =~ s/(^| )status($| )/$1bug_status$2/;
>+    $columnlist =~ s/(^| )summary($| )/$1short_desc$2/;
>+    $columnlist =~ s/(^| )summaryfull($| )/$1short_descfull$2/;

I don't quite understand this; what's wrong with:
$columnlist =~ s/[^_]severity/bug_severity/;
etc.
?

>+                   "owner"                => "Owner",

You should be converting this to assigned_to...

>                    "percentage_complete"  => "%Complete",
>+                   "platform"             => "Hardware",

...and this one is rep_platform.

Gerv

Comment 14

[Dave Miller [:justdave]]

> I don't quite understand this; what's wrong with:
> $columnlist =~ s/[^_]severity/bug_severity/;

Because that will change bug_severity to bugbug_severity. :-)

Comment 15

[Gervase Markham [:gerv]]

> > I don't quite understand this; what's wrong with:
> > $columnlist =~ s/[^_]severity/bug_severity/;
> 
> Because that will change bug_severity to bugbug_severity. :-)

No, it won't, because it won't match "bug_severity" at all, because the
character before severity has to be not an _.

Gerv

Comment 16

[Tobias Burnus]

Attachment: v7: Review fixes + include table.html.tmpl which I forgot the last time

I wrote:
> Note this does not fix/touch bug 76526
This should be bug 176526: "Buglist.cgi needs to pass priority and bug_severity
to table.html.tmpl". And the fix is not included ...

Gerv wrote:
> owner -> assigned_to.
fixed.

> >+push(@masterlist, ("short_desc", "short_descfull"));
> Given we are changing the names here, we should get it right.
> short_descfull is actually the real short_desc; the other, truncated one 
> should be short_short_desc :-)
Ok. There is now 'short_desc' and 'short_short_desc'.

> Ideally, actually, this distinction would be made in the
> template, but let's leave that for the moment.
Hmm. The problem is that this needs to be saved in the COOKIE. In
table.html.tmpl is the length of 'short_short_desc' defined.

> >+	$columnlist =~ s/(^| )summaryfull($| )/$1short_descfull$2/;
> I don't quite understand this; what's wrong with:
> $columnlist =~ s/[^_]severity/bug_severity/;
Ok. Change to this. version. My version was a bit saver, but we know what is in
the new/old cookie thus [^_] is save enough.

> >+		       "platform"	      => "Hardware",
> ...and this one is rep_platform.
Fixed.

I also included table.html.tmpl which I seemingly forgot to diff.

Comment 17

[Gervase Markham [:gerv]]

> Index: colchange.cgi
> ===================================================================
> -push(@masterlist, ("summary", "summaryfull"));
> +push(@masterlist, ("short_desc", "short_descfull"));

Oops :-)

>  elsif (defined $::COOKIE{'COLUMNLIST'}) {
> +    # Rename column names (see bug 176461)

Dating backwards-compatibility hacks is a good idea. Then we can take them out
after three years or so :-)

> Index: template/en/default/global/field-descs.html.tmpl
> ===================================================================
> @@ -40,7 +41,9 @@
>                     "keywords"             => "Keywords",
>                     "newcc"                => "CC",
>                     "op_sys"               => "OS",
> +                   "opendate"             => "Open Date",
>                     "percentage_complete"  => "%Complete",
> +                   "platform"             => "Hardware",

Oops again :-) It's rep_platform, and I think it's already in there.

Fix those nits, and r=gerv.

Gerv

Comment 18

[Tobias Burnus]

Attachment: v8: Fix review nits.

Comment 19

[Gervase Markham [:gerv]]

+DefineColumn("assigned_to."      , "map_assigned_to.login_name" , "Assignee"  
      );

Spot the mistake :-) Other than that, this looks fine - I'll test it tonight,
and then check it in if it works for me.

Gerv

Comment 20

[Tobias Burnus]

Attachment: v9: Rediff due to bug 62729; remove . from 'assigned_to.'

I updated the patch make sure it doesn't fail now that
  bug 62729, "Add real name capability to bug_list.cgi"
has been checked in yesterday.

(Should the third column from buglist's DefineColumn (i.e. 'title') be removed
since it isn't (shouldn't) be used due to field-descs.html.tmpl?)

Comment 21

[Gervase Markham [:gerv]]

OK, this is great. I'll check it in after the current freeze (which should end
in a week or so.)


Gerv

Comment 22

[Tobias Burnus]

Attachment: v10: Rediff due to bug 114696

Rediff due to patch to bug 114696. Additionally add owner_realname to the
rename-cookie function.

BTW: Can we get rid of the third column of DefineColumn (ID, Name, Title) in
buglist.cgi?

Comment 23

[Tobias Burnus]

Attachment: v11: Fix missed one 'id' => 'bug_id' change

As v10, but now has
 - push(@bugidlist, $bug->{id});
 + push(@bugidlist, $bug->{'bug_id'});

Comment 24

[Bradley Baetz (:bbaetz)]

This looks fine, but I'm going to cheat and wait for Gerv to find bugs ;)

Don't you also have to update column stuff for reports, or is that handled
differently?

Comment 25

[Gervase Markham [:gerv]]

Comment on attachment 106807 [details] [diff] [review]
v11: Fix missed one 'id' => 'bug_id' change

This all seems fine (r=gerv), except:

>@@ -128,11 +135,11 @@
>     [% tableheader %]
>   [% END %]
> 
>-  <tr class="bz_[% bug.severity %] bz_[% bug.priority %] [%+ "bz_secure" IF bug.isingroups %]">
>+  <tr class="bz_[% bug.bug_severity %] bz_[% bug.priority %] [%+ "bz_secure" IF bug.isingroups %]">
> 
>     <td>
>-      [% IF dotweak %]<input type="checkbox" name="id_[% bug.id %]">[% END %]
>-      <a href="show_bug.cgi?id=[% bug.id %]">[% bug.id %]</a>
>+      [% IF dotweak %]<input type="checkbox" name="id_[% bug.bug_id %]">[% END %]
>+      <a href="show_bug.cgi?id=[% bug.bug_id %]">[% bug.bug_id %]</a>

What's all this about? As far as I can tell, we shouldn't be changing the names
of fields in the bug object, should we?

Gerv

Comment 26

[Tobias Burnus]

gerv wrote:
> What's all this about? As far as I can tell, we shouldn't be changing the 
> names of fields in the bug object, should we?

Well this is due to your comment:
> This creates duplication in the field_descs table. Ideally, change-columns
> should use the standard database names for fields. Is it possible to do this?

and those changes:
-DefineColumn("id"                , "bugs.bug_id"                , "ID");
+DefineColumn("bug_id"            , "bugs.bug_id"                , "ID");
-DefineColumn("severity"          , "bugs.bug_severity"          , "Severity");
+DefineColumn("bug_severity"      , "bugs.bug_severity"          , "Severity");

bbaetz wrote:
> This looks fine, but I'm going to cheat and wait for Gerv to find bugs ;)
It's now your turn ;-)

> Don't you also have to update column stuff for reports, or is that handled
> differently?
From the source, it looks completely different and also contains things like:
$columns{'bug_severity'}     = "bugs.bug_severity";
which match the database names.

Comment 27

[Bradley Baetz (:bbaetz)]

Comment on attachment 106807 [details] [diff] [review]
v11: Fix missed one 'id' => 'bug_id' change

Close, but you missed one on line 705

s/id/bug_id/, and r=bbaetz

Interestingly, the reason I noticed this was because the script locked up -
some perl/apache2 interaction with too many warnings too fast??? I only
returned 40 bugs, so it wasn't that many.

strace shows perl blcked writing to stderr, and apache waiting in |poll|...

Comment 28

[Bradley Baetz (:bbaetz)]

Comment on attachment 106807 [details] [diff] [review]
v11: Fix missed one 'id' => 'bug_id' change

Actualy, just make that change and check in. Do you have cvs access?

Comment 29

[Tobias Burnus]

Attachment: v12: As v11 with the one-liner change.

> Actualy, just make that change and check in. Do you have cvs access?
No, I haven't.

This contains:
a) a consistency change of the local variable $id -> $bug_id
+     while (MoreSQLData()) {
+-	  my ($id) = FetchSQLData();
+-	  $privatebugs{$id} = 1;
++	  my ($bug_id) = FetchSQLData();
++	  $privatebugs{$bug_id} = 1;
+     }
b) the 705 change:
+-	  if ($privatebugs{$bug->{id}}) {
++	  if ($privatebugs{$bug->{'bug_id'}}) {

Comment 30

[Dave Miller [:justdave]]

a= justdave  on the condition that bbaetz does the checkin, so he has a chance
to look at what you changed before it goes in, since you changed more than he
asked for (in spite of how small it is).

Comment 31

[Bradley Baetz (:bbaetz)]

Yep, thats fine

Checked in.