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

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect, P2)

Description

[Tobias Burnus]

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.");

Comment 1

[Matthew Tuck [:CodeMachine]]

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.

Comment 2

[Dave Miller [:justdave]]

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.

Comment 3

[Matthew Tuck [:CodeMachine]]

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.

Comment 4

[Andreas Franke (gone)]

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.

Comment 5

[Matthew Tuck [:CodeMachine]]

What I'm saying if we did this, the clean solution would essentially require
undoing the patch and changing the template interface.

Comment 6

[Gervase Markham [:gerv]]

Attachment: Patch v.1

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

Comment 7

[Gervase Markham [:gerv]]

Mine, all mine...

Gerv

Comment 8

[Tobias Burnus]

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.

Comment 9

[Gervase Markham [:gerv]]

Attachment: Patch v.2

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

Comment 10

[Andreas Franke (gone)]

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.

Comment 11

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Version without merge problems. r=afranke carries; looking for 2nd review.

Gerv

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 80800 [details] [diff] [review]
Patch v.3

Works, trivial, r=myk

Comment 13

[Gervase Markham [:gerv]]

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