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

Feat/home/display latest discussions #3154

Conversation

andriacap
Copy link
Contributor

@andriacap andriacap commented Aug 19, 2024

Bonjour,

Dans le cadre d'un projet avec l'ARB-IDF, cette PR propose de pouvoir afficher (optionnellement) les dernières discussions sur la page d'accueil.

Les développements proposés sont expliqués et discutés dans cette issue : https://github.com/orgs/PnX-SI/projects/20/views/1?pane=issue&itemId=68569536

  • Information à discuter pour la review:
    Les devs proposés sont actuellement directement intégrés dans le composant "home". Il s'agit de savoir s'il l'on créé un composant global qui accueillerait les différents onglets ("Dernières discussions" , et les autres onglets mentionnés dans l'issue Enrichir la page d’accueil  #2983) ?
    --> [x] devs proposés dans la PR (à commenter dans la review si ne convient pas)

  • TODO:

  • Faire le lint du frontend (une fois que la review est faite et qu'il n'y a plus de devs à implémenter)

  • Router navigate mis en place pour rediriger l'utilisateur vers la modale d'information lié à l'id_synthese ( route 'synthese/occurence/:id_synthese') . Ajouter 'discussions' au chemin pour accéder directement à l'onglet "Discussions" de la modale ( dépend de l'issue [SYNTHESE][FRONTEND] Evolution / amélioration du routing frontend pour l'information d'une observation  #3155 et de la PR feat: add route to tabs synthese info #3169 ) --> évolution futur

Merci pour vos retours .

Issue décrite dans le projet : https://github.com/orgs/PnX-SI/projects/20/views/1?pane=issue&itemId=68569536
Epic en lien : #2983

@andriacap andriacap marked this pull request as ready for review August 21, 2024 09:30
@andriacap andriacap force-pushed the feat/home/display-latest-discussions branch from ffc96d7 to 62906d7 Compare August 28, 2024 10:27
@@ -1412,55 +1412,109 @@ def update_content_report(id_report):
session.commit()


@routes.route("/reports", methods=["GET"])
@routes.route("/reports", defaults={"id_synthese": None}, methods=["GET"])
Copy link
Member

Choose a reason for hiding this comment

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

La signature devrait être que /reports/<id_synthese> car on veut toujours les commentaires / signalement d'une observation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

est ce que c'est lié au commentaire de review que tu fais ci dessous

Il devrait y avoir deux routes différentes :

* une `/reports/id_synthese`

* une `/reports`

backend/geonature/core/gn_synthese/routes.py Outdated Show resolved Hide resolved
else:
raise BadRequest("Bad orderby")

if not id_synthese:
Copy link
Member

Choose a reason for hiding this comment

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

Il devrait y avoir deux routes différentes :

  • une /reports/id_synthese
  • une /reports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pour ce point je m'étais appuyé sur ce qui a été dit là dans le commentaire de l'issue concernée : #3138 (comment)

return jsonify(result)

# Join the User table
req = req.join(User, User.id_role == TReport.id_role)
Copy link
Member

Choose a reason for hiding this comment

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

Préferer les jointures à partir des modèles.
On avait ça avant :

    req = req.options(
        joinedload(TReport.user).load_only(User.nom_role, User.prenom_role),
        joinedload(TReport.report_type),
    )

Copy link
Contributor Author

@andriacap andriacap Sep 4, 2024

Choose a reason for hiding this comment

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

J'ai corrigé en utilisant le joinedload quand c'était possible mais pour le user en voulant récupérer la propriété hybride "nom_complet" c'est apparement pas possible de récupérer une propriété de ce type en faisant un "joinedload"

[selectionType]="'single'"
(sort)="onColumnSort($event)"
>
<ngx-datatable-row-detail [rowHeight]="150">
Copy link
Member

Choose a reason for hiding this comment

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

Le bloc détail ne rajoute pas d'info. Je me demande s'il est pertinent

Copy link
Contributor Author

@andriacap andriacap Aug 30, 2024

Choose a reason for hiding this comment

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

C'est pour le cas où le commentaire est long et serait tronqué par la largeur max de la colonne

@@ -0,0 +1,6 @@
<mat-slide-toggle
Copy link
Member

Choose a reason for hiding this comment

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

Pourquoi créer un composant uniquement pour ce bouton toggle ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je l'ai fait en anticipation à plus de filtres (si d'autres filtres venait à être ajouter ) . Alors on regrouperait /modifierait uniquement ce composant pour rassembler les filtres . Mais à discuter et voir les autres propositions ?

@andriacap
Copy link
Contributor Author

andriacap commented Sep 4, 2024

Sur la base des retours de Théo j'ai donc splitté en 2 routes (mentioné ici : #3154 (comment)).

