Last Comment Bug 846694 - "Trust junk mail headers set by" dropdown shows only a number 5
: "Trust junk mail headers set by" dropdown shows only a number 5
Status: RESOLVED FIXED
: regression
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 22.0
Assigned To: :aceman
:
:
Mentors:
Depends on:
Blocks: 824150 847172
  Show dependency treegraph
 
Reported: 2013-03-01 02:54 PST by :aceman
Modified: 2013-03-28 05:58 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed


Attachments
patch to fix the issue (1.13 KB, patch)
2013-03-01 04:25 PST, Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)
no flags Details | Diff | Splinter Review
patch v2 (1.62 KB, patch)
2013-03-03 06:44 PST, :aceman
mconley: review+
iann_bugzilla: review+
Details | Diff | Splinter Review
patch v3 (1.63 KB, patch)
2013-03-17 15:46 PDT, :aceman
acelists: review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description :aceman 2013-03-01 02:54:19 PST
"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.7  
    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.
Comment 1 :aceman 2013-03-01 04:22:29 PST
The offending patch http://hg.mozilla.org/comm-central/rev/89fa8b687178 landed on Feb 04, so this will be needed for branch 21.
Comment 2 Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2013-03-01 04:25:19 PST
Created attachment 719913 [details] [diff] [review]
patch to fix the issue

Sorry for the breakage.
Comment 3 :aceman 2013-03-01 04:36:53 PST
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.
Comment 4 :aceman 2013-03-03 06:44:30 PST
Created attachment 720405 [details] [diff] [review]
patch v2

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
Comment 5 :aceman 2013-03-03 06:45:03 PST
...retrieving the pref value returns the default value as if it was the value of the pref.
Comment 6 Mike Conley (:mconley) 2013-03-17 15:39:29 PDT
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.
Comment 7 :aceman 2013-03-17 15:46:04 PDT
Created attachment 725940 [details] [diff] [review]
patch v3

OK, thanks.
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-03-19 14:50:46 PDT
https://hg.mozilla.org/comm-central/rev/81e97e309a34
Comment 9 :aceman 2013-03-20 07:17:20 PDT
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.
Comment 10 Mark Banner (:standard8, afk until Dec) 2013-03-28 05:58:06 PDT
Checked in: https://hg.mozilla.org/releases/comm-aurora/rev/19386d8aa499

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