team-4/application-flow #3

Open
pritish-diepa wants to merge 16 commits from team-4/application-flow into develop
Member
No description provided.
rileydeman requested changes 2026-09-21 08:29:50 +00:00
Dismissed
rileydeman left a comment

/src/Module/AccountCentre/AccountsController.php:

  • Gebruikt nu "use App\Entity\Account\ProfileAvailability;", deze entity bestaat niet en resulteerd dus ook in een error. Dit moet dus veranderd worden naar de juiste class.
  • Routing "/availability/{id}/delete" is verwijderd, echter mag dit niet. Dit moet dus terug gezet worden.
  • Routing "/experience/{id}/delete" is verwijderd, echter mag dit niet. Dit moet dus terug gezet worden.
  • Routing "/notifications" is verwijderd, echter mag dit niet. Dit moet dus terug gezet worden.
  • Er wordt nu "entityManager->remove()" gebruikt, echter werken wij bij de meeste tabellen een soft delete, wat een functie is vanuit de Entity.
  • De save-formulieren hebben geen _token voor de nieuwe tokens save_availability / save_experience → elke opslag eindigt op "Ongeldig veiligheidstoken". - Alleen csrf_token('delete_availability' ~ id) staat in de template
  • De controller geeft defaultSchedule en profile niet meer mee, terwijl het template erover itereert ({% for key, day in defaultSchedule %}, regel 41) → leeg dagformulier. En het formulier post nog days_config[n][enabled]

Graag deze dingen aanpassen en goed zetten, enkel daarna zal deze PR gemerged worden!

/src/Module/AccountCentre/AccountsController.php: - Gebruikt nu "use App\Entity\Account\ProfileAvailability;", deze entity bestaat niet en resulteerd dus ook in een error. Dit moet dus veranderd worden naar de juiste class. - Routing "/availability/{id}/delete" is verwijderd, echter mag dit niet. Dit moet dus terug gezet worden. - Routing "/experience/{id}/delete" is verwijderd, echter mag dit niet. Dit moet dus terug gezet worden. - Routing "/notifications" is verwijderd, echter mag dit niet. Dit moet dus terug gezet worden. - Er wordt nu "entityManager->remove()" gebruikt, echter werken wij bij de meeste tabellen een soft delete, wat een functie is vanuit de Entity. - De save-formulieren hebben geen _token voor de nieuwe tokens save_availability / save_experience → elke opslag eindigt op "Ongeldig veiligheidstoken". - Alleen csrf_token('delete_availability' ~ id) staat in de template - De controller geeft defaultSchedule en profile niet meer mee, terwijl het template erover itereert ({% for key, day in defaultSchedule %}, regel 41) → leeg dagformulier. En het formulier post nog `days_config[n][enabled]` > Graag deze dingen aanpassen en goed zetten, enkel daarna zal deze PR gemerged worden!
patrick left a comment

Er moet nog veel worden veranderd vooral fouten door de laatste merge. Verder om conflicten met de development branch te voorkomen merg altijd de development branch in je eigen branch voordat je een pr doet.

Er moet nog veel worden veranderd vooral fouten door de laatste merge. Verder om conflicten met de development branch te voorkomen merg altijd de development branch in je eigen branch voordat je een pr doet.
@ -587,4 +300,0 @@
}
#[Route('/signoff', name: 'signoff', methods: ['GET'])]
public function signoff(): Response
Member

Weer een methode die is verwijderd.

Weer een methode die is verwijderd.
@ -504,4 +288,2 @@
}
#[Route('/notifications', name: 'notifications', methods: ['GET', 'POST'])]
public function notifications(Request $request, EntityManagerInterface $entityManager): Response
Member

Studenten mogen niet zelf een functie uit een controller verwijderen!

Studenten mogen niet zelf een functie uit een controller verwijderen!
@ -390,4 +223,2 @@
}
#[Route('/experience/{id}/delete', name: 'experience_delete', methods: ['POST'])]
public function deleteExperience(string $id, Request $request, EntityManagerInterface $entityManager): Response
Member

Studenten mogen niet zelf een functie uit een controller verwijderen!

