164970 - track AB cards deleted and save lastUpdatedTimeStamp for Palm sync

Status: VERIFIED FIXED

(MailNews Core Graveyard :: Palm Sync, defect)

Description

[Rajiv Dayal]

Implement code to save basic information (name fields and deleted timestamp) for
cards deleted in an AB.

Mork allows to create multiple tables in a mork DB, this should be implemented
as a "deleted" table in the mork DB. A method to get a list of deleted cards
should be provided. The lastUpdatedTimeStamp field in nsIAbCard should be filled
with the timestamp when deletion happened.

Also in the AB code whenever cards are modified, the lastUpdatedTimeStamp should
be saved. However, The lastUpdatedTimeStamp should NOT be saved when new cards
are created.

Comment 1

[Rajiv Dayal]

Also please make sure to add a function to get the deleted cards count. Below is
simple implementation for it:

NS_IMETHODIMP nsAddrDatabase::GetDeletedCardCount(PRUint32 *count)
{    
    mdb_err rv;
    mdb_count c;
    rv = m_mdbPalmTable->GetCount(m_mdbEnv, &c);
    if (rv == NS_OK)
        *count = c - 1;  // Don't count LastRecordKey 

    return rv;
}

Please test it and make sure that it returns the correct count.

Comment 2

[Srilatha Moturi]

Attachment: patch v1

Comment 3

[Rajiv Dayal]

In the deleted cards we need to store the Palm Record Id / Remote AB Record Id.
When the GetDeletedCards function is called this Id is returned as a separate
attribute of the card. We do similiar thing for storing _DN for ldap and
_screenname for AIM using the GetStringAttribute method of the nsAbCardProperty.
Please do the same for storing the palm record id. Please define a new method
for setting int attribute for a card and use it in the nsAbAddrDatabase code to
set and get Palm/Remote record ids.

Also like you have done in DeleteCard method you need to modify EditCard method
to store the lastUpdatedTimeStamp so that it is stored every time a card is
edited even if it is not called from the UI.

Comment 4

[Rajiv Dayal]

One more change about naming, instead of calling Palm table call it deletedCard
table. And instead of Palm Record Id call Remote AB Id since it could be re-used
for sync with other remote ABs. Leave the nsDirPrefs as it is here since those
prefs are specific for Palm sync.

Comment 5

[Rajiv Dayal]

In the GetDeletedCardCount please add the check for m_mdbPalmTable being valid.

Comment 6

[Srilatha Moturi]

Attachment: updated patch

updated the patch after talking to rajiv.
1) changed the name from palm table to deletedcards table
2) saving the palm record id for each card in the deleted cards table 
3) setting the palm record id for each deleted card in GetDeletedCardList()

I am still doing some more testing. will post comments once I am done with it.

Comment 7

[Rajiv Dayal]

+nsresult nsAddrDatabase::AddRowToDeletedCardsTable(nsIAbCard *card

+       PRUint32 lastModDate = 0;
+       card->GetLastModifiedDate(&lastModDate);
+       AddIntColumn(cardRow, m_LastModDateColumnToken, lastModDate);
+       NS_RELEASE(cardRow);

In the function above u need to set the last modified date as the time when the
card was deleted which is the present time, use PR_Now and set the
lastModifiedDate here.

Same way in the "nsAddrDatabase::EditCard" function use PR_Now and set the last
modified date for the card passed in as the time the card was edited.


+	server->PalmCategoryId = DIR_GetIntPref (prefstring, "PalmCategoryId",
tempstring, 0);
+	DIR_SetIntPref (prefstring, "PalmCategoryId", tempstring,
server->PalmCategoryId, 0);

Use the last parameter as -1 above which is the default value for PalmCategoryId.

Comment 8

[Srilatha Moturi]

Attachment: updated patch

updated patch addressing Rajiv's comments.

Comment 9

[Rajiv Dayal]

During LDAP replication, and also now Palm sync, for entries that are modified
we in fact delete the existing card and then add the modified card. This is done
because information about what fields are modified is not known. This leads to a
problem, since we keep a record for deleted entries we record this deletion too
which in fact is for modification. Thus if say, a Palm record has been modified
and synced earlier and then a Palm sync is done again the earlier modified
record might be deleted resulting into loss of the record!

Thus it makes sense that such deletions done for modifications are not recorded.
Hence we should keep a record of only those deleted entries that are deleted
from the UI by the user. This will lead to some entries deleted in LDAP being
not deleted on Palm (if the replicated LDAP AB has synced on to Palm) but that
still is better than loss of entries as mentioned in above para.

Comment 10

[Rajiv Dayal]

I guess there is another alternative, just modify all the fields of the existing
card with the values in the modified card. However we will need to change the
LDAP replication too where deleting and adding card on the client side works
just fine since it is a one way sync. Anyway, I think this strategy is better
... so please keep the delete code as it is. We can file a bug for LDAP code to
change it to do as mentioned here.

Comment 11

[Rajiv Dayal]

1) @@ -87,6 +91,7 @@
 const char *kLowerListNameColumn = "LowercaseListName";
 
 struct mdbOid gAddressBookTableOID;
