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: Idée] Ajout d'un support pour le "multi-service" #564

Draft
wants to merge 89 commits into
base: main
Choose a base branch
from

Conversation

camarm-dev
Copy link
Member

@camarm-dev camarm-dev commented Jan 3, 2025

🚀 Nouvelle Pull Request

Dans cette PR, je propose une nouvelle fonctionnalité avancée pour Papillon , qui est de rassembler plusieurs services scolaires connectés en "espaces / environnements", où chaque fonctionnalité utilise un service précis...

Mais à quoi ça sert ?

Pour ceux qui se posent la question, cela permet de rassembler les différents services que peuvent utiliser les universités ou établissement, en un espace unifié. Par exemple, dans mon cas, la gestion des notes, devoirs, edt, ect passe par pronote, tandis que les actualités et la messagerie ne sont disponibles que sur skolengo. Ainsi, en me créant un espace je pourrai rassembler toute ma vie scolaire dans un seul environnement Papillon.

Comment ça fonctionne ?

Cf, screen

Un "espace" donc, est géré comme un compte principal (PrimaryAccount) par Papillon. Il dispose de ses propres paramètres, de son propre profil utilisateur ect... Sa particularitée est la nouvelle clé associatedAccountsLocalIDs, qui contient les identifiants locaux des comptes liés à cet espace. Aussi, j'ai créé un nouveau store (useMultiService) pour y stocker les espaces créé, et pour chaque fonctionnalité d'un espace, l'identifiant du compte à utiliser.

Ensuite, pour chaque fonctionnalité dans le dossier services, on ajoute un cas pour le compte MultiService, et on redirige vers le compte à utiliser:

case AccountService.PapillonMultiService: {
  const service = getFeatureAccount(MultiServiceFeature.Grades, account.localID);
  if (!service) {
    throw new Error("No service set in multi-service space");
  }
  return await updateGradesAndAveragesInCache(service, periodName);
}

Et donc on a la possibilité de créer le nombre d'espaces que l'on souhaite (cf screens)

Informations importantes

Si vous trouvez cette fonctionnalité intéressante, il faudrait m'aider à réorganiser l'interface (nottament la sélection des services, qui est un peu moche), donc il faudra voir avec les designers.

J'ai aussi du créé une image par défaut pour les espaces (pour ne pas les confondre avec des comptes classiques, et aussi pour qu'ils possèdent une icone propre au lancement de l'app: cf screen)

N'hésitez pas à me demander du contexte supplémentaire (sur discord ce serait plus pratique) ! Merci !

Screenshots / assets

Image Description
Default img Image du compte par défaut (c'est juste une icon lucide avec les même couleurs que l'image de profil...)
image Paramètres: nouveau bouton pour les paramètres du multi-service
image Paramètres du multi-service: activer / nouvel espace / espaces existants
image Paramètre d'un espace (1)
image Paramètre d'un espace (2)
image Paramètre d'un espace (3)
image Page de sélection des comptes

Checklist d'avant pull request

Veuillez cocher toutes les cases applicables en remplaçant [ ] par [x].

  • Vous avez testé de build le projet avec vos modifications et ce build a réussi
  • Vous respectez les conventions de codage et de nommage du projet
  • Vous utilisez la tabulation pour l'indentation afin de maintenir un code lisible
  • Cette pull request n'est pas un duplicata d'une autre
  • Cette pull request est prête à être revue (review) et fusionnée (merge)
  • Il n'y a pas de TODO (aka des annotations pour du code manquant) dans vos modifications
  • Il n'y a pas d'erreurs de langue dans votre code (grammaire, vocabulaire, conjugaison, orthographe)
  • Les détails des changements ont été décrits ci-dessous
  • Cette pull-request n'est pas une "breaking-change" (des modifications qui vont entraîner la modification du fonctionnement de certaines fonctionnalités déjà existantes)

Changelogs proposés

  • Ajout d'un nouveau type de compte (src/stores/account/types.ts)
    • Ajout d'un comportement particulier pour les espaces dans la fonction switchTo (src/stores/account/types.ts)
    • Modifications dans src/views/settings/SettingsExternalServices.tsx pour résoudre les erreurs dues au nouveau type
  • Ajout d'un nouveau store pour les espaces (src/stores/multiService)
  • Création d'utilitaires pour le multi-service: src/utils/multiservice
  • Ajout de la photo par défaut des comptes multiservices: src/utils/ui/default-profile-picture.ts (& assets/images/multiservice.png)
  • Ajout de deux pages de paramètres pour les espaces (src/views/settings/SettingsMultiService.tsx & src/views/settings/SettingsMultiServiceSpace.tsx)
    • Ajout des routes & de leur types: src/router/screens/settings/index.ts src/router/helpers/types.ts
    • Ajout / modification de composants: src/components/Settings/MultiServiceContainerCard.tsx, src/components/News/Beta.tsx & src/components/Global/AccountItem.tsx
    • Modification de src/view/settings/Settings.tsx pour ajouter l'onglet dans les paramètres
  • Implémentation des espaces multi services pour les différentes fonctionnalités: src/services
  • Modification de l'affichage des comptes, pour afficher le nom de l'espace (src/components/Home/AccountSwitcherContextMenu.tsx)

