525024 - Account manager does not allow properties with '.' such as [e-mail]#junk

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Kent James (:rkent)]

Overrides of the default decision to apply junk processing to a folder use the globally unique identifier "[e-mail]#junk" as the property name. Unfortunately AccountManager.js does a string split on "." to parse an embedded property name in a DOM id. So it sees the property as simply "dobayes" which is incorrect, so I cannot easily extend the account manager to process this property..

I would prefer to fix this by allowing "." in property names in AccountManager.js

Comment 1

[Kent James (:rkent)]

Attachment: Rev a: modify AccountManager.js to include full property name

Comment 2

[:aceman]

The account manager spots you try to change are still unchanged today. Is this functionality still needed? Can you request review for the patch?

Comment 3

[:aceman]

rkent, I think there are now more places that work with the ID in the Accountmanager.js. 
So the patch may need to be extended. Can you do it or should I look at it?

Will you drive this in before I go on with bug 791093 ?

Comment 4

[Kent James (:rkent)]

Feel free to take this, obviously I've long ago abandoned this as I have used workarounds when needed.

Comment 5

[:aceman]

Attachment: patch v2

Comment 6

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

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

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

This looks OK (at least, given my limited understanding of the Account Manager)...maybe you should get Neil or Ian to look at this too.

Also, what are the chances of getting a test for this?

Comment 7

[:aceman]

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

The test is that the AM did not fall apart after this patch :)
There are no ids with surplus dots in the current tree. The test would need to change some of the existing IDs (or create a fake element with a new ID) and see if the value is preserved upon pane switches. I think it is not hard to do if you insist on that :) But I don't think this deserves a new test file. Can I add it to some existing one?

Comment 8

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

You might be able to get away with adding a new one to mail/test/mozmill/account/test-account-actions.js. Then again, copying most of that boilerplate into a new file isn't *too* expensive...

Comment 9

[:aceman]

Yeah, but a whole test file just for this is overkill :) I think conceptually this would fit into the test file for bug 755885.

Comment 10

[:aceman]

Actually I can build the test file here and then plug checks from bug 755885 into it, not the other way round;)

Comment 11

[:aceman]

We are waiting for the test here.

Comment 12

[:aceman]

Attachment: patch v3

Now with a test! :) The test properly fails if the patch to AccountManager.js is not applied.

Comment 13

[:aceman]

The test works only after applying bug 805953 first.

Comment 14

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

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

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

Just a couple of nits, but otherwise, I'm happy with it.

::: mail/test/mozmill/account/test-account-settings-infrastructure.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> +  This test checks proper operation of the account settings panes infrastructure

Please use

/**
 * This is a comment block
 */

instead of

/*
   This is a comment block
 */

@@ +45,5 @@
> +function teardownModule(module) {
> +  // Remove our test account to leave the profile clean.
> +  MailServices.accounts.removeAccount(gPopAccount);
> +  // There should be only the original accounts left.
> +  assert_equals(MailServices.accounts.allServers.Count(), gOriginalAccountCount);

Good stuff, checking this.

@@ +65,5 @@
> +  let iframe = amc.window.document.getElementById("contentFrame");
> +  // Check whether a standard element with "server.loginAtStartUp" stores its
> +  // value properly.
> +  let loginCheck = iframe.contentDocument.getElementById("server.loginAtStartUp");
> +  assert_equals(loginCheck.checked, false);

You should be able to use assert_false here instead.

@@ +77,5 @@
> +  click_account_tree_row(amc, accountRow);
> +
> +  // Check by element properties.
> +  let loginCheck = iframe.contentDocument.getElementById("server.loginAtStartUp");
> +  assert_equals(loginCheck.checked, true);

assert_true

@@ +85,5 @@
> +                                                 amc.window.getValueArrayFor(gPopAccount),
> +                                                 "server", "loginAtStartUp",
> +                                                 null, false);
> +
> +  assert_equals(rawCheckValue, true);

assert_true

Comment 15

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

I'll assume you've gotten someone to do a try-build of this for you too...I think that's prudent before landing.

Comment 16

[:aceman]

Attachment: patch v4

Comment 17

[:aceman]

Try build here: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=3df46491be8c

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/999b41d35876