-
Notifications
You must be signed in to change notification settings - Fork 3
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: sync plans between devices #103
Conversation
frontend/src/lib/auth/index.ts
Outdated
} | ||
|
||
try { | ||
const response = await fetch("https://planer.solvro.pl/api/v1/user/login", { |
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.
czemu ten adres jest na sztywno wpisany
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.
a czm nie, pozniej bede sie bawic w zmienne
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.
no o to chodzi, żebyś nie musiał mergować zmian na maina, żeby testować backend 😭
tylko odpalasz sobie oba lokalnie i lecisz
popraw nazwy commitów i je zesquashuj |
Przyszedł pan maruda niszczyciel dobrej zabawy |
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.
super, że Ci się udało zrobić pierwszą wersje, teraz czas na trochę refactoru i poprawek
cookies.delete({ | ||
name: "adonis-session", | ||
path: "/", | ||
}); | ||
cookies.delete({ | ||
name: "token", | ||
path: "/", | ||
}); |
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.
dlaczego to nie jest obsługiwane przez adonisa
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.
bo to jest po serverside, i to sie automatycznie nie ogarnia, tak samo jak musze recznie je ustawic i przekazywac
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.
ale jeszcze sie przyjrze
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.
no, ustawilem credentials include i nie przypisuje autmoayucznie cookiesow wiec musze recznie ustawiac i usuwac
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.
adonis powinien to ogarnąć, tylko w auth_controller jest tylko response.clearCookie('token')
. Jest w to w destroy()
w auth_controller.
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.
przykład z docsów adonisa: import router from '@adonisjs/core/services/router'
router.delete('cart', async ({ request, response }) => { response.clearCookie('cart_items') })
</div> | ||
</div> | ||
); | ||
return <PlansPage plans={Array.isArray(data) ? data : []} />; |
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.
dziwne, że data nie może być arrayem, plus brak error handlingu tutaj
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.
nie dziwne tylko to jest z twoim glupim lintem najlepszy i najkrotszy sposob, zeby sprawdzic czy dostalem tablice czy obiekt {error:bool, mess:string} (ktory jest bledem)
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.
dodaj error handling
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.
no ale po co, w sensie jak error czyli jesli nie jestes zalogowany to ma zwrocic pusty array, i tyle
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.
jest trochę wiecej rodzajów błędów niż to jak nie jesteś zalogowany 😭
|
||
export const PlanItem = ({ id, name }: { id: string; name: string }) => { | ||
export const PlanItem = ({ |
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.
ten komponent nadaje się już bardzo do refactoru 😭
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.
dobra dobra, jeszcze nie jest caly na czerwono xd
@@ -16,6 +16,7 @@ export function SignOutButton({ | |||
if (asChild) { | |||
const signOut = async () => { | |||
await signOutFunction(); | |||
localStorage.clear(); |
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.
odważne, żeby wipeować wszystko 😭
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.
wiem, ale chce usuwac wszystkie plany, bo sa one i tak online, moze dalej robic se lokalnie. i chcialem usuwac te plany ale jedyne co moglem to z plansv2 usunac a wszystkie plany i tak zostaja wiec uznalem ze na przyszlosc lepiej czyscic wszystko
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.
pytanie czy masz lepszy pomysl
backend: simple array front: - logout req - React.useState -> useState - better error response in plans functions
wiem bartus ze ci sie pewnie nie spodoba i chcialbys po prostu zrobic caly middleware w try catch i sama funkcja auth() jak w poprzednich tylko problem jest taki ze musze tak czy siak wykonac auth() zeby se uswierzytelnic za kazdym razem usera czy ma sesje a po tym doperio wykonac reszte logiki taka jak next jesli nie jest protected, wiec musialem tak zrobic
// List update logic: | ||
// Add unique registrations to the updatedRegistrations array | ||
// r - current registration | ||
// i - current index | ||
// a - array of registrations |
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.
krócej by Ci wyszło zmienić nazwy zmiennych 😭
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.
mozeeee, ale to jest takie ladneeeeeee
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.
🥺🥺🥺🥺🥺🥺🥺
if (payload.groups !== undefined) { | ||
await schedule.related('groups').sync(payload.groups.map((group) => group.id)) | ||
} | ||
|
||
if (payload.registrations !== undefined) { | ||
await schedule.related('registrations').sync(payload.registrations.map((group) => group.id)) | ||
} | ||
|
||
if (payload.courses !== undefined) { | ||
await schedule.related('courses').sync(payload.courses.map((group) => group.id)) | ||
} | ||
|
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.
Czemu już tutaj dodajemy groupsy, rejegistrations i courses do planu? Zamysł miałem taki, zeby po kliknięciu dodania planu na froncie szedł request po prostu z domyślną nazwą. Czemu trzeba było to zmienić?
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.
z prostego powodu, jak mam plan lokalny i chce stworzyc jego online wersje po zalogowaniu od razu moge to zrobic a nie robic dwa requesty
cookies.delete({ | ||
name: "adonis-session", | ||
path: "/", | ||
}); | ||
cookies.delete({ | ||
name: "token", | ||
path: "/", | ||
}); |
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.
adonis powinien to ogarnąć, tylko w auth_controller jest tylko response.clearCookie('token')
. Jest w to w destroy()
w auth_controller.
cookies.delete({ | ||
name: "adonis-session", | ||
path: "/", | ||
}); | ||
cookies.delete({ | ||
name: "token", | ||
path: "/", | ||
}); |
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.
przykład z docsów adonisa: import router from '@adonisjs/core/services/router'
router.delete('cart', async ({ request, response }) => { response.clearCookie('cart_items') })
@olekszczepanowski i ja bardzo bym chcial zeby to bylo automatyczne ale niestety nie dodaje tego autmatycznie po wyslaniu requesta login (idk czemu, dalem credentials inlucde i wgl) ale server side do server side nie dziala i nie ustawia wiec robie to recznie, wiec adonis nie moze tego tez automatycznie usunac bo to ja dodaje jakby swoje cookiesy, i ja je przekazuje manulanie w requestach, meczylem sie pare godzin z tym zeby to bylo autmaotcyzne ale nie chcialo za nic dzialac wiec zrobilem recznie i tyle |
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.
Looks good to me, merge it
Great! Thanks for your review! Let's go 🚀 🚀 🚀 |
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.
Copilot reviewed 15 out of 30 changed files in this pull request and generated no suggestions.
Files not reviewed (15)
- frontend/package.json: Language not supported
- frontend/src/app/plans/edit/[id]/_components/SyncedButton.tsx: Evaluated as low risk
- frontend/src/actions/plans.ts: Evaluated as low risk
- frontend/src/app/api/callback/route.ts: Evaluated as low risk
- frontend/src/app/layout.tsx: Evaluated as low risk
- frontend/src/app/plans/_components/PlansPage.tsx: Evaluated as low risk
- frontend/src/app/plans/edit/[id]/_components/CreateNewPlanPage.tsx: Evaluated as low risk
- frontend/src/app/plans/edit/[id]/_components/SyncErrorAlert.tsx: Evaluated as low risk
- frontend/src/atoms/planFamily.ts: Evaluated as low risk
- frontend/src/components/ui/button.tsx: Evaluated as low risk
- .github/workflows/ci.yaml: Evaluated as low risk
- frontend/src/app/plans/preview/[id]/_components/SharePlanPage.tsx: Evaluated as low risk
- frontend/src/app/plans/edit/[id]/page.tsx: Evaluated as low risk
- backend/app/validators/schedule.ts: Evaluated as low risk
- frontend/src/components/PlanItem.tsx: Evaluated as low risk
Comments skipped due to low confidence (1)
backend/app/controllers/schedules_controller.ts:198
- The 'updatedAt' field is being updated with the current timestamp without checking if the new timestamp is different from the previous one. Ensure that 'updatedAt' is only updated if there is a significant change.
currSchedule.updatedAt = DateTime.fromISO(new Date().toISOString())
This pull request includes various changes across multiple files to enhance the functionality and performance of the application. The most significant changes are related to the
SchedulesController
, validation schemas, and frontend components for handling plans. Below is a summary of the most important changes:Backend Changes:
SchedulesController Enhancements:
groups
,registrations
, andcourses
in thecreate
method (backend/app/controllers/schedules_controller.ts
).id
,userId
,createdAt
,updatedAt
) in the transformed schedule object (backend/app/controllers/schedules_controller.ts
).update
method in a try-catch block to handle errors more gracefully and updated theupdatedAt
field with the current timestamp (backend/app/controllers/schedules_controller.ts
) [1] [2].Validation Schema Updates:
groups
,courses
, andregistrations
to thecreateScheduleValidator
(backend/app/validators/schedule.ts
).updatedAt
field type fromdate
tostring
in theupdateScheduleValidator
(backend/app/validators/schedule.ts
).Frontend Changes:
Plan Management:
plans.ts
(frontend/src/actions/plans.ts
).NEXT_PUBLIC_API_URL
to the environment variables in the CI workflow (.github/workflows/ci.yaml
).UI Enhancements:
Toaster
component for notifications in the root layout (frontend/src/app/layout.tsx
) [1] [2].PlansPage
component to manage and display plans with Jotai state management (frontend/src/app/plans/_components/PlansPage.tsx
).Refactoring and Optimization:
CreateNewPlanPage
component to handle both creation and editing of plans (frontend/src/app/plans/edit/[id]/_components/CreateNewPlanPage.tsx
) (frontend/src/app/plans/edit/[id]/_components/CreateNewPlanPage.tsxR4-R11, frontend/src/app/plans/edit/[id]/_components/CreateNewPlanPage.tsxR34-R36, frontend/src/app/plans/edit/[id]/_components/CreateNewPlanPage.tsxR67-R151, frontend/src/app/plans/edit/[id]/_components/CreateNewPlanPage.tsxL72-R160, frontend/src/app/plans/edit/[id]/_components/CreateNewPlanPage.tsxR171-R197).logout.ts
to call the API for user logout (frontend/src/actions/logout.ts
) [1] [2].These changes collectively improve the robustness and user experience of the application by ensuring data consistency, enhancing error handling, and providing a more interactive UI.