117060 - Templatise user_prefs.cgi

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Gervase Markham [:gerv]]

So it was raining on Boxing Day...

Gerv

Comment 1

[Gervase Markham [:gerv]]

User facing -> 2.16 blocker.

Gerv 

Comment 2

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Using the Bugzilla-as-backup principle, here's the first cut. It works, apart
from the fact that the Accounts tab has some problems writing to the DB, and I
can't see why.

Gerv

Comment 3

[Gervase Markham [:gerv]]

Attachment: Patch v.2

This one's the real deal. Note the experimental "Interface" comments for the
templates - comments welcome.

Gerv

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 63023 [details] [diff] [review]
Patch v.2

Looks great. Everything worked as expected.
r=dkl

Comment 5

[Andreas Franke (gone)]

No need to attach a new patch, and I haven't done a full review, but I was
wondering if the DoFoo would be better named LoadFoo (to match with SaveFoo) or
at GetFoo, maybe even {Load_or_Get}Foo{Vars_or_Data_or_Prefs_or_Options} and
SaveFoo{Vars_or_Data_or_Prefs_or_Options}.
If I understand it correctly, the DoFoo routines store their results in the
global $vars variable, whereas the SaveFoo routines get their input from the
$::FORM variable. Maybe the comment could be extended to make this explicit?

What I've seen from the code looks good to me. I like the INTERFACE comments.

Comment 6

[Gervase Markham [:gerv]]

afranke: any chance of a full review? I think the function names are fine for
now - it's pretty obvious what's going on.

Gerv

Comment 7

[Andreas Franke (gone)]

No, sorry, I don't have enough time for a full review (it would cost me at least
half a day to do that carefully). But I have installed it on our production
install, and tested it there. It seems to work fine, and I'm keeping the patch.
Thus t=afranke, if that helps. :-) Unfortunately, there is no "tested"
attachment status flag yet. May I suggest to introduce this, and make the
checkin precondition "either two reviews, or one review and one test" or
something like this?

Comment 8

[Bradley Baetz (:bbaetz)]

Comment on attachment 63023 [details] [diff] [review]
Patch v.2

>Index: template/default/prefs/account.tmpl
>===================================================================
>RCS file: template/default/prefs/account.tmpl
>diff -N template/default/prefs/account.tmpl
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ template/default/prefs/account.tmpl	29 Dec 2001 17:52:18 -0000
>+    <td>
>+      <input type=hidden name="Bugzilla_login" value="[% login %]" />

It has occured to me that very little of our code copes with " as part of the
name. Sigh.
Not sure if this matters here, but we may want to consider it.

>+      <input type=password name="old_password" />  
>+    </td>


>Index: userprefs.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v
>retrieving revision 1.24
>diff -u -r1.24 userprefs.cgi
>--- userprefs.cgi	8 Nov 2001 00:54:15 -0000	1.24
>+++ userprefs.cgi	29 Dec 2001 17:52:22 -0000

>+use Template;

use vars qw ($template $vars); and then use the global one instead

> 
> 
> sub SaveAccount {

If the password has changed, this now needs to call InvalidateLogins().

> }
> 

This is a partial review, because the patch doesn't apply.  A glance through
the diff didn't show anything else, though.

Comment 9

[Gervase Markham [:gerv]]

Attachment: Patch v.3

> It has occured to me that very little of our code copes with " as part of the

> name. Sigh.

It shouldn't have to. Sanity checking should make sure that login names never
contain ". We use FILTER html for fields where this isn't the case.

> If the password has changed, this now needs to call InvalidateLogins().

Er... it does.

New, unrotted patch attached.

Gerv

Comment 10

[Christian Reis]

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

>Index: template/default/prefs/account.tmpl
>+  #
>+  # Contributor(s): Gervase Markham <[e-mail]>
>+  #%]
[...]
>+[%# INTERFACE:
>+    realname: string. The user's real name, if any.
>+    login:    string. The user's Bugzilla login email address.
>+ %]

You could use the same commenting style here for consistency (e.g. # before
each comment line).

This is a great idea.

>+      <input type=hidden name="Bugzilla_login" value="[% login %]" />
>+      <input type=password name="old_password" />  
>+      <input type=password name="new_password1" />
>+      <input type=password name="new_password2" />

Please quote all attribute values (e.g. type="password")

>+<table>
>+  [% IF Param('supportwatchers') %]
>+    <tr>
>+      <td colspan="4">
>+        <hr>

This should be <hr /> for consistency with the rest of the patch.