Ce qu'il reste à faire

  • Support dans reload-account.ts
    • C'est inutile car le reload est appelé que pendant le switchTo, et un cas spécial a été créé dans le switchTo
  • Résoudre le problème de key dans SettingsMultiServiceSpace.tsx
  • Supprimer le compte de l'espace quand un compte lié est déconnecté
    • Supprimer l'espace si il est le seul compte restant (c'est une sorte de groupe de compte, donc il ne sert plus à rien)

Informations supplémentaires

Chaque espace, possède un identifiant unique, qui est l'identifiant du compte de cet espace, et que l'on retrouve aussi dans l'objet qui réprésente l'envrionnement (stocké dans useMultiService).

J'ai du modifier la fonction switchTo pour ajouter un cas particulier pour les espace (comptes AccountService.PapillonMultiService):

  • Cela permet de recharger les comptes liés à cet espace (associatedAccountsLocalIDs)
  • Cela défini aussi "en dur" la valeur de instance du compte; par défaut instance est null (et jamais modifié, puisque ce n'est pas réellement un service) mais cette valeur sert à déterminer à plusieurs endroits si le compte a terminé son chargement: switchTo défini donc instance à une chaine de caractères non-nulle pour éviter les icones de chargement infini.

@camarm-dev
Copy link
Member Author

C'est prêt a être totalement review ?

Oui !

Copy link
Contributor

@Clmnnt Clmnnt left a comment

Choose a reason for hiding this comment

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

Leave a comment :trollface:

@camarm-dev
Copy link
Member Author

Ok bon je passe la PR en draft car je rencontre encore de problèmes avec la déconnexion des instances PRONOTE...

@camarm-dev camarm-dev marked this pull request as draft January 4, 2025 13:56
@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 4, 2025

Si tu mets un compte pour tous les onglets, cela change t'il qqch ?

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 4, 2025

Par contre, j'adore ton fix sur le DevMenu 😂😂

ecnivtwelve
ecnivtwelve previously approved these changes Jan 4, 2025
Copy link
Contributor

@ecnivtwelve ecnivtwelve left a comment

Choose a reason for hiding this comment

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

Très très très agréablement surpris de ton implémentation ! T'as hyper bien géré l'UX pour une feature aussi complèxe donc chapeau !

Cependant, je m'excuse d'avance si le temps de merge peut être un poil elevé ici pour le temps de test

@camarm-dev
Copy link
Member Author

Très très très agréablement surpris de ton implémentation ! T'as hyper bien géré l'UX pour une feature aussi complèxe donc chapeau !

Cependant, je m'excuse d'avance si le temps de merge peut être un poil elevé ici pour le temps de test

Haha merci beaucoup, j'ai fait de mon mieux ! J'ai repassé la PR en draft, je sais que c'est un peu le rush pour sortir la 7.7 là... Honnêtement prennez le temps de tester, j'ai encore des petits problèmes avec les comptes PRONOTE, qui se font déconnecter (je ne sais pas encore trop pourquoi), donc je repasserai en ready for review quand j'aurai résolu tout ça. Je vous notifierai sur Discord. Je vais d'ailleurs aller demander aux designers ce que je peux faire pour mon UI un peu dégeux :\

Merci pour la review !

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 4, 2025

Très très très agréablement surpris de ton implémentation ! T'as hyper bien géré l'UX pour une feature aussi complèxe donc chapeau !

Cependant, je m'excuse d'avance si le temps de merge peut être un poil elevé ici pour le temps de test

Haha merci beaucoup, j'ai fait de mon mieux ! J'ai repassé la PR en draft, je sais que c'est un peu le rush pour sortir la 7.7 là... Honnêtement prennez le temps de tester, j'ai encore des petits problèmes avec les comptes PRONOTE, qui se font déconnecter (je ne sais pas encore trop pourquoi), donc je repasserai en ready for review quand j'aurai résolu tout ça. Je vous notifierai sur Discord. Je vais d'ailleurs aller demander aux designers ce que je peux faire pour mon UI un peu dégeux :\

Merci pour la review !

C'est surtout sortir tout ça en 2 jours, impressionnant !
Ton ui n'est pas degueu, faut juste trouver une image pour présenter la nouvelle page dans les paramètres 😂
Et faut juste comprendre ce qui ne va pas du côté de pronote mais sinon, une pr parfaite !

@codeuriii
Copy link
Contributor

Oua mais c incroyable !! J'ai trop hâte que ca sorte !!
on peut être ping quand elle es merge ?

@codeuriii
Copy link
Contributor

je sais pas si ca a été suggéré avant mais est ce que tu pourrais rajouter une case a cocher ou un switch qui permet de se connecter automatiquement a cet espace multi service quand on ouvre l'appli ?

@camarm-dev
Copy link
Member Author

Oua mais c incroyable !! J'ai trop hâte que ca sorte !!
on peut être ping quand elle es merge ?

Tu as commenté la PR, donc tu devrai recevoir un mail quand elle sera merge 😉

@camarm-dev
Copy link
Member Author

je sais pas si ca a été suggéré avant mais est ce que tu pourrais rajouter une case a cocher ou un switch qui permet de se connecter automatiquement a cet espace multi service quand on ouvre l'appli ?

Ça a été suggéré de pouvoir se connecter automatiquement à un compte "par défaut" mais je n'implémenterai pas ça ici (je vais me concentrer uniquement sur le fonctionnement de base, ce sera bien suffisant)... Si personne le développe je ferai une autre PR.

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 7, 2025

je sais pas si ca a été suggéré avant mais est ce que tu pourrais rajouter une case a cocher ou un switch qui permet de se connecter automatiquement a cet espace multi service quand on ouvre l'appli ?

Ça a été suggéré de pouvoir se connecter automatiquement à un compte "par défaut" mais je n'implémenterai pas ça ici (je vais me concentrer uniquement sur le fonctionnement de base, ce sera bien suffisant)... Si personne le développe je ferai une autre PR.

Oui ça a été suggéré dans une issue spécifique mais l'intégrer ici ferait que retarder la review et l'implémentation (déjà qu'intégrer le multi-service est très complexe)

D'ailleurs, @camarm-dev j'suis en train de développer cette pr (#606) et je rencontrai le même problème avec Pronote au niveau du switchTo et je l'ai résolu comme ceci https://github.com/Kgeek33/Papillonv7/blob/feat/background/src/background/utils/accounts.ts car il ne supportait pas l'appel de hook (jspas si cela peut aider dans ton code, j'pense pas)
Et si ça ne change rien, alors ça signifie que switchTo est appelé plusieurs fois et comme tu l'as dit, Pronote n'aime pas ça

Copy link
Contributor

@Kgeek33 Kgeek33 left a comment

Choose a reason for hiding this comment

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

Code correct, faut que je trouve le temps pour retester


Par rapport à Pronote : Et si tout ce que j'ai dit ne change rien (ce qui ne serait pas surprenant 😂), alors faudrait que l'utilisateur choisisse obligatoirement un compte pour chaque onglet et voir si ça change qqch

@TinAD17tin
Copy link
Contributor

je sais pas si ca a été suggéré avant mais est ce que tu pourrais rajouter une case a cocher ou un switch qui permet de se connecter automatiquement a cet espace multi service quand on ouvre l'appli ?

J’ai déjà suggéré je sais plus dans quelle issue, des « paramatres generaux » d’application, permettant d’y mettre une connexion automatique, le choix de l’icône (parce que sinon c’est broken si plusieurs compte avec une icône différente)

@camarm-dev
Copy link
Member Author

Logique pour le typecheck :( ... J'essaye de finir la feature ce weekend, j'ai pas pu regarder ce weekend faute de temps. Il faudrait que je fasse un peu d'UI; j'avais qq petits trucs à améliorer; et puis que je règle le problème des instances PRONOTE, même si je pense savoir d'où ça vient.

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 13, 2025

Logique pour le typecheck :( ... J'essaye de finir la feature ce weekend, j'ai pas pu regarder ce weekend faute de temps. Il faudrait que je fasse un peu d'UI; j'avais qq petits trucs à améliorer; et puis que je règle le problème des instances PRONOTE, même si je pense savoir d'où ça vient.

Pour le typecheck, ça a été corrigé par @Skythrew dans #637
Ok d'accord 👍, demande-moi de review quand t'auras bouclé ton intégration !

@camarm-dev
Copy link
Member Author

Encore un weekend chargé où j'ai pas pu travailler sur Papillon... Vous inquiétez pas je compte bien finir la PR quand même, mais surment le weekend prochain :)

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.

9 participants