formdata: store datetimes with timezone (#52057) #1122

Merged
fpeters merged 1 commits from wip/52057-timezone into main 2024-02-12 15:28:35 +01:00
Owner
No description provided.
fpeters force-pushed wip/52057-timezone from 82e52ca801 to 1f7c1ad721 2024-02-10 12:36:28 +01:00 Compare
fpeters force-pushed wip/52057-timezone from 1f7c1ad721 to 65821de6d5 2024-02-10 12:54:34 +01:00 Compare
fpeters force-pushed wip/52057-timezone from 65821de6d5 to a2b4a3712e 2024-02-10 13:01:49 +01:00 Compare
fpeters force-pushed wip/52057-timezone from a2b4a3712e to cb2449d65c 2024-02-10 13:08:54 +01:00 Compare
fpeters force-pushed wip/52057-timezone from cb2449d65c to 495d867952 2024-02-10 13:56:48 +01:00 Compare
fpeters force-pushed wip/52057-timezone from 495d867952 to f1c213cc8e 2024-02-10 14:32:10 +01:00 Compare
fpeters force-pushed wip/52057-timezone from f1c213cc8e to ada22146eb 2024-02-10 14:44:47 +01:00 Compare
fpeters force-pushed wip/52057-timezone from ada22146eb to cbe0463471 2024-02-10 14:47:08 +01:00 Compare
fpeters force-pushed wip/52057-timezone from cbe0463471 to 69a1af4f32 2024-02-10 14:55:20 +01:00 Compare
fpeters force-pushed wip/52057-timezone from 69a1af4f32 to fee94ee30f 2024-02-10 14:59:27 +01:00 Compare
fpeters force-pushed wip/52057-timezone from fee94ee30f to db6ff7a6af 2024-02-10 15:12:56 +01:00 Compare
fpeters force-pushed wip/52057-timezone from db6ff7a6af to f46d7dea8d 2024-02-10 15:43:00 +01:00 Compare
fpeters force-pushed wip/52057-timezone from f46d7dea8d to 987af3224f 2024-02-10 16:35:29 +01:00 Compare
fpeters force-pushed wip/52057-timezone from 987af3224f to 0bd6105ace 2024-02-10 16:58:38 +01:00 Compare
fpeters force-pushed wip/52057-timezone from 0bd6105ace to 57c2418fb4 2024-02-11 10:10:37 +01:00 Compare
fpeters force-pushed wip/52057-timezone from 57c2418fb4 to dfc2bfb964 2024-02-11 10:44:05 +01:00 Compare
fpeters reviewed 2024-02-11 11:07:07 +01:00
@ -412,2 +412,3 @@
assert resp2.json['data'][1] == resp.json['data'][2]
assert resp2.json['data'][2] == resp.json['data'][1]
assert resp2.json['data'][3] == resp.json['data'][2]
assert resp2.json['data'][3] == resp.json['data'][0]
Author
Owner

test_user_forms, le test qui échouait parfois, était tout foireux, il récupérait une liste de demandes, puis la même en ordre inversé, et regardait si les demandes (0, 1, 2, 3) correspondaient bien aux demandes (3, 0, 1, 2). Ça n'avait pas de sens mais c'était la plupart du temps le résultat parce que les demandes 0 1 et 2 étaient créées avec le même timestamp. Maintenant qu'on a des timestamps plus précis, l'ordre (3, 2, 1, 0) qui aurait dû être vérifié depuis le début devient systématique.

test_user_forms, le test qui échouait parfois, était tout foireux, il récupérait une liste de demandes, puis la même en ordre inversé, et regardait si les demandes (0, 1, 2, 3) correspondaient bien aux demandes (3, 0, 1, 2). Ça n'avait pas de sens mais c'était la plupart du temps le résultat parce que les demandes 0 1 et 2 étaient créées avec le même timestamp. Maintenant qu'on a des timestamps plus précis, l'ordre (3, 2, 1, 0) qui aurait dû être vérifié depuis le début devient systématique.
wcs/api.py Outdated
@ -158,0 +158,4 @@
if d.get('form_receipt_datetime'):
d['form_receipt_datetime'] = make_naive(d['form_receipt_datetime'].replace(microsecond=0))
if d.get('form_last_update_datetime'):
d['form_last_update_datetime'] = make_naive(d['form_last_update_datetime'].replace(microsecond=0))
Author
Owner

Pour assurer la compatibilité des API les datetime y sont réduits pour continuer à n'avoir ni timezone, ni microsecondes.

Pour assurer la compatibilité des API les datetime y sont réduits pour continuer à n'avoir ni timezone, ni microsecondes.
@ -2788,1 +2786,3 @@
'last_update_time': datetime.datetime(*filled.last_update_time[:6]),
'receipt_time': make_naive(filled.receipt_time.replace(microsecond=0))
if filled.receipt_time
else None,
Author
Owner

C'est fait à différents endroits, ça aurait pu être fait lors de l'encodage json mais ça aurait alors pris le risque de faire perdre de l'information sur les cas où existaient déjà microsecondes et timezone.

C'est fait à différents endroits, ça aurait pu être fait lors de l'encodage json mais ça aurait alors pris le risque de faire perdre de l'information sur les cas où existaient déjà microsecondes et timezone.
wcs/sql.py Outdated
@ -515,3 +515,3 @@
(table_name,),
)
existing_fields = {x[0] for x in cur.fetchall()}
existing_field_types = {x[0]: x[1] for x in cur.fetchall()}
Author
Owner

On récupère désomrais également le type des colonnes, pour ensuite faire le changement de type si nécessaire.

