The default bug view has changed. See this FAQ.

Account manager does not allow properties with '.' such as dobayes.mailnews@mozilla.org#junk

RESOLVED FIXED in Thunderbird 19.0

Status

MailNews Core
Account Manager
--
minor
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: rkent, Assigned: aceman)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 19.0
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 years ago
Overrides of the default decision to apply junk processing to a folder use the globally unique identifier "dobayes.mailnews@mozilla.org#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
(Reporter)

Comment 1

8 years ago
Created attachment 408926 [details] [diff] [review]
Rev a: modify AccountManager.js to include full property name
(Assignee)

Comment 2

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

Comment 3

5 years ago
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 ?
Blocks: 791093
(Reporter)

Comment 4

5 years ago
Feel free to take this, obviously I've long ago abandoned this as I have used workarounds when needed.
Assignee: kent → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

5 years ago
Assignee: nobody → acelists
(Assignee)

Comment 5

5 years ago
Created attachment 666035 [details] [diff] [review]
patch v2
Attachment #408926 - Attachment is obsolete: true
Attachment #666035 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
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?
Attachment #666035 - Flags: review?(mconley) → review+
(Assignee)

Comment 7

5 years ago
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?
Attachment #666035 - Flags: review?(iann_bugzilla)
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...
(Assignee)

Comment 9

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

Comment 10

5 years ago
Actually I can build the test file here and then plug checks from bug 755885 into it, not the other way round;)
Flags: in-testsuite?

Updated

5 years ago
Attachment #666035 - Flags: review?(iann_bugzilla) → review+
Keywords: checkin-needed
(Assignee)

Comment 11

5 years ago
We are waiting for the test here.
Keywords: checkin-needed
(Assignee)

Comment 12

5 years ago
Created attachment 675956 [details] [diff] [review]
patch v3

Now with a test! :) The test properly fails if the patch to AccountManager.js is not applied.
Attachment #666035 - Attachment is obsolete: true
Attachment #675956 - Flags: review?(mconley)
(Assignee)

Comment 13

5 years ago
The test works only after applying bug 805953 first.
Depends on: 805953
(Assignee)

Updated

5 years ago
Blocks: 755885
(Assignee)

Updated

5 years ago
Blocks: 807101
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
Attachment #675956 - Flags: review?(mconley) → review+
I'll assume you've gotten someone to do a try-build of this for you too...I think that's prudent before landing.
(Assignee)

Comment 16

4 years ago
Created attachment 680193 [details] [diff] [review]
patch v4
Attachment #675956 - Attachment is obsolete: true
Attachment #680193 - Flags: review+
(Assignee)

Comment 17

4 years ago
Try build here: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=3df46491be8c
Flags: needinfo?(mconley)
(Assignee)

Updated

4 years ago
Flags: needinfo?(mconley)
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/999b41d35876
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0
You need to log in before you can comment on or make changes to this bug.