1239622 - Wrong Calendar Week range displayed for January 2021 in month view

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Alex]

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

Steps to reproduce:

Select January 2021 in Calender with Month-View.


Actual results:

Shows "CWs: 1-4"


Expected results:

Should shows "CWs: 53-4"

Comment 1

[Wayne Mery (:wsmwk)]

please also give your version of calendar addon

Comment 2

[Decathlon]

Attachment: patch-v1

Confirmed.
For the weeks interval showed in month view, it happens when displaying the January after a leap year that ends with a Thursday (e.g. 1/2021, 1/2049).

Under those conditions the code normalizes twice the year-day of the Thursday in the week and transforms a valid 366 (week #53) into a 1 (week #1).

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8708327 [details] [diff] [review]
patch-v1

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

The patch looks fine, but is unfortunately missing tests. I'd appreciate if you could write some.

::: calendar/base/src/calWeekInfoService.js
@@ +76,5 @@
>              var lastYearDate = aDateTime.clone();
>              lastYearDate.year -= 1;
>              thisWeeksThursday += lastYearDate.endOfYear.yearday;
> +        } else if (thisWeeksThursday > aDateTime.endOfYear.yearday) {
> +            // For the last few days of the year, we already are in week 1. 

Can you remove the whitespace while you are here?

Comment 4

[Decathlon]

Attachment: patch-v1 with test

What about this?
I've set the isDate property to true because this is the case when getWeekTitle() is used to dispaly the week interval in the navigation bar.
Instead I've left the dates in UTC because the test didn't pass with ical.js when I set a timezone.

Comment 5

[Decathlon]

Attachment: patch-v2 with test

Sorry Philipp, I had attached the patch with the commands for the try-server in the summary.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8711440 [details] [diff] [review]
patch-v2 with test

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

Sorry about all the comments on the testcase. If you think it is redundant to check all 7 years I'd be fine with skipping a few. We just need to make sure it also keeps working for years not affected by this bug.

::: calendar/test/unit/test_weekinfo_service.js
@@ +11,5 @@
> +                [5, 1 , "19930101T000000Z"],     // Start of week on Friday
> +                [1, 53, "20210101T000000Z"],     // Start of week on Monday
> +                [5, 1 , "20210101T000000Z"],     // Start of week on Friday
> +                [3, 53, "20490101T000000Z"],     // Start of week on Wednesday
> +                [5, 1 , "20490101T000000Z"]];    // Start of week on Friday

Can you add a few more test cases for years starting on other weekdays? In theory since there are only 7 years that have different weekday combinations, you should be able to check them all to make sure the results are correct.

@@ +13,5 @@
> +                [5, 1 , "20210101T000000Z"],     // Start of week on Friday
> +                [3, 53, "20490101T000000Z"],     // Start of week on Wednesday
> +                [5, 1 , "20490101T000000Z"]];    // Start of week on Friday
> +
> +    for (let d of data) {

You can use the following, then you don't have to use raw array indices:

for (let [wkst, wknum, dtstr] of data) {
  Preferences.set("calendar.week.start", wkst);
  ...
}

@@ +14,5 @@
> +                [3, 53, "20490101T000000Z"],     // Start of week on Wednesday
> +                [5, 1 , "20490101T000000Z"]];    // Start of week on Friday
> +
> +    for (let d of data) {
> +        Preferences.set("calendar.week.start", d[0]);

Be sure to set this pref to the old value before the test ends to make sure no other tests are thrown off.

@@ +19,5 @@
> +        let date = cal.createDateTime(d[2]);
> +        date.isDate = true;
> +        let weekNumber = cal.getWeekInfoService().getWeekTitle(date);
> +
> +        equal(weekNumber, d[1]);

Maybe you can pass the third argument to describe the expected result, e.g.

equal(weekNumber, wknum, "Week number matches for " + dtstr);

Comment 7

[Decathlon]

Attachment: patch-v3 with test

(In reply to Philipp Kewisch [:Fallen] from comment #6)

> Sorry about all the comments on the testcase.

Don't worry, all of them were necessary and useful. :)

> Can you add a few more test cases for years starting on other weekdays? In
> theory since there are only 7 years that have different weekday
> combinations, you should be able to check them all to make sure the results
> are correct.

It depends also on the start of week but testing all the 7x7 possible combinations should be exaggerated given that the fix basically changes things only for a specific case. I've add 7 years that start on all the weekdays and used different start of week than Sunday for a few.

I've also fixed all the points in the previous comment.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8711874 [details] [diff] [review]
patch-v3 with test

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

This looks great, thanks! r=philipp

Comment 9

[Decathlon]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/bbc9245c4930

the version 4.9 is missing in the Target Milestone setting.

Comment 10

[Decathlon]

@ Alex
Thanks for the bug report.