1355965 - provided recommended update interval for feeds ("Publisher recommends:") also used for calculation with different time unit

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Thunderbird Daily 55.0a1 20170408 + 54.0a2 20170412 on Windows 8.1

The provided recommended update interval for feeds ("Publisher recommends:") also gets used for the calculation with the other time unit.

Steps to reproduce:
1. Subscribe to https://leipzig.craigslist.de/search/aos?format=rss This has
> <syn:updateFrequency>6</syn:updateFrequency>
> <syn:updatePeriod>hourly</syn:updatePeriod>
2. Select the feed if necessary.
3. Change the time unit from minutes to days.
Actual result:
The publisher recommendation changes from 10 to 0.25. The 6 gets divided by 24 now:
https://hg.mozilla.org/releases/comm-aurora/rev/8571f68411c7#l20.350
Expected result:
6 / (24 * 60) ~= 0.004 shown (or the time with unit and no conversion: "6 minutes")

Comment 1

[alta88]

heh, i sure hope that's the worst bug in Feeds..  aryx, if you can do a patch fixing the calculation, i will review it.

Comment 2

[alta88]

Attachment: recUnits.patch

the correct calculation is actually (60/6)/(24*60), or 1/6/24. the value in days is merely to be correct informationally; only int values are accepted and values less than a day must use hours.

Comment 3

[alta88]

s/hours/minutes

Comment 4

[alta88]

Comment on attachment 8930287 [details] [diff] [review]
recUnits.patch

This is a prereq for Bug 1420473 to apply.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 8930287 [details] [diff] [review]
recUnits.patch

Review of attachment 8930287 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +1193,2 @@
>      if (aUpdates.updatePeriod == null)
> +      return val;

i'd keep the return "" here for easier readability

@@ +1214,5 @@
>          val = 365 * units / frequency;
>          break;
>      }
>  
> +    return val ? Math.round(val * 1000) / 1000 : val;

here too keep the "" at the end. It's easier to understand

Comment 6

[alta88]

Attachment: recUnits.patch

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7e90fcd346bb
Fix rec units for days calculation. r=mkmelin