On récupère désomrais également le type des colonnes, pour ensuite faire le changement de type si nécessaire.
wcs/sql.py Outdated
@ -539,0 +541,4 @@
if existing_field_types.get('receipt_time') not in (None, 'timestamp with time zone'):
cur.execute(f'ALTER TABLE {table_name} ALTER COLUMN receipt_time SET DATA TYPE timestamptz')
if existing_field_types.get('last_update_time') not in (None, 'timestamp with time zone'):
cur.execute(f'ALTER TABLE {table_name} ALTER COLUMN last_update_time SET DATA TYPE timestamptz')
Author
Owner

C'est postgresql qui assure tout le boulot d'ajouter la bonne timezone partout et c'est très bien. (j'ai vérifié on a au niveau de postgresql timezone = 'Europe/Paris' et timezone = 'Indian/Reunion', correctement positionnés).

C'est postgresql qui assure tout le boulot d'ajouter la bonne timezone partout et c'est très bien. (j'ai vérifié on a au niveau de postgresql timezone = 'Europe/Paris' et timezone = 'Indian/Reunion', correctement positionnés).
Owner

pourquoi None ?

pourquoi None ?
Author
Owner

Quand on récupère la liste des colonnes prsentes, last_update_time peut ne pas encore exister, et j'ai trouvé plus clair d'avoir la même ligne pour les deux colonnes. Une alternative serait d'ajouter last_update_time dès la création de la table mais faire cette partie proprement (exploiter _table_static_fields plutôt que juste dupliquer) demandait une refacto qui dépassait ce ticket.

Quand on récupère la liste des colonnes prsentes, last_update_time peut ne pas encore exister, et j'ai trouvé plus clair d'avoir la même ligne pour les deux colonnes. Une alternative serait d'ajouter last_update_time dès la création de la table mais faire cette partie proprement (exploiter _table_static_fields plutôt que juste dupliquer) demandait une refacto qui dépassait ce ticket.
Owner

ok ça se tient

ok ça se tient
wcs/sql.py Outdated
@ -1339,0 +1356,4 @@
'''SELECT table_name FROM information_schema.views
WHERE table_schema = 'public'
AND table_name LIKE %s''',
('wcs\\_carddata\\_view\\_%',),
Author
Owner

Je me suis rendu compte qu'en local j'avais des vues sur les fiches, et ça pouvait bloquer le changement de type de colonne, donc ça les dégage.

Je me suis rendu compte qu'en local j'avais des vues sur les fiches, et ça pouvait bloquer le changement de type de colonne, donc ça les dégage.
fpeters changed title from WIP: formdata: store datetimes with timezone (#52057) to formdata: store datetimes with timezone (#52057) 2024-02-11 11:08:33 +01:00
lguerin reviewed 2024-02-12 08:58:35 +01:00
@ -5325,4 +5344,4 @@
for formdef in FormDef.select() + CardDef.select():
do_formdef_tables(formdef, rebuild_views=False, rebuild_global_views=False)
migrate_views(conn, cur)
set_reindex('formdata', 'needed', conn=conn, cur=cur)
Owner

migrate_views fait déjà:

def migrate_views(conn, cur):
    drop_views(None, conn, cur)
    for formdef in FormDef.select() + CardDef.select():
        # make sure all formdefs have up-to-date views
        do_formdef_tables(formdef, conn=conn, cur=cur, rebuild_views=True, rebuild_global_views=False)
    migrate_global_views(conn, cur)

Ca fait pas redondant ?

migrate_views fait déjà: ```python def migrate_views(conn, cur): drop_views(None, conn, cur) for formdef in FormDef.select() + CardDef.select(): # make sure all formdefs have up-to-date views do_formdef_tables(formdef, conn=conn, cur=cur, rebuild_views=True, rebuild_global_views=False) migrate_global_views(conn, cur) ``` Ca fait pas redondant ?
Author
Owner

Ca fait pas redondant ?

Il me semble qu'en effet désormais, avec l'ajout du drop_views() (qui doit être lancé avant le do_formdef_tables, sinon on peut avoir postgresql en erreur parce qu'on modifie le type d'une colonne reprise dans une vue), cette partie pourrait être réduite à migrate_views().

J'aurais juste la crainte d'une refacto où à un moment le migrate_views ne ferait plus rien parce que feature flag; c'est assez flou mais j'aimerais bien laisser ainsi.

> Ca fait pas redondant ? Il me semble qu'en effet désormais, avec l'ajout du drop_views() (qui doit être lancé avant le do_formdef_tables, sinon on peut avoir postgresql en erreur parce qu'on modifie le type d'une colonne reprise dans une vue), cette partie pourrait être réduite à migrate_views(). J'aurais juste la crainte d'une refacto où à un moment le migrate_views ne ferait plus rien parce que feature flag; c'est assez flou mais j'aimerais bien laisser ainsi.
Owner

ok

ok
fpeters force-pushed wip/52057-timezone from dfc2bfb964 to 9e682154f6 2024-02-12 14:00:21 +01:00 Compare
fpeters force-pushed wip/52057-timezone from 9e682154f6 to fb509d951d 2024-02-12 14:01:18 +01:00 Compare
lguerin approved these changes 2024-02-12 14:06:04 +01:00
fpeters force-pushed wip/52057-timezone from fb509d951d to 234427f57e 2024-02-12 14:16:05 +01:00 Compare
fpeters force-pushed wip/52057-timezone from 234427f57e to e45dd098b3 2024-02-12 14:30:10 +01:00 Compare
fpeters merged commit e45dd098b3 into main 2024-02-12 15:28:35 +01:00
fpeters deleted branch wip/52057-timezone 2024-02-12 15:28:35 +01:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/wcs#1122
No description provided.