Closed Bug 261882 Opened 15 years ago Closed 11 years ago

Import & Export to/from text files (CSV,TAB) fails to include "Screenname" field

Categories

(MailNews Core :: Address Book, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: tommy.joe, Assigned: bisi)

References

Details

(Keywords: dataloss, helpwanted)

Attachments

(1 file, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.7.3) Gecko/20040913 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.7.3) Gecko/20040913 Firefox/0.10

When importing an address book from a text based file, there is no option to
match the screenname field to any of the fields present in the data file.  Also,
when an  address book containing data entries in the screen name field, they are
not included in the exported text file.

Reproducible: Always
Steps to Reproduce:
1. Choose Import from the Address Book Tools menu
2. Select "Address Books"
3. Select "Text File"
4. Choose a path to an appropriate file
5. The dialog requesting the user to match the field names with the individual
data items does not have a field for "Screenname"

Also
1. Select an address book containing data entries in the "Screenname" field
2. Choose "Export" from the "Tools" menu
3. Export to a CSV or TAB file
4. Open the file in a text editor, and the screenname data is gone
Summary: Import & Export to/from text files (LDIF,CSV,TAB) fails to include "Screenname" field → Import & Export to/from text files (LDIF,CSV,TAB) fails to include "Screenname" field
Confirming bug on tb trunk version 1.0+ (20050704). Note that LDIF import is
being fixed in bug 177968 - export was already working. Hence I've removed ldif
from the summary.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Import & Export to/from text files (LDIF,CSV,TAB) fails to include "Screenname" field → Import & Export to/from text files (CSV,TAB) fails to include "Screenname" field
Assignee: mscott → nobody
Component: Address Book → MailNews: Address Book
OS: Windows 2000 → All
Product: Thunderbird → Core
QA Contact: addressbook
Hardware: PC → All
Version: unspecified → Trunk
Keywords: helpwanted
*** Bug 353331 has been marked as a duplicate of this bug. ***
Marking as blocking-thunderbird 3, since it's a dataloss bug.
Flags: blocking-thunderbird3+
Keywords: dataloss
Product: Core → MailNews Core
Taking. I have a patch in my tree, that works with trunk builds. However, the real problem here is backwards compatibility. I'll look around, If I can find a reasonable solution.
Assignee: nobody → bisi
Žiga, are you still willing to do the patch for this bug? I'm not sure backwards compatibility would be too much of an issue as users can just choose not to import certain fields.
Attached patch wip (obsolete) — Splinter Review
Hey Mark, thanks for stopping by. :) Actually, my development laptop died a couple of months ago and I didn't get to buy a new one. But I'm also a bad bug owner. ;)

Anyway, I didn't lose my data, so I'm attaching a wip patch (there's a small merge conflict in import_helper.js and it hasn't been updated for at least two months). Well, I'm not sure if that's the right approach - should a "new field" be placed where it actually belongs or at the end of the field map?
Žiga, sorry for not getting back to you earlier.

-  {kScreenNameProperty},
+  {kScreenNameProperty, 2106},
   {kPreferMailFormatProperty},
   {kLastModifiedDateProperty},
-  {kWorkPhoneProperty, 2106},
+  {kWorkPhoneProperty, 2107},
   {kWorkPhoneTypeProperty},
-  {kHomePhoneProperty, 2107},
+  {kHomePhoneProperty, 2108},
   {kHomePhoneTypeProperty},
-  {kFaxProperty, 2108},
+  {kFaxProperty, 2109},

I'd definitely like to avoid renumbering the strings - that will put extra work on the localisers, you can just make the screen name string to be 2136 and then use the map to map kScreenNameProperty to 2136.

   case 6:
+    rv = card->GetPropertyAsAString(kScreenNameProperty, value);
+    break;
+  case 7:
     rv = card->GetPropertyAsAString(kWorkPhoneProperty, value);
     break;
-  case 7:
+  case 8:
     rv = card->GetPropertyAsAString(kHomePhoneProperty, value);

If this is doing what I think its doing (inserting the screen name property in position '6'), then I don't think we want to quite do it that way.

There is a mailnews.import.text.fieldmap that isn't defined by default, but you could use that to put the screen name in the right place for new users. For existing users it would leave the existing order unaffected so if they are already set up in a particular manner then they won't have all their fields shifted.

To do this, insert mailnews.import.text.fieldmap into mailnews/mailnews.js and set it to something like:

+0,+1,+2,+3,+36,+4, etc.

I think that would be a better solution and cut down the size of the patch.

Note: I'm taking this off the blocking TB 3 list as I doubt we'd hold the release for it because we've shipped like this before. However, I'd very much like to get the fix in - if you can't get it in soon, let me know and I'll see if I can find someone to pick it up.
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0b2
Wow, didn't know about the mailnews.import.text.fieldmap pref. This makes things much easier. I'll update the patch soon and request a review. Thanks Mark! :)
Attached patch updated patch (obsolete) — Splinter Review
Attachment #357247 - Attachment is obsolete: true
Attachment #358636 - Flags: review?(bugzilla)
Attached patch updated patch (obsolete) — Splinter Review
Argh, this is the right patch.
Attachment #358636 - Attachment is obsolete: true
Attachment #358640 - Flags: review?(bugzilla)
Attachment #358636 - Flags: review?(bugzilla)
Attachment #358640 - Attachment is patch: true
Attachment #358640 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 358640 [details] [diff] [review]
updated patch

Thanks for updating the patch, from a quick glance it looks good, but one thing I did notice:

+pref("mailnews.import.text.fieldmap", "+0+1+2+3+36+4+5+6+7+8+9+10+11+12+13+14+15+16+17+18+19+20+21+22+23+24+25+26+27+28+29+30+31+32+33+34+35");

Shouldn't they have commas in between them? My pref currently shows:

+0,+1,+2,+3,-4,-5,-6,-7,-8,-9,-10,-11,-12,-13,-14,-15,-16,-17,-18,-19,-20,-21,-22,-23,-24,-25,-26,-27,-28,-29,-30,-31,-32,-33,-34,-35,

I'll do the full review on Monday.
Attached patch updated patch (obsolete) — Splinter Review
Oh yes, and the position of "36" was also wrong in the previous patch. Here's the corrected patch.
Attachment #358640 - Attachment is obsolete: true
Attachment #358650 - Flags: review?(bugzilla)
Attachment #358640 - Flags: review?(bugzilla)
Comment on attachment 358650 [details] [diff] [review]
updated patch

+  case 36:
+    rv = database->addAimScreenName(row, pVal);
+    break;

It should be AddAimScreenName (i.e. captial A).

Otherwise this works nicely. r=me (you'll need an sr as well).
Attachment #358650 - Flags: review?(bugzilla) → review+
Attached patch A updated patch (obsolete) — Splinter Review
Thanks Mark. Asking Dan for a sr.
Attachment #358650 - Attachment is obsolete: true
Attachment #358839 - Flags: superreview?(dmose)
Comment on attachment 358839 [details] [diff] [review]
A updated patch

>diff --git a/mail/locales/en-US/chrome/messenger/importMsgs.properties b/mail/locales/en-US/chrome/messenger/importMsgs.properties
>--- a/mail/locales/en-US/chrome/messenger/importMsgs.properties
>+++ b/mail/locales/en-US/chrome/messenger/importMsgs.properties
>@@ -273,16 +273,21 @@ 2133=Custom 3
> ## @loc None
> 2134=Custom 4
> 
> # Description: Address book field name
> ## @name IMPORT_FIELD_DESC_END
> ## @loc None
> 2135=Notes
> 
>+# Description: Address book field name
>+## @name IMPORT_FIELD_DESC_END
>+## @loc None
>+2136=Screen Name

The @name comment above 2135 should change to IMPORT_FIELD_DESC, since it's no longer the end.  sr=dmose with this fixed in both versions of importMsgs.properties.  Thanks for the patch!
Attachment #358839 - Flags: superreview?(dmose) → superreview+
Attached patch final patchSplinter Review
Nits fixed. Adding the checkin-needed keyword.
Attachment #358839 - Attachment is obsolete: true
Keywords: checkin-needed
Patch checked in: http://hg.mozilla.org/comm-central/rev/c01858f3a47b

Many thanks for fixing this Žiga.
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.