aula icon indicating copy to clipboard operation
aula copied to clipboard

Localized dueDate på Huskelisten

Open ebbeflarup opened this issue 1 year ago • 21 comments

Fix til https://github.com/scaarup/aula/issues/171

Dette fix localizer dueDate på huskelisten. Jeg oplever også at den vises forkert, men dette fix viser den nu korrekt.

ebbeflarup avatar Sep 17 '24 21:09 ebbeflarup

Hej @ebbeflarup Tak for dit bidrag 👍 Jeg kan se, at du har hardcoded 'Europe/Copenhagen' som timezone - det er jeg ikke helt vild med :)

Hvordan så de timestamps ud før?

scaarup avatar Sep 18 '24 07:09 scaarup

Hej @ebbeflarup Tak for dit bidrag 👍 Jeg kan se, at du har hardcoded 'Europe/Copenhagen' som timezone - det er jeg ikke helt vild med :)

Hvordan så de timestamps ud før?

Jeg vil meget gerne lave den hardcoded time zone om. Som udgangspunkt har jeg lagt mig op af dette hardcoded valg, som er den påkrævede "zone"-header fra requested: https://github.com/scaarup/aula/blob/5a955cf927bb5b2f55835231a382c65f70f09d95/custom_components/aula/client.py#L615

Kan du fortælle hvordan du foretrækker den i stedet? Som en const værdi importeret fra custom_components/aula/const.py, eller at den automatisk udeleder den lokale tids zone?

Før jeg localized dueDate så den rå json property sådan ud: "dueDate": "2024-09-22T22:00:00Z", Dette er i aula-appen vist som '23 september', hvilket giver mening hvis du localizer med den nuværende tidszone "CET+2", hvilket så bliver til 2024-09-23 00:00.

Lad mig høre hvad du tænker @scaarup.

ebbeflarup avatar Sep 18 '24 13:09 ebbeflarup

Man kan ikke tage zonen fra systemet? Altså den maskine hvorpå Python bliver afviklet.

Men uanset, så kan det jo kun blive pænere end det var 🙂

scaarup avatar Sep 18 '24 16:09 scaarup

Jeg tænker godt man kan tage zonen fra systemet, men er vi ikke altid interesseret i en dansk tidszone i Aula? Er det ikke rimeligt at antage at Aula brugere bor i Danmark?

Jeg tænker en risiko ved at lade lokaliten være styret af en indstilling på den enkelte maskine vil vel være at nogen ikke har indstillet den korrekt til DK tid, og derfor får en forkert dato. Jeg ved godt det nok er en edge-case, men på den mere "hardcoded" måde, vil vi i det mindste sikre at det ikke sker :)

Jeg har isoleret tidszonen i en konstant. Men hvis jeg har misforstået dit argument, så sig endelig til. 😅

ebbeflarup avatar Sep 18 '24 19:09 ebbeflarup

Jeg tænker godt man kan tage zonen fra systemet, men er vi ikke altid interesseret i en dansk tidszone i Aula? Er det ikke rimeligt at antage at Aula brugere bor i Danmark?

Det tror jeg ikke man kan antage. Måske bor en forælder i udlandet - hvad ved jeg :).

Jeg tænker en risiko ved at lade lokaliten være styret af en indstilling på den enkelte maskine vil vel være at nogen ikke har indstillet den korrekt til DK tid, og derfor får en forkert dato. Jeg ved godt det nok er en edge-case, men på den mere "hardcoded" måde, vil vi i det mindste sikre at det ikke sker :)

Jeg har isoleret tidszonen i en konstant. Men hvis jeg har misforstået dit argument, så sig endelig til. 😅

Men også sådan af princip, virker det forkert at hardcode timezone. Er det et problem bare at tage den fra system? Andre steder i integrationen, konverterer vi jo timestamps.

scaarup avatar Sep 19 '24 08:09 scaarup

Jeg tænker godt man kan tage zonen fra systemet, men er vi ikke altid interesseret i en dansk tidszone i Aula? Er det ikke rimeligt at antage at Aula brugere bor i Danmark?

Det tror jeg ikke man kan antage. Måske bor en forælder i udlandet - hvad ved jeg :).

Jeg tænker vi her er ude i en edge-case, som denne linie kode alligevel umuliggører understøttelsen af i nuværende kodebase :) https://github.com/scaarup/aula/blob/5a955cf927bb5b2f55835231a382c65f70f09d95/custom_components/aula/client.py#L615

