731877 - Import module of Outlook Express should be rewritten with nsIWindowsRegKey

Status: RESOLVED FIXED

(MailNews Core :: Import, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

We can test the module of settings import with the mock of nsIWindowsRegKey

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Proposed fix

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Tests

These test covers only normal case, but I suppose it is sufficient for now.

Comment 3

[David :Bienvenu]

Comment on attachment 601830 [details] [diff] [review]
Proposed fix

thx, this looks fine - I don't have an OE profile on my machine, but the unit test works fine.

Comment 4

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

Comment on attachment 601831 [details] [diff] [review]
Tests

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

Wow, nice tests! :)

This looks really good - just a few nits - see below.

Thanks,

-Mike

::: mailnews/import/test/unit/test_oe_settings.js
@@ +1,3 @@
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const am = Cc["@mozilla.org/messenger/account-manager;1"]

am and smtpService can be pulled from MailServices (http://mxr.mozilla.org/comm-central/source/mailnews/base/util/mailServices.js).

@@ +10,5 @@
> +let gOriginalCID = Components.manager.contractIDToCID(CONTRACT_ID);
> +let factory;
> +let uuid;
> +
> +function POP3Account() {

This can be a one-liner, like:

function POP3Account() {}

@@ +12,5 @@
> +let uuid;
> +
> +function POP3Account() {
> +}
> +POP3Account.prototype = {

Newline before this.

@@ +35,5 @@
> +  'SMTP Use Sicily': 1,
> +  'SMTP User Name': 'smtpuser'
> +};
> +
> +function IMAPAccount() {

One-liner.

@@ +37,5 @@
> +};
> +
> +function IMAPAccount() {
> +}
> +IMAPAccount.prototype = {

Newline before this.

@@ +56,5 @@
> +  'SMTP Use Sicily': 2,
> +  'SMTP User Name': 'smtpuser'
> +};
> +
> +function NNTPAccount() {

One-liner.

@@ +58,5 @@
> +};
> +
> +function NNTPAccount() {
> +}
> +NNTPAccount.prototype = {

Newline before this.

@@ +73,5 @@
> +/* Outlook Express 4.0 */
> +function OE4Registry(defaultAccount) {
> +  this._defaultAccount = defaultAccount;
> +}
> +OE4Registry.prototype = {

Newline before this.

@@ +83,5 @@
> +    'Mail': {
> +      'Poll For Mail': 1234 * 60000
> +    },
> +  },
> +  get 'Identities\\{DEFAULT_ID}\\Software\\Microsoft\\Internet Account Manager\\Accounts'() {

Wow - today I learned you can define getters using a string like this.  Neat. :)

@@ +92,5 @@
> +/* Outlook Express 5.0 */
> +function OE5Registry(defaultAccount) {
> +  this._defaultAccount = defaultAccount;
> +}
> +OE5Registry.prototype = {

Newline before this.

@@ +190,5 @@
> +
> +function MockWindowsRegKey(registryData) {
> +  this._registryData = registryData;
> +}
> +MockWindowsRegKey.prototype = {

Newline before this.

@@ +198,5 @@
> +    if (!this._registryData[aRelPath])
> +      throw Cr.NS_ERROR_FAILURE;
> +    this._keyPath = aRelPath;
> +  },
> +  close: function() {

Let's separate all of these functions, for readabilities sake.

@@ +331,5 @@
> +    let account = am.accounts.QueryElementAt(i, Ci.nsIMsgAccount);
> +    am.removeAccount(account);
> +  }
> +  let smtpServers = smtpService.smtpServers;
> +  while (smtpServers.hasMoreElements()) {

Newline before this while block.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised test

Addressed mconley's comments.

Comment 6

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

Comment on attachment 603067 [details] [diff] [review]
Revised test

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

Hey hiro - sorry it took me so long to get to this.

Just a few more tiny nits. After those are fixed, and assuming these tests actually pass, r=me.

::: mailnews/import/test/unit/test_oe_settings.js
@@ +1,1 @@
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

I think it'd be good to have a comment at the start of this file saying what exactly we're testing for.

@@ +1,4 @@
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource:///modules/mailServices.js");
> +
> +const registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);

why is registrar lowercase and CONTRACT_ID in upper?  I think you should choose a consistent style.  Either all caps, or prefix with k, like kRegistrar, kContractID.

@@ +339,5 @@
> +  for (let i = 0; i < accounts.Count(); i++) {
> +    let account = accounts.QueryElementAt(i, Ci.nsIMsgAccount);
> +    MailServices.accounts.removeAccount(account);
> +  }
> +  let smtpServers = MailServices.smtp.smtpServers;

I'd put a newline after this for block finishes.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Mike Conley (:mconley) from comment #6)
> Comment on attachment 603067 [details] [diff] [review]
> @@ +1,4 @@
> > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Components.utils.import("resource:///modules/mailServices.js");
> > +
> > +const registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);
> 
> why is registrar lowercase and CONTRACT_ID in upper?

I actually prefer uppercase name for constant variable, but in this case its appearance seems weird for me.

 REGISTRAR.registerFactory(...);

So I'd propose that 'let' is used for the registrar like this:

let registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);

What do you think?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Updated fix

Adapted to the latest trunk.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised test

This patch is addressed mconley's comment.

And fixed the failure of smtp server port check on try server because the try server is not applied KB933612. See http://mxr.mozilla.org/comm-central/source/mailnews/import/oexpress/nsOESettings.cpp#873
So I've changed the expected smtp server port 465 to avoid the failure.

And also fix the run-if directive in xpcshell.ini.

I pushed these fixes to try server. The fixes does not contain the correct run-if directive and registrar name is lower-case, but I suppose it is OK.
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=0a0cc63aeb50

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 609609 [details] [diff] [review]
Updated fix

Oops! I did make this obsolete..

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Adapt to the latest trunk

Comment 12

[Ryan VanderMeulen [:RyanVM]]

This has bitrotted again. Please rebase and I'll get it checked in.

Comment 13

[Ludovic Hirlimann [:Usul]]

Hiro ?

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: de-bitrotted

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/73bc66a4817e
https://hg.mozilla.org/comm-central/rev/9f8c975b8d30