Last Comment Bug 690277 - Multiple (duplicate) Address Book Entries / checkboxes in "do not mark mail as junk if the sender is in" (Account settings -> Junk settings)
: Multiple (duplicate) Address Book Entries / checkboxes in "do not mark mail a...
Status: VERIFIED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Preferences (show other bugs)
: 7 Branch
: All All
: -- normal (vote)
: Thunderbird 10.0
Assigned To: Dan Callahan [:callahad]
:
Mentors:
: 690141 694540 699387 700636 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-29 02:22 PDT by AMK
Modified: 2012-02-08 23:29 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
+
fixed


Attachments
Address Book Junk Folders Bug Tbird 7.0.0 Win 7 x64.png (119.18 KB, image/png)
2011-09-29 02:22 PDT, AMK
no flags Details
Junk Property Sheet duplicate address book entries (157.84 KB, image/png)
2011-09-29 02:40 PDT, AMK
no flags Details
Naive fix to clear out preexisting entries before populating list (943 bytes, patch)
2011-10-29 22:48 PDT, Dan Callahan [:callahad]
standard8: review+
Details | Diff | Splinter Review
Updated patch (560 bytes, patch)
2011-10-31 09:16 PDT, Mark Banner (:standard8) (afk until 26th July)
standard8: review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description AMK 2011-09-29 02:22:46 PDT
Created attachment 563347 [details]
Address Book Junk Folders Bug Tbird 7.0.0 Win 7 x64.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0) Gecko/20100101 Firefox/7.0
Build ID: 20110922153450

Steps to reproduce:

Find multiple alternate entries of "personal address book" and "collected addresses" in junk mail property on profile with two email accounts. Originally 12 entries. 


Actual results:

Unchecking all boxes reverts to normal display. Rechecking them gives number of entries for the accounts in that profile. ie if I create two dummy email accounts I will see 8 entries in the property sheet. This increased in sequence for each email account setting you view. ie, look at the first account there are the normal two entries, personal and collected, then second one you see four, third one six etc..


Expected results:

Only personal and collected for the relevant email should be visible on its property sheet available for junk mail filtering
Comment 1 AMK 2011-09-29 02:40:46 PDT
Created attachment 563350 [details]
Junk Property Sheet duplicate address book entries
Comment 2 :aceman 2011-09-30 11:29:17 PDT
Ok, I see it too, on Linux.
Comment 3 Christophe Garault 2011-10-24 01:23:23 PDT
*** Bug 694540 has been marked as a duplicate of this bug. ***
Comment 4 :aceman 2011-10-24 02:18:25 PDT
It is needed to open Account settings->Junk Settings and then directly jumping to Junk settings of another account. The Addressbooks in the list will accumulate. Jumping to another tab of the preferences dialog (e.g. Security) will reset the list to normal (all addressbooks only once). Repeating the process will accumulate entries again.

I'll look into the source, this could be trivial, just the array populating the list not being reset and ABs reenumerated when jumping to the tab with the list.
Comment 5 Dan Callahan [:callahad] 2011-10-29 22:40:08 PDT
This is my first time diving into a Mozilla project's code, but it looks like this could be easily fixed in mailnews/base/prefs/content/am-junk.js

From what I can tell, the Junk Settings page is initialized by building a list of all the applicable address books (line 95), then appending them as children of the listbox (line 124). The reference to the listbox itself is obtained on line 91:

    var wList = document.getElementById("whiteListAbURI");

For whatever reason, this element already has child nodes when switching directly between Junk Settings pages, but not when switching from other pages to Junk Settings.

Iterating over wList and removing all children before populating it seems to resolve this issue for me:

    while (wList.firstChild)
        wList.removeChild(wList.firstChild);

I'll attach a naive patch to this effect, but I'm not sure if it's an appropriate fix. After all, it seems like the children shouldn't be there at all when onInit() is invoked.
Comment 6 Dan Callahan [:callahad] 2011-10-29 22:48:10 PDT
Created attachment 570523 [details] [diff] [review]
Naive fix to clear out preexisting entries before populating list
Comment 7 :aceman 2011-10-30 08:01:12 PDT
Hm, I think I had a comment here, that was somehow lost...

Anyway, I came to the same conclusion, that that element is not properly cleared.

Please request a review of the patch by setting the necessary flags on the attachment. Try :bienvenu as a reviewer.
Comment 8 Mark Banner (:standard8) (afk until 26th July) 2011-10-31 01:57:18 PDT
Comment on attachment 570523 [details] [diff] [review]
Naive fix to clear out preexisting entries before populating list

Thanks for the patch, I'll take a look at this in a bit.
Comment 9 Mark Banner (:standard8) (afk until 26th July) 2011-10-31 09:14:54 PDT
Comment on attachment 570523 [details] [diff] [review]
Naive fix to clear out preexisting entries before populating list

Many thanks for the patch Dan, this is spot-on. Just a couple of minor review comments:

>+  // Ensure the whitelist is empty (bug 690277)

Normally we don't reference bug numbers in code, as if we need to get back to a bug for the reason why, we follow the blame via our repository history.

>+  while (wList.firstChild)
>+      wList.removeChild(wList.firstChild);

In this situation we usually use .lastChild as it is more efficient in terms of less work in needing less updates of the list.

Also, we generally use two-space indentation rather than four, although generally keeping it to the style of the code you are in is good.

So this is r+ from me.

Normally at this stage, I'd expect you to update the patch before we land it, and then add checkin-needed to the keywords which will flag someone to land it. However, I want to get it into the final 8.0 beta which we're just about to start building, so I'll do all of that for you.
Comment 10 Mark Banner (:standard8) (afk until 26th July) 2011-10-31 09:16:33 PDT
Created attachment 570732 [details] [diff] [review]
Updated patch

Here's the updated patch. I'm also granting approval for our branches, as that's what is needed to get it landed for 9 and 8, as trunk is developing for TB 10 at the moment.
Comment 12 Wayne Mery (:wsmwk, NI for questions) 2011-11-03 04:58:30 PDT
*** Bug 690141 has been marked as a duplicate of this bug. ***
Comment 13 Wayne Mery (:wsmwk, NI for questions) 2011-11-08 09:27:25 PST
*** Bug 700636 has been marked as a duplicate of this bug. ***
Comment 14 GrafLukas 2011-12-09 01:28:22 PST
Noobie-Question: How do I apply the patch?

Another question: Could this be somehow linked to bug 536768? (Sorry, don't know how to link here... again, noobie and everything...)
Comment 15 :aceman 2011-12-09 01:56:42 PST
You would have to compile whole Thunderbird from source, which is quite hard (or maybe edit the appropriate .js file inside omni.jar). Otherwise, you can wait for Thunderbird 9, which should come out 20th Dec 2011.
Comment 16 Wayne Mery (:wsmwk, NI for questions) 2011-12-09 03:39:17 PST
You can pick up the beta of version 9 at http://www.mozilla.org/en-US/thunderbird/channel/
Comment 17 :aceman 2012-02-08 10:45:22 PST
*** Bug 699387 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.