search

cisstech / platon

Platform for Learning and Teaching Online
Other
14 stars 3 forks source link

fix(resource): permissions not returned by api #92

Closed mciissee closed 6 months ago

mciissee commented 7 months ago

@NewMeeh Un petit fix par rapport à ta PR (même si le listing fonctionne car pour l'instant on n'utilise pas les permissions, renvoyer false partout est faux d'une part et en cas de changement du front pour utiliser les permissions sur le listing ça casse le code)

codecov[bot] commented 7 months ago

Codecov Report

Attention: Patch coverage is 88.67925% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 74.88%. Comparing base (d727f62) to head (7dfb10b).

Files Patch % Lines
...e/resource/common/src/lib/models/resource.model.ts 85.00% 3 Missing and 3 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #92 +/- ## ========================================== + Coverage 70.68% 74.88% +4.19% ========================================== Files 15 17 +2 Lines 406 438 +32 Branches 115 123 +8 ========================================== + Hits 287 328 +41 + Misses 99 87 -12 - Partials 20 23 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

NewMeeh commented 7 months ago

Je comprends justement je trouvais que ça pouvait être bien temporairement, peut-être que ça pourrait être bien sinon de faire une méthode supplémentaire qui ne passe pas par les permissions et est donc un peu plus rapide pour les endroits où on applique seulement le droit en lecture basique.

mciissee commented 7 months ago

@NewMeeh J'ai fais des tests de perfs en vrai les permissions rajoutent pas plus de 200ms au temps de réponse mais je mettrais les permissions en expandable pour la retirer du contrôleur de recherche par défaut mais pour l'instant je pense qu'on peut laisser dans le contrôleur ça ne rajoute pas de problème de perfs. C'est plus les stats du dashboard qui rajoutent du temps de calcul d'après mes tests à cause des sessions et la non pagination des serchbars, je vais essayer de les stocker dans une view.

mciissee commented 6 months ago

@NewMeeh Tu préfères merge cette PR ou laisser comma ça pour le moment?

NewMeeh commented 6 months ago

@mciissee Peut-être voir pour rajouter les permissions en expandable avant de merge ? Puisque de toutes façons on ne les utilise pas pour l'instant

mciissee commented 6 months ago

Ca marche

mciissee commented 6 months ago

@NewMeeh Je te laisse regarder si tu as tout compris je merge après