Produits Entr'ouvert » w.c.s.

Je propose ce patch bien que j'ai l'impression d'avoir raté quelque chose : je n'ai pas trouvé de fonction adéquat sur laquelle m'appuyer pour éviter d'ajouter du code.

Voici un exemple de rendu (j'ai inventé un type "workflow-attachment") :

{...
    "evolution": [
        {...
            "parts": [
                {
                    "content": "/9j/4AAQSkZJ...",
                    "content_type": "image/jpeg",
                    "filename": "01_Crop_Circles_Intricate.jpg",
                    "type": "workflow-attachment" 
                }
            ],

Le test aussi me semble un peu léger : il ne fait pas explicitement l'appel aux actions "Fichier joint" et "Message dans l'historique", parce que je me suis inspiré des autres tests trouvés dans tests/api/ et que je ne vois pas où en ajouter ailleurs.

Aussi, j'ai propagé le paramètre 'include_files' (j'ai cru comprendre que c'était ce qui est demandé dans le ticket) mais je n'y trouve pas grand intérêt : je n'ai pas vu comment le positionner à False via l'api.

(remarques prisent en compte)

la signature de la méthode get_json_export_dict doit être la même partout.

oui, j'avais hésité là dessus (j'ai pris peur d'ouvrir la boite de pandore)

Remarques prises en compte,

utiliser with pour assurer le nettoyage du descripteur de fichier.

y compris celle là que j'avais oublié de traiter.

Il faut ajouter les nouveaux paramètres derrière les paramètres existants.

Même si la nouvelle signature existe déjà par ailleurs ?

$ git grep -n 'def get_json_export_dict'
wcs/formdata.py:1145:    def get_json_export_dict(self, include_files=True, anonymise=False, user=None):

(Remarques prises en compte.)

tu pourrais juster passer bool(get_request().form.get('full') == 'on').

J'écrivais ça et le résultat c'est

+            anonymise=anonymise, include_files=not bool(get_request().form.get('full') == 'on')

et ça me sonne moche comme tout (le not qui vient se caler là, plutôt que modifier la comparaison) et je m'arrête je m'arrête et j'ai totalement besoin d'une explication de comment on est arrivé à dire que passer full=on ça voudrait dire ne pas avoir les fichiers, ça me semble totalement à l'envers ?

_~~

Et je relis diligemment le tout et il y a eu confusion entre le comportement de l'API qui retourne l'ensemble des fiches (là le full=on il est pour dire "tous les attributs") et l'API qui retourne les données d'une fiche, là il n'y a pas d'option au comportement.

Il y a un test qui fait pourtant ça. Je viens de le corriger.

--- a/tests/api/test_formdata.py
+++ b/tests/api/test_formdata.py
@@ -713,7 +713,7 @@ def test_api_anonymized_formdata(pub, local_user, admin_user):
     assert 'who' not in resp.json[0]['evolution'][0]
     assert 'time' in resp.json[0]['evolution'][0]
     # check anonymise is enforced on detail view
-    resp = get_app(pub).get(sign_uri('/api/forms/test/%s/?anonymise&full=on' % resp.json[1]['id']))
+    resp = get_app(pub).get(sign_uri('/api/forms/test/%s/?anonymise' % resp.json[1]['id']))
     assert 'receipt_time' in resp.json
     assert 'fields' in resp.json
     assert 'user' not in resp.json

_~~

Cela posé, ça permet de retirer du patch la partie qui touche à wcs/forms/common.py, et donc la partie dont heureusement le style m'a heurté 2 fois parce que même la première fois je n'avais pas noté le non-sens.

_~~

Et à reprendre :

On a le déroulé include_files=False quand l'API /api/forms/<slug>/, ou /api/users/forms sont appelées, avec ?full=on, exemple:
...
Voilà donc un test supplémentaire à ajouter : assurer que tu ne reçois pas l'information sur les fichiers dans ces appels.

Et donc comme il y a eu confusion sur l'API (j'écris /api/forms/<slug>/ et c'est /api/forms/<slug>/<id> qui est traité), j'ajoute le test dont je parlais,

@@ -514,6 +514,10 @@ def test_formdata_with_evolution_part_attachment(pub, local_user):
     assert len(resp.json['evolution']) == 1
     assert 'parts' not in resp.json['evolution']

+    # check this doesn't get into list of forms API
+    resp = get_app(pub).get(sign_uri('/api/forms/test/list?full=on', user=local_user))
+    assert 'hello.txt' not in resp.text

(oui erreur de ma part l'API c'était /api/forms/<slug>/list et pas /api/forms/<slug>/).

_~~

Et tout ça amène à quelque chose qui était presque le premier patch, qui avait juste comme erreur de mettre include_files=False plutôt que True.

Et mon commentaire qui suivait :

include_files doit être à True par défaut partout; la signature de la méthode get_json_export_dict doit être la même partout.

Avec la partie sur la signature qui voulait pointer que dans le patch tu avais à la fois :

    def get_json_export_dict(self, include_files=True, anonymise=False):

et

    def get_json_export_dict(self, include_files=False, anonymise=False):

Puis mauvaise interprétation de mon "la signature doit partout être pareille", appliquée au-delà du patch.

Puis je reprends :

Bref, le patch initial, sans la confusion un coup =True un coup =False, et le test que je mentionnais. Et ne rien reprendre en anonyme.

Et c'est là sans doute que "le test que je mentionnais" (sur le ?full=on) se trouve amener l'idée qu'il y a un ?full=on sur l'API d'une demande unique, et donc l'ajout du non-sens (?full=on → moins de données).

_~~

Voilà beaucoup écrit mais je trouve crucial d'identifier ces moments de mauvaise explication&compréhension, pour tenter de les réduire.

(patch dans la branche -bis)

(et cette relecture fait office de validation du patch modifié, parce qu'à la base c'est pour l'avoir dans un tag que je me suis remis à relire ça ce soir)

commit 51d935656a8105963e7afff428d402cfb37c7758
Author: Nicolas ROCHE <nroche@entrouvert.com>
Date:   Tue Apr 27 11:15:01 2021 +0200

    api: include attachment evolution parts in {card,form}data fetch API (#53253)