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

team-3/tasks-workpackage #12

Open
enes-salimoski wants to merge 26 commits from team-3/tasks-workpackage into develop
pull from: team-3/tasks-workpackage
merge into: HNNW:develop
Conversation 4 Commits 26 Files changed 16 +388 -51
enes-salimoski commented 2026-09-25 10:59:06 +00:00
Member
Copy link
No description provided. 
Reviewed-on: #8
Reviewed-on: #9
Reviewed-on: #10
Reviewed-on: #11
rileydeman requested changes 2026-10-02 09:58:58 +00:00
Dismissed
rileydeman left a comment
Copy link

Goed dat de ChangeWorkPackageStatusHandler nu echt is uitgewerkt, met een workflow-check, en dat findActiveParticipant() een implementatie heeft. Die had eerst een lege body. Ook de toegevoegde UpdateWorkPackageDTO lost een ontbrekende class in develop op. Dit moet nog aangepast worden:

Moet opgelost worden

  1. .env.local.example is verwijderd. Dat hoort niet in deze PR, en nieuwe ontwikkelaars hebben dat bestand nodig om te installeren. Zet het terug.
  2. De settings-pagina post naar account.modify, maar modify() overschrijft alle velden:
    • laat je voornaam of achternaam leeg, dan wordt die een lege string;
    • display_name, location en bio worden null;
    • avatar_url staat niet in het formulier en wordt leeg gemaakt;
    • email_notifications_enabled staat niet in het formulier en wordt false.
      De tekst "Laat een veld leeg als je die gegevens niet wilt wijzigen" klopt dus niet, en gebruikers verliezen hun gegevens. Vul de velden vooraf in met de huidige waardes, of maak een eigen handler die alleen ingevulde velden aanpast. Er zit ook geen CSRF-token in het formulier.
  3. De CSS-link ../../../public/assets/account-centre/css/settings.css werkt niet in de browser. Gebruik {{ asset('assets/account-centre/css/settings.css') }}.
  4. In _header.html.twig staan nu drie <link>-stylesheets. main.base.html.twig laadt die al, dus ze worden dubbel geladen, en dan ook nog in de body. Haal ze weg.

Verbeterpunten
5. findActiveParticipant() controleert niet of de deelnemer echt actief is (leftAt IS NULL en/of de status), terwijl de naam dat wel zegt. Is iemand twee keer deelnemer geweest, dan gooit getOneOrNullResult() bovendien een NonUniqueResultException.
6. CurrentProfileProvider gaat van Security naar de sessie (account_id). Dat past bij de huidige login, maar PR #15 (team 6) zet de login om naar Symfony Security, en dan werkt dit niet meer. Stem dit af met team 6.
7. ChangeWorkPackageStatusHandler wordt nog nergens aangeroepen. Bij gebruik moet er ook gecontroleerd worden wie de status mag wijzigen.
8. De wijziging in core.css (margin van het logo van 100px naar 10px) geldt voor de hele site. Stem dat af.
9. De wijziging in WorkPackage.php staat al in develop (040225e). Merge develop in je branch, dan verdwijnt hij uit de diff.
10. In tasks.html.twig zijn de SEO-blocks weggehaald. Laat die staan, net als op de andere pagina's.

