Merge pull request #4 from raven0034/main

Move tz to localize and centralise inc ORM reconstructor

Author: Tomoli75

DEVELOPER.md

@@ -1,18 +1,21 @@
+<!-- omit from toc -->
 # Developer documentation for fulcrum
 
 This documentation is intended for developers working on fulcrum. This explains how to set up fulcrum locally, why some design decisions were made, and anything else you should need. If you need any help, message in #tech-team on the UWCS discord.
 
 If you're a user pls see the [user documentation](USER.md) instead.
 
+<!-- omit from toc -->
 ## Table of Contents
 
 - [Running](#running)
   - [SSL Note](#ssl-note)
-  - [libcairo-2.dll](#libcairo-2dll)
+  - [Timezones Note](#timezones-note)
 - [Stack](#stack)
 - [File Structure](#file-structure)
 - [Database schema](#database-schema)
 - [API](#api)
+  - [Creating and managing API keys](#creating-and-managing-api-keys)
 - [Styling](#styling)
 - [Icons](#icons)
   - [Updating phosphor icons](#updating-phosphor-icons)
@@ -22,6 +25,7 @@ If you're a user pls see the [user documentation](USER.md) instead.
   - [Warwick Weeks](#warwick-weeks)
   - [Warwick Map API](#warwick-map-api)
 - [Testing](#testing)
+- [Ideas for the future](#ideas-for-the-future)
 
 ## Running
 
@@ -62,6 +66,12 @@ The app uses UWCS's keycloak server for authentication. This is set to enable bo
 
 If you are not exec, you can bypass auth by setting the environment variable `DEV` to `1`.
 
+### Timezones Note
+
+The database currently stores and interprets all datetimes as naive Europe/London dts. An ORM reconstructor is used in the `Event` model to make localising of read dts consistent with what (mostly) previously occurred with a newly constructed `Event`. The API now returns full ISO-8601 dt strings for external usage.
+
+Using `replace` on `tzinfo` with pytz causes inconsistent behaviour, notably causing weird offsets of 1 minute, hence the use of `localize` instead. This is due to LMT shenanigans (see Raven's braindump about this if you're interested: https://discord.com/channels/189453139584221185/292281159495450625/1421431330440216696)
+
 ## Stack
 
 Note the site uses the same stack (Flask, SQLAlchemy) as [CS139](https://warwick.ac.uk/fac/sci/dcs/teaching/modules/cs139/), to enable easy maintenance and development by most DCS students. Notably this means that there is no frontend framework (e.g. React, Vue) and the site is rendered server-side. Should this change in the future, the API is set up to partially support this (although it will probably need some tweaking). If some js is complex, I would reccomend using a pre-existing library (see bootstrap and tags) as this will make maintenance easier.

config.py

@@ -3,8 +3,8 @@
 from datetime import datetime
 from pathlib import Path
 
-import pytz
 import requests
+from pytz import timezone
 
 # custom colours
 colours = {
@@ -86,7 +86,7 @@
 # +5 years for future proofing
 # note, this requires the app to be restarted every 5 years
 # (however given the current state of UWCS uptime, this is guaranteed to happen)
-_now = datetime.now(tz=pytz.timezone("Europe/London"))
+_now = datetime.now(tz=timezone("Europe/London"))
 _academic_year = _now.year if _now.month <= 9 else _now.year + 1  # noqa: PLR2004
 try:
     warwick_weeks = [

events/ui.py

@@ -1,9 +1,9 @@
 from datetime import datetime, timedelta
 
-import pytz
 from flask import Blueprint, abort, flash, redirect, render_template, request, url_for
 from flask import Response as FlaskResponse
 from icalendar import Calendar, Event
+from pytz import timezone
 from werkzeug.datastructures import ImmutableMultiDict
 from werkzeug.wrappers import Response
 
@@ -367,7 +367,7 @@ def get_ical() -> Response:
             "dtend",
             event.end_time if event.end_time else event.start_time + timedelta(hours=1),
         )
-        ical_event.add("dtstamp", datetime.now(tz=pytz.timezone("Europe/London")))
+        ical_event.add("dtstamp", datetime.now(timezone("Europe/London")))
         ical_event.add(
             "url",
             url_for(

events/utils.py

@@ -3,34 +3,38 @@
 from typing import Match
 from xml.etree import ElementTree as ET
 
-import pytz
 from markdown import Markdown, markdown
 from markdown.extensions import Extension
 from markdown.inlinepatterns import InlineProcessor
 from markdown.treeprocessors import Treeprocessor
 from markupsafe import escape
+from pytz import timezone
 from sqlalchemy import func
 
 from config import colours, old_dates, phosphor_icons, warwick_weeks  # , room_mapping
 from schema import Event, Tag, Week, db
 
+LONDON = timezone("Europe/London")
 
 def get_datetime_from_string(date_str: str) -> datetime | str:
-    """Convert a date string in the format 'YYYY-MM-DDTHH:MM' to a datetime object."""
+    """
+    Convert datetime str in the format 'YYYY-MM-DDTHH:MM'
+    to a Europe/London tz-aware datetime object
+    """
+
     try:
-        return datetime.strptime(date_str, "%Y-%m-%dT%H:%M").replace(
-            tzinfo=pytz.timezone("Europe/London")
-        )
+        return LONDON.localize(datetime.strptime(date_str, "%Y-%m-%dT%H:%M"))  # noqa: DTZ007
     except ValueError:
         return "Invalid date format, expected 'YYYY-MM-DDTHH:MM'"
 
 
 def get_date_from_string(date_str: str) -> datetime | str:
-    """Convert a date string in the format 'YYYY-MM-DD' to a datetime object."""
+    """
+    Convert datetime str in the format 'YYYY-MM-DD' to a Europe/London tz-aware datetime
+    """
+
     try:
-        return datetime.strptime(date_str, "%Y-%m-%d").replace(
-            tzinfo=pytz.timezone("Europe/London")
-        )
+        return LONDON.localize(datetime.strptime(date_str, "%Y-%m-%d"))  # noqa: DTZ007
     except ValueError:
         return "Invalid date format, expected 'YYYY-MM-DD'"
 
@@ -49,11 +53,6 @@ def create_event(  # noqa: PLR0913
 ) -> Event | str:
     """Create an event"""
 
-    # convert start_time and normalise end_time
-    start_time = start_time.replace(tzinfo=pytz.timezone("Europe/London"))
-
-    end_time = end_time.replace(tzinfo=pytz.timezone("Europe/London"))
-
     if end_time < start_time:
         return "End time cannot be before start time"
 
@@ -220,6 +219,7 @@ def get_week_by_date(date: datetime) -> Week | None:  # noqa: PLR0911, PLR0912
 def create_week_from_date(date: datetime) -> Week | None:
     """Create a week from a given date"""
 
+    # date.date() should already be localised by this point
     week = Week.query.filter(
         (date.date() >= Week.start_date) & (date.date() <= Week.end_date)  # type: ignore
     ).first()
@@ -260,7 +260,7 @@ def create_repeat_event(  # noqa: PLR0913
             location_url,
             icon,
             colour,
-            start_time,
+            start_time,  # localised in create_event
             end_time,
             tags,
         )
@@ -304,12 +304,10 @@ def get_event_by_id(event_id: int) -> Event | None:
 
 def get_event_by_slug(year: int, term: int, week: int, slug: str) -> Event | None:
     """Get an event by slug"""
-    return (
-        Event.query.filter(
+    return Event.query.filter(
             Event.week.has(academic_year=year, term=term, week=week),
             Event.slug == slug,  # type: ignore
-        )
-    ).first()
+        ).first()
 
 
 # shamelessly stolen from docs (https://python-markdown.github.io/extensions/api/#example_3)
@@ -375,8 +373,9 @@ def prepare_event(event: Event) -> dict:
 def group_events(events: list[Event]) -> list[dict]:
     """Group events by term, week, and day"""
 
-    # initalise dictionary
+    # initialise dictionary
     # this is not too nested i have no clue what youre talking about
+    # R: human equivalent of when copilot goes loopy
     grouped_events = defaultdict(
         lambda: defaultdict(lambda: defaultdict(lambda: defaultdict(list)))
     )
@@ -431,12 +430,13 @@ def get_events_by_time(
         query = query.filter(Event.draft.is_(False))  # type: ignore
 
     # order by start_time, end_time, and name
-    return query.order_by(Event.start_time, Event.end_time, Event.name).all()  # type: ignore
-
+    return query.order_by(
+            Event.start_time, Event.end_time, Event.name  # type: ignore
+        ).all()
 
 def get_upcoming_events(include_drafts: bool = False) -> list[Event]:
     """Get all events in this week, and future weeks"""
-    now = datetime.now(pytz.timezone("Europe/London"))
+    now = datetime.now(LONDON)
     week = get_week_by_date(now)
 
     if not week:
@@ -448,8 +448,8 @@ def get_upcoming_events(include_drafts: bool = False) -> list[Event]:
         query = query.filter(Event.draft.is_(False))  # type: ignore
 
     return query.order_by(
-        Event.start_time, Event.end_time, Event.name  # type: ignore
-    ).all()
+                Event.start_time, Event.end_time, Event.name  # type: ignore
+            ).all()
 
 
 def get_all_events(include_drafts: bool = False) -> list[Event]:
@@ -466,7 +466,7 @@ def get_all_events(include_drafts: bool = False) -> list[Event]:
 
 def get_week_events() -> list[Event]:
     """Get all events in the current week"""
-    now = datetime.now(pytz.timezone("Europe/London"))
+    now = datetime.now(LONDON)
     week = get_week_by_date(now)
 
     if not week:
@@ -494,7 +494,7 @@ def get_events_in_week_range(start: Week, end: Week) -> list[Event]:
 
 def get_days_events(days: int) -> list[Event]:
     """Get all events in the next <days> days"""
-    now = datetime.now(pytz.timezone("Europe/London"))
+    now = datetime.now(LONDON)
     end_date = now + timedelta(days=days)
 
     return (
@@ -541,21 +541,22 @@ def edit_event(  # noqa: PLR0913
     event.location_url = (
         location_url if location_url is not _KEEP else event.location_url
     )
+
     if icon is not _KEEP:
         event.icon = icon.lower() if icon is not None else event.icon  # type: ignore
         if icon in phosphor_icons:
             event.icon = f"ph-{icon}"
     event.colour = colour if colour is not _KEEP else event.colour
 
     event.start_time = (
-        start_time.replace(tzinfo=pytz.timezone("Europe/London"))  # type: ignore
+        start_time  # type: ignore
         if start_time is not _KEEP
-        else event.start_time.replace(tzinfo=pytz.timezone("Europe/London"))  # type: ignore
+        else event.start_time  # type: ignore
     )
     event.end_time = (
-        end_time.replace(tzinfo=pytz.timezone("Europe/London"))  # type: ignore
+        end_time  # type: ignore
         if end_time is not _KEEP
-        else event.end_time.replace(tzinfo=pytz.timezone("Europe/London"))  # type: ignore
+        else event.end_time  # type: ignore
     )
 
     # update the week associated with the event

schema.py

@@ -1,14 +1,16 @@
 import re
 from datetime import date, datetime, timedelta
 
-import pytz
 from flask import Flask, url_for
 from flask_sqlalchemy import SQLAlchemy
+from pytz import timezone
 from sqlalchemy import and_, func
-from sqlalchemy.orm import foreign
+from sqlalchemy.orm import foreign, reconstructor
 
 from config import colours, custom_icons, icons
 
+LONDON = timezone("Europe/London")
+
 db = SQLAlchemy()
 
 
@@ -148,9 +150,42 @@ def __init__(  # noqa: PLR0913
         self.location_url = location_url
         self.icon = icon
         self.colour = colour if colour else "blue"
-        # ensure times are london-time
-        self.start_time = start_time.replace(tzinfo=pytz.timezone("Europe/London"))
-        self.end_time = end_time.replace(tzinfo=pytz.timezone("Europe/London"))
+        self.start_time = start_time
+        self.end_time = end_time
+        self._localise_times()
+
+    @reconstructor
+    def reinit(self) -> None:
+        """
+        Localise start and end times on db load
+        Previously only happened in __init__ ie only new instantiations
+        Removes need to remember to manually localise everywhere
+        """
+
+        self._localise_times()
+
+    def _localise_times(self) -> None:
+        """
+        Helper to localise start and end times of events
+
+        Only localise if not already tz-aware - allows external localisation
+
+        For tz-aware DB, harden to strictly enforce 'Europe/London' e.g.
+        ```
+        self.end_time.tzinfo.zone == LONDON.zone
+        ```
+        None is fine rn and cleaner
+        Use localize instead of .replace to avoid LMT offset issue
+
+        Should not need to localise start_time or end_time outside this
+        """
+
+        self.start_time = LONDON.localize(self.start_time) \
+                                            if self.start_time.tzinfo is None \
+                                            else self.start_time
+        self.end_time = LONDON.localize(self.end_time) \
+                                            if self.end_time.tzinfo is None \
+                                            else self.end_time
 
     def __repr__(self) -> str:
         return (
@@ -257,7 +292,7 @@ class APIKey(db.Model):
     def __init__(self, key_hash: str, owner: str) -> None:
         self.key_hash = key_hash
         self.owner = owner
-        self.created_at = datetime.now(pytz.timezone("Europe/London"))
+        self.created_at = datetime.now(LONDON)
         self.active = True
 
     def __repr__(self) -> str: