New account autoconfiguration wizard does not obey signon.rememberSignons

RESOLVED FIXED in Thunderbird 3.3a2

Status

Thunderbird
Account Manager
--
critical
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: pilgrimd, Assigned: Daniel)

Tracking

(Blocks: 1 bug, {regression, student-project})

Thunderbird 3.3a2
regression, student-project
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-thunderbird3.1 .8+, thunderbird3.1 .8-fixed)

Details

Attachments

(1 attachment, 13 obsolete attachments)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.5) Gecko/20091204 Thunderbird/3.0

We have use a Mission Control configuration script, called from greprefs/all.js, which, among other settings, locks signon.rememberSignons=false so our users can not store their passwords in Thunderbird's insecure password file.

In Thunderbird 2.x, locking signon.rememberSignons=false has the desired effect: the user is never given the option to save their password--they must enter every time they start Thunderbird.

Thunderbird 3.x with an existing account configuration migrated from an existing profile exhibits the same behaviour.  When setting up a new profile or new mail account using the wizard, the "Remember password" check-box on the form is not disabled or removed.  If the user tells the wizard to save their password, their password is stored in the signons file and subsequent launches use the stored password, thereby completely defeating locking signon.rememberSignons=false.


Reproducible: Always

Steps to Reproduce:
1. Add an MC configuration script which disables saving passwords by calling lockPref("signon.rememberSignons", false);
2. Launch v3 with no existing profile;
3. Configure a mail account using the wizard with the "Remember password" option checked;
4. Restart Thunderbird, observe not being prompted for a password;
5. Go to Tools->Options->Security tab->Saved Passwords... button and observe stored passwords;
6. Go to Tools->Options->Advanced tab->General subtab->Config Editor... button and observe signon.rememberSignons showing a value of "false" and a status of "locked", indicating v3 does register the lockPref() call.
Actual Results:  
A properly configured IMAP account with account password stored in the signons SQLite database in my profile directory, no prompting for a password and signon.rememberSignons locked to a value of false.

Expected Results:  
If signon.rememberSignons is false (either by default or by locking), the wizard should not give the user the option to save their password.
(Reporter)

Updated

8 years ago
Severity: major → critical
blocking-thunderbird3.1: --- → ?
Version: unspecified → 3.0
Blake could this be a good student project ?
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
I think a student could do this, but I don't know about the wisdom of having student projects on a potential blocking list…

