358696 - Set.. and GetLastModifiedDate set to ("0Z")

Status: NEW

(MailNews Core :: Address Book, defect)

Description

[Günter]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1) Gecko/20061010 Firefox/2.0

AB-item "lastModifiedDate" **IS** stored internally with it's really date&time. The access-methods to set and get the card-item-values are set to 0Z.
ex for a given situation:
 card1.getCardValue('LastModifiedDate') returns "0Z"  
 card1.lastModifiedDate returns "1161347385" (date&time in seconds) 

It seems the properties' work NOT as expected (what they say 'set' & 'get' value). The higher level functions should **if required from them *** set / get that value to 0Z. It should not be done at this level.


Reproducible: Always

Steps to Reproduce:
1.for set see http://lxr.mozilla.org/mailnews/source/mailnews/addrbook/src/nsAbCardProperty.cpp#626
2.for get see http://lxr.mozilla.org/mailnews/source/mailnews/addrbook/src/nsAbCardProperty.cpp#213
3.

Actual Results:  
see above

Expected Results:  
see above

see mozilla.dev.apps.thunderbird "TB/AB lastModifiedDate Item -- how is it used?"
msd-ID: <[e-mail]>

Comment 1

[Günter]

Any comment ? 
Thought it could help to make TB/Addressbook a bit more usable & transparent for further add-on development!!

Just wrote a little XPI to display the lastModDate on the user-card-display.
It works ... but with a not so 'clean' access-mode.

Comment 2

[Günter]

Can anyone take care of this ...

The "lastModifiedDate" and "Category" items are really important to make the TB/AB a real PIM ... especially for exchanging with Personal Devices! 

For "Category" bug see also: https://bugzilla.mozilla.org/show_bug.cgi?id=271976

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #1)
> Any comment ? 
> Thought it could help to make TB/Addressbook a bit more usable & transparent
> for further add-on development!!

Generally it looks safe to correct get/setCardValue. We may just have to be careful how we handle the LDAP (and LDIF import/export) case as 'Z' is probably Zulu timezone. Also we may need to consider what happens when setting the card value, as I expect writing to the database will update the card value itself...

Comment 4

[Günter]

Hi Mark,
>>I expect writing to the database will update the card value itself...

Yes, the "lastModifiedDate" will be updated each time you update any other field. But that's why it's there.

With the "Category" item I just noticed the normal LDIF import ignores an entry like:
"mozillaCategory:gW"
That's because of:
http://mxr.mozilla.org/mozilla1.7/source/mailnews/addrbook/src/nsAddressBook.cpp#161
 161   {kCategoryColumn, MOZ_AB_LDIF_PREFIX "Category", PR_FALSE}, 
 162 };

Any idea if the PR_FALSE can be changed to PR_TRUE to enable the LDIF import?

 

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> Hi Mark,
> >>I expect writing to the database will update the card value itself...
> Yes, the "lastModifiedDate" will be updated each time you update any other
> field. But that's why it's there.

Yes, but being able to call SetLastModifiedDate is potentially a bit confusing...and should we force the last modified date on import?

Z is definitely Zulu (http://www.ietf.org/rfc/rfc2252.txt), I'm guessing that lastModifiedDate is in the current timezone? So we should at least make a note of that in nsIAbCard. We'll possibly have to translate LDAP dates into the current time zone (or keep everything as utc) and account for them in LDIF import/export.

> With the "Category" item I just noticed the normal LDIF import ignores an entry
> like:
> "mozillaCategory:gW"
> That's because of:
> http://mxr.mozilla.org/mozilla1.7/source/mailnews/addrbook/src/nsAddressBook.cpp#161
>  161   {kCategoryColumn, MOZ_AB_LDIF_PREFIX "Category", PR_FALSE}, 
>  162 };
> Any idea if the PR_FALSE can be changed to PR_TRUE to enable the LDIF import?

Not sure why you're looking at mozilla 1.7 tree, however I doubt that would work because ldap isn't set up for handling the category attribute. Thinking about it that part of the table may not even be there on trunk.

btw I think we need to keep last modified date & category in different bugs as they will be easier to track.

Comment 6

[Günter]

Mark,
if I look into the SM code, there is the same difference between accessing "GetCardValue" with the attrname="lastModifiedDate" and a direct call for the card value with "GetLastModifiedDate". (see code links below)

This results in the effect
 -- with operations using the first methodes  (like the LDIF export or if you try to add the date to the contact-list) the value will be "0Z"
 -- if accessing directly (as with XPI programming) you will get the real "lastModifiedDate" value the card was altered.

> Yes, but being able to call SetLastModifiedDate is potentially a bit
> confusing
As said, it's not about "being able to call SetLastModifiedDate", it about returning returning the paramenter "aLastModifiedDate" as with line 738.

> ...and should we force the last modified date on import?
No, importing a new contacts is a new card. If the user wants to preserve eg. the date the contavt was created with the other app, he can add that with the "Description:" item.

> Z is definitely Zulu (http://www.ietf.org/rfc/rfc2252.txt)
rfc2252 seems to me the "Lightweight Directory Access Protocol (v3)" but that one dosn't hold any definition about time zone!?

> I'm guessing that lastModifiedDate is in the current timezone? So we should at 
> least make a note of that in nsIAbCard. We'll possibly have to translate LDAP
> dates into the current time zone (or keep everything as utc) and account for 
> them in LDIF import/export.
Yes, that value returns "my" time/date if modify a card.

---------------
Here are the code links:
http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAbCardProperty.cpp#371

212 NS_IMETHODIMP nsAbCardProperty::GetCardValue(const char *attrname, nsAString &value)
213 {
... down to ...
368           // XXX todo
369           // fix me?  LDAP code gets here
370           PRUint32 lastModifiedDate;
371           rv = GetLastModifiedDate(&lastModifiedDate);
372           value.AssignLiteral("0Z");

Just to show the Set-methode works similar:
481 NS_IMETHODIMP nsAbCardProperty::SetCardValue(const char *attrname, const nsAString &value)
482 {
... down to ...
632           // XXX todo
633           // fix me?  LDAP code gets here
634           rv = SetLastModifiedDate(0);


http://lxr.mozilla.org/mailnews/source/mailnews/addrbook/src/nsAbCardProperty.cpp#737

737 NS_IMETHODIMP
738 nsAbCardProperty::GetLastModifiedDate(PRUint32 *aLastModifiedDate)
739 {
740   *aLastModifiedDate = m_LastModDate;
741   return NS_OK;
742 }
743
744 NS_IMETHODIMP
745 nsAbCardProperty::SetLastModifiedDate(PRUint32 aLastModifiedDate)
746 {
747   m_LastModDate = aLastModifiedDate;
748   return NS_OK;
749 }

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> > ...and should we force the last modified date on import?
> No, importing a new contacts is a new card. If the user wants to preserve eg.
> the date the contavt was created with the other app, he can add that with the
> "Description:" item.

Yes, that's fine now I think about it.

> > Z is definitely Zulu (http://www.ietf.org/rfc/rfc2252.txt)
> rfc2252 seems to me the "Lightweight Directory Access Protocol (v3)" but that
> one dosn't hold any definition about time zone!?

It does and it doesn't. See section "5.1.2. modifyTimestamp" and section "6.14. Generalized Time". X.208 is meant to define it fully, but I couldn't find a copy of that anywhere.
http://www.itu.int/ITU-T/studygroups/com17/languages/

> > I'm guessing that lastModifiedDate is in the current timezone? So we should at 
> > least make a note of that in nsIAbCard. We'll possibly have to translate LDAP
> > dates into the current time zone (or keep everything as utc) and account for 
> > them in LDIF import/export.
> Yes, that value returns "my" time/date if modify a card.

I think we may have to be careful here - if a user moves profile to a different computer with different timezone info, then the time/date will be wrong. So possibly we should be fixing our backend to store the modify date as zulu/utc time and convert it for display to the machine's local time. Though this may need some more thinking about.

Comment 8

[Günter]

Attachment: References