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

Modularisation et utilisation d'un fichier GraphQL pour composer la liste des articles #703

Merged
merged 65 commits into from
Jan 7, 2023

Conversation

thom4parisot
Copy link
Member

@thom4parisot thom4parisot commented Dec 21, 2022

C'est un peu un proof of concept, pour voir si c'est intéressant à appliquer sur le reste de l'appli front. Toutes les requêtes GraphQL ont été extraites. Ça a permis de virer pas mal de services, qui n'ajoutaient plus grand chose depuis qu'il y a le hook useGraphQL (sessionToken et graphqlEndpoint y sont récupérés depuis le store).

Ça demanderait encore à retravailler certaines requêtes (passer par des mutations imbriquées plutôt que des rootMutations) mais ça commence à faire un changement assez épais 😅

Todo

  • vérifier chaque *Service car y'a des chances que les requêtes GraphQL ne fonctionnent pas (mauvais paramètres passés au service, et impossibilité d'utiliser le "hook" dans le store)
  • tous les trucs de Preview HTML + bibliographie basée sur Pandoc Export 🎁 🎄  #707
  • vérifier l'accès à l'API et la récupération des données en fonction du jeton, des ressources demandées et des permissions en action
    • articles(user)

Suites

@thom4parisot thom4parisot added dependencies 🔗 Pull requests that update a dependency file [domain] user interface 🎨 labels Dec 21, 2022
@netlify
Copy link

netlify bot commented Dec 21, 2022

Deploy Preview for stylo-dev ready!

Name Link
🔨 Latest commit c774a52
🔍 Latest deploy log https://app.netlify.com/sites/stylo-dev/deploys/63b9877223962b000860ca5f
😎 Deploy Preview https://deploy-preview-703--stylo-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

front/package.json Outdated Show resolved Hide resolved
@@ -0,0 +1,58 @@
query ($user: ID!) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oui c'est clairement mieux d'avoir la requête à part 👍🏻
Surtout qu'il doit y avoir des outils permettant de faciliter l'édition via un IDE.

@thom4parisot
Copy link
Member Author

thom4parisot commented Dec 23, 2022

@Mogztter Le commit le plus intéressant est f1eda5f : je déplace la RootMutation deleteArticle() vers un resolver. Ce qui veut dire que la vérification et d'utilisateur et d'existence de l'article ont déjà eu lieu.

En déplaçant la logique 1) dans un hook pré-suppression, 2) dans une transaction, on va aussi pouvoir écrire des tests tout en réduisant la complexité du backend.

@thom4parisot thom4parisot force-pushed the feature/better-article-graphql-query branch from f1eda5f to a5e0bcc Compare December 23, 2022 04:08
@ggrossetie
Copy link
Collaborator

Le commit le plus intéressant est f1eda5f : je déplace la RootMutation deleteArticle() vers un resolver. Ce qui veut dire que la vérification et d'utilisateur et d'existence de l'article ont déjà eu lieu.

Oui j'utilise la même stratégie sur les "groups" (teams) pour ajouter un article ou supprimer un membre. Cela rends le code tout de suite plus simple car on évite de dupliquer la logique de contrôle 💯

@ggrossetie
Copy link
Collaborator

En déplaçant la logique 1) dans un hook pré-suppression, 2) dans une transaction, on va aussi pouvoir écrire des tests tout en réduisant la complexité du backend.

Intéressant 🤔
Je serais curieux de voir un test "unitaire" sur la partie GraphQL 🙌🏻

@thom4parisot
Copy link
Member Author

thom4parisot commented Dec 23, 2022

Je serais curieux de voir un test "unitaire" sur la partie GraphQL

Et bien… c'est dur sans monter de base de données :-(
J'ai tenté de mocker différents objects mais les références internes de Mongoose rendent l'opération compliquée 😅

@thom4parisot
Copy link
Member Author

Avec les derniers commits, j'ai :

  • résolu un bug lors de la création d'un article (ça n'ajoutait pas correctement les tags sélectionnés à l'article)
  • factorisé ça avec des les mutations imbriquées (et en plus, ça bouge la logique au bon endroit, hors du tagResolver)

Je m'arrête là sur ce sujet, mais c'est très marrant !

@thom4parisot thom4parisot force-pushed the feature/better-article-graphql-query branch from 80e09ae to 316e068 Compare January 5, 2023 17:12
$user: ID!
$tag: ID!
$color: String!
$color: HexColorCode!
Copy link
Collaborator

Choose a reason for hiding this comment

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

c'est un type par défaut de GraphQL ?? 🤯

Copy link
Member Author

Choose a reason for hiding this comment

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

Non j'ai ajouté le module graphql-scalars pour gagner en finesse dessus. Ce qui a permis de typer les dates d'ailleurs !

@@ -131,7 +131,7 @@ articleSchema.methods.addTags = async function addTags (...tagIds) {
{ safe: true }
)

return this.save()
return this.save({ timestamps: false })
Copy link
Collaborator

Choose a reason for hiding this comment

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

on mettait à jour la date de dernière modification sur le tag alors qu'on associe le tag à un article c'est ça ? si c'est le cas bien vu 👀

Copy link
Member Author

@thom4parisot thom4parisot Jan 6, 2023

Choose a reason for hiding this comment

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

Exactement mais sur l'article (ajouter/enlever un tag faisait que l'article était considéré comme modifié).

@@ -49,7 +49,7 @@ export function ArticleSaveState ({ state, updatedAt, stateMessage }) {

const [savedAgo, isoString] = useMemo(() => ([
formatTimeAgo(updatedAt),
new Date(parseInt(updatedAt, 10)).toISOString()
new Date(updatedAt).toISOString()
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍🏻

@ggrossetie
Copy link
Collaborator

Comme j'ai pas mal touché à GraphQL pour l'implémentation des "workspaces", je pense que c'est mieux que ce changement passe en premier car il y a plus de changements que sur ma branche.

J'ai fais une revue par commit mais c'est effectivement assez dense car il y a pas mal de choses : resolutions d'anomalies, améliorations, changements sur l'UI... tu as été prolifique !

@thom4parisot thom4parisot force-pushed the feature/better-article-graphql-query branch from ced6840 to d66e70b Compare January 7, 2023 18:07
@thom4parisot thom4parisot merged commit 9b929c7 into master Jan 7, 2023
@thom4parisot thom4parisot deleted the feature/better-article-graphql-query branch January 7, 2023 19:16
@thom4parisot thom4parisot added this to the v1.8 milestone Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies 🔗 Pull requests that update a dependency file [domain] user interface 🎨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants