Closed Bug 198303 Opened 17 years ago Closed 17 years ago

Deleted cards still show up for junk mail whitelisting, people I know views.

Categories

(SeaMonkey :: MailNews: Address Book & Contacts, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.4beta

People

(Reporter: esther, Assigned: sspitzer)

References

Details

(Keywords: regression, Whiteboard: [adt2])

Attachments

(1 file, 7 obsolete files)

Using trunk build 20030318 on winxp, macosx and linux if you delete a card from
the Personal Address book, it is still in the abook.mab file when viewing with
notepad.  This breaks the Junk Mail whitelisting against the personal Address
book in some cases.  Bug 196777 may have broken this. 

1. create a new profile
2. add a mail account 
3. add your address to the Personal Address book 
4. Exit and relaunch
5. Delete your card from the Personal Address Book.  Exit
6. View your abook.mab file with an editor, see that your email address is in
that file. 

Expected: if you delete your card from the address book your email address
should be deleted from that file too.
nominating and reassigning
Assignee: racham → sspitzer
Keywords: nsbeta1, regression
QA Contact: nbaca → esther
Mail triage team: nsbeta1+/adt2
Keywords: nsbeta1nsbeta1+
Whiteboard: [adt2]
accepting, but it isn't a regression.

(or it is, but it's not from my checkin.)

here's the problem:

we keep track of deleted cards in the mab

when we do autocomplete, or mail views, or whitelisting, we ask the PAB:

"do you have a row for email == xyz@foo.com"

rows are not bound to a table, and a mab can have multiple tables (say "cards"
and "deleted cards"), and a row can be in both.

the fix is to make sure the card is not in the deleted table, if it is, pretend
it isn't there.
Status: NEW → ASSIGNED
here's the "fix" part of this bug:

+    // fix for bug #198303
+    // here's the problem:
+    // for palm and absync, we keep track of deleted cards in the mab
+    // when we do autocomplete, or mail views, or whitelisting, we ask the PAB:
+    // "do you have a row for email == xyz@foo.com?"
+    // rows are not bound to a table, and a mab can have multiple tables (say
"cards"
+    // and "deleted cards"), and a row can be in both.
+    // the fix is to make sure the card is not in the deleted table, 
+    // if it is, act like we didn't find it.
+    if (mdbRow)
+    {
+      nsresult rv;
+      // we might not have loaded the "delete cards" table yet
+      // so do that (but don't create it, if we don't have one), 
+      // so we can see if the row is really a delete card.
+      if (!m_mdbDeletedCardsTable)
+        rv = InitDeletedCardsTable(PR_FALSE);
+      mdb_bool hasRow = PR_FALSE;
+      rv = m_mdbDeletedCardsTable->HasRow(env, mdbRow, &hasRow);
+      if (NS_SUCCEEDED(rv) && hasRow)
+        *aFindRow = nsnull;
+      else
+        NS_IF_ADDREF(*aFindRow = mdbRow);
+    }

but I'll ask cavin to review it all.
Attachment #117966 - Flags: review?(cavin)
whoops, I've removed that spurious comment:

+  //m_mdbPabTable
Attached patch update patch (obsolete) — Splinter Review
Attachment #117966 - Attachment is obsolete: true
Attachment #117971 - Flags: review?(cavin)
Attachment #117966 - Flags: review?(cavin)
Attached patch patch (obsolete) — Splinter Review
Attachment #117971 - Attachment is obsolete: true
Attachment #117971 - Flags: review?(cavin)
Attachment #117989 - Flags: review?(cavin)
Attachment #117989 - Attachment is obsolete: true
Attachment #117989 - Flags: review?(cavin)
Target Milestone: --- → mozilla1.4alpha
bugs I should test:  #146482, #183416, #73814

there could be many places where we were getting deleted data.
Comment on attachment 117995 [details] [diff] [review]
updated patch, fixes a crash when no deleted table.

r=cavin. One minor nit:

-    nsresult	 err = NS_OK;
-    nsIMdbRow	  *cardRow;
+  nsresult rv = NS_OK;
+  nsCOMPtr <nsIMdbRow> cardRow;

     if (!newCard || !m_mdbPabTable)
	 return NS_ERROR_NULL_POINTER;

-    err  = GetNewRow(&cardRow);
+  rv  = GetNewRow(getter_AddRefs(cardRow));

we can just do:

nsresult rv  = GetNewRow(getter_AddRefs(cardRow));
Attachment #117995 - Flags: review? → review+
good catch, I'll fix that before sr and checkin.
fixed.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Summary: [junk] Deleting address cards from Personal address book doesn't delete the entry in abook.mab file, causing junk mail whitelisting to break → Deleted cards still show up for junk mail whitelisting, people I know views.
Testing this on winxp with build 20030327, this is partially fixed.   
If I remove a card from the address book we do the right thing for White Listing
and Mail Views "People I Know" but if I add that same card back into my Personal
Address Book it won't recognize it as "People I Know" or for White Lisitng Junk
Mail.  Reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
good catch.  right, a card can be in both tables (if you add, delete, and add), 
so the right fix is to check the good table, instead of the bad table.

here comes a patch...
Status: REOPENED → ASSIGNED
Attachment #118782 - Attachment is obsolete: true
Attached patch updated fix (obsolete) — Splinter Review
so, the add / delete / add scenario that esther found points out more issues
with the addrbook on top of mork.

I've got a patch that addresses it, but this deleted table (for palm sync) has
really caused some problems.

I'll continue to test, and look over the mailing list side of this fix, and
report back.
Attachment #118786 - Attachment is obsolete: true
Attached patch patchSplinter Review
Attachment #117995 - Attachment is obsolete: true
Attachment #118833 - Attachment is obsolete: true
fixed.

this patch also touches some code that can be hit when you try to create mailing
lists.

to test it, I did this:

1) create a mailing list called "foo"
2) try to create a second mailing list called "foo" (should get alert, can't
have two of the same name)
3) delete "foo"
4) try to create a mailing list called "foo" again (shouldn't get alert)

esther already knows the other tests, involving cards.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.4alpha → mozilla1.4beta
the checkin here busted builds without LDAP support.  nsAddrDatabase.cpp now
requires nsIPromptService.h, which is in the windowwatcher include directory. 
But the Makefile only grabs stuff from that directory when LDAP is enabled.
I'll fix the build bustage today.

Index: Makefile.in
===================================================================
RCS file: /cvsroot/mozilla/mailnews/addrbook/src/Makefile.in,v
retrieving revision 1.43
diff -u -w -r1.43 Makefile.in
--- Makefile.in 19 Apr 2003 05:13:31 -0000      1.43
+++ Makefile.in 25 Apr 2003 15:13:22 -0000
@@ -52,6 +52,7 @@
                  appcomps \
                  intl \
                  import \
+                 windowwatcher \
                  $(NULL)

 CPPSRCS                = \
@@ -136,7 +137,6 @@
 ifdef MOZ_LDAP_XPCOM
 REQUIRES    += mozldap \
                uriloader \
-               windowwatcher \
                $(NULL)

 EXPORTS                += nsAbLDAPDirectoryQuery.h \
build bustage should be fixed now.
using trunk build 20030428 on winxp, macosx and linux the origianl test scenario
and the scenario mentioned in comment 20 this is verified fixed. 
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.