739573 - Try to merge initAcountActionsButton() and updateButtons() in AccountManager.js

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[:aceman]

1. It looks like those 2 functions have almost the same logic, just set the properties on different objects (Seamonkey buttons, Thunderbird menuitems). That could probably be merged.
2. Besides removing duplicate code (and possibility of divergence) it seems it is also inefficient as TB also calls updateButtons() on each selection of an account in AM, decides the necessary properties just to then bail out of the function without setting anything.
3. the initAcountActionsButton() function has a typo.

Do I understand this correctly? Of course I'd like to merge only the common parts. The TB specific things like "Add IM account" item should be preserved.

Comment 1

[:aceman]

Attachment: patch

UI should be unchanged by this patch, it is just code reorganization.
I have tested this works fine in Thunderbird.

Ian: please check it works on Seamonkey.

Florian: please check the hiding of "Add IM account" item still works as intended.

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 611168 [details] [diff] [review]
patch

(In reply to :aceman from comment #1)
> Florian: please check the hiding of "Add IM account" item still works as
> intended.

I haven't really tested this, just looked at the code, but I don't think it breaks the hiding of the "Add IM account" item. If you want to test it, you can use the advanced configuration editor to set mail.chat.enabled to false.

Comment 3

[:aceman]

Thanks, I did that and it worked. I just wanted to be sure I am not missing anything (as I do not use/try IM yet).

Comment 4

[Ian Neal]

Comment on attachment 611168 [details] [diff] [review]
patch

>+function initAccountActionsButtons(menupopup) {
>+  let account = getCurrentAccount();
>+  let tree = document.getElementById("accounttree");

>+  updateItems(tree, account,
You might as well inline tree and account as they're only used the once.
>+    document.getElementById("accountActionsAddMailAccount"),
>+    document.getElementById("accountActionsDropdownSetDefault"),
>+    document.getElementById("accountActionsDropdownRemove"));

>+function updateItems(tree, account, addAccountItem, setDefaultItem, removeItem) {
>+  let canSetDefault = true;
>+  let canDelete = true;
Is it worth having both these set as false to start with and adjusting the logic?

>+function updateBlockedItems(menuItems) {
As these aren't necessarily menuItems, perhaps use aItems as the variable name.

r=me with those answered/addressed.

Comment 5

[:aceman]

Attachment: patch v2

Updated, please see the inverted logic again.

Comment 6

[Ian Neal]

Comment on attachment 613737 [details] [diff] [review]
patch v2

>+    return; // Thunderbird isn't using these
Nit: Comment starts with a capital letter so should end with a full stop.

Comment 7

[:aceman]

Attachment: patch v3

Well, capital T in Thunderbird does not really start a sentence :) But no problem, at least I fixed bitrot. I still bitrot myself with other patches, seems my preventive steps are not enough.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 618018 [details] [diff] [review]
patch v3

Review of attachment 618018 [details] [diff] [review]:
-----------------------------------------------------------------

aceman:

This looks really good - again, I'm so happy that you're cleaning up our old code.  Keep up the great work!

Just two notes below.  With those fixed, I'll give it another spin / review - and you should also get someone from SeaMonkey to look at this.

-Mike

::: mailnews/base/prefs/content/AccountManager.js
@@ +694,5 @@
> + * since these buttons aren't under the IFRAME.
> + */
> +function updateBlockedItems(aItems) {
> +  // for each loop does not work here.
> +  for (let i = 0; i < aItems.length; i++) {

squib and I have been preferring this looping pattern lately:

for each (let [, item] in Iterator(aItems)) {
  let prefstring = item.getAttribute("prefstring");
  ...
}

@@ +700,4 @@
>      if (!prefstring)
>        continue;
>  
> +    if (Services.prefs.prefIsLocked(prefstring))

The old code seems to check if the preference is locked, is a bool, and is set to "true":

"if (prefBranch.prefIsLocked(prefstring) && prefBranch.getBoolPref(prefstring))"

We should probably carry over that logic, or we might risk some edge-case breakage.

Comment 9

[:aceman]

(In reply to Mike Conley (:mconley) from comment #8)
> Just two notes below.  With those fixed, I'll give it another spin / review
> - and you should also get someone from SeaMonkey to look at this.
Ian from Seamonkey looked at it.

> squib and I have been preferring this looping pattern lately:
> 
> for each (let [, item] in Iterator(aItems)) {
>   let prefstring = item.getAttribute("prefstring");
>   ...
> }
And what is the gain from this besides making the code unreadable for me? :)
Also, be aware I pass menupopup into as aItems and that is some special array that can't be iterated via for each (return 1 more element than there are menu items). So let's hope this Iterator stuff works.

> "if (prefBranch.prefIsLocked(prefstring) &&
> prefBranch.getBoolPref(prefstring))"
> 
> We should probably carry over that logic, or we might risk some edge-case
> breakage.

It does it only in the TB version, not SM. But OK let's make them the same. I do not understand why they would even need be locked (setting to true would be fine) but just carry it over.

Comment 10

[:aceman]

Attachment: patch v4

Comment 11

[Ian Neal]

Comment on attachment 619284 [details] [diff] [review]
patch v4

>+    if (Services.prefs.prefIsLocked(prefstring) &&
>+        Services.prefs.getBoolPref(prefstring))
>+      item.setAttribute("disabled", true);
This would break SeaMonkey users that use this functionality as SM currently requires the pref to be only locked, not locked, a boolean and true.
Potentially there are edge cases from the Thunderbird side where it has only been locked not set to true, though prior to 2009 I believe the requirement was the same as for SM (i.e. locked pref only).
r- :(

Comment 12

[:aceman]

Attachment: patch v5

OK, so let's differentiate TB and SM.

Comment 13

[Ian Neal]

Comment on attachment 619403 [details] [diff] [review]
patch v5


>+/**
>+ * Disable buttons/menu items if their control preference is locked.
>+ * Seamonkey: Not currently handled by WSM or the main loop yet
Nit: SeaMonkey not Seamonkey
>+ * since these buttons aren't under the IFRAME.
>+ *
>+ * @param aItems  the elements to be checked
>+ * @param aMustBeTrue  if true then the value of the pref must be set to true
Nit: "...pref must be boolean and set to true..."
>+ *                     to trigger the disabling (TB requires this, SM not)

Comment 14

[:aceman]

Attachment: patch v6

Ok, thanks.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

aceman:

Can I convince you to write some tests for this?

-Mike

Comment 16

[:aceman]

You'd have a hard time at that :) But you can try, depends on how long does it take you to teach tests to me :)

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(In reply to :aceman from comment #16)
> You'd have a hard time at that :) But you can try, depends on how long does
> it take you to teach tests to me :)

Our tests can be found under mail/test/mozmill.  You'll want to create the test in the "accounts" subdirectory of that folder.

Your test should programmatically add a variety of account types during its setup, and then select them one by one, checking that the menulist displays the correct items as enabled and disabled.

Sound do-able?

-Mike

Comment 18

[:aceman]

No, I have not yet created any xpcshell nor mozmill test.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 619405 [details] [diff] [review]
patch v6

So the code looks good, and it appears to work as advertised, but I really would like to see some tests for this stuff.

Can you prepare some tests in another patch on this bug?

Comment 20

[:aceman]

Not yet.
Depends on how much time you have to go through it on IRC :)

Comment 21

[Mike Conley (:mconley) (:⚙️)]

aceman:

Any luck on those tests?

-Mike

Comment 22

[:aceman]

We can try to look at them today.

Comment 23

[:aceman]

Attachment: WIP patch of a test

Comment 24

[:aceman]

The WIP test does not need any flags, it was only uploaded so that mconley can try it out and help me with some issues.

I'll upload a more complete version (testing all menu items) soon.

Comment 25

[:aceman]

Attachment: mozmill test

So my first mozmill test comes! Many thanks to mconley.

Mconley, please see if I do not import any unneded modules. I just copied many declarations and some may be useless for this test.

Comment 26

[:aceman]

Mconley, I hardcoded some pref names in the test. Maybe I should fetch them from the attributes of the menutitems so that the test works if they change?

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 623274 [details] [diff] [review]
mozmill test

Review of attachment 623274 [details] [diff] [review]:
-----------------------------------------------------------------

Great work aceman, especially considering this is your first Mozmill test.

Just a few things I noticed. After this, your patch will be in pretty decent shape, and I'll give it a spin.

::: mail/test/mozmill/account/test-account-actions.js
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

This should be MPL2.

@@ +34,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +var MODULE_NAME = "test-archive-actions";

I know you mostly copied most of these from other tests, but MODULE_NAME, RELATIVE_ROOT and MODULE_REQUIRES should be consts.

@@ +40,5 @@
> +var RELATIVE_ROOT = "../shared-modules";
> +var MODULE_REQUIRES = ["folder-display-helpers", "window-helpers",
> +                       "account-manager-helpers"];
> +
> +Components.utils.import("resource:///modules/mailServices.js");

You should be able to use Cu here instead of Components.utils.  It should be available automatically.

@@ +43,5 @@
> +
> +Components.utils.import("resource:///modules/mailServices.js");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +var mozmill = {};

Neither mozmill.js nor controller.js is used, and can be removed.

@@ +48,5 @@
> +Components.utils.import("resource://mozmill/modules/mozmill.js", mozmill);
> +var controller = {};
> +Components.utils.import("resource://mozmill/modules/controller.js", controller);
> +var elib = {};
> +Components.utils.import("resource://mozmill/modules/elementslib.js", elib);

I also don't think elib is required - see below for the instance where you used it.

@@ +50,5 @@
> +Components.utils.import("resource://mozmill/modules/controller.js", controller);
> +var elib = {};
> +Components.utils.import("resource://mozmill/modules/elementslib.js", elib);
> +
> +var accountManager = MailServices.accounts;

Why the alias?

@@ +57,5 @@
> +var nntpAccount;
> +var originalAccountCount;
> +
> +function setupModule(module) {
> +  let wh = collector.getModule("window-helpers");

You're not using wh, fdh, or amh, so these can be shortened to:

collector.getModule("window-helpers").installInto(module);
collector.getModule("folder-display-helpers").installInto(module);
etc

@@ +104,5 @@
> + * @param accountKey  the key of the account to select
> + * @param isSetAsDefaultEnabled  true if the menuitem should be enabled, false otherwise
> + * @param isRemoveEnabled        true if the menuitem should be enabled, false otherwise
> + * @param isAddAccountEnabled    true if the menuitems (Add Mail Account+Add Other Account)
> +                                 should be enabled, false otherwise

Nit, missing a star on this line.

@@ +106,5 @@
> + * @param isRemoveEnabled        true if the menuitem should be enabled, false otherwise
> + * @param isAddAccountEnabled    true if the menuitems (Add Mail Account+Add Other Account)
> +                                 should be enabled, false otherwise
> + */
> +function subtest_check_account_actions(amc, accountKey, isSetAsDefaultEnabled,

Nit - we tend to name function parameters starting with a, like:

aAccountKey, aController, aIsSetAsDefaultEnabled.

See https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Prefixes

@@ +109,5 @@
> + */
> +function subtest_check_account_actions(amc, accountKey, isSetAsDefaultEnabled,
> +                                       isRemoveEnabled, isAddAccountEnabled)
> +{
> +  amc.waitForElement(new elib.Elem(amc.window.document.getElementById("accountActionsButton")));

Controller.waitForElement is deprecated.

Do you really need to wait for the element to appear?  I don't think it's inserted dynamically...in which case, you can probably remove this wait.

@@ +112,5 @@
> +{
> +  amc.waitForElement(new elib.Elem(amc.window.document.getElementById("accountActionsButton")));
> +
> +  let rowIndex = 0; // count which row in the account tree we need to click
> +  let accountTreeNode = amc.window.document.getElementById("account-tree-children");

You should just be able to use amc.e("account-tree-children");

@@ +136,5 @@
> +  amc.click(amc.eid("accountActionsButton"), 5, 5);
> +  wait_for_popup_to_open(amc.e("accountActionsDropdown"));
> +
> +  let actionAddMailAccount = amc.window.document.getElementById("accountActionsAddMailAccount");
> +  assert_equals(actionAddMailAccount != undefined, true);

You can use assert_not_equals here, I believe.

@@ +139,5 @@
> +  let actionAddMailAccount = amc.window.document.getElementById("accountActionsAddMailAccount");
> +  assert_equals(actionAddMailAccount != undefined, true);
> +  assert_equals(!actionAddMailAccount.getAttribute("disabled"), isAddAccountEnabled);
> +
> +  let actionAddOtherAccount = amc.window.document.getElementById("accountActionsAddOtherAccount");

amc.e("accountActionsAddOtherAccount"). Happens a few more times in this function.

@@ +140,5 @@
> +  assert_equals(actionAddMailAccount != undefined, true);
> +  assert_equals(!actionAddMailAccount.getAttribute("disabled"), isAddAccountEnabled);
> +
> +  let actionAddOtherAccount = amc.window.document.getElementById("accountActionsAddOtherAccount");
> +  assert_equals(actionAddOtherAccount != undefined, true);

assert_not_equals. Also use a few times below when checking against undefined.

@@ +215,5 @@
> +  Services.prefs.unlockPref(disableItemPref);
> +  Services.prefs.getDefaultBranch("").deleteBranch(disableItemPref);
> +}
> +
> +function teardownModule(module) {

Please put the teardownModule directly below the setupModule.

Comment 28

[:aceman]

Attachment: mozmill test v2

Thanks, done.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 624493 [details] [diff] [review]
mozmill test v2

Review of attachment 624493 [details] [diff] [review]:
-----------------------------------------------------------------

Other than the two nits I found, this looks good. With those fixed, r=me.

Great job on your first Mozmill test!

::: mail/test/mozmill/account/test-account-actions.js
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

MPL 2.

@@ +40,5 @@
> +
> +Cu.import("resource:///modules/mailServices.js");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +var imapAccount;

We prefer let over var, and you might as well define them all in a row, like

let imapAccount, nntpAccount, originalAccountCount;

Comment 30

[:aceman]

Attachment: mozmill test v3

Thanks, done!

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/445fb8b2b687
https://hg.mozilla.org/comm-central/rev/9f3422bc53d8

Comment 32

[Mark Banner (:standard8)]

Unfortunately, this seems to have permanent orange on the Linux 32 bit builders, I've therefore backed out just the unit test:

https://hg.mozilla.org/comm-central/rev/7e8025cb9781

https://tbpl.mozilla.org/php/getParsedLog.php?id=11980780&tree=Thunderbird-Trunk

EST-START | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | setupModule
Test Failure: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgAccountManager.createIncomingServer]
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | test-account-actions.js::setupModule
TEST-START | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | test_account_actions
Test Skipped: "setupModule failed."
WARNING | test-account-actions.js::test_account_actions | (SKIP) setupModule failed.
TEST-START | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | teardownModule
Test Failure: Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIMsgAccountManager.removeAccount]
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | test-account-actions.js::teardownModule


(I would have disabled it on Linux, but I don't think we can skip setupModule with skip-if)

Comment 33

[:aceman]

Did all other platforms work?

I find it strange it fails in nsIMsgAccountManager.createIncomingServer().

Comment 34

[:aceman]

I developed the test on 32bit Linux and it worked. Any ideas?

Comment 35

[:aceman]

Attachment: mozmill test v4

Fixed test. Actually the test-archive-options.js test was not cleaning up after itself and the 2 tests collided (created an account with the same server name).
The problem did not appear if running each test standalone.

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 626948 [details] [diff] [review]
mozmill test v4

Review of attachment 626948 [details] [diff] [review]:
-----------------------------------------------------------------

Just one nit. With that fixed, I think this is good to go (again).

(I ran the tests locally, and this looked fine.)

::: mail/test/mozmill/account/test-account-actions.js
@@ +17,5 @@
> +  collector.getModule("window-helpers").installInto(module);
> +  collector.getModule("folder-display-helpers").installInto(module);
> +  collector.getModule("account-manager-helpers").installInto(module);
> +
> +  // There may already be some accounts existent.

Probably better: "There may be pre-existing accounts from other tests."

Comment 37

[:aceman]

Attachment: mozmill test v5

Thanks!

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Mozmill test checked in: https://hg.mozilla.org/comm-central/rev/9e2dc0559734