team-5/project-updates #46
No reviewers
patrick
Labels
No labels
No milestone
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
HNNW/core!46
Loading…
Reference in a new issue
No description provided.
Delete branch "team-5/project-updates"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Component voor modify updates en geplaats in de page.
import './styles/app.css'; heb ik weer verwijderd uit assets/app.js
geautoriseerde leden van een project kunnen updates van het project bewerken, verwijderen en herstellen.
Deze PR is afhankelijk van PR #28
Goed begin met de project-updates als Live Component. De opzet met een aparte handler, een eigen repository en de soft delete via
softDelete()op de entity klopt, en het weghalen vanimport './styles/app.css'is terecht. Er zitten alleen nog een paar bugs in waardoor het wijzigen van updates niet goed werkt.Moet opgelost worden
ModifyProjectUpdatesHandler::handleModifyUpdate()klopt niet:elseif ($isPublic && $update->isPublic())roeptmakePrivate()aan. Vink je "Public" aan bij een update die al publiek is, dan wordt hij privé. Vink je hem uit, dan gebeurt er niets. Dit moetelseif (!$isPublic && $update->isPublic())zijn.updateMapwordt alleen gevuld voor velden die je aanraakt (data-model="norender|..."). Pas je alleen de titel aan, dan ontbreektcontent, faalt deisset()en wordt er stil niets opgeslagen. Pas je de checkbox niet aan, dan ispublicleeg en wordt dat alsfalsegezien. Met de fix van punt 1 maak je zo elke publieke update privé zodra je alleen de tekst wijzigt. VulupdateMapbij het mounten met de huidige waardes van elke update (titel, content, public).deleteProjectUpdateById()enrestoreProjectUpdateById()doen geenflush(). De soft delete wordt dus nooit opgeslagen. Er staat ook geen check opnullin (onbekend ID geeft een fatal error).$this->projectIdhoort. MetPROJECT_EDITop je eigen project kun je via een aangepast request updates van elk ander project verwijderen. Doe dezelfde check als inhandleModifyUpdate()($update->getProject() !== $project).isGranted('PROJECT_EDIT', ...)werkt alleen als er een voter is die dat attribuut afhandelt. Op develop bestaat die niet. DeProjectVoterkomt pas met PR #28, en daar is hij nogabstract, dus hij wordt niet geregistreerd. Zonder voter is het antwoord altijd "nee", dus niemand kan updates wijzigen. Zorg dat #28 eerst (werkend) gemerged wordt, of neem dat op in deze PR, en zet in de PR-beschrijving dat deze PR van #28 afhankelijk is.Verbeterpunten
6. Verwijderde updates blijven in de lijst staan:
getProjectUpdates()gebruiktfindBy(['project' => ...])zonder filter opdeletedAt. Er is ook geen knop voorrestoreProjectUpdatein de template. Filter verwijderde updates eruit, of toon ze apart met een herstelknop.7.
getProject(): projectgeeft een TypeError als het project niet bestaat (find()geeftnull). SchrijfProjectmet een hoofdletter, en handelnullaf (bijv. eenNotFoundHttpException).8.
$errorMessageis geenLiveProp, dus de foutmelding verdwijnt meteen weer na de re-render. Maak er een (niet-writable)LivePropvan.9.
{{ importmap('app') }}staat nu ín{% block javascript %}vanorg.base.html.twig. Elke pagina die dat block overschrijft (de meeste org-pagina's hebben{% block javascript %}{% endblock %}) verliest dan de importmap, en daarmee de Live Components. Zetimportmap('app')buiten het block in de layout en haal hem uitmodify-updates.html.twig. PR #28 doet dezelfde wijziging in de layout: stem dat af.10. Kleine punten: de docblock boven
handleModifyUpdate()gaat nog over$roleMap, er staan lege regels en een lege<thead>, ongebruikte imports (ProjectUpdate,LivePropin de handler), en de opmerkingen// throw an exception(dan ook echt doen of weghalen). VoegrepositoryClass: ProjectUpdateRepository::classtoe aan#[ORM\Entity]inProjectUpdate.11. Er staat geen beschrijving in de PR. Zet erin wat de PR doet en hoe je het test.
Ik heb alle verzoeken verwerkt en aangepast.
Goed opgepakt: de publiek/privé-logica klopt nu, verwijderen en herstellen worden echt opgeslagen (met
flush()) en controleren of de update bij het project hoort, enimportmap('app')staat nu netjes buiten het block in de layout. Ook deNotFoundHttpExceptioningetProject(), deLivePropop$errorMessageen de PR-beschrijving zijn in orde.Vorige review
ProjectVoternog steedsabstract. Zolang dat zo is, geeftisGranted('PROJECT_EDIT', ...)altijd "nee". Deze PR mag dus pas gemerged worden nadat een werkende #28 erin zit.Moet opgelost worden
updateMapwordt nog steeds alleen gevuld voor velden die je aanraakt. Het weghalen vannorender|verandert daar niets aan. Pas je alleen de titel aan, dan ontbreektcontenten slaathandleModifyUpdate()stil niets op. Pas je de tekst aan maar niet de checkbox, dan ontbreektpublicen wordt datfalse. Door de (terechte) fix van punt 1 wordt een publieke update dan privé zodra je alleen de tekst wijzigt. VulupdateMapbij het mounten (bijv. in eenmount()- of#[PostMount]-methode) met de huidigetitle,contentenpublicvan elke update. Dan zijn alle velden altijd aanwezig.Verbeterpunten
2. (punt 6 vorige review) Verwijderde updates hebben nu een Restore-knop, maar ze staan nog gewoon tussen de andere updates, met een Remove- en Update-knop en zonder dat je ziet dat ze verwijderd zijn. Toon ze apart (of markeer ze duidelijk) en verberg Remove/Update als
update.deletedAtgezet is.3. (punt 10 vorige review) In
deleteProjectUpdateById()enrestoreProjectUpdateById()staat nog// throw an exception. Gooi dan ook echt een exception of haal de opmerking weg. Geef$projectUpdateIden$projectook een type (string,Project).repositoryClass: ProjectUpdateRepository::classontbreekt nog in#[ORM\Entity]vanProjectUpdate, en in de component staan nog veel lege regels.4. Mislukt een wijziging (bijv. update niet gevonden), dan gebeurt er nu stil niets. Zet in dat geval ook
$errorMessage, zodat de gebruiker ziet dat het niet gelukt is.Let op: PR #28 wijzigt dezelfde regel in
org.base.html.twig, maar zetimportmap('app')daar ín{% block javascript %}. Jouw versie (buiten het block) is de goede. Stem met team #28 af dat zij jouw versie overnemen, anders krijg je bij het mergen een conflict of komt het probleem terug.De updateMap van ModifyProjectUpdatesComponent is altijd een lege array, omdat wanneer de mount functie wordt aangeroepen projectId nog geen waarde heeft. Gebruik hierom de postMount functie inplaats van de mount functie(https://symfony.com/bundles/ux-twig-component/current/index.html#postmount-hook).
Bij de twig ModifyProjectUpdatesComponent staat update.deletedAt verkeerd gespelt, er mist een "d", op de regel ' Project delete at:{{ update.deleteAt ? update.deleteAt|date('d/m/Y') : '-' }}' hierdoor krijg je een uitzondering als een update is verwijderdt.
Er mist nu nog steeds een "d" bij de eerste update.deletedAt in de regel ' Project delete at:{{ update.deleteAt ? update.deletedAt|date('d/m/Y') : '-' }}'.
Verder wordt de functie mount nu zowel als mount(vanwege de naam) en postmount(vanwege de attribute postMount) aangeroepen. Hierdoor wordt er een extra onnodige keer naar projecten gezocht. Dus verander de naam van de functie zodat het niet meer als mount wordt aangeroepen bijvoorbeeld naar postMount.
Goed opgelost: de methode heet nu
postMount()met#[PostMount], dusupdateMapwordt gevuld nadatprojectIdgezet is. Enupdate.deleteAtis overalupdate.deletedAt, dus de pagina crasht niet meer bij verwijderde updates. De twee grootste problemen zijn daarmee weg.Vorige review
}}staat er nog.Dit moet nog aangepast worden:
Moet opgelost worden
ModifyProjectUpdatesComponent.html.twig(regel 15) staat na de datum nog een losse}}. Die verschijnt letterlijk als tekst op de pagina. Binnen{% if update.deletedAt %}is de datum altijd gezet, dus{{ update.deletedAt|date('d/m/Y') }}is genoeg.handleModifyUpdate(),deleteProjectUpdateById()enrestoreProjectUpdateById()inModifyProjectUpdatesHandlerdoen stilreturnals de update niet bestaat of bij een ander project hoort. De gebruiker klikt dan en er gebeurt niets, zonder melding. Laat zefalseteruggeven of een exception gooien, zodat de component$errorMessagekan zetten.#[ORM\Entity]opProjectUpdatemistrepositoryClass: ProjectUpdateRepository::class. Zonder die koppeling geeft$entityManager->getRepository(ProjectUpdate::class)een standaard repository zonder jouw eigen methodes. Haal ook de vijf lege regels bovenaan de component weg.Verbeterpunten
4. (punt 3 vorige review) "Project delete at:" gaat over de update, niet over het project. Bijvoorbeeld "Deleted at:".
5. (punt 4 vorige review) Het
{% else %}-blok met "No actions available" wordt nooit bereikt. Maak van{% elseif update.deletedAt %}een{% else %}en haal het laatste blok weg.Let op: deze PR hangt af van PR #28 (
ProjectVotervoorPROJECT_EDIT), dus merge hem pas nadat #28 erin zit. #28 zetimportmap('app')inorg.base.html.twignog ín{% block javascript %}; dat geeft een merge-conflict met jouw versie (buiten het block). Neem bij het oplossen jouw versie aan.Goed opgepakt: de handler gooit nu een
InvalidArgumentExceptionin plaats van stil te returnen, en de component vangt die op en zet$errorMessage. De gebruiker krijgt dus eindelijk een melding. Ook de losse}}, de tekst "Deleted at:", de lege regels en het overbodige{% else %}-blok zijn netjes aangepakt.Vorige review
repositoryClassontbreekt nog.{% endif %}ook weggehaald (zie punt 1 hieronder).Dit moet nog aangepast worden:
Moet opgelost worden
ModifyProjectUpdatesComponent.html.twigheb je bij het weghalen van het "No actions available"-blok ook de{% endif %}verwijderd. De{% if not update.deletedAt %}op regel 38 wordt nu nergens afgesloten. Twig geeft dan een syntax error en de hele pagina laadt niet meer. Zet{% endif %}terug na de Restore-knop, vóór</td>.#[ORM\Entity]opProjectUpdatemist nog steedsrepositoryClass: ProjectUpdateRepository::class. Zonder die koppeling geeft$entityManager->getRepository(ProjectUpdate::class)een standaard repository zonder jouw eigen methodes. Maak er#[ORM\Entity(repositoryClass: ProjectUpdateRepository::class)]van.$errorMessagewordt nooit leeggemaakt. Gaat één actie mis, dan blijft die foutmelding staan, ook nadat de volgende actie wel lukt. Zet$this->errorMessage = '';aan het begin vanupdateProjectUpdate(),deleteProjectUpdate()enrestoreProjectUpdate().Verbeterpunten
4. In
ModifyProjectUpdatesHandlerstaatuse http\Exception;. Die wordt nergens gebruikt (je gooit\InvalidArgumentException) enhttp\Exceptionhoort bij een PHP-extensie die we niet gebruiken. Haal die regel weg.5.
isset($updateMap['title'], $updateMap['content'])laat een lege string gewoon door, terwijl de melding "Title and Content cant be empty" zegt. Wil je lege velden echt tegenhouden, check dan ook optrim(...) === ''.6. Op regel 15 van de template is
update.deletedAt ? ... : '-'niet nodig, want binnen{% if update.deletedAt %}is de datum altijd gezet.{{ update.deletedAt|date('d/m/Y') }}is genoeg. Haal ook de dubbele lege regels na detry/catch-blokken weg.Let op: deze PR hangt nog steeds af van PR #28 (
ProjectVotervoorPROJECT_EDIT). Merge hem pas nadat #28 erin zit. Met de huidige develop geeft de proefmerge geen conflicten.Alle punten uit de vorige review zijn opgepakt. De
{% endif %}staat weer op zijn plek,ProjectUpdateheeft nurepositoryClass: ProjectUpdateRepository::class, en$errorMessagewordt aan het begin van elke actie leeggemaakt. Ook de lege-velden-check mettrim(), de weggehaaldeuse http\Exception;en de opgeschoonde template zijn netjes.Vorige review
Er is nog één klein ding, daarna kan hij erin:
Moet opgelost worden
ModifyProjectUpdatesComponent.html.twigstaanCreated by:{{ update.author.displayName }}enDeleted at:{{ update.deletedAt|date('d/m/Y') }}zonder spatie na de dubbele punt. Op de pagina zie je dan "Created by:Jan" en "Deleted at:14/10/2026". Zet er een spatie tussen:Created by: {{ ... }}enDeleted at: {{ ... }}.Verbeterpunten
2. Kleine opmaak:
restoreProjectUpdate( #[LiveArg] string $id)heeft een spatie te veel na de(, en in de component en de handler staan nog een paar dubbele lege regels (na$errorMessage, na de constructor, na$this->em->flush()). Laat eventueel PHP-CS-Fixer erover lopen.Let op: deze PR gebruikt
PROJECT_EDITuit deProjectVotervan PR #28. Die staat nog niet op develop, dus merge deze PR pas nadat #28 erin zit. Ik heb het getest: eerst #28 en daarna deze PR mergen gaat zonder conflicten (deimportmap('app')-regel inorg.base.html.twigvalt samen).View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.