Merge ~cjwatson/launchpad:date-widget-time-zone-name into launchpad:master

[Colin Watson]

Refactor time zone name handling in DateTimeWidget

`DateTimeWidget` currently works out the time zone name to display by calling `.tzname(None)` on a time zone object. This works with `pytz`, but it won't work once we switch to `dateutil.tz`, and there doesn't seem to be a reasonable way to get it to work with that time zone provider without poking about in private attributes; the best we could do would return a time zone abbreviation string rather than the original time zone name the user asked for (e.g. "CET" rather than "Europe/Paris"), which isn't very satisfactory.

Since the whole approach of expecting to be able to recover the original time zone name from a time zone object seems fundamentally dodgy, a better approach is to explicitly keep track of the original time zone name to start with. This needs some changes to `LaunchBag`, but the result should be much more robust and can easily be made to work with any time zone provider.

[Guruprasad (lgp171188)]

LGTM 👍🏼

review: Approve

diff --git a/lib/lp/app/widgets/date.py b/lib/lp/app/widgets/date.py
index acaa0f2..e024dfa 100644
--- a/lib/lp/app/widgets/date.py
+++ b/lib/lp/app/widgets/date.py
@@ -17,8 +17,9 @@ __all__ = [
     "DatetimeDisplayWidget",
 ]
 
-from datetime import datetime, timezone
+from datetime import datetime, timezone, tzinfo
 
+import pytz
 from zope.browserpage import ViewPageTemplateFile
 from zope.component import getUtility
 from zope.datetime import DateTimeError, parse
@@ -40,7 +41,6 @@ from lp.services.webapp.interfaces import ILaunchBag
 class DateTimeWidget(TextWidget):
     """A date and time selection widget with popup selector.
 
-      >>> import pytz
       >>> from zope.schema import Field
       >>> from lp.services.webapp.servers import LaunchpadTestRequest
       >>> field = Field(__name__="foo", title="Foo")
@@ -63,7 +63,7 @@ class DateTimeWidget(TextWidget):
     If there is a required time zone, then that overrides the user or system
     default, and the user is not invited to change the time zone:
 
-      >>> widget.required_time_zone = pytz.timezone("America/Los_Angeles")
+      >>> widget.required_time_zone_name = "America/Los_Angeles"
       >>> print(widget())  # doctest: +ELLIPSIS
       <BLANKLINE>
       <...in time zone: America/Los_Angeles...
@@ -114,7 +114,7 @@ class DateTimeWidget(TextWidget):
     """
 
     timeformat = "%Y-%m-%d %H:%M:%S"
-    required_time_zone = None
+    required_time_zone_name = None  # type: str
     display_zone = True
     from_date = None
     to_date = None
@@ -126,7 +126,7 @@ class DateTimeWidget(TextWidget):
     def __init__(self, context, request):
         super().__init__(context, request)
         launchbag = getUtility(ILaunchBag)
-        self.system_time_zone = launchbag.time_zone
+        self.system_time_zone_name = launchbag.time_zone_name
 
     @property
     def supported_input_formats(self):
@@ -158,7 +158,17 @@ class DateTimeWidget(TextWidget):
         return [o.strip() for o in outputs]
 
     @property
-    def time_zone(self):
+    def time_zone_name(self) -> str:
+        """The name of the widget time zone for display in the widget."""
+        if self.required_time_zone_name is not None:
+            return self.required_time_zone_name
+        assert (
+            self.system_time_zone_name is not None
+        ), "DateTime widget needs a time zone."
+        return self.system_time_zone_name
+
+    @property
+    def time_zone(self) -> tzinfo:
         """The widget time zone.
 
         This will either give you the user's time zone, or the system
@@ -167,7 +177,7 @@ class DateTimeWidget(TextWidget):
         externally-defined time zone. For example, when a person will join a
         conference in the time zone in which the conference is being held.
 
-          >>> import pytz
+          >>> from datetime import tzinfo
           >>> from zope.publisher.browser import TestRequest
           >>> from zope.schema import Field
           >>> field = Field(__name__="foo", title="Foo")
@@ -176,12 +186,12 @@ class DateTimeWidget(TextWidget):
         The time zone is a time zone object, not the string representation
         of that.
 
-          >>> print(type(widget.time_zone))
-          <class 'datetime.timezone'>
+          >>> isinstance(widget.time_zone, tzinfo)
+          True
 
-        The widget required_time_zone is None by default.
+        The widget required_time_zone_name is None by default.
 
-          >>> print(widget.required_time_zone)
+          >>> print(widget.required_time_zone_name)
           None
 
         The widget "system time zone" is generally UTC. It is the logged in
@@ -189,41 +199,32 @@ class DateTimeWidget(TextWidget):
         user. Although this isn't used directly, it influences the outcome
         of widget.time_zone.
 
-          >>> print(repr(widget.system_time_zone))
-          datetime.timezone.utc
+          >>> print(widget.system_time_zone_name)
+          UTC
 
-        When there is no required_time_zone, then we get the system time
-        zone.
+        When there is no required_time_zone_name, then we get the system
+        time zone.
 
-          >>> print(widget.required_time_zone)
+          >>> print(widget.required_time_zone_name)
           None
+          >>> print(widget.time_zone_name)
+          UTC
           >>> print(repr(widget.time_zone))
           datetime.timezone.utc
 
-        When there is a required_time_zone, we get it:
+        When there is a required_time_zone_name, we get it:
 
-          >>> widget.required_time_zone = pytz.timezone("Africa/Maseru")
+          >>> widget.required_time_zone_name = "Africa/Maseru"
+          >>> print(widget.time_zone_name)
+          Africa/Maseru
           >>> print(widget.time_zone)
           Africa/Maseru
 
         """