Jeg tænker en risiko ved at lade lokaliten være styret af en indstilling på den enkelte maskine vil vel være at nogen ikke har indstillet den korrekt til DK tid, og derfor får en forkert dato. Jeg ved godt det nok er en edge-case, men på den mere "hardcoded" måde, vil vi i det mindste sikre at det ikke sker :) Jeg har isoleret tidszonen i en konstant. Men hvis jeg har misforstået dit argument, så sig endelig til. 😅

Men også sådan af princip, virker det forkert at hardcode timezone. Er det et problem bare at tage den fra system? Andre steder i integrationen, konverterer vi jo timestamps.

Ja det er et problem at tage den fra systemet :) Som jeg skrev tidligere, er det fejlbehæftet. Nu har jeg siddet og brugt 2 timer her til aften på at udlede den ved at bruge tzlocal funktionen get_localzone_name(), hvor outputtet er 'Etc/UTC'... Jeg synes det perfekt illustrerer de potentielle uhensigtmæssigheder brugerne af denne integration kan opleve, som i øvrigt bliver utrolig svære at debugge da den enkelte brugers setup kan have indflydelse på fejlen. I mit tilfælde er årsagen til fejlen potentielt at dev miljøet kører i docker, og løsningen kunne være at mounte den lokale tidszone som volume, men selvom dette nok ville give mig det korrekte resultat på min lokale maskine, kan jeg ikke være sikker på hvordan det vil køre i et rigtigt setup.

I øvrigt må du gerne henvise til et sted i integrationen hvor der modtages en CET+0 date, som konverteres til lokal tid, for det kan jeg alstå ikke lige finde?

Jeg holder fast i den lidt mere pragmatiske og simple løsning (som nogen nok ville kalde principielt forkert da den er hardcoded), men som til gengæld giver præcis det forventede resultat hver gang :)

Jeg forstår fuldt, hvis du ikke vil have det ind i kodebasen, men jeg vil ikke bruge mere tid på det nu. Alternativet er at jeg forker løsningen da du har lavet virkelig meget godt arbejde, og jeg gerne vil tilføje endnu flere fede features ;)

Jeg vil bla gerne synliggøre mine børns lånte bøger, og hvornår de skal afleveres. Alt data inklusiv billeder er tilgængeligt i Aula :)

ebbeflarup avatar Sep 19 '24 20:09 ebbeflarup

Jeg tænker godt man kan tage zonen fra systemet, men er vi ikke altid interesseret i en dansk tidszone i Aula? Er det ikke rimeligt at antage at Aula brugere bor i Danmark?

Det tror jeg ikke man kan antage. Måske bor en forælder i udlandet - hvad ved jeg :).

Jeg tænker vi her er ude i en edge-case, som denne linie kode alligevel umuliggører understøttelsen af i nuværende kodebase :)

https://github.com/scaarup/aula/blob/5a955cf927bb5b2f55835231a382c65f70f09d95/custom_components/aula/client.py#L615

Jeg tror ikke den har nogen effekt - du får jo alligevel timestamps i CET+0 / UTC ?

Jeg tænker en risiko ved at lade lokaliten være styret af en indstilling på den enkelte maskine vil vel være at nogen ikke har indstillet den korrekt til DK tid, og derfor får en forkert dato. Jeg ved godt det nok er en edge-case, men på den mere "hardcoded" måde, vil vi i det mindste sikre at det ikke sker :) Jeg har isoleret tidszonen i en konstant. Men hvis jeg har misforstået dit argument, så sig endelig til. 😅

Men også sådan af princip, virker det forkert at hardcode timezone. Er det et problem bare at tage den fra system? Andre steder i integrationen, konverterer vi jo timestamps.

Ja det er et problem at tage den fra systemet :) Som jeg skrev tidligere, er det fejlbehæftet. Nu har jeg siddet og brugt 2 timer her til aften på at udlede den ved at bruge tzlocal funktionen get_localzone_name(), hvor outputtet er 'Etc/UTC'... Jeg synes det perfekt illustrerer de potentielle uhensigtmæssigheder brugerne af denne integration kan opleve, som i øvrigt bliver utrolig svære at debugge da den enkelte brugers setup kan have indflydelse på fejlen. I mit tilfælde er årsagen til fejlen potentielt at dev miljøet kører i docker, og løsningen kunne være at mounte den lokale tidszone som volume, men selvom dette nok ville give mig det korrekte resultat på min lokale maskine, kan jeg ikke være sikker på hvordan det vil køre i et rigtigt setup.

I øvrigt må du gerne henvise til et sted i integrationen hvor der modtages en CET+0 date, som konverteres til lokal tid, for det kan jeg alstå ikke lige finde?

