-
-
Notifications
You must be signed in to change notification settings - Fork 97
Ajout d'autres universités avec l'intégration d'Appscho #940
base: main
Are you sure you want to change the base?
Conversation
# Veuillez entrer un message de validation pour expliquer en quoi cette fusion est # nécessaire, surtout si cela fusionne une branche amont mise à jour dans une branche de sujet. # # Les lignes commençant par '#' seront ignorées, et un message vide # abandonne la validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello ✌
Tout d'abord félicitation, la PR est bonne, le design est respecté c'est très bien intégré, pour la page "Autres universités", serait-il possible de rajouter un champ pour rechercher car sur ton screen il y a l'air d'en avoir pas mal ?
Je t'ai aussi mis quelques commentaires ci-dessous pour changer certaines choses/expliquer du code que j'aurais peut-être mal compris.
PS: Si quelqu'un avec un compte pourrait aussi tester la PR ce serait parfait
src/services/appscho/data/news.ts
Outdated
import { AttachmentType } from "@/services/shared/Attachment"; | ||
import { ErrorServiceUnauthenticated } from "@/services/shared/errors"; | ||
import { getNewsFeed, NewsFeed} from "appscho"; | ||
import {parse} from "date-fns"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import {parse} from "date-fns"; | |
import { parse } from "date-fns"; |
src/services/appscho/data/news.ts
Outdated
@@ -0,0 +1,42 @@ | |||
import {AppschoAccount} from "@/stores/account/types"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import {AppschoAccount} from "@/stores/account/types"; | |
import { AppschoAccount } from "@/stores/account/types"; |
src/services/appscho/data/news.ts
Outdated
import { Information } from "../../shared/Information"; | ||
import { AttachmentType } from "@/services/shared/Attachment"; | ||
import { ErrorServiceUnauthenticated } from "@/services/shared/errors"; | ||
import { getNewsFeed, NewsFeed} from "appscho"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import { getNewsFeed, NewsFeed} from "appscho"; | |
import { getNewsFeed, NewsFeed } from "appscho"; |
room: c.rooms && Array.isArray(c.rooms) ? c.rooms.join(", ") : (c.rooms || undefined), | ||
teacher: c.teachers ? c.teachers.split("\n ").join(", ") : undefined, | ||
source: "APPSCHO", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/services/homework.ts
Outdated
case AccountService.Appscho: | ||
case AccountService.Local: { | ||
homeworks = []; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n'est-il pas préférable de laisser le cas par défaut si ça ne fait rien?
src/services/homework.ts
Outdated
case AccountService.Appscho: | ||
case AccountService.Local: { | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pareil ici
src/views/account/News/Document.tsx
Outdated
} from "lucide-react-native"; | ||
import React, { useEffect, useLayoutEffect, useState } from "react"; | ||
import { View, Linking, TouchableOpacity, type GestureResponderEvent, StyleSheet } from "react-native"; | ||
import {View, Linking, TouchableOpacity, type GestureResponderEvent, StyleSheet, Image} from "react-native"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import {View, Linking, TouchableOpacity, type GestureResponderEvent, StyleSheet, Image} from "react-native"; | |
import { View, Linking, TouchableOpacity, type GestureResponderEvent, StyleSheet, Image } from "react-native"; |
name: "appscho", | ||
title: "Autres universités", | ||
description: "Utilise ton compte CAS pour te connecter", | ||
image: require("@/../assets/images/service_appscho.png"), // Envisagez une image plus générique ici |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
càd une image "plus générique" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C'est que avant, j'avais mis un autre logo avant de faire l'icon d'appscho, j'ai oublié d'enlever le com
last: account.lastname, | ||
first: account.firstname | ||
}, | ||
className: "", // TODO ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible de ne pas mettre "" ou // TODO quand il n'y a rien ? Autant le rajouter plus tard, ça améliore la lisibilité.
import {ScrollView} from "react-native-gesture-handler"; | ||
import {NativeItem, NativeList, NativeListHeader, NativeText} from "@/components/Global/NativeComponents"; | ||
import InsetsBottomView from "@/components/Global/InsetsBottomView"; | ||
import {INSTANCES} from "appscho"; // Ajustez le chemin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import {INSTANCES} from "appscho"; // Ajustez le chemin | |
import { INSTANCES } from "appscho"; // Ajustez le chemin |
ajustez le chemin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Webstorm et ses com
Possible de fix l'erreur qui empêche ESLint de faire la vérification? (Missing: [email protected] from lock file) |
D'acc, parfait. Je vais essayer de régler ça mercredi. Si besoin je peux passer mes codes pour tester |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for the Appscho identity provider across the login flow and integrates Appscho-based data into several services.
- Introduces a searchable
AppschoUniversities
screen listing available Appscho instances - Implements the
Appscho_Login
flow, account creation, and secure storage hooks - Hooks Appscho into timetable, news, reload, attendance, and grades services, plus default personalization and profile picture
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
src/views/login/IdentityProvider/providers/AppschoUniversities.tsx | Add Appscho university search screen |
src/views/login/IdentityProvider/providers/Appscho.tsx | Implement Appscho login and account creation |
src/views/login/IdentityProvider/IdentityProviderSelector.tsx | Add Appscho option in selector |
src/views/account/News/Document.tsx | Display news image previews |
src/utils/ui/default-profile-picture.ts | Provide default Appscho profile picture |
src/stores/account/types.ts | Define AppschoAccount type and enum |
src/services/timetable.ts | Integrate Appscho timetable fetching |
src/services/news.ts | Integrate Appscho news fetching |
src/services/grades.ts | Stub Appscho for grades features |
src/services/attendance.ts | Stub Appscho for attendance features |
src/services/appscho/reload-instance.ts | Implement Appscho account reload |
src/services/appscho/default-personalization.ts | Set default Appscho personalization |
src/services/appscho/data/timetable.ts | Parse Appscho timetable data |
src/services/appscho/data/news.ts | Parse Appscho news data |
src/router/screens/login/identityProvider.ts | Register Appscho routes |
src/router/helpers/types.ts | Add Appscho route parameter types |
package.json | Add appscho dependency |
Comments suppressed due to low confidence (2)
src/views/login/IdentityProvider/providers/Appscho.tsx:27
- [nitpick] Variable name
local_account
is snake_case and inconsistent with camelCase convention. Consider renaming tolocalAccount
.
const local_account: AppschoAccount = {
src/views/login/IdentityProvider/providers/AppschoUniversities.tsx:14
- [nitpick] New screen
AppschoUniversities
lacks associated unit/integration tests. Adding tests would ensure future changes don't break its search and navigation behavior.
export const AppschoUniversities: Screen<"AppschoUniversities"> = ({ navigation }) => {
✨ Nouvelle Pull Request
Merci de contribuer à l'amélioration de Papillon !
Tu te poses des questions sur les pull requests (PR) ? Une documentation a spécialement été créée ici => https://gitbook.getpapillon.xyz/organisation/outils-internes/github
Avant toute chose...
Tu t'assures avoir respecté les points suivants (en rajoutant un
x
dans les crochets) :Résumé des changements effectués
J'ai intégré la lib Appscho à l'application. J'ai ajouté un écran avec la liste des diverses instances disponible. Dans les actualité, j'ai ajouté la possibilité d'avoir une image de contenu.
Pour le moment, Appscho ne gère que les EDT et les actualités.
Aussi, il y a divers commit que j'ai revert après, je m'avais un github action pour build mes APKs.
Je reste dispo si besoin
Capture(s) d'écran (pour rendre le test de ta PR rapide)
Le(s) capture(s) d'écran



