Closed Bug 176461 Opened 21 years ago Closed 21 years ago

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

Categories

(Bugzilla :: User Interface, defect)

2.17
x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: burnus, Assigned: burnus)

References

Details

Attachments

(1 file, 11 obsolete files)

18.05 KB, patch
Details | Diff | Splinter Review
Patch to come.
I left in change-columns.html.tmpl:
[% field_descs = { "summary" => "Summary (first 60 characters)",
		   "summaryfull" => "Full Summary" } %]
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
Review fixes.
Attachment #103963 - Attachment is obsolete: true
I missed to include globals.pl this in my diff :-(
Attachment #103976 - Attachment is obsolete: true
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?
Attachment #103977 - Attachment is obsolete: true
Target Milestone: --- → Bugzilla 2.18
Attached patch v5: Fixes to v4 for multi-edit. (obsolete) — Splinter Review
Note this does not fix/touch bug 76526
Attachment #103990 - Attachment is obsolete: true
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
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?
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
> 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.
Right. Can we put migration code in at the point where we read the COLUMNLIST
cookie?

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

> cookie?
Ok. Added COOKIE migration code.
Attachment #103998 - Attachment is obsolete: true
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
Attachment #104260 - Flags: review-
> I don't quite understand this; what's wrong with:
> $columnlist =~ s/[^_]severity/bug_severity/;

Because that will change bug_severity to bugbug_severity. :-)
> > 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
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.
Attachment #104260 - Attachment is obsolete: true
> 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
Attached patch v8: Fix review nits. (obsolete) — Splinter Review
Attachment #104524 - Attachment is obsolete: true
+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

Blocks: 176526
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?)
Attachment #104735 - Attachment is obsolete: true
OK, this is great. I'll check it in after the current freeze (which should end
in a week or so.)


Gerv
Attached patch v10: Rediff due to bug 114696 (obsolete) — Splinter Review
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?
Attachment #105094 - Attachment is obsolete: true
As v10, but now has
 - push(@bugidlist, $bug->{id});
 + push(@bugidlist, $bug->{'bug_id'});
Attachment #106658 - Attachment is obsolete: true
Attachment #106807 - Flags: review?(gerv)
Attachment #106807 - Flags: review?(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 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
Attachment #106807 - Flags: review?(gerv) → review+
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 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|...
Attachment #106807 - Flags: review?(bbaetz) → review-
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?
Attachment #106807 - Flags: review-
> 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'}}) {
Attachment #106807 - Attachment is obsolete: true
Flags: approval?
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).
Assignee: myk → burnus
Flags: approval? → approval+
Yep, thats fine

Checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.