-        if self.required_time_zone is not None:
-            return self.required_time_zone
-        assert (
-            self.system_time_zone is not None
-        ), "DateTime widget needs a time zone."
-        return self.system_time_zone
-
-    @property
-    def time_zone_name(self):
-        """The name of the widget time zone for display in the widget."""
-        # XXX cjwatson 2023-03-09: In Python < 3.6, `timezone.utc.tzname`
-        # returns "UTC+00:00" rather than "UTC".  Drop this once we require
-        # Python >= 3.6.
-        if self.time_zone is timezone.utc:
-            return "UTC"
+        if self.time_zone_name == "UTC":
+            return timezone.utc
         else:
-            return self.time_zone.tzname(None)
+            return pytz.timezone(self.time_zone_name)
 
     def _align_date_constraints_with_time_zone(self):
         """Ensure that from_date and to_date use the widget time zone."""
@@ -383,14 +384,13 @@ class DateTimeWidget(TextWidget):
     def _parseInput(self, input):
         """Convert a string to a datetime value.
 
-          >>> import pytz
           >>> from zope.publisher.browser import TestRequest
           >>> from zope.schema import Field
           >>> field = Field(__name__="foo", title="Foo")
           >>> widget = DateTimeWidget(field, TestRequest())
-          >>> widget.required_time_zone = timezone.utc
-          >>> widget.time_zone
-          datetime.timezone.utc
+          >>> widget.required_time_zone_name = "UTC"
+          >>> widget.time_zone_name
+          'UTC'
 
         The widget converts an empty string to the missing value:
 
@@ -404,7 +404,7 @@ class DateTimeWidget(TextWidget):
 
         But it will handle other time zones:
 
-          >>> widget.required_time_zone = pytz.timezone("Australia/Perth")
+          >>> widget.required_time_zone_name = "Australia/Perth"
           >>> print(widget._parseInput("2006-01-01 12:00:00"))
           2006-01-01 12:00:00+08:00
 
@@ -434,7 +434,6 @@ class DateTimeWidget(TextWidget):
     def _toFormValue(self, value):
         """Convert a date to its string representation.
 
-          >>> import pytz
           >>> from zope.publisher.browser import TestRequest
           >>> from zope.schema import Field
           >>> field = Field(__name__="foo", title="Foo")
@@ -455,7 +454,7 @@ class DateTimeWidget(TextWidget):
         The date value will be converted to the widget's time zone
         before being displayed:
 
-          >>> widget.required_time_zone = pytz.timezone("America/New_York")
+          >>> widget.required_time_zone_name = "America/New_York"
           >>> widget._toFormValue(dt)
           '2006-01-01 07:00:00'
         """
@@ -497,7 +496,6 @@ class DateWidget(DateTimeWidget):
     The DateWidget subclass can limit requests to date ranges:
 
       >>> from datetime import date
-      >>> import pytz
       >>> from zope.publisher.browser import TestRequest
       >>> from zope.schema import Field
       >>> field = Field(__name__="foo", title="Foo")
@@ -509,7 +507,7 @@ class DateWidget(DateTimeWidget):
       >>> "[[2004,04,05],[2004,04,10]]" in widget()
       True
 
-    This widget ignores required_time_zone and system_time_zone and
+    This widget ignores required_time_zone_name and system_time_zone_name and
     interprets everything as UTC. This does not matter, because it is only
     picking the date, and it will always be rendered as a date sans time
     zone even if it is stored as a datetime.
@@ -517,11 +515,11 @@ class DateWidget(DateTimeWidget):
       >>> widget.time_zone
       datetime.timezone.utc
 
-      >>> widget.system_time_zone = pytz.timezone("America/New_York")
+      >>> widget.system_time_zone_name = "America/New_York"
       >>> widget.time_zone
       datetime.timezone.utc
 
-      >>> widget.required_time_zone = pytz.timezone("America/Los_Angeles")
+      >>> widget.required_time_zone_name = "America/Los_Angeles"
       >>> widget.time_zone
       datetime.timezone.utc
 
@@ -536,7 +534,7 @@ class DateWidget(DateTimeWidget):
     """
 
     timeformat = "%Y-%m-%d"