I'm okay with changing the table HTML here. There are a lot of other little
nits I'd make
on the HTML itself, but it's out of scope and the layout will change after UI
review,
I hope.

>+  [% ELSE %]
>+    <tr>
>+      <td colspan="4">
>+        <br>                            

<br/> (perhaps do a `pmg \<br\>` :-) 

>+          [% FOREACH bit_description = has_bits %]
>+            <li>[% bit_description %]</li>
>+          [% END %]

Do we need a FILTER html here?

>+          [% FOREACH bit_description = set_bits %]
>+            <li>[% bit_description %]</li>
>+          [% END %]

Same? Hmmm, since these have been added through the interface, I guess we could
assume they were
sanitized.

In user_prefs.cgi:

> -    for (my $c=0 ; $c<$::FORM{'numqueries'} ; $c++) {
> +    for (my $c = 0; $c < $::FORM{'numqueries'}; $c++) {

I think the style is "my $c=0", actually.

+# No SavePermissions() because this panel has no changeable fields.

>+    /^permissions$/ && do {
>+        SavePermissions() if $::FORM{'dosave'};

This shouldn't call SavePermissions ever, so you should remove it. The function
isn't even
defined. (You could add a stub function, alternately)

That's all. I've read through fairly carefully, so those nits being fixed,
r=kiko. You can
attach the fixed patch and mark my review.

Comment 11

[Gervase Markham [:gerv]]

Attachment: Patch v.4

> (perhaps do a `pmg \<br\>` :-)

Good plan :-)

> Do we need a FILTER html?

No - the admin might want to use HTML to describe their groups.

All nits fixed; new patch attached.

Gerv

Comment 12

[Gervase Markham [:gerv]]

Comment on attachment 70404 [details] [diff] [review]
Patch v.4

Has r=kiko.

Gerv

Comment 13

[Bradley Baetz (:bbaetz)]

Comment on attachment 70404 [details] [diff] [review]
Patch v.4

I don't have any saved queries, and so line 241 gives me a perl warning:

    for (my $c = 0; $c < $::FORM{'numqueries'}; $c++) {

You either need to set numqueries in the template even if queries.size==0, or
add an if here. The first option is probably cleaner

You also changed the name of the old password field, but that will regress bug
45918. If the fields are specially named, then the login code will deal with
it.

Comment 14

[Gervase Markham [:gerv]]

Attachment: Patch v.5

Comments addressed. Is that r=bbaetz? ;-)

Gerv

Comment 15

[Bradley Baetz (:bbaetz)]

Comment on attachment 71174 [details] [diff] [review]
Patch v.5

Interdiff is doing strange things.

Anyway, if you're sure that color=lightblue (for the tab background) works in
ie, r=bbaetz

Comment 16

[Dave Miller [:justdave]]

Comment on attachment 71174 [details] [diff] [review]
Patch v.5

Tried it in I.E., verified lightblue works.

Gerv states in bug 23067 comment 89 that the restriction of having to enter a
new password in order to change your name or email address is being removed by
this patch, but that is not the case.  If I try to change my real name it
informs me that I must enter a new password.  Otherwise, this looks sweet, and
the other three panels work as expected.  r=justdave if you can fix it so the
new password is only required if you don't change anything else on the panel.

Bonus points if you can implement something like
http://www.justdave.net/maildemo.html for the mail preferences tab.

Comment 17

[Dave Miller [:justdave]]

Comment on attachment 71174 [details] [diff] [review]
Patch v.5

ya know what, that's the same as the existing behavior.  Whether you claimed
you were fixing it or not, let's leave it for the purpose of this patch.  We
can file new bugs for that, or fix it in bug 23067 or something.  Rest of this
works, and duplicates the current behavior, so check it in. :-)

Comment 18

[Dave Miller [:justdave]]

Comment on attachment 71174 [details] [diff] [review]
Patch v.5

OK, so I'm reversing my decision again, but on less serious grounds this
time...

The template moves the Real Name field from the bottom to the top.  This seems
pretty innocuous, but Mozilla now sees a "Name:" field followed by a
"password:" field, and offers to remember your password.  Which would be a Bad
Thing on this page.  Move the Real Name field back to the bottom so Mozilla's
password manager doesn't kick in and this is r= justdave.

Comment 19

[Gervase Markham [:gerv]]

Attachment: Patch v.6

Try this on for size. I fixed the email layout :-)

Gerv

Comment 20

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

Comment on attachment 71284 [details] [diff] [review]
Patch v.6

>Index: template/default/prefs/account.tmpl

