Produits Entr'ouvert » Authentic 2

Je vais ajouter une migration de donnée pour m'assurer qu'on a pas des utilisateurs avec un état bâtard en base (user.first_name != user.attributes.first_name), je donnerai l'avantage à user.attributes..

Paul Marillonnet a écrit :

Le code gagnerait à :

• virer complètement le champ attribute_values du modèle utilisateur. Ce champ a été introduit, il me semble à la lecture du code, pour éviter les jointures de tables entre User, Attribute et AttributeValue. Il faudrait mesurer si la perte de cette optimisation est compensée par une plus grande clarté du code.

Et le remplacer par quoi ? Le code est plus clair qu'avant grâce à attribute_values justement, l'optimisation est anecdotique.

• à faire remonter l'information de vérification du modèle AttributeValue vers le modèle Attribute (ça ne me paraît pas complètement déconnant de dire que c'est l'attribut d'un usager, qui, à un instant t et pour une valeur donnée, est vérifié -- et non pas cette valeur).

Incompréhension du modèle de donné, Attribute est unique, il n'y a pas un Attribute par utilisateur.

Benjamin Dauvergne a écrit :

Incompréhension du modèle de donné, Attribute est unique, il n'y a pas un Attribute par utilisateur.

Ok, ma faute. Et donc ça répond à ma première question.
Une erreur dans les tests (le GenericRelatedObjectsManager qui se cache derrière le attribute_values ne définit pas delete, mais clear), le build est en échec.

Paul Marillonnet a écrit :

Quelques petits pinaillages comme à mon habitude.

• mauvaise convention de nommage de la variable d'itération ici, je crois :
  [...]

• Un appel à :
  [...]
  en trop, il me semble, à la fin de Attributes.__init__

• D'ailleurs sur la ligne d'avant, je crois qu'on s'en fout d'initialiser la variable values.

Je corrige tout ça.

Par contre, plus grave à mon avis : j'ai l'impression que Attribute.set_value pour un attribut multiple est bogué : on ajoute ou met à jour des valeurs, mais on ne supprime des anciennes valeurs pour cet attribut et cet utilisateur. Si ça s'avère vrai, je peux créer un autre ticket, puisque cette partie du code est restée inchangée par tes patchs.

Jamais marché car jamais utilisé, on peut juste tout virer et remplacer par raise NotImplementedError pour l'instant.

commit ff92cb1fb9c44de2749a15e420c0e59b1ccf9673
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Thu Apr 4 14:58:37 2019 +0200

    user: replace all uses of Attribute.set_value() (#31937)

commit 1079ac2e00460247e057d91ecc425b6d9daaab18
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Wed Apr 3 16:07:02 2019 +0200

    user: add command to fix storage of first_name/last_name attributes (#31937)

commit b72b11cb83d144e2bf3a3937a052ce3603491b31
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Tue Apr 2 18:01:22 2019 +0200

    user: fix cache errors on first_name/last_name handling (fixes #31937)

    first_name/last_name are not updated anymore inside
    Attribute.set_value() but only through the Attributes object.

    In User.save() we first compare current values of first/last_name before
    resetting the corresponding AttributeValue, i.e. if the value does not
    change we never reset the verified status.

    A map cache of attributes values is kept in user._a2_attributes_cache to
    reduce the number of queries when modifying attributes.

commit 085a1b0270418e829a5bf4dba60cd181eadff945
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Tue Apr 2 16:35:19 2019 +0200

    code style (#31937)

commit 0d74afcc5d912c78d2e6835300bdf42dfa2cceb4
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Tue Apr 2 16:33:56 2019 +0200

    pep8 (#31937)