J'ai donc dû modifier les tests pour que ces derniers fonctionnent. Le test sur la route qui était existante de base à savoir list_report refonctionne.

Concernant le test ajouté pour la route list_all_reports (/reports) ce dernier fonctionne uniquement lorsque j'éxécute la route avec les différents paramètres hors contexte de test backend :

Détails des résultats (json) des appels à la route

Route éxécutée : http://127.0.0.1:8000/synthese/reports?type=discussion&my_reports=true&orderby=creation_date&sort=desc

{
  "current_page": 1,
  "items": [
    {
      "content": "commentaire 3",
      "creation_date": "Wed, 21 Aug 2024 09:03:21 GMT",
      "deleted": false,
      "id_report": 3,
      "id_role": 3,
      "id_synthese": 2,
      "report_type": {
        "id_type": 1,
        "type": "discussion"
      },
      "synthese": {
        "cd_nom": 351,
        "date_max": "Sun, 01 Jan 2017 12:05:02 GMT",
        "date_min": "Sun, 01 Jan 2017 12:05:02 GMT",
        "nom_cite": "Grenouille rousse",
        "observers": "Administrateur test"
      },
      "user": {
        "nom_complet": "Administrateur test"
      }
    },
    {
      "content": "commentaire lynx",
      "creation_date": "Wed, 21 Aug 2024 09:02:55 GMT",
      "deleted": false,
      "id_report": 2,
      "id_role": 3,
      "id_synthese": 1,
      "report_type": {
        "id_type": 1,
        "type": "discussion"
      },
      "synthese": {
        "cd_nom": 60612,
        "date_max": "Sun, 01 Jan 2017 12:05:02 GMT",
        "date_min": "Sun, 01 Jan 2017 12:05:02 GMT",
        "nom_cite": "Lynx Bor\u00e9al",
        "observers": "Administrateur test"
      },
      "user": {
        "nom_complet": "Administrateur test"
      }
    },
    {
      "content": "test discussion",
      "creation_date": "Tue, 20 Aug 2024 16:04:11 GMT",
      "deleted": false,
      "id_report": 1,
      "id_role": 3,
      "id_synthese": 3,
      "report_type": {
        "id_type": 1,
        "type": "discussion"
      },
      "synthese": {
        "cd_nom": 67111,
        "date_max": "Sun, 08 Jan 2017 23:00:00 GMT",
        "date_min": "Sun, 08 Jan 2017 20:00:00 GMT",
        "nom_cite": "Ablette",
        "observers": "Administrateur test"
      },
      "user": {
        "nom_complet": "Administrateur test"
      }
    }
  ],
  "pages": 1,
  "per_page": 10,
  "total": 4,
  "total_filtered": 3
}

Route éxecutée : http://127.0.0.1:8000/synthese/reports?type=discussion&orderby=creation_date&sort=desc

{
  "current_page": 1,
  "items": [
    {
      "content": "ah bon un lynx ?",
      "creation_date": "Wed, 21 Aug 2024 11:15:54 GMT",
      "deleted": false,
      "id_report": 4,
      "id_role": 4,
      "id_synthese": 1,
      "report_type": {
        "id_type": 1,
        "type": "discussion"
      },
      "synthese": {
        "cd_nom": 60612,
        "date_max": "Sun, 01 Jan 2017 12:05:02 GMT",
        "date_min": "Sun, 01 Jan 2017 12:05:02 GMT",
        "nom_cite": "Lynx Bor\u00e9al",
        "observers": "Administrateur test"
      },
      "user": {
        "nom_complet": "Agent test"
      }
    },
    {
      "content": "commentaire 3",
      "creation_date": "Wed, 21 Aug 2024 09:03:21 GMT",
      "deleted": false,
      "id_report": 3,
      "id_role": 3,
      "id_synthese": 2,
      "report_type": {
        "id_type": 1,
        "type": "discussion"
      },
      "synthese": {
        "cd_nom": 351,
        "date_max": "Sun, 01 Jan 2017 12:05:02 GMT",
        "date_min": "Sun, 01 Jan 2017 12:05:02 GMT",
        "nom_cite": "Grenouille rousse",
        "observers": "Administrateur test"
      },
      "user": {
        "nom_complet": "Administrateur test"
      }
    },
    {
      "content": "commentaire lynx",
      "creation_date": "Wed, 21 Aug 2024 09:02:55 GMT",
      "deleted": false,
      "id_report": 2,
      "id_role": 3,
      "id_synthese": 1,
      "report_type": {
        "id_type": 1,
        "type": "discussion"
      },
      "synthese": {
        "cd_nom": 60612,
        "date_max": "Sun, 01 Jan 2017 12:05:02 GMT",
        "date_min": "Sun, 01 Jan 2017 12:05:02 GMT",
        "nom_cite": "Lynx Bor\u00e9al",
        "observers": "Administrateur test"
      },
      "user": {
        "nom_complet": "Administrateur test"
      }
    },
    {
      "content": "test discussion",
      "creation_date": "Tue, 20 Aug 2024 16:04:11 GMT",
      "deleted": false,
      "id_report": 1,
      "id_role": 3,
      "id_synthese": 3,
      "report_type": {
        "id_type": 1,
        "type": "discussion"
      },
      "synthese": {
        "cd_nom": 67111,
        "date_max": "Sun, 08 Jan 2017 23:00:00 GMT",
        "date_min": "Sun, 08 Jan 2017 20:00:00 GMT",
        "nom_cite": "Ablette",
        "observers": "Administrateur test"
      },
      "user": {
        "nom_complet": "Administrateur test"
      }
    }
  ],
  "pages": 1,
  "per_page": 10,
  "total": 4,
  "total_filtered": 4
}