Prøv at se på linje 666-669 i client.py:

                                mytime = datetime.datetime.strptime(
                                    reminder["dueDate"], "%Y-%m-%dT%H:%M:%SZ"
                                )
                                ftime = mytime.strftime("%A %d. %B")

Her starter vi med at konvertere indholdet af "dueDate" (som kommer fra tredje part) til timestamp - hvorefter vi kan formatere det som vi vil. Hvis du gør det samme, så burde du ende med et timestamp, der matcher din lokale timezone. Eller hvad? :)

Jeg holder fast i den lidt mere pragmatiske og simple løsning (som nogen nok ville kalde principielt forkert da den er hardcoded), men som til gengæld giver præcis det forventede resultat hver gang :)

Jeg forstår fuldt, hvis du ikke vil have det ind i kodebasen, men jeg vil ikke bruge mere tid på det nu. Alternativet er at jeg forker løsningen da du har lavet virkelig meget godt arbejde, og jeg gerne vil tilføje endnu flere fede features ;)

Hvis ikke vi kan løse det på andre måder, så går vi selvfølgelig med din initielle løsning. Det er jo, som tidligere nævnt, bedre end udgangspunktet :).

Jeg vil bla gerne synliggøre mine børns lånte bøger, og hvornår de skal afleveres. Alt data inklusiv billeder er tilgængeligt i Aula :)

Lyder som en rigtig spændende usecase 👍

scaarup avatar Sep 20 '24 06:09 scaarup

Her starter vi med at konvertere indholdet af "dueDate" (som kommer fra tredje part) til timestamp - hvorefter vi kan formatere det som vi vil. Hvis du gør det samme, så burde du ende med et timestamp, der matcher din lokale timezone. Eller hvad? :)

Det er lige præcis de 4 linier kode der ikke fungerer og giver et forkert resultat i huskelisten 😅

ebbeflarup avatar Sep 20 '24 07:09 ebbeflarup

Her starter vi med at konvertere indholdet af "dueDate" (som kommer fra tredje part) til timestamp - hvorefter vi kan formatere det som vi vil. Hvis du gør det samme, så burde du ende med et timestamp, der matcher din lokale timezone. Eller hvad? :)

Det er lige præcis de 4 linier kode der ikke fungerer og giver et forkert resultat i huskelisten 😅

Ej virkelig? 🙃

Geez - jeg har ikke selv Huskelisten, så kan ikke teste. Men jeg laver lige et par manuelle tests og vender retur her.

scaarup avatar Sep 20 '24 08:09 scaarup

Her starter vi med at konvertere indholdet af "dueDate" (som kommer fra tredje part) til timestamp - hvorefter vi kan formatere det som vi vil. Hvis du gør det samme, så burde du ende med et timestamp, der matcher din lokale timezone. Eller hvad? :)

Det er lige præcis de 4 linier kode der ikke fungerer og giver et forkert resultat i huskelisten 😅

Ej virkelig? 🙃

Geez - jeg har ikke selv Huskelisten, så kan ikke teste. Men jeg laver lige et par manuelle tests og vender retur her.

Her er json response fra Huskelisten som indput til dine manuelle tests: [ { "userName": "Ældste Bassen", "userId": 4321, "courseReminders": [], "assignmentReminders": [], "teamReminders": [ { "id": 123456, "institutionName": "Kardemommeby Skole", "institutionId": 123, "dueDate": "2024-09-22T22:00:00Z", "teamId": 1234566, "teamName": "3B", "reminderText": "Kolorit: \"At regne i hovedet\" til og med s. 11", "createdBy": "Matematik Lærer", "lastEditBy": "Matematik Lærer", "subjectName": "Matematik" } ] }, { "userName": "Yngste Bassen", "userId": 1234, "courseReminders": [], "assignmentReminders": [], "teamReminders": [] } ]

Her er dueDate "2024-09-22T22:00:00Z" hvilket er CET+0, og den skal frem til CET+2 lokal tid, ellers står der "22. september" i huskelisten istedet for "23. september" :)

Selv på Aula.dk er dueDate forkert, så jeg gætter på at der findes en korrigering et sted i UI'en så det vises korrekt.

ebbeflarup avatar Sep 20 '24 11:09 ebbeflarup

Her starter vi med at konvertere indholdet af "dueDate" (som kommer fra tredje part) til timestamp - hvorefter vi kan formatere det som vi vil. Hvis du gør det samme, så burde du ende med et timestamp, der matcher din lokale timezone. Eller hvad? :)

