Closed Bug 137183 Opened 21 years ago Closed 21 years ago

userprefs.cgi: tab names should be in the template not in the .cgi

Categories

(Bugzilla :: User Interface, defect, P2)

2.15
x86
All

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: burnus, Assigned: gerv)

Details

Attachments

(1 file, 2 obsolete files)

my @tabs = ( { name => "account", description => "Account settings", 
               saveable => "1" },
             { name => "email", description => "Email settings", 
               saveable => "1" },
             { name => "footer", description => "Page footer", 
               saveable => "1" },
             { name => "permissions", description => "Permissions", 
               saveable => "0" } );

The "description" (tab header) should really be in the
template/default/pref/userprefs.tmpl. This blocks the translation of the
templates: bug 126955 and bug 135604 and is highly user visible.

For the longer term also the error messages need to be moved into a translatable
form:
DisplayError("I was unable to retrieve your old password from the database.");
DisplayError("You did not enter your old password correctly.");
DisplayError("The two passwords you entered did not match.");
DisplayError("You must enter a new password.");
DisplayError("You must enter your old password to change email address.");
DisplayError("Email change already in progress; please check your email.");
DisplayError("Account $new_login_name already exists");
"An email has been sent to both old and new email addresses to confirm the
change of email address.";
DisplayError("Hmm, the $name query seems to have gone away.");
Moving to 2.16 for consideration, but from what I can see I don't think this
should be fixed at all.  It doesn't belong in the template and we shouldn't move
it there given we will be looking at full i18n during 2.18.
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.16
on the contrary, I think this does need to be fixed, for i18n reasons.  Only the
tab "description" which is user visible should be in the template.  The rest of
the stuff should stay in the code.
We are not going to achieve full i18n for 2.16 by a long shot (given
untemplatised output and strings in CGIs) and that never was the intention. 
Given this, I don't think we should be making little hacks because it still
won't be fully done.

For 2.18, message files will deal with the CGI strings and give us a full, if
still suboptimal solution.
Matty: you seem to be saying that if it's not going to be fixed completely, it
should stay broken. I think this is the wrong approach. I18n is a big effort,
and every step counts, and remember that the "bazaar" approach makes software
evolve in steps, and not by doing "perfect" solutions all the time. (Note that I
would understand if this didn't make 2.16 because nobody had time to write or
review a patch.) It's definitely not a 2.16 blocker, but it someone comes up
with a trivial patch, it shouldn't be refused only because it doesn't fix a
dozen other bugs.
What I'm saying if we did this, the clean solution would essentially require
undoing the patch and changing the template interface.
Attached patch Patch v.1 (obsolete) — Splinter Review
This has bits of template migration caught up in it; but it fixes the problem.
Kudos to the reporter - this is a good catch.

Gerv
Mine, all mine...

Gerv
Assignee: myk → gerv
Keywords: patch, review
In the .cgi you change
  -$template->process("prefs/userprefs.tmpl", $vars)
  +$template->process("account/prefs/prefs.html.tmpl", $vars)

and in the template you change
  -[% INCLUDE global/header.html.tmpl
  +[% INCLUDE global/header

I would have thought that -/+ should be reversed:
"global/header.html.tmpl" looks better since you changed templates/en/ whereas
changing "userprefs.cgi" whould move to the new template directory (would fix
part of bug 135707) but I didn't thought that was the purpose of this fix.
Attached patch Patch v.2 (obsolete) — Splinter Review
Here's v.2. Now that the INCLUDE->PROCESS stuff is checked in, this doesn't
have those changes in. Otherwise it's the same.

Gerv
Attachment #80456 - Attachment is obsolete: true
Comment on attachment 80681 [details] [diff] [review]
Patch v.2

Looks good, works. r=afranke.

I get a small reject (userprefs.cgi.rej) because of the template name change:

- $template->process("prefs/userprefs.tmpl", $vars)
+ $template->process("account/prefs/prefs.html.tmpl", $vars)

but this could be my local tree testing the "change-all-names" patch, or if
it's not, it can be safely ignored. 

Now looking for second review.
Attachment #80681 - Flags: review+
Attached patch Patch v.3Splinter Review
Version without merge problems. r=afranke carries; looking for 2nd review.

Gerv
Attachment #80681 - Attachment is obsolete: true
Attachment #80800 - Flags: review+
Comment on attachment 80800 [details] [diff] [review]
Patch v.3

Works, trivial, r=myk
Attachment #80800 - Flags: review+
Fixed.

Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.34; previous revision: 1.33
done
Checking in template/en/default/account/prefs/prefs.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/prefs.html.tmpl,v
 <--  prefs.html.tmpl
new revision: 1.4; previous revision: 1.3
done

Gerv
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.