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 User image Tobias Burnus 2002-10-24 04:24:20 PDT
Patch to come.
Comment 1 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Tobias Burnus 2002-10-31 00:06:43 PST
Created attachment 104735 [details] [diff] [review]
v8: Fix review nits.
Comment 19 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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.