Project

General

Profile

Development #17685

"préblocage" d'une réservation

Added by Frédéric Péters over 4 years ago. Updated 5 months ago.

Status:
Nouveau
Priority:
Normal
Category:
-
Target version:
-
Start date:
18 Jul 2017
Due date:
% Done:

0%

Estimated time:
Patch proposed:
Yes
Planning:
No

Description

On imagine un formulaire où on demande à choisir un événement mais derrière il y a encore des pages et pourtant on ne voudrait pas qu'au bout la personne soit déçue et n'ait pas sa place. Bref, imaginer une période de préblocage.


Files

0001-api-handle-lock-on-fillslots-and-datetimes-17685.patch (11.1 KB) 0001-api-handle-lock-on-fillslots-and-datetimes-17685.patch Thomas Noël, 05 Jun 2018 09:32 AM
0001-api-add-lock_code-parameter-to-fillslot-and-datetime.patch (31.5 KB) 0001-api-add-lock_code-parameter-to-fillslot-and-datetime.patch Benjamin Dauvergne, 16 Mar 2021 07:28 PM
0001-api-add-lock_code-parameter-to-fillslot-and-datetime.patch (30.9 KB) 0001-api-add-lock_code-parameter-to-fillslot-and-datetime.patch Benjamin Dauvergne, 18 May 2021 08:02 PM
0003-to-fixup-rename-lease-migration.patch (635 Bytes) 0003-to-fixup-rename-lease-migration.patch Benjamin Dauvergne, 03 Jun 2021 05:26 PM
0004-to-fixup-adapt-dependency-of-lease-migration.patch (823 Bytes) 0004-to-fixup-adapt-dependency-of-lease-migration.patch Benjamin Dauvergne, 03 Jun 2021 05:26 PM
0002-to-fixup-rename-sql-snippets.patch (1.52 KB) 0002-to-fixup-rename-sql-snippets.patch Benjamin Dauvergne, 03 Jun 2021 05:26 PM
0001-api-add-lock_code-parameter-to-fillslot-and-datetime.patch (31 KB) 0001-api-add-lock_code-parameter-to-fillslot-and-datetime.patch Benjamin Dauvergne, 03 Jun 2021 05:26 PM
datasource-agenda.wcs (341 Bytes) datasource-agenda.wcs Benjamin Dauvergne, 03 Jun 2021 06:22 PM
wscall-lock_rdv.wcs (499 Bytes) wscall-lock_rdv.wcs Benjamin Dauvergne, 03 Jun 2021 06:22 PM
workflow-rdv.wcs (9.05 KB) workflow-rdv.wcs Benjamin Dauvergne, 03 Jun 2021 06:22 PM
form-test-agenda-lock.wcs (2.3 KB) form-test-agenda-lock.wcs Benjamin Dauvergne, 03 Jun 2021 06:22 PM
0007-to-fixup-remove-useless-comment.patch (1001 Bytes) 0007-to-fixup-remove-useless-comment.patch Benjamin Dauvergne, 08 Jun 2021 03:17 PM
0003-to-fixup-rename-lease-migration.patch (635 Bytes) 0003-to-fixup-rename-lease-migration.patch Benjamin Dauvergne, 08 Jun 2021 03:17 PM
0004-to-fixup-adapt-dependency-of-lease-migration.patch (823 Bytes) 0004-to-fixup-adapt-dependency-of-lease-migration.patch Benjamin Dauvergne, 08 Jun 2021 03:17 PM
0002-to-fixup-rename-sql-snippets.patch (1.52 KB) 0002-to-fixup-rename-sql-snippets.patch Benjamin Dauvergne, 08 Jun 2021 03:17 PM
0009-to-fixup-let-DRF-handle-empty-lock_code-errors.patch (1.48 KB) 0009-to-fixup-let-DRF-handle-empty-lock_code-errors.patch Benjamin Dauvergne, 08 Jun 2021 03:17 PM
0008-to-fixup-removed-unused-Lease.as_interval.patch (697 Bytes) 0008-to-fixup-removed-unused-Lease.as_interval.patch Benjamin Dauvergne, 08 Jun 2021 03:17 PM
0001-api-add-lock_code-parameter-to-fillslot-and-datetime.patch (31 KB) 0001-api-add-lock_code-parameter-to-fillslot-and-datetime.patch Benjamin Dauvergne, 08 Jun 2021 03:17 PM
0005-to-fixup-rename-migration-from-91-to-0091.patch (645 Bytes) 0005-to-fixup-rename-migration-from-91-to-0091.patch Benjamin Dauvergne, 08 Jun 2021 03:17 PM
0006-to-fixup-remove-blank-line.patch (725 Bytes) 0006-to-fixup-remove-blank-line.patch Benjamin Dauvergne, 08 Jun 2021 03:17 PM

Related issues

Related to Chrono - Development #23008: api: ajouter une action "lockslot" pour bloquer des slots pendant x minutesRejeté06 Apr 2018

Actions
Related to w.c.s. - Development #24635: Générer un identifiant pour un formdata dès la saisieNouveau19 Jun 2018

Actions
Related to Chrono - Development #54956: Revert de #17685 « "préblocage" d'une réservation »Solution déployée17 Jun 2021

Actions
Has duplicate Chrono - Development #20437: possibilité de vérouiller un créneau horaireRejeté05 Dec 2017

Actions

Associated revisions

Revision 2f9bf16a (diff)
Added by Benjamin Dauvergne 6 months ago

api: add lock_code parameter to fillslot and datetimes (#17685)

History

#2

Updated by Thomas Noël over 4 years ago

Note dans #17681 : « À côté de ce côté interface on doit aussi réfléchir aux mécanismes pour éviter que ça ne soit abusé et sans doute d'autres points. »

#3

Updated by Frédéric Péters almost 4 years ago

#4

Updated by Frédéric Péters over 3 years ago

  • Related to Development #23008: api: ajouter une action "lockslot" pour bloquer des slots pendant x minutes added
#6

Updated by Thomas Noël over 3 years ago

J'importe ici mes notes sur #23008 :

On peut avoir des conflits de réservation si plusieurs personnes arrivent sur le front, voient les mêmes datetimes et sélectionnent le même. Lors de l'appel fillslot, seul le premier réussi, les suivants passent en échec.

On pourrait éviter que cela se produise dans le workflow de traitement de la demande, en appelant le webservice en condition de sortie pour vérifier qu'une place est encore disponible et pour la "locker" pendant un certain temps.

Quelque chose comme « /api/agenda/<id>/lockslot/<id>/ » qui renvoie la même chose que fillslot, mais ne lock en réalité que pendant 10 minutes. Ou alors en payload d'un appel à fillslot, avec un "lock:10" ou "lock:True".

Techniquement on imagine un Booking.auto_cancellation_datetime (None par défaut) qui s'il existe, effectue un cancel de la réservation à l'instant donné, via un cron lancé chaque 5 minute.

La difficulité : on ne pourra pas dans w.c.s. stocker l'id de la pré-réservation ainsi obtenue. Donc, dans le workflow, lors du vrai fillslot, il faudra accepter même si booking existe, et dans ce cas le confirmer.

#7

Updated by Thomas Noël over 3 years ago

  • Priority changed from Normal to Haut
#9

Updated by Thomas Noël over 3 years ago

  • Status changed from Nouveau to En cours
  • Priority changed from Haut to Normal

Voici une première proposition de fonctionnement.

Tout se passe à travers un "code de blocage" (lock_code) qui doit être unique dans chrono. Via wcs on utilisera la référence de la demande (nnn-mmm).

  • On cherche d'abord la liste des dispo pour ce code, disons "MYLOCK". C'est la liste des dispos habituelle.
  • Une fois le choix fait, on lance la réservation en mode "lock", en envoyant un fillslot avec le lock_code
    • cela se fera via une condition de sortie de page
    • cela créé un booking, qui a comme particularité d'avoir un lockcode et une date d'expiration de ce lock
  • A partir de là, la liste des dispos ne contient plus cette résa
    • et donc les autres personnes ne voient plus la dispo
    • et même si elles l'avaient vu, elles ne peuvent plus la réserver (la condition de sortie va échouer)
    • Cependant, si on envoie le lockcode MYLOCK, la dispo est bien visible (cas d'un aller-retour sur la page de choix des resa)
    • Si on lock une autre dispo avec MYLOCK, la résa précédemment lockée ne l'est plus
  • à la fin, pour valider vraiment la réservation, on la renvoie, avec le même lockcode et un confirm_after_lock=True. Cela créé un "vrai" booking, ie sans lockcode, ie non temporaire

Par défaut, un lock dure 10 minutes, ça se change via les settings ou lors de l'appel. Il manque encore dans ce code un cron de nettoyage des lock périmés.

Voilà, avant de chercher à peaufiner et notamment étendre les tests, je veux bien un avis sur :
  • le principe
  • l'intérêt de la chose sur les agenda events ? (je n'y ai pas trop reflechi)
  • la façon dont c'est codé, parce qu'il y a une partie un peu obscure qui est la suppression des lockcode qui suppriment des events sur un agenda de type meeting (rendez-vous), ces events étant en effet bidons.
#11

Updated by Benjamin Dauvergne over 3 years ago

Je ne comprends pas bien le besoin du lock_code, pourquoi ne pas simplement ajouter un paramètre booléen lock si celui-ci présent on pose lock_expiration. On renvoie le event.id ou un truc du genre. Sur un nouvel appel avec cet event_id on vire toutes les histoires de lock, c'est la confirmation de la réservation.

Ça évitera d'avoir à générer un lock_code coté w.c.s. (maintenant c'est peut-être volontaire de pouvoir envoyer un truc un peu intelligible comme l'identifiant du formulaire je ne sais pas trop).

Pour moi le code serait plus clair avec deux web-services plutôt que de tout surcharger sur un un seul (un de pré-réservation l'autre de réservation).

#12

Updated by Thomas Noël over 3 years ago

Benjamin Dauvergne a écrit :

Je ne comprends pas bien le besoin du lock_code, pourquoi ne pas simplement ajouter un paramètre booléen lock si celui-ci présent on pose lock_expiration. On renvoie le event.id ou un truc du genre. Sur un nouvel appel avec cet event_id on vire toutes les histoires de lock, c'est la confirmation de la réservation.

Ouaip, c'est ce qui semble naturel... Mais la contrainte vient de w.c.s. : il peut rien stocker lors du formulaire. Il va juste lancer une condition de sortie, c'est tout (il ne peut rien faire du résultat). À ce stade le lock_code choisi par l'appelant est la seule solution que j'ai trouvée.

Un petit avantage, c'est que si le lock a finalement expiré entre temps mais que la confirmation réussi quand même (parce qu'il reste quand même une place), et bien tout se passe tranquillement. (parce qu'on ne cherche pas à valider une résa existante, mais bien à faire une résa, en considérant le lock comme libre).

#13

Updated by Benjamin Dauvergne over 3 years ago

Et du disposes de quoi pour construire ton lock code ? Un numéro de session ? Un id de draft ?

#14

Updated by Thomas Noël over 3 years ago

Benjamin Dauvergne a écrit :

Et du disposes de quoi pour construire ton lock code ? Un numéro de session ? Un id de draft ?

Oui, je pensais à form_number.

#15

Updated by Benjamin Dauvergne over 3 years ago

Ça m'a l'air ok.

#16

Updated by Thomas Noël over 3 years ago

  • Patch proposed changed from Yes to No

En fait ça ne marche pas, parce qu'on n'a pas de lock_code disponible à tout coup (form_number n'existe pas lors d'une saisie sur une demande qui n'a pas de mode brouillon)

#17

Updated by Benjamin Dauvergne over 3 years ago

  • Related to Development #24635: Générer un identifiant pour un formdata dès la saisie added
#18

Updated by Thomas Noël almost 2 years ago

On a maintenant session_hash_id #39784 qui peut faire l'affaire en première approximation ; ce ticket peut reprendre vie.

#20

Updated by Benjamin Dauvergne 9 months ago

  • Assignee set to Benjamin Dauvergne
#21

Updated by Benjamin Dauvergne 9 months ago

J'ai commencé par reprendre le patch de Thomas et puis en bossant dessus je me suis rendu compte que c'était bien compliqué d'ajouter encore de nouveaux champs à Booking/Event&co. J'ai préféré introduire un nouveau modèle sur le coté pour stocker juste les locks. Il se nettoye tout seul (Delete à chaque fois que qu'on le requête) et ça permet de gérer les locks aussi sur les ressources au passage. Je n'ai pas particulièrement optimisé les requêtes dans get_all_slots() parce qu'il faut supposer qu'il n'y aura que ttrès peu de locks vivant en même temps (à peu près autant que d'utilisateurs concurrent sur une période de longueur CHRONO_LOCK_DURATION).

Je n'ai pas particulièrement réfléchi à la réponse de fillslot quand on utilise lock_code, là ça retourne juste {'err': 0}.

J'ai ajouté le même genre d'index d'exclusion que sur les Event mais sans prévenir les bloquages en cas d'absence de l'extension, je me suis dit que ce stade était passé.

Le seul bémol par rapport à une implémentation plus compliqué c'est qu'il reste une très faible possibilité qu'un fillslot ait lieu au moment d'un fillslot?lock_code=XXX. La meilleur protection c'est de recommencer le fillslot?lock_code=XXX en fin de formulaire (et même à chaque page, ainsi ça prolonge le lock à chaque fois). Dans ce cas c'est impossible d'avoir d'erreur (si le formulaire a plus d'une page et que le choix du rdv n'est pas sur la dernière).

PS: j'ai aussi complètement bloqué la fonctionnalité pour le cas des évènements, je n'avais pas de test sous la main et je me suis dit que ce serait mieux de reprendre ça à froid si ça devient nécessaire un jour.

PS2: et bien sûr, mais ça n'a pas été dit avant explicitement, ça ne pourra jamais marcher avec un formulaire ayant des brouillons, ou plutôt si on prévoit des conditions de sortie de page un peu partout, ça devrait lever une erreur à la récupération du brouillon.

#24

Updated by Valentin Deniaud 6 months ago

Qu'est-ce que fait le SQL de la migration ? EXCLUDE USING GIST(desk_id WITH =, tstzrange(start_datetime, end_datetime) WITH &&) franchement qui a inventé une syntaxe pareille. (l'explication peut s'accompagner d'un renommage de sql_forwards en quelque chose de plus parlant)

Mais avant de relire le code je voulais tester et justement, en essayant d'appliquer la migration,

  Applying agendas.0088_lease...Traceback (most recent call last):
  File "/home/vdeniaud/envs/publik-env-py3/lib/python3.7/site-packages/django/db/backends/utils.py", line 82, in _execute
    return self.cursor.execute(sql)
psycopg2.errors.UndefinedObject: data type integer has no default operator class for access method "gist" 
HINT:  You must specify an operator class for the index or define a default operator class for the data type.

#25

Updated by Frédéric Péters 6 months ago

je voulais tester

De bout en bout ? i.e. avec une démarche wcs ?

(ici appel du pied à Benjamin pour qu'il la partage)

#26

Updated by Benjamin Dauvergne 6 months ago

  • Status changed from Solution proposée to En cours

Ok, j

Valentin Deniaud a écrit :

Qu'est-ce que fait le SQL de la migration ? EXCLUDE USING GIST(desk_id WITH =, tstzrange(start_datetime, end_datetime) WITH &&) franchement qui a inventé une syntaxe pareille. (l'explication peut s'accompagner d'un renommage de sql_forwards en quelque chose de plus parlant)

Si c'est la première fois que la syntaxe SQL te parait bizarre c'est que tu es t

Mais avant de relire le code je voulais tester et justement, en essayant d'appliquer la migration,
[...]

C'est bizarre parce qu'on utilise déjà une contrainte équivalente sur les event:

chrono/agendas/migrations/0053_event_date_range_constraint.py:EXCLUDE USING GIST(desk_id WITH =, tstzrange(start_datetime, _end_datetime) WITH &&)

et que ça passe crême sur jenkins et chez moi.

Est-ce que la migration 0053 n'échouerait pas chez toi ? (ton user n'est pas administrateur de postgresql).

Alors je peux cacher la non création de l'index de contrainte, encore comme dans 0053, si vous voulez.

Frédéric Péters a écrit :

De bout en bout ? i.e. avec une démarche wcs ?
(ici appel du pied à Benjamin pour qu'il la partage)

Ok je fais ça.

#28

Updated by Valentin Deniaud 6 months ago

Benjamin Dauvergne a écrit :

Si c'est la première fois que la syntaxe SQL te parait bizarre c'est que tu es t

Je n'ose imaginer de quel nom tu voulais me traiter !

Est-ce que la migration 0053 n'échouerait pas chez toi ? (ton user n'est pas administrateur de postgresql).

Pas testé, pas envie de shooter ma db ni de créer un autre tenant juste pour voir. Par contre j'ai googlé et il me manquait un CREATE EXTENSION btree_gist; et c'est désormais bon.

#29

Updated by Benjamin Dauvergne 6 months ago

Valentin Deniaud a écrit :

Benjamin Dauvergne a écrit :

Si c'est la première fois que la syntaxe SQL te parait bizarre c'est que tu es t

Je n'ose imaginer de quel nom tu voulais me traiter !

Je voulais écrire très tolérant :)

Est-ce que la migration 0053 n'échouerait pas chez toi ? (ton user n'est pas administrateur de postgresql).

Pas testé, pas envie de shooter ma db ni de créer un autre tenant juste pour voir. Par contre j'ai googlé et il me manquait un CREATE EXTENSION btree_gist; et c'est désormais bon.

Le CREATE EXTENSION btree_gist est justement dans la migration 0053.

#30

Updated by Benjamin Dauvergne 6 months ago

Exports de test, il faut:
  • un appel webservice vers api_fillslot_url avec un seul paramètre lock_code={{ session_hash_id }} pour la condition de sortie de la page qui contient le sélecteur de RdV, pour vérifier la dispo et verrouiller (si un verrou pour le lock_code existe déjà il est supprimé et ça passe, donc on peut revenir en arrière et changer son RdV); l'appel peut resservir pour tous les formulaires, si le champ liste a toujours le même nom de variable (je vois que dans la doc on incite fortement à toujours le nommer "event", dans ce cas ça marchera, comme je ne connaissais pas l'usage ici la variable se nomme "rdv"),
  • une source de donnée agenda spécifique parce qu'elle doit préciser ?lock_code={{ session_hash_id|urlencode }} pour ne pas cacher le RdV préalablement verrouillé en cas de retour en arrière (et éviter le message d'erreur "la sélection est invalide")
  • un formulaire/workflow de réservation de RdV bateau, juste l'appel à api_fillslot_url doit ajouter lock_code={{ session_hash_id }} et confirm_after_lock=True (en expression python).

Il faudrait voir pour que ça marche avec les sources de donnée agenda auto-générées en ajoutant automatiquement lock_code={{ session_hash_id|urlencode }}, Ça ne devrait pas poser de problème car si le formulaire n'utilise pas le verrouillage ça n'aura pas de conséquence. De même on pourrait automatiquement créer un web-service "lock_event" qui marchera, si la variable s'appelle "event" par convention.

Je pense que j'ai tout dit, de mes premiers tests ça marche nickel (un formulaire ouvert en navigation privé et un en navigation classique, le passage de la première page exclut bien le RdV pour l'autre formulaire avec un message "Désolé, ce rendez-vous est déjà pris.", des retours en arrière puis changement de RdV débloquent la situation).

#31

Updated by Benjamin Dauvergne 6 months ago

  • Status changed from Solution validée to Solution proposée
#32

Updated by Valentin Deniaud 6 months ago

Ça marche bien chez moi aussi, pas vraiment de remarques sur le fond, sur la forme :

Renommage de migration, mettre 0091 et pas juste 91.

+      If it is booked but match the lock code, report the slot as open.
+
     """ 

Ligne blanche en trop.
+        # only lock related to on of the agenda or the resource

to one of (commentaire pas si utile ceci-dit)

La méthode as_interval de Lease n'est pas utilisée.

Dans fillslot tu as mis un allow_blank=True et ensuite tu vérifies que le champ n'est pas vide, il faudrait virer les deux et ça marcherait pareil.

#34

Updated by Valentin Deniaud 6 months ago

  • Status changed from Solution proposée to Solution validée
#35

Updated by Benjamin Dauvergne 6 months ago

  • Status changed from Solution validée to Résolu (à déployer)
commit 2f9bf16a57bab183b3f2650d882ecf37348ed053
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Tue Mar 16 14:29:41 2021 +0100

    api: add lock_code parameter to fillslot and datetimes (#17685)
#36

Updated by Benjamin Dauvergne 5 months ago

  • Related to Development #54956: Revert de #17685 « "préblocage" d'une réservation » added
#37

Updated by Benjamin Dauvergne 5 months ago

  • Status changed from Résolu (à déployer) to Nouveau
#38

Updated by Benjamin Dauvergne 5 months ago

Commit en cours de revert.

#39

Updated by Frédéric Péters 5 months ago

De #54780#note-26,

Le code de pré-blocage peut fonctionner, mais plus mal, sans l'index, les gens qui n'ont pas une base à jour auront juste des bugs qu'on aura pas.

Identifiés, ils peuvent être pris en compte dans une nouvelle version de ce patch ? Ou bien il s'agit de bug "quand même parfois sous forte charge il y aura une réservation qui passera" ?

Aussi/surtout, à passer sur les chaines à traduire j'ai vu "lock_code does not work with events" qui correspond à ce commentaire plus haut :

PS: j'ai aussi complètement bloqué la fonctionnalité pour le cas des évènements, je n'avais pas de test sous la main et je me suis dit que ce serait mieux de reprendre ça à froid si ça devient nécessaire un jour.

Les grosses affluences ça a souvent été pour les événements, je pense par exemple aux piscines à Toulouse.

De la description du ticket d'ailleurs : « choisir un événement » (c'est plus loin qu'il commence à être question de rendez-vous et conflits de réservation).

Also available in: Atom PDF