>+      <input type="hidden" name="Bugzilla_login" value="[% login %]" />

Quotation marks can appear in email addresses according to RFC 822,
and Bugzilla allows such email addresses by default and could be
configured to accept them even if they weren't accepted by default
(via the emailregexp parameter), so "login" needs to be HTML filtered.


>Index: template/default/prefs/email.tmpl
>Index: template/default/prefs/footer.tmpl
>Index: template/default/prefs/permissions.tmpl

>Index: template/default/prefs/userprefs.tmpl

>+  [% filename = BLOCK %]prefs/[% current_tab.name %].tmpl[% END %]
>+  [% INCLUDE $filename %]

Nit: [% INCLUDE "prefs/${current_tab.name}.tmpl" %]


>Index: userprefs.cgi

>-# Shut up misguided -w warnings about "used only once".  "use vars" just
>+# Shut up misguided -w warnings about "used only once". "use vars" just

Nit: two spaces simulates correct inter-sentence spacing in a fixed 
font display system like the ones that tend to be used in code editors.


>+    (my $flagstring) = FetchSQLData();

Nit: my ($flagstring)


>+            my %role;
>+            $vars->{$1} = \%role;

Nit: $vars->{$1} = {};

When I go to the "Page Footer" tab and turn off the My Bugs query
and my other query, I get the following error:

---
Attempted to send tainted string 'UPDATE namedqueries 
SET linkinfooter = 0 WHERE userid = 27300 AND name = 'blah'' 
to the database at globals.pl line 220.
---

Other than that it looks good.

Comment 21

[Gervase Markham [:gerv]]

Attachment: Patch v.7

All comments addressed apart from double-spacing periods. I believe in
single-spacing, as I think it looks nicer.

Ready for re-review.

Gerv

Comment 22

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

Comment on attachment 71633 [details] [diff] [review]
Patch v.7

Looks good, works.  r=myk

Note that putting the real name field below the password field does not
necessarily prevent Mozilla from asking to save the information (try entering
just your old password and your real name), but Mozilla appears to behave when
filling in all the fields as is currently necessary.  We should move the real
name field up to the top and obscure the field names to suppress Mozilla
sillyness, but that can wait for a future bug.

Comment 23

[Gervase Markham [:gerv]]

Dave, bbaetz - can one of you rubber-stamp patch v.7? :-)

Gerv

Comment 24

[Dave Miller [:justdave]]

Comment on attachment 71633 [details] [diff] [review]
Patch v.7

r= justdave

Comment 25

[Dave Miller [:justdave]]

bug 128158 has been filed to deal with the password manager issue.

Comment 26

[Gervase Markham [:gerv]]

Fixed. Thanks, everyone :-) Only 10 left...

Gerv

Comment 27

[Matthew Tuck [:CodeMachine]]

Who here ran their test suite, hmm?

-  [% INCLUDE "prefs/${current_tab.name}.tmpl" %]
+  [% INCLUDE "prefs/${current_tab.name}.tmpl" IF current_tab.name.defined %]

Checking in template/default/prefs/userprefs.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/prefs/userprefs.tmpl,v  <-- 
userprefs.tmpl
new revision: 1.2; previous revision: 1.1
done

Comment 28

[Gervase Markham [:gerv]]

Not me. <sigh>. I really need to include that in my auto-checkin script for
Bugzilla, obviously...

Gerv

Comment 29

[David Lawrence [:dkl]]

Found a problem with the latest version of userprefs.cgi that I may just be 
crazy but wanted to run it by you anyway. Seems that the ExcludeSelf settings 
are not getting save properly on my mozilla tip installation. Looks like the 
following lines are responsible 
 
# Determine the value of the "excludeself" global email preference. 
    # Note that the value of "excludeself" is assumed to be off if the 
    # preference does not exist in the user's list, unlike other  
    # preferences whose value is assumed to be on if they do not exist. 
    if (exists($emailflags{'excludeself'})  
        && $emailflags{'excludeself'} eq 'on') 
    { 
        $vars->{'excludeself'} = 1; 
    } 
    else { 
        $vars->{'excludeself'} = 0; 
    } 
 
Should be (at least makes it work for me) 
 
    if (exists($emailflags{'ExcludeSelf'}) 
        && $emailflags{'ExcludeSelf'} eq 'on') 
 
Does anyone else see this or am I out of date? 

Comment 30

[Gervase Markham [:gerv]]

There's definitely a problem here. Please file a new bug and either attach your
patch or assign it to me :-)

Gerv