feat: fix visu / suppression de doublons pour un organisme #3272

sbenfares · 2023-10-05T12:19:38Z

Bugs remontés par @ClemLan : l'écran n'était accessible que pour les admins.

Modifications faites testables via impostures :

Ajout de l'écran doublons pour un organisme
Modification de l'API pour remonter directement le détail des doublons dans la route duplicates, plus besoin d'un appel API spécifique à un détail d'effectif
Modification de la route de delete pour permettre à un organisme de supprimer ses effectifs avec un controle sur l'organisme id.

totakoko

Il me reste juste à valider en local et je valide demain !

totakoko · 2023-10-05T15:36:27Z

ui/pages/effectifs/doublons.tsx

+          {organisme && <EffectifsDoublonsPage isMine={true} />}
+        </Container>
+      </Box>
+    </Page>


Y'a un composant SimplePage qui simplifie un peu les choses et pour harmoniser avec le reste des pages, il faut le titre en bleu cf 👇

<SimplePage title={title}> <Container maxW="xl" p="8"> <Heading as="h1" color="#465F9D" fontSize="beta" fontWeight="700" mb="4w"> Titre de page en bleu </Heading> ... </Container> </SimplePage>

Comme ça le SimplePage laisse la possibilité au contenu de choisir la largeur souhaitée avec le container (par exemple sur la page d'accueil, j'avais besoin d'avoir la largeur complète). Et pas besoin du Box intermédiaire. 🙂

totakoko · 2023-10-05T16:14:03Z

ui/modules/mon-espace/effectifs/doublons/EffectifDoublonDeleteAlertDialog.tsx

@@ -57,7 +62,7 @@ const EffectifDoublonDeleteAlertDialog = ({
            <Button
              colorScheme="red"
              onClick={async () => {
-                await _delete(`/api/v1/effectif/${effectifId}`);
+                await _delete(`/api/v1/organismes/${organisme?._id}/duplicate/${duplicateDetail?.id}`);


Vu qu'on a l'id de l'effectif et que ce n'est pas relatif 1, 2, 3, 4. pourquoi ajouter organisme id en paramètre ?
Dans tous les cas, pour les permissions, on devrait récupérer l'effectif, et vérifier que l'organisme_id est accessible selon notre profil. (j'ai pas encore vu la partie backend)

Updated, désormais la route est différente plus besoin de l'organisme_id en param.

totakoko · 2023-10-05T16:14:40Z

server/src/http/server.ts

+        requireOrganismePermission("manageEffectifs"),
+        async (req, res, next) => {
+          res.locals.duplicateId = new ObjectId((req.params as any).effectifId);
+          console.log("res.locals.duplicateId :>> ", res.locals.duplicateId);


Faudra supprimer tous les console.log de debug 🧹

totakoko · 2023-10-05T16:15:27Z

server/src/http/server.ts

+
+          if (!isEffectifLinkedToOrganisme) return res.status(401).json({ error: "Accès non autorisé" });
+          next();
+        },


Tu voudrais pas mettre tout ce code dans le handler plutôt ?

totakoko · 2023-10-05T16:16:34Z

server/src/http/server.ts

+          )?.organisme_id.equals(res.locals.organismeId);
+          console.log("isEffectifLinkedToOrganisme:>> ", isEffectifLinkedToOrganisme);
+
+          if (!isEffectifLinkedToOrganisme) return res.status(401).json({ error: "Accès non autorisé" });


throw Boom.forbidden("Permissions invalides") pour avoir un 403 qui correspond à un accès interdit selon les permissions.
401 correspond plutôt à une erreur d'authentification.

totakoko · 2023-10-05T16:17:47Z

server/src/http/server.ts

+
+          const isEffectifLinkedToOrganisme = (
+            await effectifsDb().findOne({ _id: res.locals.duplicateId })
+          )?.organisme_id.equals(res.locals.organismeId);


A voir, mais y'a moyen qu'un organisme parent doivent / puisse gérer les effectifs des organismes formateurs. (il me semble que c'était l'idée jusque-là)

cf https://www.notion.so/Permissions-afd9dc14606042e8b76b23aa57f516a8

Et en relisant le code, en fait on vérifie juste que l'effectif correspond à l'organisme passé en paramètre. Du coup c'est pas super secure... 🙃
Je recommande de supprimer l'organisme des paramètres car on peut de toute façon retrouver l'organisme_id afin de vérifier les droits.

Done, ajout d'une fonction spécifique dans les permissions

totakoko

👍 J'ai pu tester et ça fonctionne. Il y a juste quelques points qu'on peut améliorer je pense : sécu route suppression + afficher les nom et prénoms originaux.

totakoko · 2023-10-06T08:33:12Z

ui/modules/mon-espace/effectifs/doublons/EffectifDoublonDetailModal.tsx

-                {`Détail de l'apprenant ${toPascalCase(effectifDetail?.apprenant?.prenom)} ${toPascalCase(
-                  effectifDetail?.apprenant?.nom
+                {`Détail de l'apprenant ${toPascalCase(duplicateDetail?.apprenant?.prenom)} ${toPascalCase(
+                  duplicateDetail?.apprenant?.nom


Le toPascaleCase semble un peu pourri car il supprime les tirets entre autres, du coup le nom affiché est pas génial. On peut pas juste afficher les valeurs telles quelles ? (dans la dialog de détail + dans la liste)

+1 pour prendre le nom tel quel dans l'affichage (même si on transforme pour comparer)

totakoko · 2023-10-06T08:36:55Z

server/src/http/server.ts

+
+          const isEffectifLinkedToOrganisme = (
+            await effectifsDb().findOne({ _id: res.locals.duplicateId })
+          )?.organisme_id.equals(res.locals.organismeId);


Et en relisant le code, en fait on vérifie juste que l'effectif correspond à l'organisme passé en paramètre. Du coup c'est pas super secure... 🙃
Je recommande de supprimer l'organisme des paramètres car on peut de toute façon retrouver l'organisme_id afin de vérifier les droits.

rap2hpoutre

Ok pour moi après la prise en compte des commentaires de la review de @totakoko

server/src/http/server.ts

rap2hpoutre

On peut merge quand vous voulez quand c'est prêt

server/src/http/server.ts

sbenfares · 2023-10-12T15:36:26Z

Qqs updates et ajout d'un système de vérification des permissions pour permettre :

Admin
OF Concerné
OF responsables

server/src/http/server.ts

totakoko

👍 J'ai fait quelques derniers commentaires ! 😄

totakoko · 2023-10-12T15:48:29Z

server/src/common/actions/helpers/permissions.ts

+        throw new Error("organisme de l'organisation non trouvé");
+      }
+
+      const organismesResponsablesIdsOfOrganisme = findOrganismeFormateursIds(userOrganisme);


... organismesResponsables... = findOrganismesFormateurs .... 🤔 😁

En relisant, je pense que tu voudrais utiliser findOrganismesAccessiblesByOrganisationOF pour retourner un tableau contenant l'organisme Id + ceux de ces formateurs.

En fait ça revient à faire comme la fonction canManageOrganismeEffectifs.

totakoko · 2023-10-12T15:49:28Z

server/src/common/actions/helpers/permissions.ts

+
+      return (
+        organismeIdForEffectif.equals(userOrganisme._id) ||
+        organismesResponsablesIdsOfOrganisme.includes(organismeIdForEffectif)


cf commentaire précédent (plus haut), donc potentiellement obsolète.

Attention ici le includes ne va pas fonctionner car organismeIdForEffectif n'est pas un type primitif mais un ObjectId.

totakoko · 2023-10-12T15:51:39Z

server/src/common/actions/helpers/permissions.ts

+ * @param effectifId
+ * @returns
+ */
+export const canDeleteEffectifDuplicate = async (ctx: AuthContext, effectifId: ObjectId) => {


Actuellement effectifId n'est pas un ObjectId mais un string !

Tu pourrais le définir en ObjectId directement côté handler, et supprimer le new ObjectId(effectifId) ici.

totakoko · 2023-10-12T15:52:30Z

ui/pages/effectifs/doublons.tsx

+      </Head>
+      <Box w="100%" pt={[4, 6]} px={[1, 1, 2, 4]} mb={16}>
+        <Container maxW="xl" px={0}>
+          {organisme && <EffectifsDoublonsPage isMine={true} />}


On peut utiliser SimplePage ici aussi ! 🙃

sbenfares · 2023-10-13T07:41:37Z

Update ici 👉🏼 d0b3e16

Et ici 👉🏼 c5e0f5a

server/src/http/server.ts

+      returnResult(async (req) => {
+        const effectifId = new ObjectId(req.params.id);
+        if (!(await canDeleteEffectif(req.user, effectifId))) throw Boom.forbidden("Permissions invalides");
+        await effectifsDb().deleteOne({ _id: effectifId });
+      })


wip

sbenfares · 2023-10-13T10:04:49Z

Ajout d'un test sur la route : e4bdef5 @totakoko

github-actions · 2023-10-13T10:11:21Z

🚀 Prévisualisation

https://3272.tdb-preview.apprentissage.beta.gouv.fr/

github-actions · 2023-10-13T12:24:03Z

🎉 This PR is included in version 3.64.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

sbenfares force-pushed the feature/fix-droits-ecran-doublons branch from e78dab5 to 6dcda44 Compare October 5, 2023 13:50

sbenfares changed the title ~~wip: fix doublons~~ feat: fix visu / suppression de doublons pour un organisme Oct 5, 2023

sbenfares marked this pull request as ready for review October 5, 2023 15:04

sbenfares requested review from rap2hpoutre and totakoko October 5, 2023 15:04

totakoko reviewed Oct 5, 2023

View reviewed changes

totakoko approved these changes Oct 6, 2023

View reviewed changes

rap2hpoutre approved these changes Oct 6, 2023

View reviewed changes

sbenfares force-pushed the feature/fix-droits-ecran-doublons branch from 805c174 to 3c78f8b Compare October 9, 2023 08:33

github-advanced-security bot found potential problems Oct 9, 2023

View reviewed changes

server/src/http/server.ts Fixed Show fixed Hide fixed

rap2hpoutre approved these changes Oct 12, 2023

View reviewed changes

sbenfares force-pushed the feature/fix-droits-ecran-doublons branch from 3c78f8b to a1c476a Compare October 12, 2023 13:04

github-advanced-security bot found potential problems Oct 12, 2023

View reviewed changes

server/src/http/server.ts Fixed Show fixed Hide fixed

sbenfares force-pushed the feature/fix-droits-ecran-doublons branch from 8808a81 to ac2af8c Compare October 12, 2023 15:32

github-advanced-security bot found potential problems Oct 12, 2023

View reviewed changes

server/src/http/server.ts Fixed Show fixed Hide fixed

totakoko approved these changes Oct 12, 2023

View reviewed changes

github-advanced-security bot found potential problems Oct 13, 2023

View reviewed changes

sbenfares added 10 commits October 13, 2023 10:58

feat(ui): fix de la page doublons pour un organisme

50aaa2b

feat: simplification de la récupération des détails de doublons

41573cd

wip

feat(ui): fix DuplicateEffectifDetail model

54c2569

wip

feat: update delete api method

f893dec

fix(server): post review

341106a

fix(ui): post review

a1289a4

fix: permissions

5bb679a

feat(ui): ajout simplePage & update wording

30b49d6

feat: update delete effectif route

93920e2

feat(server): ajout test unitaire

e4bdef5

wip

sbenfares force-pushed the feature/fix-droits-ecran-doublons branch from c5e0f5a to e4bdef5 Compare October 13, 2023 10:03

sbenfares merged commit 9abaf6e into master Oct 13, 2023

sbenfares deleted the feature/fix-droits-ecran-doublons branch October 13, 2023 12:17

github-actions bot added the released label Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fix visu / suppression de doublons pour un organisme #3272

feat: fix visu / suppression de doublons pour un organisme #3272

sbenfares commented Oct 5, 2023 •

edited

Loading

totakoko left a comment

totakoko Oct 5, 2023

sbenfares Oct 12, 2023

totakoko Oct 5, 2023

sbenfares Oct 12, 2023

totakoko Oct 5, 2023

sbenfares Oct 12, 2023

totakoko Oct 5, 2023

sbenfares Oct 12, 2023

totakoko Oct 5, 2023

sbenfares Oct 12, 2023

totakoko Oct 5, 2023

totakoko Oct 6, 2023

sbenfares Oct 12, 2023

totakoko left a comment

totakoko Oct 6, 2023

rap2hpoutre Oct 6, 2023

sbenfares Oct 12, 2023

totakoko Oct 6, 2023

rap2hpoutre left a comment

rap2hpoutre left a comment

sbenfares commented Oct 12, 2023

totakoko left a comment

totakoko Oct 12, 2023

totakoko Oct 12, 2023

totakoko Oct 12, 2023

totakoko Oct 12, 2023

sbenfares commented Oct 13, 2023 •

edited

Loading

sbenfares commented Oct 13, 2023

github-actions bot commented Oct 13, 2023

github-actions bot commented Oct 13, 2023

feat: fix visu / suppression de doublons pour un organisme #3272

feat: fix visu / suppression de doublons pour un organisme #3272

Conversation

sbenfares commented Oct 5, 2023 • edited Loading

totakoko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

totakoko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rap2hpoutre left a comment

Choose a reason for hiding this comment

rap2hpoutre left a comment

Choose a reason for hiding this comment

sbenfares commented Oct 12, 2023

totakoko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbenfares commented Oct 13, 2023 • edited Loading

sbenfares commented Oct 13, 2023

github-actions bot commented Oct 13, 2023

🚀 Prévisualisation

github-actions bot commented Oct 13, 2023

sbenfares commented Oct 5, 2023 •

edited

Loading

sbenfares commented Oct 13, 2023 •

edited

Loading