Det er lige præcis de 4 linier kode der ikke fungerer og giver et forkert resultat i huskelisten 😅

Ej virkelig? 🙃 Geez - jeg har ikke selv Huskelisten, så kan ikke teste. Men jeg laver lige et par manuelle tests og vender retur her.

Her er json response fra Huskelisten som indput til dine manuelle tests: [ { "userName": "Ældste Bassen", "userId": 4321, "courseReminders": [], "assignmentReminders": [], "teamReminders": [ { "id": 123456, "institutionName": "Kardemommeby Skole", "institutionId": 123, "dueDate": "2024-09-22T22:00:00Z", "teamId": 1234566, "teamName": "3B", "reminderText": "Kolorit: \"At regne i hovedet\" til og med s. 11", "createdBy": "Matematik Lærer", "lastEditBy": "Matematik Lærer", "subjectName": "Matematik" } ] }, { "userName": "Yngste Bassen", "userId": 1234, "courseReminders": [], "assignmentReminders": [], "teamReminders": [] } ]

Her er dueDate "2024-09-22T22:00:00Z" hvilket er CET+0, og den skal frem til CET+2 lokal tid, ellers står der "22. september" i huskelisten istedet for "23. september" :)

Selv på Aula.dk er dueDate forkert, så jeg gætter på at der findes en korrigering et sted i UI'en så det vises korrekt.

Tak for den info @ebbeflarup 👍 Men er dueDate forkert pga. tidszonen på den maskine, der afvikler din HA? Jeg kører HAOS og min tidszone er CET+2 / CEST: image

scaarup avatar Sep 20 '24 12:09 scaarup

import json
from datetime import datetime, timezone
json_data = '''
[
    {
        "userName": "Ældste Bassen",
        "userId": 4321,
        "courseReminders": [],
        "assignmentReminders": [],
        "teamReminders": [
            {
                "id": 123456,
                "institutionName": "Kardemommeby Skole",
                "institutionId": 123,
                "dueDate": "2024-09-22T22:00:00Z",
                "teamId": 1234566,
                "teamName": "3B",
                "reminderText": "Kolorit: \\"At regne i hovedet\\" til og med s. 11",
                "createdBy": "Matematik Lærer",
                "lastEditBy": "Matematik Lærer",
                "subjectName": "Matematik"
            }
        ]
    },
    {
        "userName": "Yngste Bassen",
        "userId": 1234,
        "courseReminders": [],
        "assignmentReminders": [],
        "teamReminders": []
    }
]
'''

data = json.loads(json_data)

local_timezone = datetime.now(timezone.utc).astimezone().tzinfo
print(local_timezone)

for item in data:
    for reminder in item.get('teamReminders', []):
        due_date = datetime.strptime(reminder['dueDate'], "%Y-%m-%dT%H:%M:%SZ")
        local_due_date = due_date.replace(tzinfo=timezone.utc).astimezone(local_timezone)
        formatted_due_date = local_due_date.strftime("%A, %d %B %Y %H:%M")
        print(f"Due Date1: {due_date}")
        print(f"Due Date2: {local_due_date}")
        print(f"Due Date3: {formatted_due_date}")

image

?

scaarup avatar Sep 20 '24 13:09 scaarup

Ja det er korrekt 👍

Hvis du kører det i devcontainer miljøet i vs code i dit repo, så vil du højest sandsynligt få et andet resultat da tidszonen ikke nødvendigvis er korrekt indstillet. Det nuværende kode i master giver også et forkert resultat når det kører på en rpi. Det skal jeg ikke kunne sige om det kode du har der gør eller ej.

Men om ikke andet, så er resultatet af det kode du har der helt afhængigt af miljøet omkring det.

Min pointe er at jeg ikke kan garantere resultatet af konverteringen medmindre jeg eksplicit fortæller hvilket tidszone jeg gerne vil have datoen i.

ebbeflarup avatar Sep 20 '24 14:09 ebbeflarup

Jeg tænker hensigten med API'et, som udstiller huskelisten igennem momo, har været at localize i forhold browseren eller app'ens tidszone eftersom det er muligt at angive tidszonen i http headeren, hvilket giver super god mening, men det virker tydeligvis ikke 😬

ebbeflarup avatar Sep 20 '24 14:09 ebbeflarup

Ja det er korrekt 👍

Hvis du kører det i devcontainer miljøet i vs code i dit repo, så vil du højest sandsynligt få et andet resultat da tidszonen ikke nødvendigvis er korrekt indstillet. Det nuværende kode i master giver også et forkert resultat når det kører på en rpi. Det skal jeg ikke kunne sige om det kode du har der gør eller ej.

