betagouv / eac-api

API de la plateforme EAC. Distribue des données sur les Acteurs Culturels et leurs actions.
https://api.education-artistique-culturelle.fr/
MIT License
2 stars 1 forks source link

department is auto created #18

Closed vinyll closed 6 years ago

rap2hpoutre commented 6 years ago

Je ne valide pas tout de suite car c'est moins urgent que https://github.com/betagouv/eac-api/pull/20 et ça nécessite d'enquêter sur les tests automatisés.

rap2hpoutre commented 6 years ago

FYI: ça pète aussi quand on lance les tests en local. J'essaie d'analyser car ça m'intrigue.

MAIS SINON, j'ai une autre proposition : utiliser simplement un getter côté mongoose. En effet, le département est ~une information redondante (une sorte de formattage sur une info existante de code postal). Et donc, généralement (bien qu'il y ait des exceptions) on évite ça dans les DB, on fait plutôt des sortes de vues (pas au sens SQL, simplement au premier sens). Et alors je me dis que les getters sont exactement fait pour ça, surtout qu'on a mis ça : https://github.com/betagouv/eac-api/blob/bcbe6d4a2d03c0f18e0616ba8f4b7d555d0e48a4/models/actor.js#L34

Donc si j'ai bien compris ça signifie que les getters sont exécutés avant de retourner les objets ou de sérialiser, ce qui est exactement ce qu'on veut(?).

Donc en gros ça pourrait être :

class Actor {
  // ...
  get department () {
    return this.department || (this.postalCode && this.postalCode.slice(0, -3))
  }

Ce code est NON TESTÉ, c'est juste pour soumettre l'idée.

Quel est ton avis @vinyll ?

vinyll commented 6 years ago

J'ai l'impression que ca suit la même logique que ta proposition de createdAt / updatedAt : avoir une base de données nettoyé. Ca permettrait en bonus de trier / rechercher éventuellement par département.

rap2hpoutre commented 6 years ago

Il semblerait qu'au delà du problème rencontré avec les tests (probablement lié au fait qu'il n'y ait pas de extends sur la classe alors qu'il y a un super dans mes tests) il ne faille pas surcharger la méthode save (ce qui semble légitime au vu des problèmes que peuvent poser l'héritage sur des classes externes). Mongoose affiche ce message dans la console :

mongoose: the method name "save" is used by mongoose internally, overwriting it may cause bugs. If you're sure you know what you're doing, you can suppress this error by using schema.method('save', fn, { suppressWarning: true })

Peut-être qu'il faudrait faire un pre hook, ce qui semble plus adapté d'après ce que je vois dans les différentes docs, comme par exemple :

ActorSchema.pre('save', function(next) {
  this.department = !this.department && this.postalCode && this.postalCode.slice(0, -3)
  next()
})

(mais bon comme d'habitude je n'y connais pas grand chose à mongo ni mongoose, donc il ne faut peut-être pas se fier à moi)

Ce qui m'amène à une autre question pour rebondir sur le problème des infos redondantes. Si on met le code d'enregistrement du département dans la base, à savoir :

this.department = !this.department && this.postalCode && this.postalCode.slice(0, -3)

... ça va poser de toute façon un problème de cohérence de données. Car si en tant qu'acteur culturel, je change l'information de code postal (erreur de frappe, ou n'importe quel autre cas), ça ne va pas mettre à jour le département, ce qui me semble poser problème. @vinyll Comment éviter ce problème ?

rap2hpoutre commented 6 years ago

@vinyll On ferme ?

vinyll commented 6 years ago

On peut la fermer et faire un ticket à la place pour générer les départements :+1: