feat: Add partners entity - #25
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Looks okay. Missing description for migration.
AI review points out a few issues that should be handled.
I also think the point about not doing the same thing in 3 different ways (delete confirm) is a good idea.
AI Review
Code Review: PR-25 — feature/8085-add-partners-entity
Scope reviewed: full develop...HEAD diff — 34 files, ~1350 insertions. Two independent passes; every finding below verified against the code.
What the branch adds: a Partner entity, admin CRUD at /admin/partners, a free-tagging partners field on the initiative form, partners in the CSV export, and a dialog-based delete confirmation
listing the initiatives a partner would be detached from.
Confirmed issues
A. Partners are not searchable — src/Repository/InitiativeRepository.php:32-58
search() builds i.id IN (SELECT …) subqueries for createdBy, tags, strategies, stakeholders, contacts, organizationalAnchoring and area. Partners were never added.
An initiative displays "Alexandra Instituttet" and exports it to CSV, but /initiatives?q=Alexandra returns nothing. Every other name-bearing collection is covered, so this reads as an oversight.
One line, mirroring the contacts subquery.
B. Assert\Valid on $partners can make an initiative silently unsaveable
src/Entity/Initiative.php:75 cascades validation into Partner, so violations surface at path partners[0].name. But PartnersTextType is a non-compound TextType, so ViolationMapper can't resolve
that path — and autosave is the only save path on the form. On 422, autosave_controller.js:131 shows just "Save failed — your changes are kept here" and never re-renders the form.
Net effect: paste a 300-character partner name and the initiative stops saving with no indication why. Better to validate names in the transformer/type so the error lands on the partners field
itself.
C. findOrCreate() + unique index is a new 500 path
Partner is the first entity both created inline from the autosaving form and backed by a single-column UNIQUE INDEX — Contact/Term have no unique constraint; Area/Department do but are never
created inline. So the lookup-then-insert in PartnerRepository::findOrCreate() now has a constraint to violate: two overlapping saves with the same new name → uncaught
UniqueConstraintViolationException. UniqueEntity doesn't help, since validation runs before either commit.
Structural points
- Third literal copy of the free-tagging plumbing. PartnersTextType is byte-for-byte ContactsTextType with the entity swapped; same for the transformers. With TermsTextType that's three copies
of ~130 lines. All three repositories already expose identical findOrCreate() + findAllOrdered(). The JS (assets/app.js:108-126) duplicates the same way. Mostly a refactor of pre-existing code —
probably a follow-up issue. - Delete confirmation is a third idiom, and hydrates late. Everything else uses onsubmit="return confirm(...)" or data-turbo-confirm. The new controller is well built, but the trigger is a real
type="submit" and the only guard is the Stimulus handler, which loads after parse — a click in the hydration window deletes unconfirmed, with no server-side check. The inline onsubmit it
replaces was active from parse time. - Index page scale. One per row with its full initiative list, plus findInitiativeUsage() materialising every pair. 200 partners × ~20 initiatives ≈ 4000 anchors on one page.
Solid, no action
Unidirectional ManyToMany + ON DELETE CASCADE handled deliberately and asserted from both sides (PartnerControllerTest.php:118); ULID-vs-raw-FK hydration in findInitiativeUsage() pinned by
tests; Assert\Url(protocols: ['http','https']) correct given the value renders as an href; comma ban protecting the transformer, with a test; CSV header/row in sync; 'partners' added to the
preload loop; symmetric da/en translations with plural syntax matching existing precedent; CHANGELOG per ITK convention; migration boilerplate consistent with the existing two.
Two things to confirm rather than fix
- #[IsGranted('ROLE_USER')] lets any editor delete a partner and detach it from every initiative system-wide. Matches contacts/departments/areas, so presumably intentional.
- Partners aren't in InitiativeFilterType and don't count toward COMPLETION_FIELDS — consistent with contacts, but flagging in case partner filtering was in 8085's scope.
| { | ||
| public function getDescription(): string | ||
| { | ||
| return ''; |
Findings: * Partners were exported and displayed but never added to the free-text search, unlike every other name-bearing collection. One subquery, mirroring contacts. * Assert\Valid on the partners association raised violations at partners[0].name — a path the single text input cannot render, on a form autosave never redraws. The transformer now checks the one thing this form can get wrong, so the error lands on the field itself. * The unique index on partner.name made lookup-then-insert a 500 waiting to happen, since partners are created inline from the autosaving form. Dropped in favour of the UniqueEntity check Contact and Term already rely on: the worst a race can now do is leave two rows to merge. * The migration says what it does. Delete confirmation was three idioms — onsubmit=confirm(), data-turbo-confirm, and the new dialog. Now one component, everywhere. Its trigger is no longer a submit button, so a click landing before Stimulus hydrates does nothing rather than deleting unguarded.
A TransformationFailedException desynchronised the form, so autosave — the initiative form's only save path — returned a bodiless 422 and showed the generic 'check the required fields' error. Nothing was missing and the field carried no error, so every later save failed with no way to see why.
Submitting from inside a confirm dialog cached the page with the dialog still open; restoring that snapshot rendered it inline, out of the top layer.
The note on Partner::$name claimed Contact and Term as precedent for skipping a unique index, but Term carries uniq_term_name_vocabulary while taking the same lookup-then-insert race, and UniqueEntity only guards the admin form. Drops a TODO from the delete docblock too.
Link to ticket
#8085
Description
Add a partner entity to the system, for coupling initiatives with the partners
collaborating on a project. Implemented with a multiselect and quick-add functionality.
The partner entity comes with a CRUD in the administration panel, for completing and managing existing partners. When a partner is "quick-added", only the name is filled out. The user will be nudget to fill out the remaining information about the partner via other sources (Blink or the "Dit igangværende arbejde" panel on the dashboard)
Upon deletion of a partner in the CRUD, display a prompt listing the initiatives that utilized said partner. Allow the user to delete the partner anyway, which will delete the relation on the given initiatives as well.
Screenshot of the result
N/A
Checklist