(Thinking about it, I propose that we should mark them as student-project, and if it turns out that it is blocking and isn't being moved forward quickly enough, someone else can take it over and drive it in.)

Thanks,
Blake.
Keywords: student-project
Daniel might be interested in working on this as part of a student project - assigning to him.
Assignee: nobody → greenrecyclebin
Status: NEW → ASSIGNED

Comment 4

8 years ago
Not a blocker, though we'd welcome a patch.
blocking-thunderbird3.1: ? → -
(Assignee)

Comment 5

8 years ago
Created attachment 429723 [details] [diff] [review]
Patch #1

- Add onFocus() function to correct the behavior of remember_password checkbox in case we change signon.rememberSignons = true and come back.

- Minor changes to oninputPassword() and onblurPassword().
Attachment #429723 - Flags: review?(gary)
Attachment #429723 - Flags: review?(bwinton)
Comment on attachment 429723 [details] [diff] [review]
Patch #1

Unfortunately, I'm not in a position to review the code, deferring to bienvenu instead...
Attachment #429723 - Flags: review?(gary) → review?(bienvenu)

Comment 7

8 years ago
Comment on attachment 429723 [details] [diff] [review]
Patch #1

Blake's review should be sufficient, but some comments anyway -

can you use let instead of var? Also, change the name of "pref" to something that indicates which pref it is, e.g., let rememberSignons = prefs.getBoolPref("...");

Also, you can use Ci instead of Components.interfaces.

Updated

8 years ago
Attachment #429723 - Flags: review?(bienvenu)
Comment on attachment 429723 [details] [diff] [review]
Patch #1

>+    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                    .getService(Components.interfaces.nsIPrefBranch);
>+    var pref = prefs.getBoolPref("signon.rememberSignons");

Sorry, I'm going to chime in here as well. Using:

Application.prefs.getValue("signon.rememberSignons", true);

(where true is the default value)

would be a much simpler way, the added benefit is that Application caches the prefs service in javascript so that we're not obtaining it from the component cache all the time. Not many people have realised this, which is why I'm bringing it up.
(Assignee)

Comment 9

8 years ago
Thanks everyone for all the feedback. I will revise my patch and include tests soon.
(Assignee)

Comment 10

8 years ago
Created attachment 430096 [details] [diff] [review]
Patch #2

Same behavior as Patch #1, apart from changes below:
    - Change pref variable to a meaningful name, i.e. rememberSignons_pref
    - Use Application.prefs.getValue("signon.rememberSignons", true); as Standard8 suggested
    - Use "let" instead of "var" for local variables
Attachment #429723 - Attachment is obsolete: true
Attachment #430096 - Flags: review?(bwinton)
Attachment #429723 - Flags: review?(bwinton)
(Assignee)

Comment 11

8 years ago
Created attachment 430097 [details]
rememberSignons = true (default value) test

Test case when signon.rememberSignons is set to "true" (default value)
(Assignee)

Comment 12

8 years ago
Created attachment 430098 [details]
rememberSignons = false test

Test case when signon.rememberSignons is set to "false"
(Assignee)

Comment 13

8 years ago
Apart from the two testcases uploaded, there is still one case we have to consider.

That is when the user is currently in the account wizard, and decided to toggle signon.rememberSignons value (from true->false, or false->true) and come back. The onFocus() function will ensure correct behavior for this situation. (I'm not sure if someone actually do this, but just to be sure =) ).

When running the test, you will have to manually set signon.rememberSignons to the correct value. Sorry for this troublesome, but I haven't figured out how to set preference value inside Mozmill test.

Thanks.
(Assignee)

Comment 14

8 years ago
Created attachment 430111 [details] [diff] [review]
Patch #3

Patch with tests included.

Thanks.
Attachment #430096 - Attachment is obsolete: true
Attachment #430097 - Attachment is obsolete: true
Attachment #430098 - Attachment is obsolete: true
Attachment #430111 - Flags: review?(bwinton)
Attachment #430096 - Flags: review?(bwinton)
Comment on attachment 430111 [details] [diff] [review]
Patch #3

Getting closer, but it looks like this patch contains the tests and a file which is the first patch, instead of containing all the changes from both the tests and the first patch…

(Specifically, I don't expect to see these lines in the diff:
diff -r 6804e330ba06 patch/emailWizard.patch2
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/patch/emailWizard.patch2	Thu Mar 04 03:29:43 2010 +0800
)

(If that doesn't make sense, please jump onto IRC and I'll try to explain it more.)

Thanks,
Blake.
Attachment #430111 - Flags: review?(bwinton)
(Assignee)

Comment 16

8 years ago
Created attachment 432739 [details] [diff] [review]
Patch #4

This is a new patch which contains only the necessary fix to the bug.

Thanks,
Daniel
Attachment #430111 - Attachment is obsolete: true
Attachment #432739 - Flags: review?(bwinton)
Attachment #432739 - Flags: review?(bwinton) → review-
Comment on attachment 432739 [details] [diff] [review]
Patch #4

>+++ b/mail/test/mozmill/accountcreation-wizard/test-accountcreation-wizard-pref_false.js	Tue Mar 16 13:31:40 2010 +0800
>+++ b/mail/test/mozmill/accountcreation-wizard/test-accountcreation-wizard-pref_true.js	Tue Mar 16 13:31:40 2010 +0800

These two files seem _really_ similar.  Perhaps you could make one file with a function that takes a parameter, and call that function twice, so that we don't duplicate logic?

>@@ -0,0 +1,34 @@
>+var setupModule = function (module) {  
>+    module.controller = new mozmill.controller.MozMillController(mozmill.utils.getWindowByType("mail:autoconfig"));

But, that's not why I'm giving this an r-.  I'm giving it an r- because the previous line causes both tests to fail for me:
TEST-UNEXPECTED-FAIL |  setupModule
  EXCEPTION: window is null
    at: controller.js line 200
            controller.js 200
            test-accountcreation-wizard-pref_false.js 8
            frame.js 463
            frame.js 498
            frame.js 562
            frame.js 411
            frame.js 568
            server.js 164
            server.js 168
TEST-PASS |  test_remember_checked_when_enter_pass_and_pref
TEST-UNEXPECTED-FAIL |  setupModule
  EXCEPTION: window is null
    at: controller.js line 200
            controller.js 200
            test-accountcreation-wizard-pref_true.js 8
            frame.js 463
            frame.js 498
            frame.js 562
            frame.js 411
            frame.js 568
            server.js 164
            server.js 168
TEST-PASS |  test_remember_checked_when_enter_pass_and_pref

Oh, and you should replace:
  var setupModule = function (module) {
with:
  function setupModule(module)
  {

And add "accountcreation-wizard" to mail/test/mozmill/mozmilltests.list

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js	Tue Mar 16 13:31:40 2010 +0800
>@@ -157,6 +157,24 @@
>     document.getElementById("fullspacer").width = document.width;
>     window.sizeToContent();
>   },
>+  
>+  // Correct the behaviour of remember_password checkbox in case we
>+  // change signon.rememberSignons = true and come back

Please switch that to javadoc style.
(with /** 
       * comments here.
       */
)

>+  onFocus: function() {
>+    let passwordElt = document.getElementById("password");
>+    let rememberPasswordElt = document.getElementById("remember_password");
>+    let rememberSignons_pref = Application.prefs.getValue("signon.rememberSignons", true);

Since we already have "passwordElt", and "rememberPasswordElt", what do you think about changing this to "rememberSignonsPref", for consistency?

And that's probably enough out of me for this review.  :)

I look forward to reviewing the next version of this patch, and to seeing what clarkbw thinks of the UI.

Thanks,
Blake.
(Assignee)

Comment 18

8 years ago
Hi Blake,

The test is written to be run by Mozmill extension right after you start Thunderbird/Shredder. The test assumes that the wizard is opened before the test begin. (This is the default behavior; when you start, the wizard will be opened first).

Moreover, because I cannot dynamically set the preference signons.rememberSignons to the desired value using Mozmill (Mozmill cannot select the preference value, it can only recognize the "configTreeBody" -> therefore, we cannot double-click or do anything to change the value), I choose to create 2 test cases for it. That explains the troublesome of manually setting the preference to the correct value before running the test.

Thanks,
Daniel

PS: Please take a look at the screenshot, it explains why Mozmill cannot select a preference value (this may be a bug?? I'm not sure).
(Assignee)

Comment 19

8 years ago
Created attachment 433536 [details]
Mozmill cannot select a preference value

Mozmill only recognizes the container as "configTreeBody". It cannot select a particular preference value.
(In reply to comment #18)
> The test is written to be run by Mozmill extension right after you start
> Thunderbird/Shredder. The test assumes that the wizard is opened before the
> test begin. (This is the default behavior; when you start, the wizard will be
> opened first).

Okay, that makes sense, but it would be really nice if we could run the tests in an automated way from the command line, instead of having to start Thunderbird manually.

Fortunately, we can do this!  ;)

Check out <https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing>, and on your command line, try to run "make SOLO_TEST=accountcreation-wizard mozmill-one", and see what you can do to get that working.

> Moreover, because I cannot dynamically set the preference
> signons.rememberSignons to the desired value using Mozmill (Mozmill cannot
> select the preference value, it can only recognize the "configTreeBody" ->
> therefore, we cannot double-click or do anything to change the value), I choose
> to create 2 test cases for it. That explains the troublesome of manually
> setting the preference to the correct value before running the test.

If you check mail/test/mozmill/content-tabs/test-content-tab.js you can see how to get and set a pref programmatically from a mozmill test.

Thanks,
Blake.
(Assignee)

Comment 21

7 years ago
Created attachment 435102 [details] [diff] [review]
Patch #5
Attachment #432739 - Attachment is obsolete: true
Attachment #433536 - Attachment is obsolete: true
Attachment #435102 - Flags: review?(bwinton)
Andrew: iirc there was a reason why we don't use the mc.assert* functions in the mozmill tests with Thunderbird? Was it something like the stacks don't always unwind correctly?
The asserts just end up throwing exceptions (although some of them do emit side-channel action meta-info over jsbridge) which is equivalent to what we do.

I think some of the DOM-related assert stuff just didn't work.  Otherwise, I probably just didn't see an advantage to calling an assert, especially if it was going to result in a generic exception being thrown instead of a very specific one.

I would make sure that any asserts that might rely on magic (like DOM fun and XBL bindings) fail appropriately.  For example, change the DOM id or cause a checkbox to not be checked when it should be checked, etc.  (This all would be done via human tweaking and not part of the permanent test.)
Comment on attachment 435102 [details] [diff] [review]
Patch #5

>+++ b/mail/test/mozmill/accountcreation-wizard/test.js	Fri Mar 26 01:53:39 2010 +0800

Please merge these tests into the (newly-created) mail/test/mozmill/account/test-mail-account-setup.wizard.js

(I recommend creating a new test function in that file to test the things that you want to.  It should already have the correct scaffolding.)

>+function setupModule (module) {
[snip…]
>+  plan_for_modal_dialog("mail:autoconfig", autoconfig_callback); // running test with pref value set to true

In particular, I don't think running the tests from the setupModule function was really what you wanted to do here.

>+++ b/mail/test/mozmill/mozmilltests.list	Fri Mar 26 01:53:39 2010 +0800
>@@ -6,3 +6,4 @@
> folder-pane
>+accountcreation-wizard

And we won't need this change, either, if you put it in the other file.

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js	Fri Mar 26 01:53:39 2010 +0800
>@@ -157,6 +157,26 @@
>+  

You have some trailing whitespace here which we should remove.

>+  /**
>+   * Correct the behaviour of remember_password checkbox in case we
>+   * change signon.rememberSignons = true and come back
>+   */
>+  onFocus: function() {
>+    let passwordElt = document.getElementById("password");
>+    let rememberPasswordElt = document.getElementById("remember_password");
>+    let rememberSignons_pref = Application.prefs.getValue("signon.rememberSignons", true);
>+ 
>+    if (rememberSignons_pref) {
>+      rememberPasswordElt.disabled = false;
>+      if (!this._userChangedPassword && passwordElt.value.length >= 1)
>+        rememberPasswordElt.checked = true;

Why are we checking if the user changed their password here?  Is it because the value will be the emptyText if they didn't?

>+    }
>+    else {
>+      rememberPasswordElt.disabled = true;
>+      rememberPasswordElt.checked = false; 

I don't think it matters, but I'ld prefer to see this unchecked before it's disabled.  (It makes more sense to me that you would uncheck it while it's still enabled.)

>@@ -387,15 +407,22 @@
>-    rememberPasswordElt.disabled = false;
>+    let rememberSignonsPref = Application.prefs.getValue("signon.rememberSignons", true);
>+    if (rememberSignonsPref)
>+      rememberPasswordElt.disabled = false;
>+    else {
>+      rememberPasswordElt.disabled = true;
>+      rememberPasswordElt.checked = false; 
>+    }
>     if (passwordElt.value.length < 1) {

I'ld like to see a blank line before this if statement, so that I don't think it's an else if from the previous block.

>-    else if (!this._userChangedPassword)
>-      rememberPasswordElt.checked = true;
>-  },
>+    else if (!this._userChangedPassword && rememberSignonsPref)
>+       rememberPasswordElt.checked = true;
>+   },

You seem to have indented this closing } one too many spaces.

Other than those nits, I'm really happy with it, but I'm going to r- because I want to see how you fix the tests.

Finally, since this is affecting some UI, we should probably get a ui-r from clarkbw before this gets committed.  Please post a list of steps Bryan can use to see what you've changed in the UI.

Thanks,
Blake.
Attachment #435102 - Flags: ui-review?(clarkbw)
Attachment #435102 - Flags: review?(bwinton)
Attachment #435102 - Flags: review-
(Assignee)

Comment 25

7 years ago
> >+  /**
> >+   * Correct the behaviour of remember_password checkbox in case we
> >+   * change signon.rememberSignons = true and come back
> >+   */
> >+  onFocus: function() {
> >+    let passwordElt = document.getElementById("password");
> >+    let rememberPasswordElt = document.getElementById("remember_password");
> >+    let rememberSignons_pref = Application.prefs.getValue("signon.rememberSignons", true);
> >+ 
> >+    if (rememberSignons_pref) {
> >+      rememberPasswordElt.disabled = false;
> >+      if (!this._userChangedPassword && passwordElt.value.length >= 1)
> >+        rememberPasswordElt.checked = true;
> 
> Why are we checking if the user changed their password here?  Is it because the
> value will be the emptyText if they didn't?
Yes, this is exactly the reason why. The password field's type is "text" when it is empty. Its type is changed to "password" only when it is not empty.

> Finally, since this is affecting some UI, we should probably get a ui-r from
> clarkbw before this gets committed.  Please post a list of steps Bryan can use
> to see what you've changed in the UI.

I'm not sure if my code made any changes to the UI or not. It only registered the onFocus() function in emailWizard.xul to handle the case where user changes signon.rememberSignons, then comes back to the wizard.

Finally, the test has been added to "mail/test/mozmill/account/test-mail-account-setup.wizard.js".

Thanks,
Daniel
(Assignee)

Comment 26

7 years ago
Created attachment 443056 [details] [diff] [review]
Patch #6
Attachment #435102 - Attachment is obsolete: true
Attachment #443056 - Flags: ui-review?(clarkbw)
Attachment #443056 - Flags: review?(bwinton)
Attachment #435102 - Flags: ui-review?(clarkbw)
Attachment #443056 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 443056 [details] [diff] [review]
Patch #6

This looks fine, I probably would have recommended just hiding the option from the dialog when it's not available but disabled will work as well.  I haven't tested this yet, I'm a little concerned that the onFocus will lead to the item looking enabled and then changing to disabled.  Without any feedback as to why this will be a bit confusing and possibly upsetting for some users; if this is the case.

In general though this looks fine.
Comment on attachment 443056 [details] [diff] [review]
Patch #6

>+++ b/mail/test/mozmill/account/test-mail-account-setup-wizard.js	Mon May 03 16:34:46 2010 +0800
>@@ -186,3 +186,62 @@
>+function test_remember_password() {
>+  remember_password_test(true);
>+  remember_password_test(false);
>+}
>+
>+/* Test remember_password checkbox behavior with
>+ * signon.rememberSignons set to "prefValue"
>+ */
>+function remember_password_test(prefValue) {

We like to name our parameters "aPrefValue", to distinguish them from local variables.

>+  let pref = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
>+  let rememberSignons_pref_save = pref.getBoolPref("signon.rememberSignons", true); // save the pref for backup purpose

This line is way over 80 characters, and doesn't really need to be if you move the comment to a different line.

>+  

This line and some others have some trailing spaces.  Please remove them.

>+  pref.setBoolPref("signon.rememberSignons", prefValue);
>+  let rememberSignons_pref = pref.getBoolPref("signon.rememberSignons", true);

Didn't you just set that to prefValue?  I think you can just use prefValue instead of rememberSignons_pref in the rest of this function.

>+  // password field is empty and the checkbox is disabled initially -> uncheck checkbox

If you break this after the ->, it'll fit into 80 characters.
(See below for some examples.)

>+  awc.sleep(1000);

Why are you sleeping here?  It makes the tests much slower, and there are usually better ways to do whatever it is you want to do.

>+  if (rememberSignons_pref) {
>+    // password field is not empty any more -> enable and check checkbox
>+    awc.assertProperty(rememberPassword, "disabled", false);
>+    awc.assertChecked(rememberPassword);
>+  }
>+  else {
>+    // password field is not empty any more, but prefValue is false -> disable and uncheck checkbox
>+    awc.assertProperty(rememberPassword, "disabled", true);
>+    awc.assertNotChecked(rememberPassword);
>+  }

You could do both the assertProperty checks with the single line:
awc.assertProperty(rememberPassword, "disabled", !aPrefValue);
which would turn that block into:

  awc.assertProperty(rememberPassword, "disabled", !aPrefValue);
  if (aPrefValue)
    // password field is not empty any more, and aPrefValue is true ->
    //   enable and check checkbox
    awc.assertChecked(rememberPassword);
  else
    // password field is not empty any more, and prefValue is false ->
    //   disable and uncheck checkbox
    awc.assertNotChecked(rememberPassword);

>+  // empty the password field
>+  awc.keypress(password, 'a', {ctrlKey: true});
>+  awc.keypress(password, 'VK_DELETE', {});

Heh.  Nice!

So, my final problem is that I can't get this test to run successfully.
I'm using "make SOLO_TEST=account/test-mail-account-setup-wizard.js mozmill-one", but it fails with the following error:
TEST-PASS |  setupModule
TEST-PASS |  test_mail_account_setup
TEST-UNEXPECTED-FAIL |  test_remember_password
  EXCEPTION: Timed out waiting for window open!
    at: test-window-helpers.js line 193
       Error("Timed out waiting for window open!")  0
       WindowWatcher_waitForWindowOpen("mail:autoconfig") test-window-helpers.js 193
       wait_for_new_window("mail:autoconfig") test-window-helpers.js 504
       open_mail_account_setup_wizard() test-mail-account-setup-wizard.js 68
       remember_password_test(true) test-mail-account-setup-wizard.js 202
       test_remember_password() test-mail-account-setup-wizard.js 191
            frame.js 468
            frame.js 520
            frame.js 562
            frame.js 420
            frame.js 574
            server.js 164
            server.js 168

Apart from that, the logic looks good, so if we can just fix these few things, you'll get an r+ from me.  (But we're not quite there yet.)

Thanks,
Blake.
Attachment #443056 - Flags: review?(bwinton) → review-
Daniel, (small world.) Are you able to do another patch iteration?

Looks like my institution might be bit by this.
Blocks: 564148
blocking-thunderbird3.1: - → ?
Keywords: regression
(Assignee)

Comment 30

7 years ago
(In reply to comment #29)
> Daniel, (small world.) Are you able to do another patch iteration?
> 
> Looks like my institution might be bit by this.

Sure, I will try to close this bug by the end of this week :P I'm currently in the middle of the last week of GSoC2010, lots of documentation to finish :)
(Assignee)

Comment 31

7 years ago
Created attachment 465944 [details] [diff] [review]
Patch #7

I have fixed all the mentioned errors. This now I used http://beaufour.dk/jst-review/ to check my patch before submitting :P

There is a point I don't understand. In the js test file, if without "mc.sleep(0);" then after the two original tests finished, the mail account wizard cannot be opened again.
Attachment #443056 - Attachment is obsolete: true
Attachment #465944 - Flags: review?(bwinton)
(Assignee)

Comment 32

7 years ago
Created attachment 466205 [details] [diff] [review]
Patch #8

Remove all "awc.sleep(1000);" instructions and one longer-than-80-char line.
Attachment #465944 - Attachment is obsolete: true
Attachment #466205 - Flags: review?(bwinton)
Attachment #465944 - Flags: review?(bwinton)
Comment on attachment 466205 [details] [diff] [review]
Patch #8

>+++ b/mail/test/mozmill/account/test-mail-account-setup-wizard.js	Mon Aug 16 10:31:38 2010 +0800
>@@ -242,3 +242,65 @@
>+function test_remember_password() {
>+  remember_password_test(true);
>+  remember_password_test(false);
>+}
>+
>+/* Test remember_password checkbox behavior with
>+ * signon.rememberSignons set to "prefValue"

This should be "aPrefValue".

>+function remember_password_test(aPrefValue) {
[snip]
>+  let password = new elementslib.ID(awc.window.document, "password");
>+  let rememberPassword =
>+      new elementslib.ID(awc.window.document, "remember_password");
>+  

Still some trailing spaces here.

>+  pref.setBoolPref("signon.rememberSignons", aPrefValue);
>+  

And here, and a few other places in this file.

>+  awc.assertProperty(rememberPassword, "disabled", !aPrefValue);
>+  if (aPrefValue) {
>+    // password field is not empty any more
>+    // -> enable and check checkbox
>+    awc.assertChecked(rememberPassword);
>+  }
>+  else {
>+    // password field is not empty any more, but prefValue is false

This should also be "aPrefValue".

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js	Mon Aug 16 10:31:38 2010 +0800
>@@ -158,6 +158,27 @@
>+  onFocus: function() {
>+    let passwordElt = document.getElementById("password");
>+    let rememberPasswordElt = document.getElementById("remember_password");
>+    let rememberSignons_pref =
>+      Application.prefs.getValue("signon.rememberSignons", true);
>+
>+    if (rememberSignons_pref) {
>+      rememberPasswordElt.disabled = false;
>+
>+      if (!this._userChangedPassword && passwordElt.value.length >= 1)
>+        rememberPasswordElt.checked = true;
>+    }
>+    else {
>+      rememberPasswordElt.checked = false;
>+      rememberPasswordElt.disabled = true;

I get the feeling we could be more clever here...

rememberPasswordElt.disabled = !rememberSignons_pref;
rememberPasswordElt.checked = rememberSignons_pref &&
                              !this._userChangedPassword &&
                              passwordElt.value.length >= 1;

But I don't know if that's actually a win.

>@@ -420,13 +441,22 @@
>   {
>     let passwordElt = document.getElementById("password");
>     let rememberPasswordElt = document.getElementById("remember_password");
>-    rememberPasswordElt.disabled = false;
>+    let rememberSignons_pref =
>+        Application.prefs.getValue("signon.rememberSignons", true);
>+
>+    if (rememberSignons_pref)
>+      rememberPasswordElt.disabled = false;
>+    else {
>+      rememberPasswordElt.checked = false;
>+      rememberPasswordElt.disabled = true;
>+    }
>+
>     if (passwordElt.value.length < 1) {
>       rememberPasswordElt.disabled = true;
>       rememberPasswordElt.checked = false;
>       this._userChangedPassword = false;
>     }
>-    else if (!this._userChangedPassword)
>+    else if (!this._userChangedPassword && rememberSignons_pref)
>       rememberPasswordElt.checked = true;

This code looks suspiciously familiar.  Can we merge it with the code above?

And finally, this test fails under Windows 7.

$ make SOLO_TEST=account/test-mail-account-setup-wizard.js mozmill-one
TEST-PASS |  setupModule
TEST-PASS |  test_mail_account_setup
TEST-PASS |  test_bad_password_uses_old_settings
TEST-UNEXPECTED-FAIL | c:\Thunderbird\comm-central\mail\test\mozmill\account\test-mail-account-setup-wizard.js | test_remember_password
  EXCEPTION: Controller.assertProperty(ID: remember_password) : false == true
    at: controller.js line 796
       Error("Controller.assertProperty(ID: remember_password) : false == true") 0
            controller.js 796
       remember_password_test(true) test-mail-account-setup-wizard.js 274
       test_remember_password() test-mail-account-setup-wizard.js 247
            frame.js 474
            frame.js 530
            frame.js 573
            frame.js 426
            frame.js 585
            server.js 164
            server.js 168
TEST-PASS |  teardownModule
make: *** [mozmill-one] Error 1

Thanks,
Blake.
Attachment #466205 - Flags: review?(bwinton) → review-
Hey Daniel,
the patch at <http://pastebin.mozilla.org/770781> passes on my Windows 7 netbook, so if you want to post that as a new patch, with the rest of my suggestions, I'll give it an r+.

Thanks,
Blake.
(Assignee)

Comment 35

7 years ago
Created attachment 467796 [details] [diff] [review]
Patch #9

Checkbox is disabled and unchecked either when signon.rememberSignons == false or password's textfield is empty.

Address all bwinton's suggestion. r=bwinton.

Thanks,
Daniel
Attachment #466205 - Attachment is obsolete: true
Attachment #467796 - Flags: review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/a219ce33c927

Thanks for all the work Daniel.
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 3.2a1
Unfortunately I had to back this out due to test failures:

http://hg.mozilla.org/comm-central/rev/5797048efddd

TEST-PASS |  setupModule
TEST-PASS |  test_mail_account_setup
TEST-PASS |  test_bad_password_uses_old_settings
TEST-UNEXPECTED-FAIL | test-mail-account-setup-wizard.js | test_remember_password
  EXCEPTION: no message!
    at: nonesuch line 0
TEST-PASS |  teardownModule
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
INFO | (runtestlist.py) | account: 4 passed, 1 failed
(Assignee)

Comment 38

7 years ago
(In reply to comment #37)
> TEST-UNEXPECTED-FAIL | test-mail-account-setup-wizard.js |
> test_remember_password
>   EXCEPTION: no message!
>     at: nonesuch line 0

This is strange. I've never encountered such exception before. I'll look into this later.
blocking-thunderbird3.1: ? → needed
Daniel have you had a chance to look into it ?
blocking-thunderbird3.1: needed → .6+
(In reply to comment #38)
> (In reply to comment #37)
> > TEST-UNEXPECTED-FAIL | test-mail-account-setup-wizard.js |
> > test_remember_password
> >   EXCEPTION: no message!
> >     at: nonesuch line 0
> This is strange. I've never encountered such exception before. I'll look into
> this later.

I think I've hit this when I've tried to call a function/method that didn't exist in the client.  Perhaps that will give you a place to start looking, at least…

Later,
Blake.
Created attachment 491827 [details] [diff] [review]
Patch v10

This is updated a bit, but the test still doesn't pass for me.

I discovered that input_value needed an extra argument, and the close_mail_account_setup_wizard() call was to an undefined function.

I've tried replacing that call with one from the previous test, but now I get:

SUMMARY-UNEXPECTED-FAIL | test-mail-account-setup-wizard.js | test_remember_password
  EXCEPTION: this.window.document is null
    at: test-window-helpers.js line 629
       _get_element_by_id_helper("cancel_button") test-window-helpers.js 629
       remember_password_test(true) test-mail-account-setup-wizard.js 296
       test_remember_password() test-mail-account-setup-wizard.js 235
            frame.js 530
            frame.js 598
            frame.js 641
            frame.js 480
            frame.js 653
            server.js 164
            server.js 168

I've run out of time today, but hopefully someone else can move this along a bit more.
I'm assigning this to Blake in the hopes that he can finish fixing the unit test. Daniel, if you get time in the meantime do feel free to take another look at it - we will get this landed soon.
Assignee: greenrecyclebin → bwinton
Created attachment 493286 [details] [diff] [review]
A patch that lets the tests pass.

It was the classic "ctrlKey" vs. "accelKey" on Mac bug.
I wonder if we can get the MozMill folk to rename "ctrlKey" to "ctrlKeyThatDoesntWorkOnMacAndYouReallyWantAccelKeyInstead"?  :)

Later,
Blake.
Attachment #467796 - Attachment is obsolete: true
Attachment #491827 - Attachment is obsolete: true
Attachment #493286 - Flags: review?(bugzilla)
Comment on attachment 493286 [details] [diff] [review]
A patch that lets the tests pass.

That's better, thanks Blake.
Attachment #493286 - Flags: review?(bugzilla) → review+
Checked into trunk: http://hg.mozilla.org/comm-central/rev/e757b415fed3

We've decided not to go with this for 3.1.7 as we are very close to release and this hasn't had any time for testing. This will be included in 3.1.8 assuming no issues are reported.

And a big thanks to Daniel for working on fixing this bug.
Assignee: bwinton → greenrecyclebin
Status: ASSIGNED → RESOLVED
blocking-thunderbird3.1: .7+ → .8+
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: Thunderbird 3.3a1 → Thunderbird 3.3a2
Comment on attachment 493286 [details] [diff] [review]
A patch that lets the tests pass.

I'm going to take this for 1.9.2, the tests pass there and this fixes a regression from 2.0 for enterprise set-ups. a=Standard8
Attachment #493286 - Flags: approval-thunderbird3.1.8+
Checked into 1.9.2:

http://hg.mozilla.org/releases/comm-1.9.2/rev/7e2b12c5b9ec
status-thunderbird3.1: --- → .8-fixed
You need to log in before you can comment on or make changes to this bug.