Studenten mogen niet zelf een functie uit een controller verwijderen!
@ -270,4 +150,2 @@
return $this->render('pages/account-centre/availability.html.twig', [
'profile' => $profile,
'availabilities' => $availabilities,
'defaultSchedule' => $defaultSchedule
Member

Verwijder nooit zomaar waardes uit de render. deze waardes zijn nodig voor het laten zien van de pagina en worden gebruikt in availability.html.twig.

Verwijder nooit zomaar waardes uit de render. deze waardes zijn nodig voor het laten zien van de pagina en worden gebruikt in availability.html.twig.
@ -275,4 +153,2 @@
}
#[Route('/availability/{id}/delete', name: 'availability_delete', methods: ['POST'])]
public function deleteAvailability(string $id, Request $request, Connection $connection): Response
Member

Studenten mogen niet zelf een functie uit een controller verwijderen!

Studenten mogen niet zelf een functie uit een controller verwijderen!
@ -91,241 +91,83 @@ final class AccountsController extends AbstractController
}
#[Route('/availability', name: 'availability', methods: ['GET', 'POST'])]
public function availability(Request $request, EntityManagerInterface $entityManager, Connection $connection): Response
Member

Tijdens de commit "Merge branch 'team-4/Jayden-Muller/application-flow' and resolve conflicts" #a547ae5d1. Is heel veel van de code van availability veranderd ik weet niet of dit de bedoeling was. De vorige code ziet er beter uit aangezien die ook de aangemaakte DTO en enum gebruikt.

Tijdens de commit "Merge branch 'team-4/Jayden-Muller/application-flow' and resolve conflicts" #a547ae5d1. Is heel veel van de code van availability veranderd ik weet niet of dit de bedoeling was. De vorige code ziet er beter uit aangezien die ook de aangemaakte DTO en enum gebruikt.
@ -6,8 +6,8 @@ use App\Entity\Account\Account;
use App\Entity\Account\Profile;
use App\Entity\Account\AccountSetting;
use App\Entity\Account\ProfileExperience;
use App\Entity\Account\ProfileAvailability;
Member

ProfileAvailability bestaat niet er is alleen Availability.