-    time_zone = timezone.utc
+    time_zone_name = "UTC"
 
     # ZPT that renders our widget
     __call__ = ViewPageTemplateFile("templates/date.pt")
diff --git a/lib/lp/app/widgets/templates/datetime.pt b/lib/lp/app/widgets/templates/datetime.pt
index d53d9ba..1042d65 100644
--- a/lib/lp/app/widgets/templates/datetime.pt
+++ b/lib/lp/app/widgets/templates/datetime.pt
@@ -9,7 +9,7 @@
                    disabled view/disabled_flag" />
 <tal:display_zone condition="view/display_zone">
   <span> in time zone: <tal:tz replace="view/time_zone_name" />
-  <a tal:condition="not: view/required_time_zone"
+  <a tal:condition="not: view/required_time_zone_name"
      href="/people/+me/+editlocation">
     <img src="/@@/edit"/>
   </a></span>
diff --git a/lib/lp/blueprints/browser/sprint.py b/lib/lp/blueprints/browser/sprint.py
index a89a362..41148c0 100644
--- a/lib/lp/blueprints/browser/sprint.py
+++ b/lib/lp/blueprints/browser/sprint.py
@@ -291,9 +291,9 @@ class SprintAddView(LaunchpadFormView):
         self.widgets["time_ends"].timeformat = timeformat
         time_zone_widget = self.widgets["time_zone"]
         if time_zone_widget.hasValidInput():
-            tz = pytz.timezone(time_zone_widget.getInputValue())
-            self.widgets["time_starts"].required_time_zone = tz
-            self.widgets["time_ends"].required_time_zone = tz
+            tz_name = time_zone_widget.getInputValue()
+            self.widgets["time_starts"].required_time_zone_name = tz_name
+            self.widgets["time_ends"].required_time_zone_name = tz_name
 
     def validate(self, data):
         time_starts = data.get("time_starts")
@@ -373,11 +373,11 @@ class SprintEditView(LaunchpadEditFormView):
         time_zone_widget = self.widgets["time_zone"]
         # What time zone are the start and end values relative to?
         if time_zone_widget.hasValidInput():
-            tz = pytz.timezone(time_zone_widget.getInputValue())
+            tz_name = time_zone_widget.getInputValue()
         else:
-            tz = pytz.timezone(self.context.time_zone)
-        self.widgets["time_starts"].required_time_zone = tz
-        self.widgets["time_ends"].required_time_zone = tz
+            tz_name = self.context.time_zone
+        self.widgets["time_starts"].required_time_zone_name = tz_name
+        self.widgets["time_ends"].required_time_zone_name = tz_name
 
     def validate(self, data):
         time_starts = data.get("time_starts")
diff --git a/lib/lp/blueprints/browser/sprintattendance.py b/lib/lp/blueprints/browser/sprintattendance.py
index 2964ffd..194f340 100644
--- a/lib/lp/blueprints/browser/sprintattendance.py
+++ b/lib/lp/blueprints/browser/sprintattendance.py
@@ -44,11 +44,10 @@ class BaseSprintAttendanceAddView(LaunchpadFormView):
 
     def setUpWidgets(self):
         LaunchpadFormView.setUpWidgets(self)
-        tz = pytz.timezone(self.context.time_zone)
         self.starts_widget = self.widgets["time_starts"]
         self.ends_widget = self.widgets["time_ends"]
-        self.starts_widget.required_time_zone = tz
-        self.ends_widget.required_time_zone = tz
+        self.starts_widget.required_time_zone_name = self.context.time_zone
+        self.ends_widget.required_time_zone_name = self.context.time_zone
         # We don't need to display seconds
         timeformat = "%Y-%m-%d %H:%M"
         self.starts_widget.timeformat = timeformat
@@ -57,8 +56,9 @@ class BaseSprintAttendanceAddView(LaunchpadFormView):
         # after the sprint. We will accept a time just before or just after
         # and map those to the beginning and end times, respectively, in
         # self.getDates().