L'erreur est renvoyée sur l'éxécution de cette ligne :
https://github.com/PnX-SI/GeoNature/blob//develop/backend/geonature/core/gn_synthese/models.py#L520-L522

L'erreur qui est retournée est la suivante :

[1ec25745-f5d5-4543-88b0-c93a233d4876] Exception on /synthese/reports [GET]
Traceback (most recent call last):
  File "/home/andriac/applications/dev/geonature-dev/geonature/backend/venv/lib/python3.9/site-packages/flask/app.py", line 880, in full_dispatch_request
    rv = self.dispatch_request()
  File "/home/andriac/applications/dev/geonature-dev/geonature/backend/venv/lib/python3.9/site-packages/flask/app.py", line 865, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)  # type: ignore[no-any-return]
  File "/home/andriac/applications/dev/geonature-dev/geonature/backend/geonature/core/gn_permissions/decorators.py", line 62, in decorated_view
    return view_func(*args, **kwargs)
  File "/home/andriac/applications/dev/geonature-dev/geonature/backend/geonature/core/gn_synthese/routes.py", line 1457, in list_all_reports
    query = Synthese.filter_by_scope(scope=scope, user=g.current_user, query=query)
  File "/home/andriac/applications/dev/geonature-dev/geonature/backend/dependencies/Utils-Flask-SQLAlchemy/src/utils_flask_sqla/models.py", line 87, in _
    result = method(*args, **kwargs)
  File "/home/andriac/applications/dev/geonature-dev/geonature/backend/geonature/core/gn_synthese/models.py", line 521, in filter_by_scope
    TDatasets.filter_by_readable(user).with_entities(TDatasets.id_dataset)
  File "/home/andriac/applications/dev/geonature-dev/geonature/backend/dependencies/Utils-Flask-SQLAlchemy/src/utils_flask_sqla/models.py", line 87, in _
    result = method(*args, **kwargs)
TypeError: filter_by_readable() got multiple values for argument 'query'   <---------------- Un problème avec le query

Est ce que vous avez déjà rencontré ce genre de problèmes en environnement de tests backend ? Si oui je veux bien des pistes pour résoudre ce problème.

Merci

@andriacap
Copy link
Contributor Author

Salut,

On a pas eu trop le temps d'en discuter quand on s'est vu à Gap, mais si vous avez des questions pour pouvoir jeter un oeil à la PR n'hésitez pas . Je suis dispo @Pierre-Narcisi @jacquesfize

@jacquesfize
Copy link
Contributor

jacquesfize commented Sep 13, 2024

Salut @andriacap,

Je pense que l'appelle de la fonction est pas le bon... est-ce que tu peux remplacer TDatasets.filter_by_readable(user).with_entities(TDatasets.id_dataset) par TDatasets.filter_by_readable(user=user).with_entities(TDatasets.id_dataset) dans le fichier gn_synthese/models.py

@andriacap
Copy link
Contributor Author

Salut @andriacap,

Je pense que l'appelle de la fonction est pas le bon... est-ce que tu peux remplacer TDatasets.filter_by_readable(user).with_entities(TDatasets.id_dataset) par TDatasets.filter_by_readable(user=user).with_entities(TDatasets.id_dataset) dans le fichier gn_synthese/models.py

Ca marche je teste ça , merci !

@andriacap
Copy link
Contributor Author

Bon je viens de voir ce que tu mentionnes . Ca correspond pas au code que j'ai modifié mais l'existant.
J'ai quand même testé ce que tu dis mais ça ne résoud pas le problème, le test ne passe toujours pas et l'erreur apparait toujours à cet endroit lorsque je debug : https://github.com/PnX-SI/GeoNature/blob//develop/backend/geonature/core/gn_synthese/models.py#L520-L522

@andriacap
Copy link
Contributor Author

Le problème sur les tests backend ont été résolus. J'ai utilisé l'autre manière pour vérifier les droits de lecture sur la synthese et ça ne renvoie plus d'erreur coté backend .

@edelclaux edelclaux force-pushed the feat/home/display-latest-discussions branch 2 times, most recently from 73d6263 to 52b8532 Compare September 17, 2024 11:09
Copy link

codecov bot commented Sep 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.76%. Comparing base (c3f7f65) to head (8c2b874).
Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3154      +/-   ##
===========================================
+ Coverage    81.62%   81.76%   +0.13%     
===========================================
  Files           86       86              
  Lines         6945     6976      +31     
===========================================
+ Hits          5669     5704      +35     
+ Misses        1276     1272       -4     
Flag Coverage Δ
pytest 81.76% <100.00%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@edelclaux edelclaux force-pushed the feat/home/display-latest-discussions branch from e1ee939 to b2f58c9 Compare September 18, 2024 08:33
@edelclaux edelclaux force-pushed the feat/home/display-latest-discussions branch from fbc122b to ad13e5d Compare September 30, 2024 09:10
- Add user to reports list in fixtures
- Test with and without id_synthese path parameters
- test orderby, sort , my_reports filters

Reviewed-by: andriacap
- take account sort dir on click column
- add ngx-datatable to get pages

Reviewed-by: andriacap
- Add config in order to display or not latest discussions
- Manage in template html and controler typescript

Reviewed-by: andriacap
use columnMode="force" to use all width

Reviewed-by: andriacap
Reviewed-by: andriacap
refact the block of code (all in home-content)
into different component.

WIP: see added TODOs

Reviewed-by: andriacap
Forget idea to use generic table (todo in another PR)

Reviewed-by: andriacap
andriacap and others added 20 commits October 2, 2024 13:20
Check all config related to block if any exist
hide block of information

Reviewed-by: andriacap
same css fow selected row (same as synthese-list
datatable)

Reviewed-by: andriacap
Use handlePageChange instead of pageChanged

Reviewed-by: andriacap
Add router navigate in order to redirect on modal
synthese info related to id_synthese

TOOD: add 'discussion' to path link to button click

Reviewed-by: andriacap
Reviewed-by: andriacap
Reviewed-by: andriacap
Add total_filtered in order to display to the user
the total filtered / total Rows .
Sample: here the user can show only their discussions
and see how many discussions he is involved compare
to total discussions

Reviewed-by: andriacap
- Remove comment backend
- Style : remove row selected style
- Style : add padding for toggle component

Reviwed-by: andriacap
- replace join way by joinedload
- fix no orderby params sent from front
- split routes as asked in review
- add filter_by_scope for synthese in order to get
only reports related to readable synthese

Reviewed-by: andriacap
- Split test to target the two differents routes
- WIP :  make tests work for `/reports` list_all_reports

Reviewed-by: andriacap
Use decorator @permissions_required("R", module_code="SYNTHESE")
to make test backend working.

Reviewed-by: andriacap
@edelclaux edelclaux force-pushed the feat/home/display-latest-discussions branch from 4f0f027 to 7d768a9 Compare October 2, 2024 11:20
else:
raise BadRequest("Bad orderby")

total = TReport.query.count()
paginated_results = req.paginate(page=page, per_page=per_page, error_out=False)
# Pagination
Copy link
Contributor

Choose a reason for hiding this comment

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

C'est une bonne pratique ça de faire la pagination à la mano ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Dans GeoNature, c'est flask-sqlalchemy qui fait la pagination. Le soucis, c'est qu'ils utilisent la méthode scalars() lors de l'exécution d'une requête construite avec un objet Select (remplace Query dans SQLA 2.0) (https://github.com/pallets-eco/flask-sqlalchemy/blob/main/src/flask_sqlalchemy/pagination.py#L328-L364). Ici, c'est pas ce que l'on souhaite. SI on veut que la pagination flask-sqlalchemy fonctionne ici, il faudrait passer par une requête construite avec un object Query mais qui n'est plus recommandé dans SQLA 2.0 😕

Du coup, je propose de faire la pagination "à la main" en s'appuyant sur quasi le même fonctionnement de flask-sqlalchemy.

Copy link
Contributor

Choose a reason for hiding this comment

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

On peut aussi modifier la requête, récupérer le nom_complet de l'utilisateur dans un joinedload et repasser sur la pagination flask-sqlalchemy, ça devrait marcher 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

c'est fait :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Top, merci !

@jacquesfize jacquesfize merged commit 73e1ecb into PnX-SI:develop Oct 4, 2024
7 checks passed
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.

5 participants