HNNW/core
Watch
28
2
Fork
You've already forked core
 0
Code Issues Pull requests 8 Releases Activity Actions

team-6/symfony-forms #15

Open
shondrique-jong wants to merge 23 commits from team-6/symfony-forms into develop
pull from: team-6/symfony-forms
merge into: HNNW:develop
Conversation 3 Commits 23 Files changed 36 +1161 -449
shondrique-jong commented 2026-09-25 11:38:39 +00:00
Member
Copy link
No description provided. 
Moet wachten op de database, geen ETA voor de database. Niet mijn taak.
Hierdoor kan ik niet testen.

Ik heb comment

Ondertussen zoek ik naar andere forms onder templates/pages/ die nog niet omgezet zijn naar symfony forms waar nodig.
de knoppen en verborgen _action velden zijn niet aangepast, die werkten al. id's die de javascript opzoekt heb ik niet veranderd.
Reviewed-on: #4
Reviewed-on: #7
Reviewed-on: #14
rileydeman requested changes 2026-10-02 09:57:25 +00:00
Dismissed
rileydeman left a comment
Copy link

De form types en DTO's zitten netjes in elkaar en de validatie staat nu op de goede plek. Ook goed dat de dd() uit de registratie weg is en dat de verificatiemail buiten de transactie wordt verstuurd. In deze vorm kan de PR alleen niet gemerged worden:

Moet opgelost worden

  1. De org-templates (create, modify, modify-participants, modify-work-packages) gebruiken nu form of taskForms, maar de controllers geven die niet mee (in de comments staat "taak 4"). Deze pagina's crashen dus met "Variable form does not exist". Ook de Org form types (CreateProjectType, ModifyProjectType, PackageTaskType, ParticipantRolesType, WorkPackageType) worden nergens gebruikt. Pas de controllers in deze PR aan, of zet de templates pas om als de controllers klaar zijn.
  2. Er zijn nu twee manieren om in te loggen. form_login met check_path: auth.login vangt de POST naar /login af, dus de POST-code in AuthController::login() draait nooit meer. Dat geldt voor password_verify, $account->login() en session->set('account_id'). Gevolgen: die code is dood, de laatste login wordt niet meer bijgewerkt, en code die account_id uit de sessie leest werkt niet meer. Dat laatste geldt ook voor andere teams, bijvoorbeeld de CurrentProfileProvider in PR #12. Kies één manier en haal de andere weg.
  3. Het loginformulier stuurt _csrf_token mee, maar enable_csrf: true staat niet bij form_login. Het token wordt dus niet gecontroleerd.
  4. Er staat geen logout in de firewall. Hoe log je uit met de nieuwe login?
  5. Availability: de insert gaat nog via raw SQL met hours_per_week en start_date. hours_per_week is gedropt in migratie Version20260914092926 en de entity gebruikt valid_from, day_of_week, enzovoort. Opslaan mislukt dus. Team 4 (PR #3) bouwt availability ook om. Stem af wie dit doet, zodat het niet twee keer gebeurt.
  6. Er zijn merge-conflicten in .env en config/packages/security.yaml. Merge develop in je branch. Develop heeft inmiddels zelf een app_user_provider.

Verbeterpunten
7. Op de modify-pagina kun je e-mailadres en weergavenaam niet meer wijzigen. In het oude formulier kon dat wel. Is dat bewust?
8. Notifications: newsletterEnabled schrijft naar hetzelfde veld als emailNotificationsEnabled. Daardoor lijkt het een aparte instelling die het niet is. Haal hem weg of geef hem een eigen veld.
9. DashboardController geeft lege arrays mee (logs, accounts, profiles) om de template werkend te krijgen. Dat hoort niet in deze PR.
10. Haal .php-version en de newline-wijziging in .env uit de PR.
11. Er staan nog method_exists(..., 'softDelete')-checks in. De klasse is bekend, dus die zijn niet nodig.

Let op: PR #28 (team 5) zet modify-participants om naar een Live Component. Stem af welke aanpak blijft.

De form types en DTO's zitten netjes in elkaar en de validatie staat nu op de goede plek. Ook goed dat de `dd()` uit de registratie weg is en dat de verificatiemail buiten de transactie wordt verstuurd. In deze vorm kan de PR alleen niet gemerged worden: **Moet opgelost worden** 1. De org-templates (`create`, `modify`, `modify-participants`, `modify-work-packages`) gebruiken nu `form` of `taskForms`, maar de controllers geven die niet mee (in de comments staat "taak 4"). Deze pagina's crashen dus met "Variable form does not exist". Ook de Org form types (CreateProjectType, ModifyProjectType, PackageTaskType, ParticipantRolesType, WorkPackageType) worden nergens gebruikt. Pas de controllers in deze PR aan, of zet de templates pas om als de controllers klaar zijn. 2. Er zijn nu twee manieren om in te loggen. `form_login` met `check_path: auth.login` vangt de POST naar /login af, dus de POST-code in `AuthController::login()` draait nooit meer. Dat geldt voor `password_verify`, `$account->login()` en `session->set('account_id')`. Gevolgen: die code is dood, de laatste login wordt niet meer bijgewerkt, en code die `account_id` uit de sessie leest werkt niet meer. Dat laatste geldt ook voor andere teams, bijvoorbeeld de CurrentProfileProvider in PR #12. Kies één manier en haal de andere weg. 3. Het loginformulier stuurt `_csrf_token` mee, maar `enable_csrf: true` staat niet bij `form_login`. Het token wordt dus niet gecontroleerd. 4. Er staat geen `logout` in de firewall. Hoe log je uit met de nieuwe login? 5. Availability: de insert gaat nog via raw SQL met `hours_per_week` en `start_date`. `hours_per_week` is gedropt in migratie Version20260914092926 en de entity gebruikt `valid_from`, `day_of_week`, enzovoort. Opslaan mislukt dus. Team 4 (PR #3) bouwt availability ook om. Stem af wie dit doet, zodat het niet twee keer gebeurt. 6. Er zijn merge-conflicten in `.env` en `config/packages/security.yaml`. Merge develop in je branch. Develop heeft inmiddels zelf een `app_user_provider`. **Verbeterpunten** 7. Op de modify-pagina kun je e-mailadres en weergavenaam niet meer wijzigen. In het oude formulier kon dat wel. Is dat bewust? 8. Notifications: `newsletterEnabled` schrijft naar hetzelfde veld als `emailNotificationsEnabled`. Daardoor lijkt het een aparte instelling die het niet is. Haal hem weg of geef hem een eigen veld. 9. `DashboardController` geeft lege arrays mee (`logs`, `accounts`, `profiles`) om de template werkend te krijgen. Dat hoort niet in deze PR. 10. Haal `.php-version` en de newline-wijziging in `.env` uit de PR. 11. Er staan nog `method_exists(..., 'softDelete')`-checks in. De klasse is bekend, dus die zijn niet nodig. Let op: PR #28 (team 5) zet `modify-participants` om naar een Live Component. Stem af welke aanpak blijft.
rileydeman requested changes 2026-10-12 12:56:19 +00:00
rileydeman left a comment
Copy link

Sinds de vorige review is enable_csrf: true toegevoegd aan form_login (punt 3 opgelost), staat er een logout in de firewall, en is de ontbrekende } in home.css gefixt. Verder is de branch niet veranderd. De punten hieronder staan dus nog open.

