1494160 - Timezone list should contain recent and future rules

Status: RESOLVED FIXED

(Calendar :: Internal Components, enhancement)

Description

[Geoff Lankow (:darktrojan)]

Our zones file only contains one set of rules per timezone, but there's a change in timezone rules, then future events beyond the date of the change are wrong, and past events before the change are wrong once the change happens.

The timezone service is capable of handling multiple rules in the zones file, but we need to work out how to select which rules to have (avoiding unnecessary junk inflating the file size), and how to do it without changing just about every rule in the file (currently we're using sanitised data, but that exists only for the current rules).

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1494160-tzupdate-1.diff

Fixing this was a lot harder than it looks, especially while trying to minimise the changes to zones.json. Please check those changes, I *think* they all correlate to actual changes, but I could easily have missed something.

Comment 2

[[:MakeMyDay]]

Would it be possible with a reasonable effort to incorporate also definitions for past times here? See bug 1497463.

Comment 3

[Geoff Lankow (:darktrojan)]

Possible, yes, but it would result in a much larger and more difficult to review zones file, and what does it achieve?

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9013236 [details] [diff] [review]
1494160-tzupdate-1.diff

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

It looks pretty hard, tbh, which is why I've been delaying this. I'm not quite sure how to best check this patch, the code looks fine.

What I'm wondering, sometimes there are timezone changes that are corrections due to the rules previously being wrong. Would that be handled here, or would it then show the wrong (but technically correct) information?

Comment 5

[Geoff Lankow (:darktrojan)]

We'd be shipping the latest information about current and past timezones (i.e. with mistakes hopefully corrected), not the latest and previous versions of the information.

I guess if we shipped faulty information that could result in events changing time when the fault gets corrected, since we generally only record the timezone ID of an event. But that's already the case.

Does that answer your question, or am I misunderstanding?

Comment 6

[Geoff Lankow (:darktrojan)]

I need to update the zones information again with this code, so I'll do that and get it checked. NI'ing myself so I remember.

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Yes, that answers my question. Thanks!

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1494160-tzsplit-1.diff

I want to add a part zero to this, because checking changes make my brain explode. I propose here to store each STANDARD/DAYLIGHT component on a separate line of the JSON file (i.e. in an array of strings, not one big long string) for easier reading.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: 1494160-tzupdate-2.diff

This is just a minor update to the earlier patch, and I've taken the actual zone changes out of it.

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1494160-dotzupdate-1.diff

And these are the actual changes, still at 2018i although there is a 2019a. It should be much easier to review now. I think I've done the right thing about bug 1515937.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9059410 [details] [diff] [review]
1494160-tzsplit-1.diff

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

With a change like this we'd likely have to make a change to the major version of the timezone file, but since we are not distributing the extension I think we can get away without.

Comment 12

[Geoff Lankow (:darktrojan)]

I'm unhappy with this, so I haven't shipped it. There's definitely some bad data produced, although we already had some bad data. Casablanca, for example, has daylight savings time but we don't list it. I bet we don't have many Moroccan users, but if we can't get the timezone right we'll have even fewer.

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 1494160-tzupdate-3.diff

Comment 14

[Geoff Lankow (:darktrojan)]

Comment on attachment 9064635 [details] [diff] [review]
1494160-tzupdate-3.diff

Oh damn, now I remember why I didn't post this yesterday.

Comment 15

[Geoff Lankow (:darktrojan)]

Attachment: 1494160-tzupdate-4.diff

Okay, finally I think I have this right.

I've included the changes this patch would introduce for the years 2018-2022, which actually doesn't include the America/Caracas changes that the tests rely on. I'm not sure if we should go back that far, but if we do it will be easier to review as a second change.

I also haven't updated to version 2019a in this patch.

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9064663 [details] [diff] [review]
1494160-tzupdate-4.diff

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

::: calendar/timezones/update-zones.py
@@ +241,5 @@
>  
>          zones = {}
>          for entry in os.listdir(path):
> +            if entry == "Etc":
> +                continue

Are we not including Etc zones?

Comment 17

[Geoff Lankow (:darktrojan)]

Are we not including Etc zones?

We didn't include them before. I changed to a newer version of vzic, and I think that's what caused them to appear in the list.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/116e52b10f8f
Store timezone components in an array of strings, not one long string; r=Fallen
https://hg.mozilla.org/comm-central/rev/8c2e0c1e7756
Modify timezone update script so that zones.json can contain recent and future rules; r=Fallen

Comment 19

[Geoff Lankow (:darktrojan)]

I ended up pushing this without the tests that rely on America/Caracas changing in 2016. I did however, add a test for America/Sao_Paulo.