Multiple (duplicate) Address Book Entries / checkboxes in "do not mark mail as junk if the sender is in" (Account settings -> Junk settings)

VERIFIED FIXED in Thunderbird 10.0

Status

Thunderbird
Preferences
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: AMK, Assigned: callahad)

Tracking

7 Branch
Thunderbird 10.0

Thunderbird Tracking Flags

(thunderbird8+ fixed, thunderbird9+ fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
Created attachment 563350 [details]
Junk Property Sheet duplicate address book entries
Attachment #563347 - Attachment is obsolete: true

Updated

6 years ago
Component: General → Preferences
QA Contact: general → preferences

Comment 2

6 years ago
Ok, I see it too, on Linux.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → x86

Updated

6 years ago
Duplicate of this bug: 694540

Comment 4

6 years ago
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.
Summary: Multiple Address Book Entry In Junk Mail Property → Multiple (duplicate) Address Book Entries In "do not mark mail as junk if the sender is in" (Account settings -> Junk settings)
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
Created attachment 570523 [details] [diff] [review]
Naive fix to clear out preexisting entries before populating list

Comment 7

6 years ago
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.
tracking-thunderbird8: --- → +
tracking-thunderbird9: --- → +
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.
Attachment #570523 - Flags: review?(mbanner)
Assignee: nobody → dan.callahan
Hardware: x86 → All
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.
Attachment #570523 - Flags: review?(mbanner) → review+
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.
Attachment #570523 - Attachment is obsolete: true
Attachment #570732 - Flags: review+
Attachment #570732 - Flags: approval-comm-beta+
Attachment #570732 - Flags: approval-comm-aurora+
Checked in:

http://hg.mozilla.org/comm-central/rev/2edbbb6c5f59
http://hg.mozilla.org/releases/comm-aurora/rev/17b5b7a4fee0
http://hg.mozilla.org/releases/comm-beta/rev/42a8fe4e9215

Thanks again Dan :-)
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-thunderbird8: --- → fixed
status-thunderbird9: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0
Duplicate of this bug: 690141
Duplicate of this bug: 700636
Summary: Multiple (duplicate) Address Book Entries 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 as junk if the sender is in" (Account settings -> Junk settings)

Comment 14

6 years ago
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

6 years ago
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.

Updated

6 years ago
Status: RESOLVED → VERIFIED
You can pick up the beta of version 9 at http://www.mozilla.org/en-US/thunderbird/channel/

Updated

6 years ago
Duplicate of this bug: 699387
You need to log in before you can comment on or make changes to this bug.