+struct mdbOid deletedCardsTableOID;
 
What is the need for this variable to be global? Please make it a local variable
in the function nsAddrDatabase::InitDeletedCardsTable() you are using it.

2) Instead of doing this:
+   nsresult err;
+   if (!m_mdbDeletedCardsTable)
+     err = InitDeletedCardsTable();
+   else
+     err = NS_OK;
+   if (NS_SUCCEEDED(err)) {

just do this:
   nsresult err = NS_OK;
   if (!m_mdbDeletedCardsTable)
      err = InitDeletedCardsTable();
   if (NS_SUCCEEDED(err)) {

3) Here:
+    nsIMdbStore *store = GetStore();
+    deletedCardsTableOID.mOid_Scope = m_CardRowScopeToken;
+    deletedCardsTableOID.mOid_Id = ID_DELETEDCARDS_TABLE;
+
+    nsresult err = GetStore()->GetTable(GetEnv(), &deletedCardsTableOID,
&m_mdbDeletedCardsTable);

what if GetStore() returns null here. Use the 'store' variable you have defined
above and check that GetStore() did not return null.

4) 
+    mdb_err mdberr = (nsresult) store->NewTableWithOid(GetEnv(),
&deletedCardsTableOID, 

Here too make sure that 'store' is not null. If you check immediately after 
+    nsIMdbStore *store = GetStore();
you will not have to do it everytime you use 'store' variable.

5) Here:
+NS_IMETHODIMP nsAddrDatabase::GetDeletedCardList(PRUint32 *aCount,
nsISupportsArray **aDeletedList)
+{
+  nsresult rv;
+  nsISupportsArray *result;
+  rv = NS_NewISupportsArray(&result);
+  if (NS_FAILED(rv)) return rv;
+  if (!result)
+      return NS_ERROR_OUT_OF_MEMORY;
+  *aCount = 0;
+  if (m_mdbDeletedCardsTable)
+  {

it is most likely that m_mdbDeletedCardsTable is null, unless the same
nsIAddrDatabase object is used to delete a card and make this GetDeletedCardList
function call, a case which i guess will never happen. So do this:
Add these lines:
+  if (!m_mdbDeletedCardsTable) {
+    struct mdbOid deletedCardsTableOID;
+    deletedCardsTableOID.mOid_Scope = m_CardRowScopeToken;
+    deletedCardsTableOID.mOid_Id = ID_DELETEDCARDS_TABLE;
+    nsIMdbStore *store = GetStore();
+    if(store)
+        store->GetTable(GetEnv(), &deletedCardsTableOID, &m_mdbDeletedCardsTable);
+  }
before this code chunk above:
+  *aCount = 0;
+  if (m_mdbDeletedCardsTable)
+  {
      
I have tested and made sure that this works fine.

6) Do the same as in 5 for the 
+NS_IMETHODIMP nsAddrDatabase::GetDeletedCardCount(PRUint32 *count)
+{
+    if (m_mdbDeletedCardsTable) {    

7) In DirPrefs:
+	/* fields for palm Sync */
+	PRUint32 PalmCategoryId;

donot define PalmCategoryId as unsigned int. It takes value -1.

Comment 12

[Rajiv Dayal]

Better, for 5) and 6) above make those lines as separate function, modify the
InitDeletedCardsTable() such that it takes a boolean isCreate which if not set,
return the existing table, and if set, create a new table if one doesnot exist.

Comment 13

[Srilatha Moturi]

Attachment: updated patch

Comment 14

[Rajiv Dayal]

The changes to nsDirPrefs for saving PalmCategoryId to prefs.js still doesnot
initialize it properly to -1 which leads to problems during sync. Passing -1 as
the default value to DIR_SetIntPref / DIR_GetIntPref is not enogh. You will have
to initialize the PalmCategoryId field of the DIR_Server struct whenever a new
server is created or loaded. 

In the function DIR_InitServer (DIR_Server *server):
        // initialize the palm category
        server->PalmCategoryId = -1;

Do this change and you have r=rdayal. But definitly do this change or else sync
will not work.

David would be the best person to super review this (lot of Mork usage).

Comment 15

[Srilatha Moturi]

Attachment: setting palmcategoryid to -1

Comment 16

[David :Bienvenu]

using comptrs makes the code a lot less fragile. It makes it so someone can't
come along and stick a return in the middle of your method and cause lots of
memory leaks.
use nsCOMPtrs for nsIMdbRows, e.g., here:
nsresult nsAddrDatabase::AddRowToDeletedCardsTable(nsIAbCard *card, nsIMdbRow
**pCardRow)
+{
+   nsresult err=NS_OK;
+   if (!m_mdbDeletedCardsTable)
+     err = InitDeletedCardsTable(PR_TRUE);
++   if (NS_SUCCEEDED(err)) {
+     nsIMdbRow *cardRow;
+     err  = GetNewRow(&cardRow);

and, do an addref here:

+         *pCardRow = cardRow;
+     }
e.g., NS_IF_ADDREF(*pCardRow = cardRow);


you can use a comptr here too:

+  if (m_mdbDeletedCardsTable)
+  {
+    nsIMdbTableRowCursor*       rowCursor = nsnull;
+    mdb_pos                        rowPos;
+    PRBool                      done = PR_FALSE;
+

and here:
+  nsISupportsArray *result;
+  nsresult rv = NS_NewISupportsArray(&result);

and then do an NS_IF_ADDREF(*aDeletedList = result);

here, why not just:
InitDeletedCardsTable()
if (m_mdbDeletedCardsTable)
  return m_mdbDeletedCardsTable->GetCount(m_mdbEnv, count);

instead of all this:
+    InitDeletedCardsTable();
+    if (m_mdbDeletedCardsTable) {    
+      mdb_err rv;
+      mdb_count c;
+      rv = m_mdbDeletedCardsTable->GetCount(m_mdbEnv, &c);
+      if (rv == NS_OK)
+        *count = c;
+
+      return rv;
+    }


no need for extra braces here:
+    PRUint32 lastModDate = 0;
+    err = GetIntColumn(cardRow, m_LastModDateColumnToken, &lastModDate, 0);
+    if (NS_SUCCEEDED(err))
+    {
+      newCard->SetLastModifiedDate(lastModDate);
+    }
+

And finally, when does the deleted cards table get cleared out/cleaned up?

Comment 17

[Rajiv Dayal]

Hi David,
Currently purging of cards from the deleted table does not happen. There is a
function DeleteRowFromDeletedCardsTable(cardRow) which can be called from a
purge routine. What do you suggest would be the best place to get it called
from. What if from nsAddrDatabase::CloseMDB a purge routine is called which
removes all rows in the deleted table older than 6 months from PR_Now()? thanks.

Comment 18

[Rajiv Dayal]

I would be taking care of the super review comments since Srilatha is not
available to take care of this bug.

I think it is better to call the purge routine from AddRowToDeletedCardsTable()
which is called everytime a card is deleted, instead of from the CloseMDB
function. This way everytime the DB is closed, irrespective of whether any data
is updated or not, there would not be the overhead of the purge routine, rather
it is called everytime a new record is added to the deleted table, i.e a card is
deleted from the database.

Comment 19

[Rajiv Dayal]

Attachment: patch with Purge routine and to take care of super review comments

Comment 20

[Rajiv Dayal]

Hi David,
Can u please super review this patch, thanks.

Comment 21

[David :Bienvenu]

+  nsresult rv = NS_NewISupportsArray(getter_AddRefs(result));
+  if (NS_FAILED(rv)) return rv;
+  if (!result)
+      return NS_ERROR_OUT_OF_MEMORY;
you don't need to check result, rv succeeding is sufficient, I believe - the
other check is just a little bit of code bloat.

could you call result something more descriptive, like resultCardArray? It makes
the code easier to understand.
+NS_IMETHODIMP nsAddrDatabase::GetDeletedCardList(PRUint32 *aCount,
nsISupportsArray **aDeletedList)
+  nsCOMPtr<nsISupportsArray> result;
+  nsresult rv = NS_NewISupportsArray(getter_AddRefs(result));
+  if (NS_FAILED(rv)) return rv;
+  if (!result)
+      return NS_ERROR_OUT_OF_MEMORY;

"c" is not a good var name.  cardCount would make the code easier to read.
+    if (m_mdbDeletedCardsTable) {
+        mdb_count c=0;
+        // if not too many cards let it be
+        m_mdbDeletedCardsTable->GetCount(m_mdbEnv, &c);
+        if(c < PURGE_CUTOFF_COUNT)
+            return NS_OK;

indentation here is inconsistent: tabs?
+   	NS_IMETHOD GetDeletedCardList(PRUint32 *aCount, nsISupportsArray
**aDeletedList);
+	NS_IMETHOD GetDeletedCardCount(PRUint32 *count);
+    NS_IMETHOD PurgeDeletedCardTable();
+


+NS_IMETHODIMP nsAddrDatabase::PurgeDeletedCardTable()

this routine doesn't seem to use the strategy we talked about over aim - if a
card was deleted less than six months ago, then we know all the following cards
were deleted less than six months ago, and we can break out of the loop, right?
This will make it much less onerous to call this routine multiple times, e.g.,
when the user deletes 10 cards at a time...

Comment 22

[Rajiv Dayal]

Attachment: updated patch

Comment 23

[David :Bienvenu]

Comment on attachment 102039 [details] [diff] [review]
updated patch

sr=bienvenu

Comment 24

[Rajiv Dayal]

thanks David.

Comment 25

[Rajiv Dayal]

I have checked in the code, marking it as fixed.

Also this has been QA by Gregg Meehan, changing QA contact to him.

Comment 26

[Ninoschka Baca]

Trunk build 2003-02-20: WinXP
Verified Fixed. This has been fixed for awhile.