Moet opgelost worden

  1. (punt 1 vorige review) De org-templates (create, modify, modify-participants, modify-work-packages) gebruiken form/taskForms, maar de controllers geven die nog steeds niet mee. Die pagina's crashen met "Variable form does not exist". De Org form types worden ook nog nergens gebruikt.
  2. (punt 2) Er zijn nog steeds twee login-systemen. form_login vangt de POST naar /login af, dus AuthController::login() (met session->set('account_id')) draait nooit. Alle controllers in het Account Centre lezen account_id uit de sessie, dus na inloggen via form_login krijg je daar "Je moet ingelogd zijn". Daarnaast vereist de nieuwe access_control ROLE_USER voor ^/account, en wie via de oude sessie-login binnenkomt heeft die rol niet. Kies één systeem (bij voorkeur Symfony Security), haal de POST-code uit AuthController::login() en zet de controllers om naar $this->getUser() of de CurrentProfileProvider.
  3. De logout staat op path: account.signoff. De firewall vangt die route af, dus de signoff()-pagina van het Account Centre is niet meer te bereiken. Is "signoff" bedoeld als uitloggen of als account opzeggen? Maak een aparte route auth.logout (/logout) voor het uitloggen.
  4. (punt 5) Availability slaat nog op via raw SQL met hours_per_week/start_date. Die kolommen bestaan niet meer, dus opslaan mislukt. Team 4 (PR #3) bouwt dit ook om: stem af wie het doet.
  5. (punt 6) Er zijn nog steeds merge-conflicten met develop in .env en config/packages/security.yaml. Merge develop in je branch.

Verbeterpunten (nog open uit de vorige review)
6. E-mailadres en weergavenaam zijn niet meer te wijzigen op de modify-pagina.
7. newsletterEnabled schrijft naar hetzelfde veld als emailNotificationsEnabled.
8. DashboardController geeft lege arrays mee om de template werkend te krijgen.
9. .php-version en de newline-wijziging in .env staan nog in de PR.
10. Er staan nog method_exists(..., 'softDelete')-checks in.
11. De wijzigingen in security.yaml van ' naar " zijn alleen opmaak. Laat die weg, dan wordt het conflict met develop kleiner.

Let op: PR #37 bevat de logout-commit die al op deze branch staat. Die PR kan dicht. PR #12 (team 3) heeft de CurrentProfileProvider aangepast zodat hij met beide logins werkt. Stem met team 3 af zodra hier gekozen is.

Sinds de vorige review is `enable_csrf: true` toegevoegd aan `form_login` (punt 3 opgelost), staat er een `logout` in de firewall, en is de ontbrekende `}` in `home.css` gefixt. Verder is de branch niet veranderd. De punten hieronder staan dus nog open. **Moet opgelost worden** 1. (punt 1 vorige review) De org-templates (`create`, `modify`, `modify-participants`, `modify-work-packages`) gebruiken `form`/`taskForms`, maar de controllers geven die nog steeds niet mee. Die pagina's crashen met "Variable form does not exist". De Org form types worden ook nog nergens gebruikt. 2. (punt 2) Er zijn nog steeds twee login-systemen. `form_login` vangt de POST naar `/login` af, dus `AuthController::login()` (met `session->set('account_id')`) draait nooit. Alle controllers in het Account Centre lezen `account_id` uit de sessie, dus na inloggen via `form_login` krijg je daar "Je moet ingelogd zijn". Daarnaast vereist de nieuwe `access_control` `ROLE_USER` voor `^/account`, en wie via de oude sessie-login binnenkomt heeft die rol niet. Kies één systeem (bij voorkeur Symfony Security), haal de POST-code uit `AuthController::login()` en zet de controllers om naar `$this->getUser()` of de `CurrentProfileProvider`. 3. De logout staat op `path: account.signoff`. De firewall vangt die route af, dus de `signoff()`-pagina van het Account Centre is niet meer te bereiken. Is "signoff" bedoeld als uitloggen of als account opzeggen? Maak een aparte route `auth.logout` (`/logout`) voor het uitloggen. 4. (punt 5) Availability slaat nog op via raw SQL met `hours_per_week`/`start_date`. Die kolommen bestaan niet meer, dus opslaan mislukt. Team 4 (PR #3) bouwt dit ook om: stem af wie het doet. 5. (punt 6) Er zijn nog steeds merge-conflicten met develop in `.env` en `config/packages/security.yaml`. Merge develop in je branch. **Verbeterpunten** (nog open uit de vorige review) 6. E-mailadres en weergavenaam zijn niet meer te wijzigen op de modify-pagina. 7. `newsletterEnabled` schrijft naar hetzelfde veld als `emailNotificationsEnabled`. 8. `DashboardController` geeft lege arrays mee om de template werkend te krijgen. 9. `.php-version` en de newline-wijziging in `.env` staan nog in de PR. 10. Er staan nog `method_exists(..., 'softDelete')`-checks in. 11. De wijzigingen in `security.yaml` van `'` naar `"` zijn alleen opmaak. Laat die weg, dan wordt het conflict met develop kleiner. Let op: PR #37 bevat de logout-commit die al op deze branch staat. Die PR kan dicht. PR #12 (team 3) heeft de `CurrentProfileProvider` aangepast zodat hij met beide logins werkt. Stem met team 3 af zodra hier gekozen is.
patrick requested changes 2026-10-13 10:52:29 +00:00
patrick left a comment
Copy link

De 'getAuthenticatedAccount' en account check in 'AccountsController.php' is overbodig de security.yaml blokkerd all alle niet ingelogde gebruikers, dus account is nooit null.
Bij de availability pagina zijn er twee input velden zonder beschrijving verder is de tekst in de velden wit op een witte achtergrond dus niet leesbaar.
Bij de ervaringen pagina hoort de checkbox voor de optie "Ik ben momenteel werkzaam in deze functie" op dezelde rij als de label te staan.

De 'getAuthenticatedAccount' en account check in 'AccountsController.php' is overbodig de security.yaml blokkerd all alle niet ingelogde gebruikers, dus account is nooit null. Bij de availability pagina zijn er twee input velden zonder beschrijving verder is de tekst in de velden wit op een witte achtergrond dus niet leesbaar. Bij de ervaringen pagina hoort de checkbox voor de optie "Ik ben momenteel werkzaam in deze functie" op dezelde rij als de label te staan.
This pull request has changes conflicting with the target branch.
  • .env
  • config/packages/security.yaml
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-6/symfony-forms:team-6/symfony-forms
git switch team-6/symfony-forms
Sign in to join this conversation.
Reviewers
No reviewers
rileydeman
patrick
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
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!15
No description provided.