Goed dat de `ChangeWorkPackageStatusHandler` nu echt is uitgewerkt, met een workflow-check, en dat `findActiveParticipant()` een implementatie heeft. Die had eerst een lege body. Ook de toegevoegde `UpdateWorkPackageDTO` lost een ontbrekende class in develop op. Dit moet nog aangepast worden: **Moet opgelost worden** 1. `.env.local.example` is verwijderd. Dat hoort niet in deze PR, en nieuwe ontwikkelaars hebben dat bestand nodig om te installeren. Zet het terug. 2. De settings-pagina post naar `account.modify`, maar `modify()` overschrijft alle velden: - laat je voornaam of achternaam leeg, dan wordt die een lege string; - `display_name`, `location` en `bio` worden `null`; - `avatar_url` staat niet in het formulier en wordt leeg gemaakt; - `email_notifications_enabled` staat niet in het formulier en wordt `false`. De tekst "Laat een veld leeg als je die gegevens niet wilt wijzigen" klopt dus niet, en gebruikers verliezen hun gegevens. Vul de velden vooraf in met de huidige waardes, of maak een eigen handler die alleen ingevulde velden aanpast. Er zit ook geen CSRF-token in het formulier. 3. De CSS-link `../../../public/assets/account-centre/css/settings.css` werkt niet in de browser. Gebruik `{{ asset('assets/account-centre/css/settings.css') }}`. 4. In `_header.html.twig` staan nu drie `<link>`-stylesheets. `main.base.html.twig` laadt die al, dus ze worden dubbel geladen, en dan ook nog in de body. Haal ze weg. **Verbeterpunten** 5. `findActiveParticipant()` controleert niet of de deelnemer echt actief is (`leftAt IS NULL` en/of de status), terwijl de naam dat wel zegt. Is iemand twee keer deelnemer geweest, dan gooit `getOneOrNullResult()` bovendien een NonUniqueResultException. 6. `CurrentProfileProvider` gaat van Security naar de sessie (`account_id`). Dat past bij de huidige login, maar PR #15 (team 6) zet de login om naar Symfony Security, en dan werkt dit niet meer. Stem dit af met team 6. 7. `ChangeWorkPackageStatusHandler` wordt nog nergens aangeroepen. Bij gebruik moet er ook gecontroleerd worden wie de status mag wijzigen. 8. De wijziging in `core.css` (margin van het logo van 100px naar 10px) geldt voor de hele site. Stem dat af. 9. De wijziging in `WorkPackage.php` staat al in develop (040225e). Merge develop in je branch, dan verdwijnt hij uit de diff. 10. In `tasks.html.twig` zijn de SEO-blocks weggehaald. Laat die staan, net als op de andere pagina's.
Reviewed-on: #38
Reviewed-on: #39
Feat: Force merge because of merge conflicts (PR #40).
# Conflicts:
#	templates/pages/account-centre/index.html.twig
Reviewed-on: #41
Reviewed-on: #42
rileydeman requested changes 2026-10-12 12:56:51 +00:00
rileydeman left a comment
Copy link

Goed opgepakt: .env.local.example is terug, de SEO-blocks staan er weer, de dubbele stylesheets in _header.html.twig zijn weg, en de CurrentProfileProvider werkt nu met zowel Symfony Security als de huidige sessie-login. Daarmee is de afstemming met team 6 (punt 6) opgelost. Een paar punten staan nog open.

Moet opgelost worden

  1. (punt 2 vorige review) De settings-pagina post nog steeds naar account.modify, en die overschrijft alle velden. Laat je voornaam of achternaam leeg, dan wordt die een lege string. display_name, location en bio worden null, avatar_url wordt leeg gemaakt en email_notifications_enabled wordt false. De tekst "Laat een veld leeg als je die gegevens niet wilt wijzigen" klopt dus nog steeds niet. Vul de velden vooraf in met de huidige waardes (de settings()-controller geeft nu niets mee), of maak een eigen handler die alleen ingevulde velden aanpast. Voeg ook een CSRF-token toe en controleer het.
  2. (punt 3) settings.html.twig laadt de CSS nog via ../../../public/assets/account-centre/css/settings.css. Dat pad werkt niet in de browser. Gebruik {{ asset('assets/account-centre/css/settings.css') }}.

Verbeterpunten
3. (punt 8) core.css wordt nu ook in ac.base.html.twig geladen, en de logo-margin is daarin van 100px naar 10px gezet. Die wijziging geldt voor de hele site, op het Account Centre én de main site. Stem dat af of zet het in een eigen class.
4. account-centre/index.html.twig laadt nu assets/main/css/home.css, een stylesheet van de main site. Is dat bewust? Zo niet: zet de benodigde styles in de account-centre-CSS.
5. (punt 5) findActiveParticipant() controleert nog steeds niet of de deelnemer actief is (leftAt IS NULL), en kan een NonUniqueResultException geven als iemand twee keer deelnemer is geweest. PR #28 (team 5) voegt in dezelfde repository findLatestActiveParticipantByProjectAndProfile() toe, die dit wel goed doet. Stem af en gebruik één methode.
6. (punt 7) ChangeWorkPackageStatusHandler wordt nog nergens aangeroepen. Als je hem gaat gebruiken, werkt hij ook niet met de huidige data: hij zoekt statussen met scope 'work_package', de WorkPackageWorkflowService verwacht namen als 'Draft'/'Open' (met hoofdletter), de fixtures maken scope 'WorkPackage' met naam 'active', en de WorkPackageCrudService gebruikt 'open' (kleine letters). Elke overgang wordt dus geweigerd. Kies één schrijfwijze voor scope en statusnamen en gebruik die overal.
7. (punt 9) Merge develop in je branch. De WorkPackage.php-wijziging staat al in develop en verdwijnt dan uit de diff.
8. In tasks.html.twig zijn de blocks stylesheets en javascript en de commentaarregels weggehaald. Laat die staan zoals op de andere pagina's. De titel is nu ook "Taken" zonder projectnaam.
9. Na inschrijven op de work-packages-pagina stuurt enrollTask() je nu door naar de takenpagina in plaats van terug naar work-packages. Is dat bewust?
10. De pagina skills.html.twig is alleen een lege opmaak ("Je hebt nog geen vaardigheden toegevoegd") zonder data. Prima als placeholder, maar zet in de PR-beschrijving dat het nog niet werkt.

Goed opgepakt: `.env.local.example` is terug, de SEO-blocks staan er weer, de dubbele stylesheets in `_header.html.twig` zijn weg, en de `CurrentProfileProvider` werkt nu met zowel Symfony Security als de huidige sessie-login. Daarmee is de afstemming met team 6 (punt 6) opgelost. Een paar punten staan nog open. **Moet opgelost worden** 1. (punt 2 vorige review) De settings-pagina post nog steeds naar `account.modify`, en die overschrijft alle velden. Laat je voornaam of achternaam leeg, dan wordt die een lege string. `display_name`, `location` en `bio` worden `null`, `avatar_url` wordt leeg gemaakt en `email_notifications_enabled` wordt `false`. De tekst "Laat een veld leeg als je die gegevens niet wilt wijzigen" klopt dus nog steeds niet. Vul de velden vooraf in met de huidige waardes (de `settings()`-controller geeft nu niets mee), of maak een eigen handler die alleen ingevulde velden aanpast. Voeg ook een CSRF-token toe en controleer het. 2. (punt 3) `settings.html.twig` laadt de CSS nog via `../../../public/assets/account-centre/css/settings.css`. Dat pad werkt niet in de browser. Gebruik `{{ asset('assets/account-centre/css/settings.css') }}`. **Verbeterpunten** 3. (punt 8) `core.css` wordt nu ook in `ac.base.html.twig` geladen, en de logo-margin is daarin van 100px naar 10px gezet. Die wijziging geldt voor de hele site, op het Account Centre én de main site. Stem dat af of zet het in een eigen class. 4. `account-centre/index.html.twig` laadt nu `assets/main/css/home.css`, een stylesheet van de main site. Is dat bewust? Zo niet: zet de benodigde styles in de account-centre-CSS. 5. (punt 5) `findActiveParticipant()` controleert nog steeds niet of de deelnemer actief is (`leftAt IS NULL`), en kan een `NonUniqueResultException` geven als iemand twee keer deelnemer is geweest. PR #28 (team 5) voegt in dezelfde repository `findLatestActiveParticipantByProjectAndProfile()` toe, die dit wel goed doet. Stem af en gebruik één methode. 6. (punt 7) `ChangeWorkPackageStatusHandler` wordt nog nergens aangeroepen. Als je hem gaat gebruiken, werkt hij ook niet met de huidige data: hij zoekt statussen met scope `'work_package'`, de `WorkPackageWorkflowService` verwacht namen als `'Draft'`/`'Open'` (met hoofdletter), de fixtures maken scope `'WorkPackage'` met naam `'active'`, en de `WorkPackageCrudService` gebruikt `'open'` (kleine letters). Elke overgang wordt dus geweigerd. Kies één schrijfwijze voor scope en statusnamen en gebruik die overal. 7. (punt 9) Merge develop in je branch. De `WorkPackage.php`-wijziging staat al in develop en verdwijnt dan uit de diff. 8. In `tasks.html.twig` zijn de blocks `stylesheets` en `javascript` en de commentaarregels weggehaald. Laat die staan zoals op de andere pagina's. De titel is nu ook "Taken" zonder projectnaam. 9. Na inschrijven op de work-packages-pagina stuurt `enrollTask()` je nu door naar de takenpagina in plaats van terug naar work-packages. Is dat bewust? 10. De pagina `skills.html.twig` is alleen een lege opmaak ("Je hebt nog geen vaardigheden toegevoegd") zonder data. Prima als placeholder, maar zet in de PR-beschrijving dat het nog niet werkt.
rileydeman commented 2026-10-13 07:13:33 +00:00
Owner
Copy link

Correctie op de vorige review:
Punt "3. (punt 8) core.css wordt nu ook in ac.base.html.twig geladen, en de logo-margin is daarin van 100px naar 10px gezet. Die wijziging geldt voor de hele site, op het Account Centre én de main site. Stem dat af of zet het in een eigen class." kan genegeerd worden, deze verandering is bedoeld en met toestemming express op groter schaal. Dit kan dus blijven.

Correctie op de vorige review: Punt "3. (punt 8) core.css wordt nu ook in ac.base.html.twig geladen, en de logo-margin is daarin van 100px naar 10px gezet. Die wijziging geldt voor de hele site, op het Account Centre én de main site. Stem dat af of zet het in een eigen class." kan genegeerd worden, deze verandering is bedoeld en met toestemming express op groter schaal. Dit kan dus blijven.
patrick requested changes 2026-10-13 09:52:17 +00:00
patrick left a comment
Copy link

Op dit moment is de settings pagina min of meer een copy van de account modify pagina, terwijl dit een aparte pagina is. Volgens nieuwe gekregen informatie van de stakeholders heeft de settings pagina de volgende requirments:

  1. Toon profiel openbaar ja/nee, optionele meldingen ja/nee en wachtwoord wijzigen. Toon effect van elke keuze.
  2. Wachtwoordwijziging vraagt huidig wachtwoord, nieuw wachtwoord en herhaling; een onjuist huidig wachtwoord wordt niet geaccepteerd.
  3. Na wachtwoordwijziging/reset alle bestaande sessies intrekken en opnieuw inloggen

In templates/pages/main/projects/tasks.html.twig worden twig functies gebruikt om informatie uit de database te halen, dit hoort niet. De informatie hoort via de controller gegeven te worden. Voor het het krijgen van de gebruikers profiel kan in de twig file 'app.user.profile' gebruikt worden zodra het loginsysteem overgezet is naar de symfony methode (pr #15).
Verder moet de pagina aan de volgende eisen voldoen:
Toon taak, work package, taak bechrijving, status, uitvoerder en eventuele deadline; filter op work package, status en ‘mijn taken’.

Ter infomarie zijn hier alvast de huidige eisen voor het maken van de skillpagina deze zullen later nog verder uitgewerkt worden:

Producteisen

  • P1: Zoek en selecteer skills uit de bestaande catalogus; kies niveau beginner, zelfstandig of gevorderd. Toevoegen, aanpassen en verwijderen zijn mogelijk.
  • P2: Dezelfde skill staat hooguit eenmaal in het eigen profiel; toon bij geen skills een uitnodiging om er één toe te voegen.

Technische eisen

  • T1: Gebruik een bestaande skillrelatie met unieke combinatie account/skill; niveau moet een toegestane keuze zijn.
  • T2: Een onbekende skill-ID of account-ID uit de payload geeft geen nieuwe catalogusitems of eigendomsoverdracht.

Acceptatiecriteria

  • A1: Toevoegen en een niveau wijzigen blijft na herladen zichtbaar; verwijderen verwijdert alleen de eigen koppeling.
  • A2: Dezelfde skill tweemaal toevoegen levert maximaal één koppeling op, ook bij dubbele verzending.
  • A3: Ongeldige skill of niveau wordt geweigerd;
Op dit moment is de settings pagina min of meer een copy van de account modify pagina, terwijl dit een aparte pagina is. Volgens nieuwe gekregen informatie van de stakeholders heeft de settings pagina de volgende requirments: 1. Toon profiel openbaar ja/nee, optionele meldingen ja/nee en wachtwoord wijzigen. Toon effect van elke keuze. 2. Wachtwoordwijziging vraagt huidig wachtwoord, nieuw wachtwoord en herhaling; een onjuist huidig wachtwoord wordt niet geaccepteerd. 3. Na wachtwoordwijziging/reset alle bestaande sessies intrekken en opnieuw inloggen In templates/pages/main/projects/tasks.html.twig worden twig functies gebruikt om informatie uit de database te halen, dit hoort niet. De informatie hoort via de controller gegeven te worden. Voor het het krijgen van de gebruikers profiel kan in de twig file 'app.user.profile' gebruikt worden zodra het loginsysteem overgezet is naar de symfony methode (pr #15). Verder moet de pagina aan de volgende eisen voldoen: Toon taak, work package, taak bechrijving, status, uitvoerder en eventuele deadline; filter op work package, status en ‘mijn taken’. Ter infomarie zijn hier alvast de huidige eisen voor het maken van de skillpagina deze zullen later nog verder uitgewerkt worden: **Producteisen** - **P1:** Zoek en selecteer skills uit de bestaande catalogus; kies niveau beginner, zelfstandig of gevorderd. Toevoegen, aanpassen en verwijderen zijn mogelijk. - **P2:** Dezelfde skill staat hooguit eenmaal in het eigen profiel; toon bij geen skills een uitnodiging om er één toe te voegen. **Technische eisen** - **T1:** Gebruik een bestaande skillrelatie met unieke combinatie account/skill; niveau moet een toegestane keuze zijn. - **T2:** Een onbekende skill-ID of account-ID uit de payload geeft geen nieuwe catalogusitems of eigendomsoverdracht. **Acceptatiecriteria** - **A1:** Toevoegen en een niveau wijzigen blijft na herladen zichtbaar; verwijderen verwijdert alleen de eigen koppeling. - **A2:** Dezelfde skill tweemaal toevoegen levert maximaal één koppeling op, ook bij dubbele verzending. - **A3:** Ongeldige skill of niveau wordt geweigerd;
This pull request doesn't have enough approvals yet. 0 of 2 approvals granted.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin team-3/tasks-workpackage:team-3/tasks-workpackage
git switch team-3/tasks-workpackage
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!12
No description provided.