342848 - Not actually changing user preferences should not say user preferences were changed

Status: NEW

(Bugzilla :: User Accounts, defect)

Description

[timeless]

steps:
1. load https://bugzilla.mozilla.org/userprefs.cgi
2. add 25 spaces before your real name
3. enter your password (please, don't trust url fields, load user prefs manually)
4. click submit changes

actual results:
 The changes to your account preferences have been saved.

expected results:
 Some message indicating that there were no changes so nothing was done.

Comment 1

[Max Kanat-Alexander]

Yeah, I agree, that makes sense.

Comment 2

[Emmanuel Seyman]

Attachment: Keep track if changes have been made

Comment 3

[Frédéric Buclin]

Comment on attachment 294790 [details] [diff] [review]
Keep track if changes have been made

>Index: userprefs.cgi

>+                if (crypt($pwd1, $oldcryptedpwd) ne $oldcryptedpwd) {
>+                    $vars->{'changes_made'}++;
>+                }

This IF test is useless as you already know $pwd1 is different from Bugzilla_password. Moreover, write $vars->{'changes_made'} = 1 instead of using your syntax.


>+    if ($realname ne $user->name) {
>+        $vars->{'changes_made'}++;
>+    }

Enclose UPDATE in the IF block so that we only call it if the realname is indeed different. And use =1 instead of ++ as mentioned above.



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

>+  # changes_made : boolean. True if user has changed his password or his realname.

"True if the user has..."


>+  # email_changes_saved: boolean. True if user has requested a change of email
>+  #                      address

Same comment here.


>+  [% ELSE %]
>+    [% UNLESS email_changes_saved %]
>+      No changes to your [% current_tab.label FILTER lower %] were made.

[% UNLESS email_changes_saved %] should go away in this block. If email_changes_saved is true, then changes_made should be true as well.

Comment 4

[Emmanuel Seyman]

Attachment: Taking into account LpSolit's comments

Comment 5

[Frédéric Buclin]

Comment on attachment 294799 [details] [diff] [review]
Taking into account LpSolit's comments

>Index: userprefs.cgi

>+                $vars->{'changes_made'} = 1;



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

>+  [% IF changes_made %]
>     The changes to your [% current_tab.label FILTER lower %] have been saved.


This patch is incorrect. You only define changes_made when accessing the "Name and Password" tab, but account/prefs/prefs.html.tmpl is used for *all* tabs, and so all changes made in other tabs are reported as "No changes made".

Comment 6

[Emmanuel Seyman]

(In reply to comment #5)
>
>                        account/prefs/prefs.html.tmpl is used for *all* tabs,
> and so all changes made in other tabs are reported as "No changes made".

The behavior that timeless describes in the bug description is actually valid for all the preferences panels, not just the "Name and Password" one. Fixing this bug would require changing all the Save* functions in userprefs.cgi to keep track of whether the changes are original or not and I'm not convinced the problem is worth it. Leaving the bug so that somebody else can pick it up.