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)
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.
Assignee | ||
Comment 1•21 years ago
|
||
I left in change-columns.html.tmpl: [% field_descs = { "summary" => "Summary (first 60 characters)", "summaryfull" => "Full Summary" } %]
Comment 2•21 years ago
|
||
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 4•21 years ago
|
||
I missed to include globals.pl this in my diff :-(
Attachment #103976 -
Attachment is obsolete: true
Assignee | ||
Comment 5•21 years ago
|
||
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•21 years ago
|
Target Milestone: --- → Bugzilla 2.18
Assignee | ||
Comment 6•21 years ago
|
||
Note this does not fix/touch bug 76526
Attachment #103990 -
Attachment is obsolete: true
Comment 7•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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•21 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.
Comment 11•21 years ago
|
||
Right. Can we put migration code in at the point where we read the COLUMNLIST cookie? Gerv
Assignee | ||
Comment 12•21 years ago
|
||
> 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 13•21 years ago
|
||
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-
Comment 14•21 years ago
|
||
> 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•21 years ago
|
||
> > 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•21 years ago
|
||
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
Comment 17•21 years ago
|
||
> 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•21 years ago
|
||
Attachment #104524 -
Attachment is obsolete: true
Comment 19•21 years ago
|
||
+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
Assignee | ||
Comment 20•21 years ago
|
||
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
Comment 21•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
As v10, but now has - push(@bugidlist, $bug->{id}); + push(@bugidlist, $bug->{'bug_id'});
Assignee | ||
Updated•21 years ago
|
Attachment #106658 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #106807 -
Flags: review?(gerv)
Assignee | ||
Updated•21 years ago
|
Attachment #106807 -
Flags: review?(bbaetz)
Comment 24•21 years ago
|
||
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•21 years ago
|
||
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•21 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 27•21 years ago
|
||
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 28•21 years ago
|
||
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•21 years ago
|
||
> 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•21 years ago
|
Flags: approval?
Comment 30•21 years ago
|
||
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+
Comment 31•21 years ago
|
||
Yep, thats fine Checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•