Projet

Général

Profil

Development #58207

autosave peut sauver des données invalides

Ajouté par Benjamin Dauvergne il y a plus de 2 ans. Mis à jour il y a environ 2 ans.

Statut:
Rejeté
Priorité:
Normal
Assigné à:
Version cible:
-
Début:
26 octobre 2021
Echéance:
% réalisé:

0%

Temps estimé:
Patch proposed:
Oui
Planning:
Non

Description

Dans autosave, on a le code suivant :

        form = self.create_form(page=page)
        data = self.formdef.get_data(form)

or get_data ne vérifie pas si le widget est en état d'erreur (ex. un ItemField dans la source de donnée est en erreur pendant un instant, la valeur soumise n'est plus valide et widget.parse() renvoie None) avant d'utiliser la valeur renvoyée par widget.parse(...), si jamais l'autosave se passe après une soumission il est possible d'écraser des valeurs valides par des valeurs invalides.


Fichiers

Historique

#1

Mis à jour par Benjamin Dauvergne il y a plus de 2 ans

  • Sujet changé de autosave se peut sauver des données invalides à autosave peut sauver des données invalides
#3

Mis à jour par Benjamin Dauvergne il y a plus de 2 ans

  • Assigné à mis à Benjamin Dauvergne
#4

Mis à jour par Benjamin Dauvergne il y a plus de 2 ans

Première idée de correction :

diff --git a/wcs/formdef.py b/wcs/formdef.py
index 2997c836..d2db6202 100644
--- a/wcs/formdef.py
+++ b/wcs/formdef.py
@@ -818,11 +818,11 @@ class FormDef(StorableObject):

     def get_data(self, form):
         d = {}
         for field in self.fields:
             widget = form.get_widget('f%s' % field.id)
-            if widget:
+            if widget and not widget.has_error():
                 d.update(self.get_field_data(field, widget))
         return d

     def export_to_json(self, include_id=False, indent=None, anonymise=True):
         charset = get_publisher().site_charset

on va voir ce que ça donne au niveau des tests, j'ai quand même réussi à simuler un autosave un peu sauvage lié à une datasource (j'aurai pu utiliser n'importe quoi comme un StringField avec validation).

#5

Mis à jour par Frédéric Péters il y a plus de 2 ans

Je ne sais pas ce que diront les tests mais c'est prévu que des données invalides puissent être enregistrées, pour que l'usager puisse revenir sur une page dans l'état où il était occupé à la saisir, la validation s'opérant en sortie de page.

#6

Mis à jour par Frédéric Péters il y a plus de 2 ans

Les tests passant je viens de pousser un test explicitant le fonctionnement attendu de sauvegarde de données avant leur validation. (test_form_autosave_with_invalid_data).

#7

Mis à jour par Frédéric Péters il y a plus de 2 ans

i.e. je pense qu'il faut être plus fin, attraper la situation précise ("échec de la source de données"), ça pourrait être une exception remontée jusqu'à l'autosave qui dans ce cas s'interromperait (= je ne pense pas qu'il y ait à tenter une sauvegarde partielle "tous les champs sauf ceux où la source de données a échoué").

#8

Mis à jour par Benjamin Dauvergne il y a plus de 2 ans

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

i.e. je pense qu'il faut être plus fin, attraper la situation précise ("échec de la source de données"), ça pourrait être une exception remontée jusqu'à l'autosave qui dans ce cas s'interromperait (= je ne pense pas qu'il y ait à tenter une sauvegarde partielle "tous les champs sauf ceux où la source de données a échoué").

Ok, je vais limiter ça à ItemField et ItemsField, par contre je ne pense pas qu'on ait besoin de remonter jusqu'à la détection de l'exception car on a que deux cas d'invalidité :
  • le champ est requis et on soumet la valeur vide, on peut sauver ça,
  • ou soumet une valeur non vide mais elle ne valide pas, ça veut forcément dire que la valeur soumise ne correspond pas à une des valeurs dans widget.options, dans tous les cas on ne veut pas de cette valeur à ce moment là, une valeur invalide ne devrait pas être possible dans un Item/ItemsField de toute façon (contrairement aux autres champs en saisie plus ou moins libre) même de façon temporaire.
#9

Mis à jour par Benjamin Dauvergne il y a plus de 2 ans

Rebasé sur main et donc avec la restriction aux champs ItemField et ItemsField quand la valeur est non nulle/vide mais qu'elle ne valide pas.

#10

Mis à jour par Benjamin Dauvergne il y a plus de 2 ans

  • Statut changé de Solution proposée à En cours
#11

Mis à jour par Benjamin Dauvergne il y a plus de 2 ans

Sans dépendre de Widget.parse(), on ne peut pas l'appeler deux fois.

#13

Mis à jour par Frédéric Péters il y a plus de 2 ans

J'ai l'impression qu'on peut ignorer tout ça à partir du moment où

si jamais l'autosave se passe après une soumission

cette situation ne peut plus arriver que c'est l'objet de #58276 ?

Aussi, que ça serait de toute façon à revoir après #56824 ?

#14

Mis à jour par Benjamin Dauvergne il y a plus de 2 ans

  • Statut changé de Solution proposée à En cours
#15

Mis à jour par Benjamin Dauvergne il y a plus de 2 ans

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

cette situation ne peut plus arriver que c'est l'objet de #58276 ?

Le ticket a pour titre "limiter au maximum" ce qui est un grand progrès par rapport à "on s'en fout complètement", donc non je ne pourrai pas jurer que ça ne peut plus arriver; la remarque suivante est plus intéressante.

Aussi, que ça serait de toute façon à revoir après #56824 ?

Ça devient effectivement inutile après #56824 qui est allé plus loin que prévu initialement (ici ça fera foirer l'autosave complètement avec une erreur "form deserialization failed"), donc oui, si on peut avoir #56824 vite on peut rejeter celui-ci.

#16

Mis à jour par Benjamin Dauvergne il y a environ 2 ans

  • Statut changé de En cours à Rejeté

Je me rejette dans le mazout; les autres tickets liés à #58260 devraient corriger ce problème.

Formats disponibles : Atom PDF