-        from_date = self.context.time_starts.astimezone(tz)
-        to_date = self.context.time_ends.astimezone(tz)
+        time_zone = pytz.timezone(self.context.time_zone)
+        from_date = self.context.time_starts.astimezone(time_zone)
+        to_date = self.context.time_ends.astimezone(time_zone)
         self.starts_widget.from_date = from_date - timedelta(days=1)
         self.starts_widget.to_date = to_date
         self.ends_widget.from_date = from_date
diff --git a/lib/lp/services/webapp/doc/launchbag.rst b/lib/lp/services/webapp/doc/launchbag.rst
index 304707d..41e7b6e 100644
--- a/lib/lp/services/webapp/doc/launchbag.rst
+++ b/lib/lp/services/webapp/doc/launchbag.rst
@@ -79,20 +79,23 @@ will be [e-mail].
     [e-mail]
 
 
-time_zone
----------
+time_zone_name and time_zone
+----------------------------
 
-The time_zone attribute gives the user's time zone, as an pytz object.
+The time_zone_name attribute gives the name of the user's time zone; the
+time_zone attribute gives it as a tzinfo object.
 
     >>> from lp.testing.factory import LaunchpadObjectFactory
     >>> factory = LaunchpadObjectFactory()
     >>> person = factory.makePerson()
     >>> ignored = login_person(person)
+    >>> launchbag.time_zone_name
+    'UTC'
     >>> launchbag.time_zone
-    <UTC>
+    datetime.timezone.utc
 
-It's cached, so even if the user's time zone is changed, it will stay
-the same. This is to optimize the look-up time, since some pages look it
+They're cached, so even if the user's time zone is changed, they will stay
+the same. This is to optimize the look-up time, since some pages look them
 up a lot of times.
 
     >>> person.setLocation(
@@ -101,11 +104,15 @@ up a lot of times.
     ...     "Europe/Paris",
     ...     launchbag.user,
     ... )
+    >>> launchbag.time_zone_name
+    'UTC'
     >>> launchbag.time_zone
-    <UTC>
+    datetime.timezone.utc
 
 After the LaunchBag has been cleared, the correct time zone is returned.
 
     >>> launchbag.clear()
+    >>> launchbag.time_zone_name
+    'Europe/Paris'
     >>> launchbag.time_zone
     <... 'Europe/Paris' ...>
diff --git a/lib/lp/services/webapp/interfaces.py b/lib/lp/services/webapp/interfaces.py
index c6ef41a..ad0a103 100644
--- a/lib/lp/services/webapp/interfaces.py
+++ b/lib/lp/services/webapp/interfaces.py
@@ -335,6 +335,7 @@ class ILaunchBag(Interface):
     user = Attribute("Currently authenticated IPerson, or None")
     login = Attribute("The login used by the authenticated person, or None")
 
+    time_zone_name = Attribute("The name of the user's time zone")
     time_zone = Attribute("The user's time zone")
 
     developer = Bool(
diff --git a/lib/lp/services/webapp/launchbag.py b/lib/lp/services/webapp/launchbag.py
index a9d12d7..f20dfe0 100644
--- a/lib/lp/services/webapp/launchbag.py
+++ b/lib/lp/services/webapp/launchbag.py
@@ -30,8 +30,6 @@ from lp.services.webapp.interaction import get_current_principal
 from lp.services.webapp.interfaces import ILoggedInEvent, IOpenLaunchBag
 from lp.soyuz.interfaces.distroarchseries import IDistroArchSeries
 
-_utc_tz = timezone.utc
-
 
 @implementer(IOpenLaunchBag)
 class LaunchBag:
@@ -92,6 +90,7 @@ class LaunchBag:
             setattr(store, attribute, None)
         store.login = None
         store.time_zone = None
+        store.time_zone_name = None
 
     @property
     def person(self):
@@ -147,13 +146,22 @@ class LaunchBag:
         return getattr(self._store, "bugtask", None)
 
     @property
-    def time_zone(self):
-        if getattr(self._store, "time_zone", None) is None:
+    def time_zone_name(self):
+        if getattr(self._store, "time_zone_name", None) is None:
             if self.user:
-                self._store.time_zone = pytz.timezone(self.user.time_zone)
+                self._store.time_zone_name = self.user.time_zone
             else:
                 # fall back to UTC
-                self._store.time_zone = _utc_tz
+                self._store.time_zone_name = "UTC"
+        return self._store.time_zone_name
+
+    @property
+    def time_zone(self):
+        if getattr(self._store, "time_zone", None) is None:
+            if self.time_zone_name == "UTC":
+                self._store.time_zone = timezone.utc
+            else:
+                self._store.time_zone = pytz.timezone(self.time_zone_name)
         return self._store.time_zone