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

RESOLVED FIXED in Bugzilla 2.18

Status

()

Bugzilla
User Interface
RESOLVED FIXED
15 years ago
5 years ago

People

(Reporter: Tobias Burnus, Assigned: Tobias Burnus)

Tracking

2.17
Bugzilla 2.18
x86
Linux
Bug Flags:
approval +

Details

Attachments

(1 attachment, 11 obsolete attachments)

18.05 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
Patch to come.
(Assignee)

Comment 1

15 years ago
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" } %]
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
(Assignee)

Comment 3

15 years ago
Created attachment 103976 [details] [diff] [review]
v2: Change 3 field names in buglist and add missing to field-descs.html.tmpl

Review fixes.
Attachment #103963 - Attachment is obsolete: true
(Assignee)

Comment 4

15 years ago
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 :-(
Attachment #103976 - Attachment is obsolete: true
(Assignee)

Comment 5

15 years ago
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?
Attachment #103977 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Target Milestone: --- → Bugzilla 2.18
(Assignee)

Comment 6

15 years ago
Created attachment 103998 [details] [diff] [review]
v5: Fixes to v4 for multi-edit.

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
(Assignee)

Comment 10

15 years ago
> 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
(Assignee)

Comment 12

15 years ago
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.
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
(Assignee)

Comment 16

15 years ago
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.
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
(Assignee)

Comment 18

15 years ago
Created attachment 104735 [details] [diff] [review]
v8: Fix review nits.
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

Updated

15 years ago
Blocks: 176526
(Assignee)

Comment 20

15 years ago
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?)
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
(Assignee)

Comment 22

15 years ago
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?
Attachment #105094 - Attachment is obsolete: true
(Assignee)

Comment 23

15 years ago
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'});
(Assignee)

Updated

15 years ago
Attachment #106658 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #106807 - Flags: review?(gerv)
(Assignee)

Updated

15 years ago
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+
(Assignee)

Comment 26

15 years ago
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-
(Assignee)

Comment 29

15 years ago
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'}}) {
Attachment #106807 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
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
Last Resolved: 15 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.