workflows : add an identifier to automatic and on submit jumps (#74723) #161

Open
ecazenave wants to merge 2 commits from wip/74723-last-jump into main
Owner
No description provided.
ecazenave force-pushed wip/74723-last-jump from c1dad3fac7 to 69cc3c3949 2023-03-13 15:23:49 +01:00 Compare
ecazenave force-pushed wip/74723-last-jump from 69cc3c3949 to d3559cfe71 2023-03-13 16:37:54 +01:00 Compare
ecazenave force-pushed wip/74723-last-jump from d3559cfe71 to eb67c4ab20 2023-03-13 17:09:35 +01:00 Compare
ecazenave changed title from WIP: workflows : add an identifier to automatic and on submit jumps (#74723) to workflows : add an identifier to automatic and on submit jumps (#74723) 2023-03-13 17:27:47 +01:00
Author
Owner

Hésitation sur le fait qu'il faille rendre ces infos disponibles via les noms 'form_jumps' et 'form_last_jump' plutôt que 'form_workflow_data_...'.

Dans le doute j'en reste à 'form_workflow_data_...'.

Hésitation sur le fait qu'il faille rendre ces infos disponibles via les noms 'form_jumps' et 'form_last_jump' plutôt que 'form_workflow_data_...'. Dans le doute j'en reste à 'form_workflow_data_...'.
Owner

Avant de continuer ici il va falloir décider de la possibilité, ou pas, d'identifiants identiques, et si on les accepte du comportement à adopter. (#74132)

Avant de continuer ici il va falloir décider de la possibilité, ou pas, d'identifiants identiques, et si on les accepte du comportement à adopter. (#74132)
fpeters reviewed 2023-03-21 16:07:42 +01:00
wcs/workflows.py Outdated
@ -3049,0 +3064,4 @@
jumps = formdata.workflow_data.get('jumps', [])
if self.identifier:
jumps.append(self.identifier)
formdata.update_workflow_data({'jumps': jumps, 'last_jump': self.identifier})
Owner

Ça ne conserve pas le statut dans lequel est le saut, ça ne permet ainsi pas de distinguer deux sauts qui auraient le même identifiant et seraient dans des statuts différents. C'est volontaire ?

Ça ne conserve pas le statut dans lequel est le saut, ça ne permet ainsi pas de distinguer deux sauts qui auraient le même identifiant et seraient dans des statuts différents. C'est volontaire ?
Owner

Il me semble que ça rendrait la chose plus difficile à l'usage voir inutilisable en condition Django. Ici jumps est une bête liste de chaîne donc on peut faire comme indiqué dans le ticket

'xx' in form_workflow_jumps

si ça devient une liste de tuple

(formdata.status, self.identifier)

je ne vois pas trop comment l'utiliser simplement dans une condition et si on form_workflow_jumps ne reflète que le deuxième élément du tuple, alors autant ne pas le garder c'est une information de toute façon visible dans l'historique de la demande.

Il me semble que ça rendrait la chose plus difficile à l'usage voir inutilisable en condition Django. Ici jumps est une bête liste de chaîne donc on peut faire comme indiqué dans le ticket 'xx' in form_workflow_jumps si ça devient une liste de tuple (formdata.status, self.identifier) je ne vois pas trop comment l'utiliser simplement dans une condition et si on form_workflow_jumps ne reflète que le deuxième élément du tuple, alors autant ne pas le garder c'est une information de toute façon visible dans l'historique de la demande.
bdauvergne requested changes 2023-04-18 14:08:24 +02:00
wcs/workflows.py Outdated
@ -3049,0 +3065,4 @@
if self.identifier:
jumps.append(self.identifier)
formdata.update_workflow_data({'jumps': jumps, 'last_jump': self.identifier})
Owner

Aussi, si le dernier n'a pas d'identifiant, ne devrait-on pas remettre last_jump à None tout de même ? Pour que ça reste vraiment l'identifiant du dernier saut, et pas l'identifiant du dernier saut nommé.

Aussi, si le dernier n'a pas d'identifiant, ne devrait-on pas remettre last_jump à None tout de même ? Pour que ça reste vraiment l'identifiant du dernier saut, et pas l'identifiant du dernier saut nommé.
Author
Owner

Tenu compte, j'ai mis 'undefined'.

Tenu compte, j'ai mis 'undefined'.
bdauvergne requested changes 2023-04-18 14:10:28 +02:00
bdauvergne left a comment
Owner

Remarque plus générale: il me semblait qu'on essayait de déprécier l'usage de formdata.workflow_data (j'exagère peut-être ce mouvement), autant pour un autre ticket sur les marqueurs qui l'utilisent de toute façon déjà ce n'est pas gênant, mais là vu que c'est un nouvel usage est-ce qu'on ne devrait pas passer par une EvolutionPart (c'est tout de suite plus chiant que de taper 2 trucs dans un dico, je sais).

Remarque plus générale: il me semblait qu'on essayait de déprécier l'usage de formdata.workflow_data (j'exagère peut-être ce mouvement), autant pour un autre ticket sur les marqueurs qui l'utilisent de toute façon déjà ce n'est pas gênant, mais là vu que c'est un nouvel usage est-ce qu'on ne devrait pas passer par une EvolutionPart (c'est tout de suite plus chiant que de taper 2 trucs dans un dico, je sais).
ecazenave force-pushed wip/74723-last-jump from eb67c4ab20 to 301d1f18b0 2023-05-10 11:58:38 +02:00 Compare
ecazenave force-pushed wip/74723-last-jump from 301d1f18b0 to e37714196a 2023-05-10 12:27:47 +02:00 Compare
ecazenave force-pushed wip/74723-last-jump from e37714196a to 3fa6f36ec0 2023-05-10 12:57:04 +02:00 Compare
Author
Owner

En supposant l'unicité des identifiants, via #74132.

Pour la dépréciation de workflow_data, je veux bien une confirmation et surtout qu'on me pointe un exemple de la méthode alternative.

En supposant l'unicité des identifiants, via #74132. Pour la dépréciation de workflow_data, je veux bien une confirmation et surtout qu'on me pointe un exemple de la méthode alternative.
Owner

Pour la dépréciation de workflow_data, je veux bien une confirmation et surtout qu'on me pointe un exemple de la méthode alternative.

Oui, tu peux voir la méthode actuelle via LazyFormDataEmailsBase/EmailEvolutionPart pour les mails envoyés, ou LazyFormDataWorkflowForms/WorkflowFormEvolutionPart pour les formulaires de workflow.

> Pour la dépréciation de workflow_data, je veux bien une confirmation et surtout qu'on me pointe un exemple de la méthode alternative. Oui, tu peux voir la méthode actuelle via LazyFormDataEmailsBase/EmailEvolutionPart pour les mails envoyés, ou LazyFormDataWorkflowForms/WorkflowFormEvolutionPart pour les formulaires de workflow.
ecazenave force-pushed wip/74723-last-jump from 3fa6f36ec0 to f906d97b94 2023-05-15 14:47:39 +02:00 Compare
ecazenave force-pushed wip/74723-last-jump from f906d97b94 to c9b1abad55 2023-06-29 11:27:59 +02:00 Compare
ecazenave force-pushed wip/74723-last-jump from c9b1abad55 to 6acaf757cd 2023-07-05 11:47:36 +02:00 Compare
ecazenave force-pushed wip/74723-last-jump from 6acaf757cd to 291bbebc3b 2023-07-05 12:11:11 +02:00 Compare
ecazenave force-pushed wip/74723-last-jump from 291bbebc3b to 438f813140 2023-07-05 12:24:25 +02:00 Compare
ecazenave force-pushed wip/74723-last-jump from 438f813140 to 081dfb344c 2023-07-05 13:27:54 +02:00 Compare
ecazenave force-pushed wip/74723-last-jump from 081dfb344c to 1474049e08 2023-07-05 13:57:37 +02:00 Compare
ecazenave force-pushed wip/74723-last-jump from 1474049e08 to 6f16eaba83 2023-07-05 14:15:50 +02:00 Compare
ecazenave reviewed 2023-07-05 15:17:01 +02:00
ecazenave left a comment
Author
Owner

Voilà avec la gestion via des Part, pas de tout repos.

Voilà avec la gestion via des Part, pas de tout repos.
@ -167,12 +167,14 @@ def test_trigger_jumps(pub):
formdata.id = 1
formdata.data = {'0': 'Alice', '1': 'alice@example.net'}
formdata.status = 'wf-%s' % st1.id
formdata.just_created()
Author
Owner

Sinon le tests crash sur add_jump_part qui ne trouve pas d'évolution.

L'absence d'evolution me parait être une situation artificielle du test, d'où l'ajout du just_created.

Sinon le tests crash sur add_jump_part qui ne trouve pas d'évolution. L'absence d'evolution me parait être une situation artificielle du test, d'où l'ajout du just_created.
wcs/formdata.py Outdated
@ -807,3 +807,3 @@
and self.evolution[-1].status == status
and not self.evolution[-1].comment
and not [x for x in self.evolution[-1].parts or [] if not isinstance(x, ContentSnapshotPart)]
and not [
Author
Owner

Pour préserver le non ajout d'une nouvelle evolution lors d'un saut sur le même statut (test_form_worklow_multiple_identical_status).

Pour préserver le non ajout d'une nouvelle evolution lors d'un saut sur le même statut (test_form_worklow_multiple_identical_status).
wcs/wf/choice.py Outdated
@ -116,6 +114,7 @@ class ChoiceWorkflowStatusItem(WorkflowStatusJumpItem):
formdata.record_workflow_event('button', action_item_id=self.id)
evo.status = 'wf-%s' % wf_status[0].id
self.handle_markers_stack(formdata)
self.add_jump_part(formdata, evo)
Author
Owner

On passe l'evolution qui vient d'être créée sinon pas de persistance de la JumpEvolutionPart (voir plus bas).

On passe l'evolution qui vient d'être créée sinon pas de persistance de la JumpEvolutionPart (voir plus bas).
wcs/wf/jump.py Outdated
@ -253,2 +254,4 @@
self.handle_markers_stack(formdata)
self.add_jump_part(formdata)
formdata.status = 'wf-%s' % wf_status[0].id
formdata.store()
Author
Owner

Sans le formdata.store, pas de persistance de JumpEvolutionPart.

Ça se joue dans formdata.store, dont le fonctionnement me semble suspect sur les evolution :

1/ si une evolution n'a pas d'attribut _sql_id, elle (et éventuellement les suivantes) sont sotckées dans la DB), pas de mise jour en DB pour les evolution qui ont déjà _sql_id
2/ si toutes les evolutions ont un _sql_id elles sont toutes mise à jour

  • le formdata.store introduit ici tire parti de 2/
  • si je ne fais pas pas ça la JumpEvolutionPart n'est pas stockée en DB parce que le code appelant perform_items ajoute ensuite une nouvelle evolution puis fait formdata.store qui ne stocke que cette nouvelle évolution et laisse en rade celle d'avant où j'ai mis la JumpEvolutionPart (1/)
Sans le formdata.store, pas de persistance de JumpEvolutionPart. Ça se joue dans formdata.store, dont le fonctionnement me semble suspect sur les evolution : 1/ si une evolution n'a pas d'attribut _sql_id, elle (et éventuellement les suivantes) sont sotckées dans la DB), pas de mise jour en DB pour les evolution qui ont déjà _sql_id 2/ si toutes les evolutions ont un _sql_id elles sont toutes mise à jour * le formdata.store introduit ici tire parti de 2/ * si je ne fais pas pas ça la JumpEvolutionPart n'est pas stockée en DB parce que le code appelant perform_items ajoute ensuite une nouvelle evolution puis fait formdata.store qui ne stocke que cette nouvelle évolution et laisse en rade celle d'avant où j'ai mis la JumpEvolutionPart (1/)
Author
Owner

Je raconte n'importe quoi dans 2/, si toutes les evolution on un _sql_id, ça met juste à jour la dernière.

Je raconte n'importe quoi dans 2/, si toutes les evolution on un _sql_id, ça met juste à jour la dernière.
ecazenave requested review from fpeters 2023-07-06 11:13:18 +02:00
fpeters requested changes 2023-09-15 09:30:57 +02:00
wcs/workflows.py Outdated
@ -3153,0 +3169,4 @@
def add_jump_part(self, formdata, evo=None):
if evo is None:
evo = formdata.evolution[-1]
evo.add_part(JumpEvolutionPart(self.identifier or 'undefined'))
Owner

Désolé cette PR était un peu oubliée; ici s'il n'y a pas d'identifiant, je serais pour ne pas ajouter la Part.

Désolé cette PR était un peu oubliée; ici s'il n'y a pas d'identifiant, je serais pour ne pas ajouter la Part.
Author
Owner

Ça casserait le fonctionnement de latest_jump, dont la description dans le ticket est "donne l'identifiant d'un saut uniquement si la demande est dans un statut qui vient d'être atteint par ce saut".

Si je n'enregistre rien lors des sauts qui n'ont pas d'identifiants, je ne sais comment sortir l'information comme quoi tel ou tel est saut est le dernier saut à avoir été effectué.

Ça casserait le fonctionnement de latest_jump, dont la description dans le ticket est "donne l'identifiant d'un saut uniquement si la demande est dans un statut qui vient d'être atteint par ce saut". Si je n'enregistre rien lors des sauts qui n'ont pas d'identifiants, je ne sais comment sortir l'information comme quoi tel ou tel est saut est le dernier saut à avoir été effectué.
Owner

C'est un peu dommage de charger l'historique de toutes les demandes pour quelque chose qui ne sera pas tant utilisé :/

Alors au moins enregistrer l'identifiant comme étant None, plutôt qu'une chaine particulière ?

C'est un peu dommage de charger l'historique de toutes les demandes pour quelque chose qui ne sera pas tant utilisé :/ Alors au moins enregistrer l'identifiant comme étant None, plutôt qu'une chaine particulière ?
Author
Owner

Fait ainsi.

Fait ainsi.
ecazenave force-pushed wip/74723-last-jump from 6f16eaba83 to fad22d4195 2023-09-15 12:09:19 +02:00 Compare
ecazenave force-pushed wip/74723-last-jump from fad22d4195 to a64592c57d 2023-09-15 14:27:14 +02:00 Compare
ecazenave requested review from fpeters 2023-09-15 14:49:49 +02:00
fpeters requested changes 2023-09-15 15:08:40 +02:00
fpeters left a comment
Owner

(cf commentaire)

(cf commentaire)
ecazenave force-pushed wip/74723-last-jump from a64592c57d to dce0a5ebc6 2023-09-15 15:43:45 +02:00 Compare
ecazenave force-pushed wip/74723-last-jump from dce0a5ebc6 to bc078bb906 2023-10-05 16:58:27 +02:00 Compare
All checks were successful
gitea/wcs/pipeline/head This commit looks good
This pull request has changes conflicting with the target branch.
  • tests/backoffice_pages/test_all.py
  • wcs/formdata.py
  • wcs/wf/choice.py
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
3 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#161
No description provided.