Simplify adding users to quotes and invoices #1668
Conversation
WalkthroughThis update refactors the user-contact relationship model for invoices, recurring invoices, and quotes. It removes intermediary entities ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form
participant DynamicFormBuilder
participant DB
User->>Form: Open Invoice/Quote form
Form->>DynamicFormBuilder: Build form
DynamicFormBuilder->>DB: Fetch clients
User->>Form: Select client
DynamicFormBuilder->>DB: Fetch contacts for selected client
Form->>User: Show users field with contacts
User->>Form: Select users (contacts)
Form->>DB: Save Invoice/Quote with selected contacts (many-to-many)
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Bundle ReportChanges will decrease total bundle size by 407.05kB (-5.1%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: solidinvoice-webpack-bundle-array-pushAssets Changed:
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/InvoiceBundle/Form/Type/RecurringInvoiceType.php (1)
51-61: Consider using a query-builder instead of eager loading all clientsFetching all clients via
findAll()loads every record into memory, which can become costly once the client table grows.
Prefer passing aquery_builderclosure so Doctrine can stream results or paginate if needed. This is consistent with the pattern already used for theusersfield below.- 'choices' => $this->registry->getRepository(Client::class)->findAll() + 'query_builder' => fn (EntityRepository $r) => $r->createQueryBuilder('c')->orderBy('c.name', 'ASC')src/InvoiceBundle/Form/Type/InvoiceType.php (1)
54-55: Minor: re-using the$buildervar hides the original builder
new DynamicFormBuilder($builder)returns a decorator; shadowing the parameter is harmless but slightly confusing when stepping through with a debugger.
Consider assigning to a separate variable ($dynBuilder) or injecting BuilderDecorator via composition to improve readability.src/QuoteBundle/Form/Type/QuoteType.php (1)
55-56: Variable shadowing of$builderFor consistency with the other form types you might rename the decorated instance to avoid confusion (see remark in
InvoiceType).src/ClientBundle/Entity/Contact.php (2)
218-233: Consider aligning association naming and provide helper mutatorsThe inverse side is mapped by the
usersfield on the owning entities (Invoice,RecurringInvoice,Quote). Because this entity is calledContact, consumers may find theusersnaming unintuitive (it still reflects the old “user-contact” abstraction).
If renaming on the owning side is not feasible right now, at minimum add a short PHPDoc comment explaining the historical reason so that new contributors do not look for a missingcontactsproperty on those entities.In addition, exposing the raw
Collectionwithoutadd*/remove*helpers makes it harder to keep the bidirectional relation in sync from this side and invites accidental writes to the inverse side. Even if this side stays inverse-only, explicit helpers that delegate to the owning side improve DX and make intent obvious.
379-396: Return typed, read-only collections or add defensive cloning
getInvoices(),getRecurringInvoices(), andgetQuotes()return the liveArrayCollectioninstance.
External callers can therefore mutate the collection directly (e.g.$contact->getInvoices()->clear()), breaking the invariant that only the owning side should manipulate the relationship.Two lightweight options:
- public function getInvoices(): Collection + /** + * Returns an immutable view of the invoices collection to prevent + * accidental modification from the inverse side. + */ + public function getInvoices(): Collectionor, if PHPStan/Psalm is used, change the return type to
Collection<int, Invoice>&\Doctrine\Common\Collections\ReadonlyCollection.Alternatively, expose dedicated
addInvoice()/removeInvoice()that proxy to$invoice->addUser($this)and return a read-only view from the getter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
src/ClientBundle/Entity/Contact.php(2 hunks)src/CoreBundle/Form/Transformer/UserToContactTransformer.php(0 hunks)src/CoreBundle/Tests/Form/Transformer/UserToContactTransformerTest.php(0 hunks)src/FormBundle/Test/FormHandlerTestCase.php(1 hunks)src/InvoiceBundle/Cloner/InvoiceCloner.php(1 hunks)src/InvoiceBundle/Entity/Invoice.php(2 hunks)src/InvoiceBundle/Entity/InvoiceContact.php(0 hunks)src/InvoiceBundle/Entity/RecurringInvoice.php(3 hunks)src/InvoiceBundle/Entity/RecurringInvoiceContact.php(0 hunks)src/InvoiceBundle/Form/EventListener/InvoiceUsersSubscriber.php(0 hunks)src/InvoiceBundle/Form/Type/InvoiceType.php(3 hunks)src/InvoiceBundle/Form/Type/RecurringInvoiceType.php(2 hunks)src/InvoiceBundle/Manager/InvoiceManager.php(1 hunks)src/InvoiceBundle/Repository/RecurringInvoiceContactRepository.php(0 hunks)src/InvoiceBundle/Resources/config/services/services.php(0 hunks)src/InvoiceBundle/Tests/Form/Handler/InvoiceCreateHandlerTest.php(1 hunks)src/InvoiceBundle/Tests/Form/Type/InvoiceTypeTest.php(1 hunks)src/QuoteBundle/Cloner/QuoteCloner.php(1 hunks)src/QuoteBundle/Entity/Quote.php(2 hunks)src/QuoteBundle/Entity/QuoteContact.php(0 hunks)src/QuoteBundle/Form/EventListener/QuoteUsersSubscriber.php(0 hunks)src/QuoteBundle/Form/Type/QuoteType.php(3 hunks)src/QuoteBundle/Resources/config/services/services.php(0 hunks)src/QuoteBundle/Tests/Form/Handler/QuoteCreateHandlerTest.php(1 hunks)src/QuoteBundle/Tests/Form/Type/QuoteTypeTest.php(1 hunks)
💤 Files with no reviewable changes (10)
- src/InvoiceBundle/Resources/config/services/services.php
- src/QuoteBundle/Resources/config/services/services.php
- src/InvoiceBundle/Repository/RecurringInvoiceContactRepository.php
- src/QuoteBundle/Entity/QuoteContact.php
- src/InvoiceBundle/Entity/InvoiceContact.php
- src/CoreBundle/Form/Transformer/UserToContactTransformer.php
- src/CoreBundle/Tests/Form/Transformer/UserToContactTransformerTest.php
- src/InvoiceBundle/Entity/RecurringInvoiceContact.php
- src/InvoiceBundle/Form/EventListener/InvoiceUsersSubscriber.php
- src/QuoteBundle/Form/EventListener/QuoteUsersSubscriber.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/QuoteBundle/Cloner/QuoteCloner.php (1)
src/QuoteBundle/Entity/Quote.php (2)
getUsers(274-277)addUser(279-286)
src/FormBundle/Test/FormHandlerTestCase.php (3)
src/InvoiceBundle/Form/Type/InvoiceType.php (1)
InvoiceType(41-137)src/CoreBundle/Generator/BillingIdGenerator.php (1)
BillingIdGenerator(23-58)src/QuoteBundle/Form/Type/QuoteType.php (1)
QuoteType(40-136)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: DB (PostgreSQL pdo_pgsql 16)
- GitHub Check: DB (MySQL pdo_mysql 5.7)
- GitHub Check: DB (PostgreSQL pdo_pgsql 17)
- GitHub Check: DB (MariaDB pdo_mysql 10.11)
- GitHub Check: DB (MariaDB pdo_mysql 11.4)
- GitHub Check: DB (MariaDB pdo_mysql 11)
- GitHub Check: DB (MariaDB pdo_mysql 11.0)
- GitHub Check: DB (MariaDB pdo_mysql 10.6)
- GitHub Check: DB (MariaDB pdo_mysql 10.5)
- GitHub Check: DB (MySQL pdo_mysql 8.4)
- GitHub Check: DB (MySQL pdo_mysql 8.0)
- GitHub Check: DB (MySQL pdo_mysql 9)
- GitHub Check: DB (MySQL pdo_mysql 8.3)
- GitHub Check: Unit ( PHP 8.2 )
🔇 Additional comments (11)
src/InvoiceBundle/Manager/InvoiceManager.php (1)
122-124: Updated user assignment to follow individual entity management patternThe change from bulk users assignment to individual user addition follows best practices for managing collections in Doctrine entities. This maintains proper relationship tracking and avoids potential issues with the collection's internal state.
src/QuoteBundle/Tests/Form/Handler/QuoteCreateHandlerTest.php (1)
225-225: Constructor signature updated to reflect form type changesThe
QuoteTypeconstructor signature was properly updated to remove theManagerRegistrydependency, aligning with the refactoring that replaces event subscribers with dynamic dependent fields for user selection.src/InvoiceBundle/Tests/Form/Handler/InvoiceCreateHandlerTest.php (1)
222-222: Constructor signature updated to reflect form type changesThe
InvoiceTypeconstructor signature was properly updated to remove theManagerRegistrydependency, aligning with the broader refactoring that simplifies form handling and entity relationships.src/QuoteBundle/Cloner/QuoteCloner.php (1)
59-61: Updated user assignment to follow collection management best practicesThe change from bulk users assignment to individual user addition properly handles the new direct many-to-many relationship between
QuoteandContact. This approach ensures proper entity tracking by Doctrine and matches the implementation in theInvoiceManager.src/InvoiceBundle/Cloner/InvoiceCloner.php (1)
67-69: User association logic updated to use addUser methodThe code now iterates through each user in the original invoice and adds them individually to the new invoice using the
addUser()method. This change aligns with the broader refactoring that removed the intermediate entity and switched from bulk assignment to direct management of many-to-many relationships.src/FormBundle/Test/FormHandlerTestCase.php (1)
80-81: Constructor calls updated to match new signaturesThe constructor calls for
InvoiceTypeandQuoteTypehave been updated to remove the$this->registryparameter. This change correctly aligns with the updated form type implementations whereManagerRegistrydependency was removed in favor of dynamic dependent fields for user selection.src/QuoteBundle/Tests/Form/Type/QuoteTypeTest.php (1)
69-74: QuoteType instantiation updated to match new constructor signatureThe
QuoteTypeinstantiation has been modified to provide only the required parameters ($systemConfigandBillingIdGenerator), removing the$this->registryparameter. This correctly aligns with the refactoredQuoteTypeclass that no longer uses the registry for event subscribers.src/InvoiceBundle/Tests/Form/Type/InvoiceTypeTest.php (1)
101-106: InvoiceType instantiation updated to match new constructor signatureThe
InvoiceTypeinstantiation has been updated to provide only the required parameters ($systemConfigandBillingIdGenerator), removing the$this->registryparameter. This change properly reflects the refactoredInvoiceTypeclass that no longer relies on the registry for managing user fields.src/QuoteBundle/Entity/Quote.php (1)
225-233: Validate mapping & cascades for the new Many-to-Many associationThe new association is now the owning side (
inversedBy="quotes") with an explicit join table.
Two points to double-check:
If a brand-new
Contactis ever created via the Quote API payload, it will not be automatically persisted because no cascade is configured (cascade:["persist"]).
This is probably fine (contacts are usually created elsewhere) – just be sure this behaviour is intentional.Make sure the inverse side in
Contact::$quotesis declared withmappedBy="users". A mismatch will raise a doctrine‐mapping error at runtime.src/InvoiceBundle/Entity/Invoice.php (1)
175-183: Migration note – join-table re-use
invoice_contactalready exists (previously backed by theInvoiceContactentity).
Because the old table contained extra columns (id,company_id, …), simply re-using the name will break hydration unless a migration drops those columns first.Please verify that the accompanying migration removes/renames the obsolete columns or picks a fresh table name.
src/InvoiceBundle/Entity/RecurringInvoice.php (1)
112-125: Confirm inverse mapping & legacy table clean-upThe new join table
recurringinvoice_contactsreplaces the oldrecurringinvoice_contactentity table.
Ensure:
Contact::$recurringInvoicesis mapped withmappedBy="users".- The migration drops the legacy table or its extra columns to avoid mapping conflicts.
fd4de2d to
a52243c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.3.x #1668 +/- ##
============================================
- Coverage 49.13% 48.77% -0.36%
+ Complexity 2706 2685 -21
============================================
Files 498 491 -7
Lines 9721 9654 -67
============================================
- Hits 4776 4709 -67
Misses 4945 4945
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Docstrings generation was requested by @pierredup. * #1668 (comment) The following files were modified: * `migrations/Version20305.php` * `src/ClientBundle/Entity/Contact.php` * `src/FormBundle/Test/FormHandlerTestCase.php` * `src/InvoiceBundle/Cloner/InvoiceCloner.php` * `src/InvoiceBundle/Entity/Invoice.php` * `src/InvoiceBundle/Entity/RecurringInvoice.php` * `src/InvoiceBundle/Form/Type/InvoiceType.php` * `src/InvoiceBundle/Form/Type/RecurringInvoiceType.php` * `src/InvoiceBundle/Manager/InvoiceManager.php` * `src/QuoteBundle/Cloner/QuoteCloner.php` * `src/QuoteBundle/Entity/Quote.php` * `src/QuoteBundle/Form/Type/QuoteType.php`
|
Note Generated docstrings for this pull request at #1669 |
Remove intermediate entity for mapping quotes and invoices to users. This simplifies the users on quotes and invoices by a lot, and prevents subtle bugs and errors (E.G users were always duplicated on an invoice and quote when editing the quote/invoice). One drawback is that we don't save the company_id on the (invoice|quote)_contact table anymore, but generally this shouldn't be an issue since we're only loading the users for an active client based on the current company (so we should never land in a situation where invalid/unrelated users are added to a quote/invoice)
Summary by CodeRabbit
New Features
Refactor
Chores