Ja hvis devcontaineren kører UTC eller en anden timezone, men det er jo netop meningen. Og det samme, hvis din rpi kører UTC, så kommer det i UTC. Men der er ingen der siger din rpi skal køre UTC. Men hvis det kun er et problem i dit udviklingsmiljø, så kan du bare sætte timezone i din devcontainer:

    "remoteEnv": {
        "TZ": "Europe/Copenhagen"
    }

Min HAOS kører CEST, så der vil det heller ikke være noget problem. Mon ikke de fleste kører en lokal tidszone?

Men om ikke andet, så er resultatet af det kode du har der helt afhængigt af miljøet omkring det.

Og det er også præcis meningen. Og sikkert også derfor, at data ligger hos tredjepart med UTC timestamps. Det er normal praksis.

Min pointe er at jeg ikke kan garantere resultatet af konverteringen medmindre jeg eksplicit fortæller hvilket tidszone jeg gerne vil have datoen i.

Må jeg spørge hvad du kører HA på og om det er besværligt at gå fra UTC til CEST?

Jeg tænker hensigten med API'et, som udstiller huskelisten igennem momo, har været at localize i forhold browseren eller app'ens tidszone eftersom det er muligt at angive tidszonen i http headeren, hvilket giver super god mening, men det virker tydeligvis ikke 😬

Nå det virker simpelthen ikke for dem selv? Sjovt :) Ville jeg umiddelbart mene de nemt kunne løse med noget javascript i browseren.

scaarup avatar Sep 20 '24 16:09 scaarup

Det gør de helt sikkert også da visningen i ui'en er korrekt, men data fra API'et er som eksemplet. Så der findes helt sikkert noget JavaScript der korrigerer. Og ja, ser heller ikke noget specielt unormalt i deres praksis, det virker bare ikke lige altid så godt her :)

Nå men altså, som du kan se i det issue jeg linker til, så er jeg ikke den eneste med problemet i den nuværende kodebase 😄

Problemet er nok tydeligere her da dataen ikke leveres til en browser som kører på en maskine i den korrekte tidszone, men i stedet til folks obskure setups, inklusiv min egen 😉

Anyways, jeg vil ikke gøre mere ud af det. Jeg vil heller ikke bare lukke pr'et før du endegyldigt har afvist det.

ebbeflarup avatar Sep 20 '24 19:09 ebbeflarup

Du vil ikke bare lave denne her:

        due_date = datetime.strptime(reminder['dueDate'], "%Y-%m-%dT%H:%M:%SZ")
        local_due_date = due_date.replace(tzinfo=timezone.utc).astimezone(local_timezone)
        formatted_due_date = local_due_date.strftime("%A, %d %B %Y %H:%M")

Og så justere din timezone på din server eller container? Problem solved? ;)

scaarup avatar Sep 20 '24 19:09 scaarup

Det kan jeg godt hvis det er det som det kræver for at få det ind, men ærligt talt, så synes jeg der er lidt for meget "works on my machine"-energi i den løsning af alle de grunde jeg allerede har nævnt :)

ebbeflarup avatar Sep 20 '24 20:09 ebbeflarup

Det kan jeg godt hvis det er det som det kræver for at få det ind, men ærligt talt, så synes jeg der er lidt for meget "works on my machine"-energi i den løsning af alle de grunde jeg allerede har nævnt :)

For mig og sikkert de fleste andre, ville det jo bare virke :) Min timezone er CEST og jeg kører HAOS.

scaarup avatar Sep 23 '24 06:09 scaarup

Skal nok lige komme tilbage og få denne afsluttet asap. Blev lige ramt af noget arbejde de sidste par dage, men checker dit forslag ind hurtigst muligt 👍

ebbeflarup avatar Sep 24 '24 04:09 ebbeflarup

@scaarup Har fået tilføjet de ønskede ændringere til dueDate.

Derudover kunne 'subjectName' være tom og ville derfor medføre en fejl ved opslag i dictionary. Timedelta er sat ned til en uge. Ved ikke om det interval er fint, men eftersom det er en HA integration, så virker det rimeligt da man sikkert vil kigge dagligt og dermed ikke har brug for at vide hvad der sker langt ind i næste år 😅 . Alternativet var at listen af 'reminders' kunne være meget lang, hvilket den var i mit tilfælde da lærere ligger reminders ind langt ud i fremtiden.

ebbeflarup avatar Sep 26 '24 20:09 ebbeflarup

Undskyld det tog så langt tid :)

scaarup avatar Oct 12 '24 09:10 scaarup