Last Comment Bug 176461 - Move descs strings from change-columns.html.tmpl to field-descs.html.tmpl
: Move descs strings from change-columns.html.tmpl to field-descs.html.tmpl
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: User Interface (show other bugs)
: 2.17
: x86 Linux
: -- normal (vote)
: Bugzilla 2.18
Assigned To: Tobias Burnus
: default-qa
:
Mentors:
Depends on:
Blocks: 176526
  Show dependency treegraph
 
Reported: 2002-10-24 04:24 PDT by Tobias Burnus
Modified: 2012-12-18 20:46 PST (History)
3 users (show)
justdave: approval+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1: Move descs strings from change-columns.html.tmpl to field-descs.html.tmpl (3.85 KB, patch)
2002-10-24 04:28 PDT, Tobias Burnus
no flags Details | Diff | Splinter Review
v2: Change 3 field names in buglist and add missing to field-descs.html.tmpl (6.61 KB, patch)
2002-10-24 08:49 PDT, Tobias Burnus
no flags Details | Diff | Splinter Review
v3: As v2 but include also globals.pl changes (default_column_list) (7.28 KB, patch)
2002-10-24 08:52 PDT, Tobias Burnus
no flags Details | Diff | Splinter Review
v4: As v3 but additionally summary -> short_desc, id -> bug_id, + table.html.tmpl (11.37 KB, patch)
2002-10-24 10:38 PDT, Tobias Burnus
no flags Details | Diff | Splinter Review
v5: Fixes to v4 for multi-edit. (15.49 KB, patch)
2002-10-24 11:26 PDT, Tobias Burnus
no flags Details | Diff | Splinter Review
v6: Add migration code to buglist.cgi due to renamed cookies (12.96 KB, patch)
2002-10-26 05:07 PDT, Tobias Burnus
gerv: review-
Details | Diff | Splinter Review
v7: Review fixes + include table.html.tmpl which I forgot the last time (16.43 KB, patch)
2002-10-29 10:32 PST, Tobias Burnus
no flags Details | Diff | Splinter Review
v8: Fix review nits. (16.33 KB, patch)
2002-10-31 00:06 PST, Tobias Burnus
no flags Details | Diff | Splinter Review
v9: Rediff due to bug 62729; remove . from 'assigned_to.' (18.07 KB, patch)
2002-11-04 11:00 PST, Tobias Burnus
no flags Details | Diff | Splinter Review
v10: Rediff due to bug 114696 (17.31 KB, patch)
2002-11-18 07:32 PST, Tobias Burnus
no flags Details | Diff | Splinter Review
v11: Fix missed one 'id' => 'bug_id' change (17.57 KB, patch)
2002-11-19 09:29 PST, Tobias Burnus
gerv: review+
bbaetz: review+
Details | Diff | Splinter Review
v12: As v11 with the one-liner change. (18.05 KB, patch)
2002-12-07 09:36 PST, Tobias Burnus
no flags Details | Diff | Splinter Review

Description Tobias Burnus 2002-10-24 04:24:20 PDT
Patch to come.
Comment 1 Tobias Burnus 2002-10-24 04:28:28 PDT
Created attachment 103963 [details] [diff] [review]
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] 2002-10-24 05:10:35 PDT
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 2002-10-24 08:49:20 PDT
Created attachment 103976 [details] [diff] [review]
v2: Change 3 field names in buglist and add missing to field-descs.html.tmpl

Review fixes.
Comment 4 Tobias Burnus 2002-10-24 08:52:18 PDT
Created attachment 103977 [details] [diff] [review]
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 2002-10-24 10:38:59 PDT
Created attachment 103990 [details] [diff] [review]
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 2002-10-24 11:26:36 PDT
Created attachment 103998 [details] [diff] [review]
v5: Fixes to v4 for multi-edit.

Note this does not fix/touch bug 76526
Comment 7 Gervase Markham [:gerv] 2002-10-24 13:51:31 PDT
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) 2002-10-24 21:04:41 PDT
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] 2002-10-24 23:58:26 PDT
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 2002-10-25 01:21:37 PDT
> 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] 2002-10-25 11:37:42 PDT
Right. Can we put migration code in at the point where we read the COLUMNLIST
cookie?

Gerv
Comment 12 Tobias Burnus 2002-10-26 05:07:25 PDT
Created attachment 104260 [details] [diff] [review]
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] 2002-10-28 11:07:31 PST
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] (justdave@bugzilla.org) 2002-10-28 15:17:52 PST
> 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] 2002-10-28 15:47:18 PST
> > 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 2002-10-29 10:32:22 PST
Created attachment 104524 [details] [diff] [review]
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] 2002-10-30 14:58:36 PST
> 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 2002-10-31 00:06:43 PST
Created attachment 104735 [details] [diff] [review]
v8: Fix review nits.
Comment 19 Gervase Markham [:gerv] 2002-11-04 10:03:35 PST
+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 2002-11-04 11:00:29 PST
Created attachment 105094 [details] [diff] [review]
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] 2002-11-04 14:38:22 PST
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 2002-11-18 07:32:07 PST
Created attachment 106658 [details] [diff] [review]
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 2002-11-19 09:29:41 PST
Created attachment 106807 [details] [diff] [review]
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) 2002-11-30 21:49:19 PST
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] 2002-12-05 14:17:44 PST
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 2002-12-05 14:42:27 PST
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) 2002-12-06 22:24:12 PST
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) 2002-12-06 23:07:20 PST
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 2002-12-07 09:36:25 PST
Created attachment 108598 [details] [diff] [review]
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] (justdave@bugzilla.org) 2002-12-07 10:12:23 PST
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) 2002-12-07 16:43:39 PST
Yep, thats fine

Checked in.

Note You need to log in before you can comment on or make changes to this bug.