Closed Bug 439819 Opened 16 years ago Closed 16 years ago

LDIF import does not include mozillahomestreet

Categories

(MailNews Core :: Import, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1a2

People

(Reporter: pi, Assigned: pi)

References

Details

(Keywords: dataloss)

Attachments

(2 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1.14) Gecko/20080604 Firefox/2.0.0.14
Build Identifier: Thunderbird version 3.0a2pre (2008061723)

LDIF failed in my newly written import test (bug coming soon) on the homeAddress and workAddress fields (but not homeAddress2 or workAddress2).  The export was fine.  This appears to be broken in both the MOZILLA_1_8_BRANCH and HEAD branches.

Looking at mailnews/addrbook/src/nsAbLDIFService.cpp (http://mxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAbLDIFService.cpp#564), I do not see mozillahomestreet or mozillaworkstreet, but mozillahomestreet2 and mozillaworkstreet2 are present.

I will attach a patch momentarily.

Reproducible: Always

Steps to Reproduce:
1.Export one or more contacts in LDIF that have a Home Address and Work Address (both on line 1)
2.Import that file
Actual Results:  
The first line of the home address and work address will not be present in the imported contact(s)
Attached patch Patch v1 (obsolete) — Splinter Review
A simple patch for mailnews/addrbook/src/nsAbLDIFService.cpp that just adds two else ifs.  Tested in Gentoo and passes my test.
Attachment #325500 - Flags: review?(bugzilla)
Summary: LDIF import does not include mozillahomestreet or mozillaworkstreet → LDIF import does not include mozillahomestreet
Attachment #325500 - Flags: review?(bugzilla)
Assignee: nobody → joshgeenen+bugzilla
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → 1.8 Branch
Attached patch Patch v1.1Splinter Review
As Standard8 pointed out to me in IRC, there is no mozillaworkstreet attribute; it is just street, which is imported successfully.  The new patch just fixes mozillahomestreet.
Attachment #325500 - Attachment is obsolete: true
Attachment #325606 - Flags: review?(bugzilla)
Comment on attachment 325606 [details] [diff] [review]
Patch v1.1

Well spotted!
Attachment #325606 - Flags: review?(bugzilla) → review+
Attachment #325606 - Flags: superreview?(bienvenu)
Requested sr for the patch and I will wait for Bug 437556 before attaching unit
tests for LDIF since the test expands on one of the scripts in that bug.
Status: NEW → ASSIGNED
Version: 1.8 Branch → Trunk
Comment on attachment 325606 [details] [diff] [review]
Patch v1.1

thx for the patch
Attachment #325606 - Flags: superreview?(bienvenu) → superreview+
Checking in mailnews/addrbook/src/nsAbLDIFService.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsAbLDIFService.cpp,v  <--  nsAbLDIFService.cpp
new revision: 1.9; previous revision: 1.8
done

Josh, did you say you had a unit test for this patch?
I do have a unit test, but it started failing in Vista right after I last replied.  For some reason copyTo fails on an nsIFile in Vista only.  I'm trying to fix it now, but haven't figured it out yet.
Attached patch Unit Test v1 (obsolete) — Splinter Review
Thanks for your help finding the mistake.  Passes in Linux and Vista now with the LDIF patch for this bug applied.
Attached patch Unit Test v1.1 (obsolete) — Splinter Review
I made a few minor changes (fixed typos and removed a comment)
Attachment #326526 - Attachment is obsolete: true
Attachment #327005 - Flags: review?(bugzilla)
Having taken a look at this, I'd like to suggest an improvement over using a separate address book file as the comparison method. JSON is a format where you can easily convert objects to and from strings (hence save an object's property as strings), see

http://developer.mozilla.org/en/docs/JSON

for more info.

What I'm proposing therefore, is that the expected results file is a JSON style file, and that is loaded and then used as the basis for the comparison. This way, we could potentially have many tests in one file.

I must admit I haven't used JSON before, but I think it would be ideal to try for this test case.
Comment on attachment 327005 [details] [diff] [review]
Unit Test v1.1

Cancelling for the time being per previous comment.
Attachment #327005 - Flags: review?(bugzilla)
Attached patch Unit Test v2 (obsolete) — Splinter Review
A newer unit test with a rewrite of import_helper.js that uses an array from a JSON file to compare with the cards of the imported address book in response to the previous comment.  Updates the test for Bug 437556 as well.
Attachment #327005 - Attachment is obsolete: true
Attachment #329743 - Flags: review?(bugzilla)
Comment on attachment 329743 [details] [diff] [review]
Unit Test v2

Not too far off, just a few things to address. We also need to try and address bug 142123 before checking this one in, as otherwise we'll be getting test failures in debug mode due the assertions.

>Index: mailnews/import/test/resources/AB_README

>+Here is a sample unit test that checks the results:
>+function run_test()
>+{
>+  getJSONAb("basic_addressbook");
>+  var file = do_get_file("mailnews/import/test/resources/basic_ldif_addressbook.ldif");
>+  importTextFile(file, "ldif", "basic_ldif_addressbook", "basic_addressbook").begin;
>+}

Nit: should be ".begin();" - assuming the first version is right.

>Index: mailnews/import/test/resources/import_helper.js

>+var helper;

You seem to assign once to this, but never use it.

>+  // get the attributes supported for the given type of import

nit: "the extra attributes"

>+  switch (aType[0])

nit: I don't think we need to optimise for speed/case here, checking for the whole name in one case will be enough.

>+  isSupportedAttribute: function(aAttribute)
>+  {
>+    var arr = this.mSupportedAttributes;
>+    var isSupported = false;
>+    for (var i = 0; i < arr.length && !isSupported; i++)
>+      isSupported = isSupported || (aAttribute == arr[i]);
>+    return isSupported;

This can be simplified to:

return this.mSupportedAttributes.indexOf(aAttribute) >= 0;

>+  checkResults: function()
>+  {
>+    if (!this.mJsonCards)
>+      do_throw("The address book must be setup before checking results");
>+    try {
>+      // make sure an address book was created
>+      var newAb = this.getAbByName(this.mAbName);
>+      do_check_neq(newAb, false);
> 
>-  // Make sure the module was found.  If not, return false
>-  if (!module)
>+      var iter = newAb.childCards;
>+      var count = 0;
>+      for (; iter.hasMoreElements(); count++) {
>+        var importedCard = iter.getNext().QueryInterface(Ci.nsIAbCard);
>+        this.compareCards(this.mJsonCards[count], importedCard);
>+      }
>+      // make sure there are the same number of cards in the address book and
>+      // the JSON array
>+      do_check_eq(count, this.mJsonCards.length);
>+      do_test_finished();
>+    } catch(e) { do_throw(e); }
>+  },

I don't see why you are catching and re-throwing the error here. I don't think you need the try catch statement.
Attachment #329743 - Flags: review?(bugzilla) → review-
Attached patch Unit Test v2.1Splinter Review
This patch has an updated readme and all the mistakes/nits were fixed except for the following, as discussed in #maildev.  The helper variable is necessary (for the checkProgress method--comment added) and has been renamed to gAbImportHelper.  The try/catch block was put there because, when do_test_pending() was called, the test will hang if there is an error.  Calling do_throw works as expected, however, so I left it there and added a comment explaining why.  isSupportedAttribute was removed since I added its functionality to the calling method.
Attachment #329743 - Attachment is obsolete: true
Attachment #330622 - Flags: review?(bugzilla)
Comment on attachment 330622 [details] [diff] [review]
Unit Test v2.1

r=me I'll check this in in a few mins.
Attachment #330622 - Flags: review?(bugzilla) → review+
Checked in, changeset id 37:68765c9e5a95

This is going to cause some problems with debug builds running this test, I'll try and fix bug 142123 in the next day or so.
Flags: in-testsuite+
Target Milestone: --- → mozilla1.9.1a2
Product: Core → MailNews Core
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: