formdata: add an page_id attribute (#85091) #1036

Merged
ecazenave merged 2 commits from wip/85091-draft-internal-identifier into main 2024-02-09 15:55:41 +01:00
Owner
No description provided.
ecazenave force-pushed wip/85091-draft-internal-identifier from 737fcba866 to 84bd6649f6 2024-01-16 17:32:31 +01:00 Compare
ecazenave force-pushed wip/85091-draft-internal-identifier from 84bd6649f6 to d8535bed40 2024-01-16 18:00:57 +01:00 Compare
ecazenave force-pushed wip/85091-draft-internal-identifier from 12029fc2cc to 368c07b8fb 2024-01-19 17:28:31 +01:00 Compare
ecazenave force-pushed wip/85091-draft-internal-identifier from 368c07b8fb to 650c6328c3 2024-01-19 17:50:53 +01:00 Compare
ecazenave force-pushed wip/85091-draft-internal-identifier from 650c6328c3 to 78ae5d0f18 2024-01-22 15:28:24 +01:00 Compare
Author
Owner

Par dessus https://dev.entrouvert.org/issues/85940.

Rien de particulier à mon sens.

Par dessus https://dev.entrouvert.org/issues/85940. Rien de particulier à mon sens.
ecazenave changed title from WIP: formdata: add an page_id attribute (#85091) to formdata: add an page_id attribute (#85091) 2024-01-22 15:39:16 +01:00
fpeters requested changes 2024-01-22 17:04:53 +01:00
@ -2437,2 +2437,4 @@
def test_migration_99_formdata_page_id(pub):
# prout
Owner

Nope.

Nope.
Author
Owner

Oups, je fais ça tout le temps pour pouvoir retrouver le code que je touche plus facilement, ménage fait.

Oups, je fais ça tout le temps pour pouvoir retrouver le code que je touche plus facilement, ménage fait.
ecazenave force-pushed wip/85091-draft-internal-identifier from 78ae5d0f18 to 8fbc88cee8 2024-01-22 18:18:44 +01:00 Compare
fpeters requested changes 2024-01-27 10:10:15 +01:00
@ -2436,6 +2436,35 @@ def test_migration_86_card_uuid(pub):
cur.close()
def test_migration_99_formdata_page_id(pub):
Owner

Je suggérerais de ne pas mettre le numéro dans le nom du test, pour avoir un endroit de moins où ne pas avoir à le changer.

Je suggérerais de ne pas mettre le numéro dans le nom du test, pour avoir un endroit de moins où ne pas avoir à le changer.
@ -2439,0 +2439,4 @@
def test_migration_99_formdata_page_id(pub):
FormDef.wipe()
formdef = FormDef()
formdef.name = 'tests migration 99'
Owner

Pareil ici.

Pareil ici.
@ -2439,0 +2453,4 @@
cur.execute('UPDATE wcs_meta SET value = 98 WHERE key = %s', ('sql_level',))
# drop uuid column
cur.execute('ALTER TABLE %s DROP COLUMN page_id' % sql.get_formdef_table_name(formdef))
Owner

Le commentaire sent le copié/collé, c'est la colonne page_id qui est retirée.

Le commentaire sent le copié/collé, c'est la colonne page_id qui est retirée.
@ -2439,0 +2459,4 @@
sql.migrate()
assert column_exists_in_table(cur, sql.get_formdef_table_name(formdef), 'page_id')
assert migration_level(cur) >= 99
Owner

Il ne faudra pas oublier de modifier le numéro ici, par contre.

Il ne faudra pas oublier de modifier le numéro ici, par contre.
@ -1435,0 +1423,4 @@
try:
page = self.pages[page_no]
except IndexError:
page = None
Owner

Si ça a lieu ça serait utile d'avoir un commentaire pour expliquer les circonstances.

Si ça a lieu ça serait utile d'avoir un commentaire pour expliquer les circonstances.
@ -1624,3 +1611,3 @@
return redirect(get_publisher().get_root_url())
def autosave_draft(self, draft_id, page_no, form_data, where=None):
def autosave_draft(self, draft_id, page_no, page, form_data, where=None):
Owner

Je ne pense pas intéressant de dupliquer l'information dans les paramètres; il me semble que tout le temps l'objet page peut être retrouvée via page_no, self.pages[page_no], ça n'est pas le cas ?

Je ne pense pas intéressant de dupliquer l'information dans les paramètres; il me semble que tout le temps l'objet page peut être retrouvée via page_no, `self.pages[page_no]`, ça n'est pas le cas ?
@ -1711,3 +1699,3 @@
return json.dumps({'result': 'success'})
def save_draft(self, data, page_no=None, where=None):
def save_draft(self, data, page_no=None, page=None, where=None):
Owner

Même commentaire que sur autosave_draft, je ne suis pas sûr de la nécessité du paramètre.

Même commentaire que sur autosave_draft, je ne suis pas sûr de la nécessité du paramètre.
@ -1721,2 +1709,4 @@
if page_no is not None:
filled.page_no = page_no
if page is not None:
filled.page_id = page.id
Owner

Il semble me manquer un élément crucial par rapport à l'objectif, c'est l'information comme quoi on est sur la page de validation.

In fine pour reprendre plusieurs commentaires, je pense que ça pourrait être :

     def get_page_id(self, page_no):
           if self.has_confirmation_page() and page_no > len(self.pages):
                 return '_confirmation_page'
           page = self.pages[page_no - 1]
           return page.id if page else None

(code tapé ici sans vérifier, pour montrer l'idée)

et utiliser cette méthode pour remplir l'attribut page_id.

Il semble me manquer un élément crucial par rapport à l'objectif, c'est l'information comme quoi on est sur la page de validation. In fine pour reprendre plusieurs commentaires, je pense que ça pourrait être : ``` def get_page_id(self, page_no): if self.has_confirmation_page() and page_no > len(self.pages): return '_confirmation_page' page = self.pages[page_no - 1] return page.id if page else None ``` (code tapé ici sans vérifier, pour montrer l'idée) et utiliser cette méthode pour remplir l'attribut page_id.
ecazenave force-pushed wip/85091-draft-internal-identifier from 8fbc88cee8 to 4cf9b418d0 2024-01-29 16:06:44 +01:00 Compare
ecazenave force-pushed wip/85091-draft-internal-identifier from 4cf9b418d0 to ae10758ae1 2024-02-02 15:20:00 +01:00 Compare
ecazenave force-pushed wip/85091-draft-internal-identifier from ae10758ae1 to 6fa8b2c72e 2024-02-02 16:25:54 +01:00 Compare
ecazenave force-pushed wip/85091-draft-internal-identifier from 6fa8b2c72e to 24ee1a3e3a 2024-02-02 16:37:24 +01:00 Compare
ecazenave force-pushed wip/85091-draft-internal-identifier from 24ee1a3e3a to 201f194826 2024-02-02 16:48:12 +01:00 Compare
ecazenave force-pushed wip/85091-draft-internal-identifier from 201f194826 to c32427109d 2024-02-02 16:59:52 +01:00 Compare
Author
Owner

Voilà toutes les remarques prises en compte.

En plus de '_confirmation_page', j'introduis l'identifiant spécial '_first_page' pour la première page des formulaires qui n'ont pas de champ page.

Voilà toutes les remarques prises en compte. En plus de '_confirmation_page', j'introduis l'identifiant spécial '_first_page' pour la première page des formulaires qui n'ont pas de champ page.
fpeters requested changes 2024-02-08 18:01:49 +01:00
fpeters left a comment
Owner

Me reste juste cette interrogation.

Me reste juste cette interrogation.
@ -1601,0 +1606,4 @@
return self.pages[page_no].id
if self.has_confirmation_page():
return '_confirmation_page'
return None
Owner

Ça n'est pas évident pour moi la situation où on arrive à cette dernière ligne, j'aurais cru qu'on n'y arrivait jamais mais le coverage me détrompe (dans les nouveaux tests test_draft_store_page_id_no_confirmation et test_draft_store_page_id_when_no_page_and_no_confirmation passent par là).

Un commentaire pourrait être ajouté pour expliciter ?

Ça n'est pas évident pour moi la situation où on arrive à cette dernière ligne, j'aurais cru qu'on n'y arrivait jamais mais le coverage me détrompe (dans les nouveaux tests test_draft_store_page_id_no_confirmation et test_draft_store_page_id_when_no_page_and_no_confirmation passent par là). Un commentaire pourrait être ajouté pour expliciter ?
Author
Owner

Voilà.

Voilà.
ecazenave force-pushed wip/85091-draft-internal-identifier from c32427109d to bfd099a950 2024-02-09 11:24:23 +01:00 Compare
ecazenave force-pushed wip/85091-draft-internal-identifier from bfd099a950 to 5d21b253a9 2024-02-09 11:53:21 +01:00 Compare
fpeters approved these changes 2024-02-09 12:56:27 +01:00
ecazenave merged commit 4ef907d7f8 into main 2024-02-09 15:55:41 +01:00
ecazenave deleted branch wip/85091-draft-internal-identifier 2024-02-09 15:55:41 +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#1036
No description provided.