ProfileAvailability bestaat niet er is alleen Availability.
@ -182,2 +113,2 @@
} catch (\Exception $e) {
$this->addFlash('error', 'Databasefout bij opslaan: ' . $e->getMessage());
// Verwijder eerst de oude beschikbaarheid voor dit profiel
$existingAvailabilities = $entityManager->getRepository(ProfileAvailability::class)->findBy(['profile' => $profile]);
Member

Gebruik altijd een orderby (2de parameter in deze functie) als je data uit de database in een bepaalde volgorde wil hebben. Als je dat niet gebruikt kan je niet verwachten dat de data automatisch in de oudste objecten als eerst terug geeft.

Gebruik altijd een orderby (2de parameter in deze functie) als je data uit de database in een bepaalde volgorde wil hebben. Als je dat niet gebruikt kan je niet verwachten dat de data automatisch in de oudste objecten als eerst terug geeft.
@ -183,1 +113,3 @@
$this->addFlash('error', 'Databasefout bij opslaan: ' . $e->getMessage());
// Verwijder eerst de oude beschikbaarheid voor dit profiel
$existingAvailabilities = $entityManager->getRepository(ProfileAvailability::class)->findBy(['profile' => $profile]);
foreach ($existingAvailabilities as $oldAvailability) {
Member

Het is niet logisch om de oude availabilities op deze manier te verwijderen. Ze hebben een validFrom en validUntil die bepaalt wanner ze geldig zijn, hierdoor kan het zijn dat je een availability verwijderd die langer geldig is dan de nieuwe. Verder wordt er ook niet naar de dag van de availability gekeken, dus als een gebruiker een nieuwe tijd voor dinsdag insteld dan wordt maandag verwijderd als dat de oudste availability is.

Het is niet logisch om de oude availabilities op deze manier te verwijderen. Ze hebben een validFrom en validUntil die bepaalt wanner ze geldig zijn, hierdoor kan het zijn dat je een availability verwijderd die langer geldig is dan de nieuwe. Verder wordt er ook niet naar de dag van de availability gekeken, dus als een gebruiker een nieuwe tijd voor dinsdag insteld dan wordt maandag verwijderd als dat de oudste availability is.
@ -186,0 +123,4 @@
$endTimeStr = $endTimes[$dayOfWeek] ?? '17:00';
$availability = new ProfileAvailability();
if (method_exists($availability, 'setId')) {
Member

Je weet de klasse van $availability dus je zou nooit een method_exists moeten doen aangezien je kan weten welke functies deze klasse heeft.

Je weet de klasse van $availability dus je zou nooit een method_exists moeten doen aangezien je kan weten welke functies deze klasse heeft.
@ -186,0 +124,4 @@
$availability = new ProfileAvailability();
if (method_exists($availability, 'setId')) {
$availability->setId(Uuid::v4());
Member

Entities van de database krijgen automatisch een id wanneer ze opgeslagen worden dus geef ze nooit zelf een id. Dat is ook de reden dat ze geen setId hebben.

Entities van de database krijgen automatisch een id wanneer ze opgeslagen worden dus geef ze nooit zelf een id. Dat is ook de reden dat ze geen setId hebben.
@ -186,0 +132,4 @@
$availability->setStartTime(new \DateTimeImmutable($startTimeStr));
$availability->setEndTime(new \DateTimeImmutable($endTimeStr));
if (method_exists($availability, 'setCreatedAt')) {
Member

Hetzelfde geldt ook voor createdAt. Deze wordt automatisch gezet wanneer het object wordt aangemaakt in de database.

Hetzelfde geldt ook voor createdAt. Deze wordt automatisch gezet wanneer het object wordt aangemaakt in de database.
rileydeman requested changes 2026-10-02 09:59:58 +00:00
Dismissed
rileydeman left a comment

Goed dat de feedback van de vorige review grotendeels is verwerkt:

  • er staat een orderBy bij applications;
  • de method_exists-check voor softDelete is weg;
  • ProfileAvailability is weg;
  • de delete-route is terug, met een eigenaarscheck;
  • er zijn geen controllerfuncties meer verwijderd;
  • de DTO, de enum en de handler worden nu gebruikt.

De beschikbaarheidspagina werkt alleen nog niet:

Moet opgelost worden

  1. availability() POST: foreach ($dayEnums as $dayNumber => $dayOfWeek), maar $dayEnums wordt nergens gedefinieerd. Er wordt dus nooit iets opgeslagen.
  2. Het GET-gedeelte gebruikt nog $connection->fetchAllAssociative(...), maar Connection $connection is uit de parameters gehaald, dus de pagina crasht. Die query leest bovendien kolommen die niet meer bestaan: hours_per_week is gedropt in Version20260914092926 en de entity gebruikt valid_from, day_of_week en start_time. Gebruik $availabilityRepository->findActiveForProfile($profile) en pas availability.html.twig daarop aan.
  3. Het oude blok met $activeDays, $totalHours en zo verder is dode code. $daysInput wordt ook twee keer opgehaald. Haal dat weg.
  4. $csrfToken wordt opgehaald, maar nooit gecontroleerd met isCsrfTokenValid().
  5. Er is een leeg bestand git toegevoegd in de root. Verwijder het.
  6. Er is een merge-conflict in composer.lock. Merge develop in je branch en voer daarna composer update symfony/object-mapper opnieuw uit.

Verbeterpunten
7. applications wordt nu wel meegegeven aan de template, maar applications.html.twig gebruikt nog profile.projectApplications. De orderBy heeft dus nog geen effect.
8. In notifications() staan nog method_exists($settings, 'setId') en setCreatedAt. Patrick heeft eerder uitgelegd dat die automatisch gezet worden. Haal ze weg.
9. AvailabilityService wordt nergens gebruikt. Haal hem weg of gebruik hem.
10. Elke keer opslaan maakt nieuwe records bij de bestaande. Bepaal hoe een bestaande beschikbaarheid voor dezelfde dag wordt afgesloten (bijvoorbeeld met validUntil), dan krijg je geen dubbele records.
11. Kleine punten: de inspringing bij #[Route('/availability' ...)] klopt niet, en de newline aan het eind van een aantal bestanden ontbreekt.

Let op: PR #15 (team 6) past dezelfde availability-code ook aan. Stem dat af.

Goed dat de feedback van de vorige review grotendeels is verwerkt: - er staat een orderBy bij applications; - de `method_exists`-check voor softDelete is weg; - `ProfileAvailability` is weg; - de delete-route is terug, met een eigenaarscheck; - er zijn geen controllerfuncties meer verwijderd; - de DTO, de enum en de handler worden nu gebruikt. De beschikbaarheidspagina werkt alleen nog niet: **Moet opgelost worden** 1. `availability()` POST: `foreach ($dayEnums as $dayNumber => $dayOfWeek)`, maar `$dayEnums` wordt nergens gedefinieerd. Er wordt dus nooit iets opgeslagen. 2. Het GET-gedeelte gebruikt nog `$connection->fetchAllAssociative(...)`, maar `Connection $connection` is uit de parameters gehaald, dus de pagina crasht. Die query leest bovendien kolommen die niet meer bestaan: `hours_per_week` is gedropt in Version20260914092926 en de entity gebruikt `valid_from`, `day_of_week` en `start_time`. Gebruik `$availabilityRepository->findActiveForProfile($profile)` en pas `availability.html.twig` daarop aan. 3. Het oude blok met `$activeDays`, `$totalHours` en zo verder is dode code. `$daysInput` wordt ook twee keer opgehaald. Haal dat weg. 4. `$csrfToken` wordt opgehaald, maar nooit gecontroleerd met `isCsrfTokenValid()`. 5. Er is een leeg bestand `git` toegevoegd in de root. Verwijder het. 6. Er is een merge-conflict in `composer.lock`. Merge develop in je branch en voer daarna `composer update symfony/object-mapper` opnieuw uit. **Verbeterpunten** 7. `applications` wordt nu wel meegegeven aan de template, maar `applications.html.twig` gebruikt nog `profile.projectApplications`. De orderBy heeft dus nog geen effect. 8. In `notifications()` staan nog `method_exists($settings, 'setId')` en `setCreatedAt`. Patrick heeft eerder uitgelegd dat die automatisch gezet worden. Haal ze weg. 9. `AvailabilityService` wordt nergens gebruikt. Haal hem weg of gebruik hem. 10. Elke keer opslaan maakt nieuwe records bij de bestaande. Bepaal hoe een bestaande beschikbaarheid voor dezelfde dag wordt afgesloten (bijvoorbeeld met `validUntil`), dan krijg je geen dubbele records. 11. Kleine punten: de inspringing bij `#[Route('/availability' ...)]` klopt niet, en de newline aan het eind van een aantal bestanden ontbreekt. Let op: PR #15 (team 6) past dezelfde availability-code ook aan. Stem dat af.
rileydeman left a comment

Goed dat de oude raw-SQL-query weg is en dat de GET nu $availabilityRepository->findActiveForProfile($profile) gebruikt. Ook de method_exists-checks in notifications() en de inspringing van de route zijn opgelost. De beschikbaarheidspagina werkt alleen nog steeds niet:

Moet opgelost worden

  1. (punt 1 vorige review) foreach ($dayEnums as $dayNumber => $dayOfWeek): $dayEnums wordt nog steeds nergens gedefinieerd. Er wordt dus nooit iets opgeslagen en je krijgt altijd "Selecteer minimaal één dag". Bouw de lijst bijv. op met DayOfWeek::cases() en koppel die aan de dagnummers 0-6 uit het formulier.
  2. De controller geeft defaultSchedule niet meer mee, maar availability.html.twig loopt er nog over ({% for key, day in defaultSchedule %}). De pagina crasht of toont een leeg dagformulier. Geef defaultSchedule weer mee, of bouw het in de template op.
  3. De tabel in de template gebruikt availability.daysSummary en availability.startDate. Die bestaan niet op de entity Availability (die heeft dayOfWeek, startTime, endTime, validFrom, validUntil). Zodra er één record is, crasht de pagina. Pas de template aan op de entity.
  4. (punt 4) $csrfToken wordt nog steeds opgehaald maar niet gecontroleerd. Voeg isCsrfTokenValid('save_availability', $csrfToken) toe, plus een _token-veld in het formulier.
  5. (punt 5) Het lege bestand git staat nog in de root. Verwijder het.
  6. (punt 6) Er is nog steeds een merge-conflict in composer.lock. Merge develop in je branch en voer composer update symfony/object-mapper opnieuw uit.

Verbeterpunten
7. (punt 3) Het oude blok met $activeDays, $startTime, $endTime en $totalHours is nog dode code, en $daysInput wordt nog twee keer opgehaald. Haal het eerste blok weg.
8. (punt 7) applications.html.twig gebruikt nog profile.projectApplications in plaats van de meegegeven applications.
9. (punt 9) AvailabilityService wordt nog steeds nergens gebruikt.
10. (punt 10) Elke keer opslaan maakt nog nieuwe records bij de bestaande. Sluit de bestaande beschikbaarheid voor dezelfde dag af (validUntil).
11. 'Fout bij opslaan: ' . $e->getMessage() laat interne foutmeldingen aan de gebruiker zien. Toon een algemene melding en log de details.

Let op: PR #15 (team 6) wijzigt dezelfde availability-code nog steeds (met de oude raw SQL). Stem af wie dit afmaakt.

Goed dat de oude raw-SQL-query weg is en dat de GET nu `$availabilityRepository->findActiveForProfile($profile)` gebruikt. Ook de `method_exists`-checks in `notifications()` en de inspringing van de route zijn opgelost. De beschikbaarheidspagina werkt alleen nog steeds niet: **Moet opgelost worden** 1. (punt 1 vorige review) `foreach ($dayEnums as $dayNumber => $dayOfWeek)`: `$dayEnums` wordt nog steeds nergens gedefinieerd. Er wordt dus nooit iets opgeslagen en je krijgt altijd "Selecteer minimaal één dag". Bouw de lijst bijv. op met `DayOfWeek::cases()` en koppel die aan de dagnummers 0-6 uit het formulier. 2. De controller geeft `defaultSchedule` niet meer mee, maar `availability.html.twig` loopt er nog over (`{% for key, day in defaultSchedule %}`). De pagina crasht of toont een leeg dagformulier. Geef `defaultSchedule` weer mee, of bouw het in de template op. 3. De tabel in de template gebruikt `availability.daysSummary` en `availability.startDate`. Die bestaan niet op de entity `Availability` (die heeft `dayOfWeek`, `startTime`, `endTime`, `validFrom`, `validUntil`). Zodra er één record is, crasht de pagina. Pas de template aan op de entity. 4. (punt 4) `$csrfToken` wordt nog steeds opgehaald maar niet gecontroleerd. Voeg `isCsrfTokenValid('save_availability', $csrfToken)` toe, plus een `_token`-veld in het formulier. 5. (punt 5) Het lege bestand `git` staat nog in de root. Verwijder het. 6. (punt 6) Er is nog steeds een merge-conflict in `composer.lock`. Merge develop in je branch en voer `composer update symfony/object-mapper` opnieuw uit. **Verbeterpunten** 7. (punt 3) Het oude blok met `$activeDays`, `$startTime`, `$endTime` en `$totalHours` is nog dode code, en `$daysInput` wordt nog twee keer opgehaald. Haal het eerste blok weg. 8. (punt 7) `applications.html.twig` gebruikt nog `profile.projectApplications` in plaats van de meegegeven `applications`. 9. (punt 9) `AvailabilityService` wordt nog steeds nergens gebruikt. 10. (punt 10) Elke keer opslaan maakt nog nieuwe records bij de bestaande. Sluit de bestaande beschikbaarheid voor dezelfde dag af (`validUntil`). 11. `'Fout bij opslaan: ' . $e->getMessage()` laat interne foutmeldingen aan de gebruiker zien. Toon een algemene melding en log de details. Let op: PR #15 (team 6) wijzigt dezelfde availability-code nog steeds (met de oude raw SQL). Stem af wie dit afmaakt.
This pull request has changes conflicting with the target branch.
  • composer.lock
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin team-4/application-flow:team-4/application-flow
git switch team-4/application-flow
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
HNNW/core!3
No description provided.