Closed Bug 846694 Opened 7 years ago Closed 7 years ago

"Trust junk mail headers set by" dropdown shows only a number 5


(MailNews Core :: Account Manager, defect)

Not set


(thunderbird21+ fixed)

Thunderbird 22.0
Tracking Status
thunderbird21 + fixed


(Reporter: aceman, Assigned: aceman)



(Keywords: regression)


(1 file, 2 obsolete files)

"Trust junk mail headers set by" dropdown in account manager/ junk settings shows only a number 5 instead of the names of the server classifiers supported.

I think this regressed by bug 824150:
    14.2 +++ b/mailnews/base/prefs/content/am-junk.js
    14.3 @@ -272,21 +272,21 @@ function buildServerFilterListFromDir(aD
    14.4    // now iterate over each file in the directory looking for .sfd files
    14.5    let entries = aDir.directoryEntries
    14.6                      .QueryInterface(Components.interfaces.nsIDirectoryEnumerator);
    14.8    while (entries.hasMoreElements())
    14.9    {
   14.10      let entry = entries.nextFile;
   14.11      // we only care about files that end in .sfd
   14.12 -    if (entry.isFile() && /\.sfd$/.test(entry.leafName))
   14.13 +    if (entry.isFile() && entry.leafName.endsWith(".sfd"))
   14.14      {
   14.15        let fileName = RegExp.leftContext;
   14.16        // if we've already added an item with this name, then don't add it again.
   14.17 -      if (!aISPHeaderList.getElementsByAttribute("value", fileName).item(0))
   14.18 +      if (!aISPHeaderList.querySelector('[value="'+ fileName + '"]'))
   14.19          aISPHeaderList.appendItem(fileName, fileName);
   14.20      }
   14.21    }
   14.22  }

As the /regular expression/ was removed, it looks like the RegExp object(?) now returns something else as expected. This is easy to fix by converting that line to string operations on entry.leafName.
I'll do it shortly.
The offending patch landed on Feb 04, so this will be needed for branch 21.
Attached patch patch to fix the issue (obsolete) — Splinter Review
Sorry for the breakage.
Attachment #719913 - Flags: review?(mconley)
I wanted to convert the ".sfd" to a constant so that the .endWith and -4 could be derived from it. In case it changes in the future.

Even with the patch, I am not sure the pref mail.server.%serverkey%.serverFilterName is saved properly (I did not find it in prefs.js), I'd like to check that out.
Attached patch patch v2 (obsolete) — Splinter Review
Ok, it looks like the mail.server.serverX.serverFilterName pref is not set if the value matches mail.server.default.serverFilterName . But that seems fine, as retrieving the
Attachment #719913 - Attachment is obsolete: true
Attachment #720405 - Flags: review?(mconley)
...retrieving the pref value returns the default value as if it was the value of the pref.
Blocks: 847172
Attachment #720405 - Flags: review?(iann_bugzilla)
Attachment #720405 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 720405 [details] [diff] [review]
patch v2

Review of attachment 720405 [details] [diff] [review]:

Just one nit - beyond that, I'm happy with it.

::: mailnews/base/prefs/content/am-junk.js
@@ +269,5 @@
>   */
>  function buildServerFilterListFromDir(aDir, aISPHeaderList)
>  {
> +  // Now iterate over each file in the directory looking for .sfd files.
> +  const suffix = ".sfd";

consts should begin with k's, so this should be const kSuffix.
Attachment #720405 - Flags: review?(mconley) → review+
Attached patch patch v3Splinter Review
OK, thanks.
Attachment #720405 - Attachment is obsolete: true
Attachment #725940 - Flags: review+
Keywords: checkin-needed
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 22.0
Comment on attachment 725940 [details] [diff] [review]
patch v3

[Approval Request Comment]
Regression caused by (bug #): bug 824150
User impact if declined: server junk filtering gets broken/unable to be set when the user enters the Junk settings pane.
Testing completed (on c-c, etc.): Verified fix on Daily 20130320030725.
Risk to taking this patch (and alternatives if risky): probably can't be much worse than it is.
Attachment #725940 - Flags: approval-comm-aurora?
Attachment #725940 - Flags: approval-comm-aurora? → approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.