Closed Bug 1257736 Opened 4 years ago Closed 3 years ago

Fix timezone update script so that it runs

Categories

(Calendar :: Internal Components, defect)

Lightning 4.9
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(4 files)

The changes to the update script to save the previous data in the tree don't actually work. I'm going to fix it and update the zones.
Attached patch 1257736a-1.diffSplinter Review
As well as fixing the bug I've cleaned up some formatting nits my linter was complaining about.
Attachment #8731988 - Flags: review?(philipp)
Attached patch 1257736b-1.diffSplinter Review
Update to 2016b. This diff has whitespace changes ignored due to previous.json having Windows line-endings that I've removed.
Attachment #8731991 - Flags: review?(philipp)
Patch 1257736b-1.diff should be moved to bug 1255878 and then applied to all branches including both esr branches.

We should leave this bug for patch 1257736a-1.diff as that needs only to go to cc and ca.
Component: General → Internal Components
Version: unspecified → Lightning 4.9
Comment on attachment 8731991 [details] [diff] [review]
1257736b-1.diff

The version says 2.2015g, but the new timezones are added with 2.2016b. Can you fix this and then maybe move that to the bug mentioned?
Attachment #8731991 - Flags: review?(philipp) → review-
Attachment #8731988 - Flags: review?(philipp) → review+
(In reply to Philipp Kewisch [:Fallen] from comment #4)
> Comment on attachment 8731991 [details] [diff] [review]
> 1257736b-1.diff
> 
> The version says 2.2015g, but the new timezones are added with 2.2016b. Can
> you fix this and then maybe move that to the bug mentioned?

Sure you're not reading the version number of previous.json?
Comment on attachment 8731991 [details] [diff] [review]
1257736b-1.diff

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

I certainly am, sorry about that!
Attachment #8731991 - Flags: review- → review+
Blocks: 1255878
Attached patch 1257736c-1.diffSplinter Review
Might as well do this while waiting for the tree to open.
Attachment #8734995 - Flags: review?(philipp)
Comment on attachment 8734995 [details] [diff] [review]
1257736c-1.diff

That timezone changes look odd. If you look for example at the change for Baku, the DST is removed. That means now an offset will be applied to all previous events in the previous dst period of previous years by one hour (the same will be true when adding new DST - just the other way around) - so imho the existing dst definition has to stay but needs to be extended by an dtend some time around last October.

With other words, if the gdata provider test fails after update, it's not the test that needs to be fixed here but the timezone definition.

This seems to be a problem not just for the changes made with this patch but to all nearly definitions in zones.json.
Comment on attachment 8734995 [details] [diff] [review]
1257736c-1.diff

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

I'll inquire about how other clients handle the timezone changes when I am at calconnect. r/a+ for this patch on all branches including both ESR's, sorry for the delay.
Attachment #8734995 - Flags: review?(philipp)
Attachment #8734995 - Flags: review+
Attachment #8734995 - Flags: approval-calendar-release+
Attachment #8734995 - Flags: approval-calendar-beta+
Attachment #8734995 - Flags: approval-calendar-aurora+
Thanks. Patch B still needs the same approvals.
Attachment #8731991 - Flags: approval-calendar-release+
Attachment #8731991 - Flags: approval-calendar-beta+
Attachment #8731991 - Flags: approval-calendar-aurora+
Comment on attachment 8731991 [details] [diff] [review]
1257736b-1.diff

This patch cannot land on aurora, beta, and release as is because it adds new strings and therefore has L10N impact.

Maybe a new patch could be created that updates only the existing timezone definitions but doesn't add new ones on the release branch?
As per http://mxr.mozilla.org/comm-central/source/calendar/locales/filter.py#11 I believe this shouldn't be a problem, since the strings do not cause errors. Now that we do l10n merges, the english strings will be merged in.

While this isn't ideal, it is better than not doing timezone updates on an old branch. I can reconfirm with Pike though to make sure this won't cause any issues.
The patches B and C still need an uplift to all branches including both ESR branches - at best before the merge after the upcoming weekend. When landing on beta, make sure to land on default. Geoff, can you take care, too?
Target Milestone: --- → 5.0
To backport the timezone updates (patch B and C) to esr38, further backports from bug 1180522, bug 1210458, bug 1232209, bug 1232216 and bug 1237085 are needed. This patch cummulates all relevant parts of them. This is intended to land on esr38 only.

The change from 1237085 to calendar/providers/gdata/modules/timezoneMap.jsm is not included as gdata provider bases on cc, which has the respective change already.
Attachment #8742109 - Flags: review?(philipp)
Attachment #8742109 - Flags: approval-calendar-release?(philipp)
Backports for the tz updates are landed up to esr45 (referenced in bug 1255878). Only esr38 is missing.
Comment on attachment 8742109 [details] [diff] [review]
1232209etal-esr38-v1.diff

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

Doing an AMO release for Lightning 4.0 after I have uploaded 4.7 will probably be tricky/impossible, but I'm ok with this for the last ESR38 distribution release.
Attachment #8742109 - Flags: review?(philipp)
Attachment #8742109 - Flags: review+
Attachment #8742109 - Flags: approval-calendar-release?(philipp)
Attachment #8742109 - Flags: approval-calendar-release+
TZ updates backported to esr38 (referenced in bug 1255878).
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.