Last Comment Bug 525024 - Account manager does not allow properties with '.' such as dobayes.mailnews@mozilla.org#junk
: Account manager does not allow properties with '.' such as dobayes.mailnews@m...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 19.0
Assigned To: :aceman
:
:
Mentors:
Depends on: 805953
Blocks: 755885 791093 807101
  Show dependency treegraph
 
Reported: 2009-10-28 11:50 PDT by Kent James (:rkent)
Modified: 2012-11-12 14:02 PST (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Rev a: modify AccountManager.js to include full property name (2.45 KB, patch)
2009-10-28 13:54 PDT, Kent James (:rkent)
no flags Details | Diff | Splinter Review
patch v2 (2.59 KB, patch)
2012-09-28 13:32 PDT, :aceman
mconley: review+
iann_bugzilla: review+
Details | Diff | Splinter Review
patch v3 (8.60 KB, patch)
2012-10-28 08:15 PDT, :aceman
mconley: review+
Details | Diff | Splinter Review
patch v4 (8.69 KB, patch)
2012-11-09 12:42 PST, :aceman
acelists: review+
Details | Diff | Splinter Review

Description Kent James (:rkent) 2009-10-28 11:50:16 PDT
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
Comment 1 Kent James (:rkent) 2009-10-28 13:54:25 PDT
Created attachment 408926 [details] [diff] [review]
Rev a: modify AccountManager.js to include full property name
Comment 2 :aceman 2012-02-08 08:59:34 PST
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 2012-09-18 01:48:42 PDT
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) 2012-09-18 08:42:09 PDT
Feel free to take this, obviously I've long ago abandoned this as I have used workarounds when needed.
Comment 5 :aceman 2012-09-28 13:32:10 PDT
Created attachment 666035 [details] [diff] [review]
patch v2
Comment 6 Mike Conley (:mconley) 2012-10-02 13:51:55 PDT
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 2012-10-02 14:11:12 PDT
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) 2012-10-02 14:21:06 PDT
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 2012-10-02 14:30:24 PDT
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 2012-10-02 23:53:00 PDT
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 2012-10-18 02:06:32 PDT
We are waiting for the test here.
Comment 12 :aceman 2012-10-28 08:15:17 PDT
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.
Comment 13 :aceman 2012-10-28 08:15:50 PDT
The test works only after applying bug 805953 first.
Comment 14 Mike Conley (:mconley) 2012-11-07 18:13:26 PST
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) 2012-11-07 18:14:14 PST
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 2012-11-09 12:42:53 PST
Created attachment 680193 [details] [diff] [review]
patch v4
Comment 17 :aceman 2012-11-09 17:18:58 PST
Try build here: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=3df46491be8c
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-11-12 14:02:10 PST
https://hg.mozilla.org/comm-central/rev/999b41d35876

Note You need to log in before you can comment on or make changes to this bug.