Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Geofit/mes lieux #984

Merged
merged 12 commits into from
Sep 15, 2020
Merged

Geofit/mes lieux #984

merged 12 commits into from
Sep 15, 2020

Conversation

metourneau
Copy link
Contributor

fonctionnalité mes lieux

@camillemonchicourt
Copy link
Member

La BDD et le code source de GeoNature doivent être en anglais.
Il faut donc remplacer "lieu" par "place" (avec ou sans S selon le cas) dans la BDD et dans le code source.
Ainsi t_lieux devient t_places.
Et dans le code source Liste est ainsi à remplacer par List, en anglais.

@camillemonchicourt
Copy link
Member

Il manque la mise à jour de la BDD pour ceux qui ont déjà GeoNature.
Il faut donc ajouter la création de la table t_places dans le fichier data/migrations/update2.4.1to2.4.2.sql.
Ce fichier est dans la branche develop. Si cela entraîne des conflits, on le fera, mais pour ne pas oublier.

@metourneau
Copy link
Contributor Author

Fonctionnalité mes-lieux angliciser. Le fichier update2.4.1to2.4.2.sql a été ajouté.
Un petit bug sur le clean des géométries a été également corrigé.

@camillemonchicourt
Copy link
Member

Comme indiqué dans les spécifications la fonctionnalité "Mes lieux" n'est pas spécifique à Occtax mais globale pour être utilisée depuis n'importe quel module. C'est pourquoi elle est stockée dans "gn_commons'.
De la même manière la fonctionnalité ne doit pas dans Occtax au niveau du modèle, des routes et des composants frontend...

@camillemonchicourt
Copy link
Member

Les spécifications indiquaient aussi que cette fonctionnalité devait pouvoir être affichée ou non, avec un paramètre.


--MET 22/07/2020 Table t_places pour la fonctionnalité mes-lieux
CREATE TABLE gn_commons.t_places
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Voir les conventions des fichiers SQL existants:
les contraintes (PK et FK) sont écrites dans une section à part.

--MET 22/07/2020 Table t_places pour la fonctionnalité mes-lieux
CREATE TABLE gn_commons.t_places
(
id_role integer NOT NULL,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je rajouterais un champs id_place pour la PK en serial.

//--------------------------------------------------------------------------------------
//----------------Geofit additional code map.service.ts
//liste des lieux
getPlaces(): Observable<any> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

// Supprimer lieu
deletePlace(nom:String): Observable<{}> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

//Ajouter lieu
addPlace(place:GeoJSON.Feature): Observable<any>{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Même remarque

@@ -591,3 +592,58 @@ def export(info_role):
error=message,
redirect=current_app.config["URL_APPLICATION"] + "/#/occtax",
)
#######################################################################################
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ces routes ne devraient pas être dans le module "occtax" mais dans le fichier de routes du schéma gn_commons: https://github.com/PnX-SI/GeoNature/blob/master/backend/geonature/core/gn_commons/routes.py


#######################################################################################
# supprimer un lieu
@blueprint.route("/delPlace/<string:nom>", methods=["DELETE"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Les noms de "lieux" ne sont pas unique. Cette route devrait prendre en paramètre un id_place (voir remarque sur le SQL) plutôt que le nom .
De plus, ici une personne peut supprimer des lieux qui ne lui appartienne pas. Il faut vérifier que info_role.id_role == t_places.id_role
Sur les conventions de style du code, il faut respecter la PEP8 (notamment le snake_case: cf delOnePlace)

#######################################################################################
@serializable
@geoserializable
class TPlaces(DB.Model):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheoLechemia
Copy link
Member

Autres remarques:

  • Ajouter deux lieux avec le même nom entraine une erreur 500 ( (psycopg2.errors.UniqueViolation) ERREUR: la valeur d'une clé dupliquée rompt la contrainte unique « pk_t_places ») , sans retour en interface. Il faudrait vérifier de ne pas pouvoir ajouter deux lieux du même nom pour une même personne. En enlever la PK sur (place_name, id_role) pour la passer sur un serial.
  • Il manque un bouton pour "ajouter" le lieu à la carte sans avoir à quitter la modale (même si l'action est faite à la sélection du select, c'est contre intuitif.) Le bouton supprimer devrait être en rouge.
    meslieux

@metourneau
Copy link
Contributor Author

Nous avons pris en compte vos remarques dans la dernière livraison. Ajout d'un ID unique, déplacement de la fonctionnalité dans gn_commons, conventions sql et PEP8 et ergonomie de la fonctionnalité.

@TheoLechemia
Copy link
Member

Merci pour les corrections. Je vais pouvoir merger.
Pour la clarification ergonomique, je vais faire une proposition et voir ce que @RomainBaghi en pense.

@TheoLechemia
Copy link
Member

Désolé, encore quelques petits points à corriger:

  • On peut ajouter deux lieux qui ont le même nom
  • La suppression du dernier lieu de la liste entraîne une erreur 500 et ne supprime pas l'élement:
No row was found for one()
127.0.0.1 - - [15/Sep/2020 10:41:57] "DELETE /gn_commons/place/3 HTTP/1.1" 500 -

@camillemonchicourt
Copy link
Member

On peut ajouter 2 lieux qui ont le même nom, mais pas pour un même utilisateur.
Il doit y avoir une unicité sur l’association d'un place_name et d'un id_role

@metourneau
Copy link
Contributor Author

La livraison doit corriger tout cela. Je ne suis pas sur de la bonne gestion des erreurs coté Flask, surtout pour retourner une liste de lieu vide, et son traitement coté Angular. En tout cas, cela semble fonctionner

this.listPlacesSub = this._dfs.
getPlaces()
.subscribe(res => {
if(Object.keys(res[0]).length > 0){
Copy link
Member

@TheoLechemia TheoLechemia Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je ne suis pas sûr de comprendre cette ligne ? Quel est le cas d'usage ?
if(Object.keys(res[0]).length > 0){

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si l'objet retourné de getPlaces n'est pas vide. Si la liste des lieux est vide, ca renvoie un tableau à un élément (objet) vide => [{}]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.
Du coup la pratique qu'on a sur ce genre de cas, c'est de renvoyer un tableau vide côté python si il n'y a aucune données. Le fait de renvoyer un tableau vide ou None, fait que le décorateur @json_resp renvoie une 404 (qui est le code approprié dans ce cas). Donc ce que tu avais écrit avant le commit était ok:

@json_resp
def get_places(info_role):
    id_role = info_role.id_role
    data = DB.session.query(TPlaces).filter(TPlaces.id_role == id_role).all()
    return [n.as_geofeature("place_geom", "id_place") for n in data]

Côté frontend, le subscribe sur la route, fait passer la 404 dans les erreurs :

  fetchPlaces() {
    this._dfs.getPlaces().subscribe((res) => {
        this.places = res;
        this.place = this.places[0];
    },
      (err) => {
        if (err.status === 404) {
          this.places = [];
          this.place = null;
        }
      }
    );
  }

J'ai corrigé de mon côté.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants