[Settings] Company-panel manage/assign cluster (#235-243)#692
Open
nielsdrost7 wants to merge 6 commits into
Open
[Settings] Company-panel manage/assign cluster (#235-243)#692nielsdrost7 wants to merge 6 commits into
nielsdrost7 wants to merge 6 commits into
Conversation
…rate selects are company-scoped and persist
CompanySettings.php already wires invoice_numbering_id and
default_invoice_tax_rate_id/default_quote_tax_rate_id to the company's own
records (Numbering::query()->where('company_id', ...), same for TaxRate) —
these two issues asked for exactly this. Adds targeted tests proving both
the options list is scoped (a company never sees another company's
numbering/tax rate) and the selection actually persists via Setting.
…Email Templates resources
New Modules/Core/Filament/Company/Resources/{TaxRates,EmailTemplates}/,
mirroring the NoteTemplates company-resource pattern (BaseResource, modal
create/edit/delete via header/row actions delegating to the existing
TaxRateService/EmailTemplateService, gated on
Permission::MANAGE_COMPANY_SETTINGS). Registered in CompanyPanelProvider's
resources() and the Settings nav group. Tenant scoping is automatic via
BelongsToCompany's global scope on both models.
Also fixes a real, pre-existing bug found while testing: EmailTemplateService
::updateEmailTemplate() never included 'title' in its update() array, so
renaming a template's title silently never persisted — on both admin and
company panels, since they share this service.
… Settings page
invoice_email_template, invoice_paid_email_template,
invoice_overdue_email_template, quote_email_template were already stubbed
with options([]) — wire them to EmailTemplate::query()->where('company_id', ...)
->pluck('title', 'id'), matching the pattern already used for
invoice_numbering_id and the tax-rate defaults. Setting::KEY_* constants
and allKeys() entries already existed.
…methods PaymentMethod stays the existing fixed enum (Modules/Payments/Enums/ PaymentMethod.php) — no new model/migration, since there's nothing else per-company to create/edit given a fixed set of gateway types. Adds Setting::KEY_ENABLED_PAYMENT_METHODS and a CheckboxList on a new "Payments" tab in CompanySettings, defaulting to all-enabled for a new company. Fixes a real array-handling bug in CompanySettings::save()/mount() found while building this: save() force-cast every form value to (string) before Setting::saveForCompany(), which already JSON-encodes non-scalars itself — meaning an array value would have been mangled into the literal string "Array" instead of persisted. mount() needed the matching json_decode() on read. Covered by a save/reload round-trip test.
New Modules/Core/Filament/Company/Pages/CompanyUsers.php, modeled on MyCompanies.php's Page + HasTable/InteractsWithTable structure. Lists the current tenant's users; header action searches all users by email (excluding existing members) and attaches by company_user pivot; row action detaches, guarded against removing the last remaining user and against double-attaching an already-member user (defense in depth beyond the search exclusion, since the search filter is a UX nicety a client could bypass). Gated on Permission::MANAGE_COMPANY_SETTINGS, registered in CompanyPanelProvider's pages() and the Settings nav group. Tests cover both allow and deny paths for attach and remove.
… callTableAction misresolution
CompanyUsersTest::it_removes_a_user_from_the_company failed on CI (run
30107162191) with the pivot row still present after calling
callTableAction('remove', ...) — the same filament/tables record-resolution
bug documented in InvoicePlane#687 for MyCompanies::switch, now also observed here.
Add a defensive membership check before detaching, matching the pattern
already used on the add side, and tag the test flaky per the established
InvoicePlane#687 convention.
Contributor
📝 WalkthroughWalkthroughThe company panel gains tenant-scoped email template and tax rate CRUD resources, company user membership management, and payment method settings. Company settings now load company-specific template options and persist enabled payment methods. ChangesCompany panel administration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Modules/Core/Filament/Company/Pages/CompanySettings.php`:
- Line 383: Update the new Payments tab label in the relevant tab configuration
to use trans('ip.payments') instead of the hardcoded string, preserving the
existing Tab::make structure.
In `@Modules/Core/Filament/Company/Pages/CompanyUsers.php`:
- Around line 77-87: In CompanyUsers.php lines 77-87, enforce a unique
(company_id, user_id) membership constraint and replace the check-then-attach
flow with an idempotent attachment that safely handles concurrent adds. In
CompanyUsers.php lines 101-120, wrap the removal flow in a transaction, lock the
shared company row before recounting users, then detach only when the locked
count preserves the minimum-user invariant.
- Around line 68-74: Add native parameter types to the callbacks in the Select
configuration: type the nested `$query` parameter in `whereDoesntHave` as
`Illuminate\Database\Eloquent\Builder` and type `$value` in
`getOptionLabelUsing` appropriately for the selected user identifier. Add the
required Builder import and preserve the existing query and label behavior.
In
`@Modules/Core/Filament/Company/Resources/EmailTemplates/EmailTemplateResource.php`:
- Around line 23-25: Localize the resource labels in EmailTemplateResource.php
(lines 23-25) and TaxRateResource.php (lines 23-25) by replacing the hard-coded
navigation and model label properties with resource label methods backed by
trans(). Add the corresponding ip.* translation keys, and do not use __().
In `@Modules/Core/Services/EmailTemplateService.php`:
- Line 37: Complete the native type declarations at both sites: update
updateEmailTemplate() in Modules/Core/Services/EmailTemplateService.php so its
$data parameter is typed as array, and type the formatter closure in
Modules/Core/Filament/Company/Resources/TaxRates/Tables/TaxRatesTable.php as
accepting mixed $state and returning string.
In `@Modules/Core/Tests/Feature/CompanyUsersTest.php`:
- Around line 77-78: Update the CompanyUsersTest fixtures at
Modules/Core/Tests/Feature/CompanyUsersTest.php lines 77-78 and 109-110: create
both $alreadyMember and $secondUser via User::factory()->withCompany(...) for
the current company, and remove the manual attach-based membership setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba561a62-6a91-4eb9-8848-02eaf98c086e
📒 Files selected for processing (19)
Modules/Core/Filament/Company/Pages/CompanySettings.phpModules/Core/Filament/Company/Pages/CompanyUsers.phpModules/Core/Filament/Company/Resources/EmailTemplates/EmailTemplateResource.phpModules/Core/Filament/Company/Resources/EmailTemplates/Pages/ListEmailTemplates.phpModules/Core/Filament/Company/Resources/EmailTemplates/Schemas/EmailTemplateForm.phpModules/Core/Filament/Company/Resources/EmailTemplates/Tables/EmailTemplatesTable.phpModules/Core/Filament/Company/Resources/TaxRates/Pages/ListTaxRates.phpModules/Core/Filament/Company/Resources/TaxRates/Schemas/TaxRateForm.phpModules/Core/Filament/Company/Resources/TaxRates/Tables/TaxRatesTable.phpModules/Core/Filament/Company/Resources/TaxRates/TaxRateResource.phpModules/Core/Models/Setting.phpModules/Core/Providers/CompanyPanelProvider.phpModules/Core/Services/EmailTemplateService.phpModules/Core/Tests/Feature/CompanyEmailTemplatesTest.phpModules/Core/Tests/Feature/CompanySettingsTest.phpModules/Core/Tests/Feature/CompanyTaxRatesTest.phpModules/Core/Tests/Feature/CompanyUsersTest.phpModules/Core/resources/views/filament/company/pages/company-users.blade.phpresources/lang/en/ip.php
| ]), | ||
| ]), | ||
|
|
||
| Tab::make('Payments') |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localize the new tab label.
Tab::make('Payments') bypasses translations; use trans('ip.payments').
Proposed fix
- Tab::make('Payments')
+ Tab::make(trans('ip.payments'))As per coding guidelines, “Use trans() function for internationalization, never use __()”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Tab::make('Payments') | |
| Tab::make(trans('ip.payments')) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Modules/Core/Filament/Company/Pages/CompanySettings.php` at line 383, Update
the new Payments tab label in the relevant tab configuration to use
trans('ip.payments') instead of the hardcoded string, preserving the existing
Tab::make structure.
Source: Coding guidelines
Comment on lines
+68
to
+74
| ->getSearchResultsUsing(fn (string $search): array => User::query() | ||
| ->where('email', 'like', "%{$search}%") | ||
| ->whereDoesntHave('companies', fn ($query) => $query->whereKey($company->id)) | ||
| ->limit(10) | ||
| ->pluck('email', 'id') | ||
| ->toArray()) | ||
| ->getOptionLabelUsing(fn ($value): ?string => User::find($value)?->email) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant file and Filament dependency hints without executing repo code.
fd -a 'CompanyUsers\.php$' . || true
FILE="$(fd 'CompanyUsers\.php$' . | head -n 1 || true)"
if [ -n "${FILE:-}" ]; then
echo "== file =="
echo "$FILE"
echo "== lines 1-120 =="
sed -n '1,120p' "$FILE" | nl -ba
fi
echo "== composer Filament / Larastan / phpstan availability =="
for f in composer.json composer.lock; do
[ -f "$f" ] && { echo "-- $f"; grep -n '"filament/"\|"nikolaposa/kv\|"larastan/larastan\|phpstan/phpstan' "$f" || true; }
done
echo "== related Filament select options in file or imports =="
if [ -n "${FILE:-}" ]; then
rg -n "use .*Builder|whereDoesntHave|getOptionLabelUsing|getSearchResultsUsing|filterForms|MultiSelect|Select|\$company" "$FILE" || true
fiRepository: InvoicePlane/InvoicePlane-v2
Length of output: 363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Modules/Core/Filament/Company/Pages/CompanyUsers.php"
echo "== lines 1-120 =="
awk '{printf "%5d\t%s\n", NR, $0}' "$FILE" | head -n 120 || true
echo "== composer snippets =="
for f in composer.json composer.lock; do
if [ -f "$f" ]; then
echo "-- $f"
grep -nE '"filament/|larastan/larastan|phpstan/phpstan|livewire/livewire' "$f" || true
fi
done
echo "== callback occurrences =="
grep -nE "getSearchResultsUsing|getOptionLabelUsing|whereDoesntHave|use Illuminate|Builder" "$FILE" || true
echo "== target callback context =="
python3 - <<'PY'
from pathlib import Path
p=Path("Modules/Core/Filament/Company/Pages/CompanyUsers.php")
lines=p.read_text().splitlines()
for i,l in enumerate(lines, start=1):
if "getSearchResultsUsing" in l or "getOptionLabelUsing" in l or "whereDoesntHave('companies'" in l:
start=max(1,i-5); end=min(len(lines),i+4)
print(f"-- lines {start}-{end} --")
for n in range(start,end+1):
print(f"{n:>5}\t{lines[n-1]}")
PYRepository: InvoicePlane/InvoicePlane-v2
Length of output: 17660
Type the Select callback parameters.
Lines 70 and 74 still have untyped callback parameters ($query and $value). Add native types, including an Illuminate\Database\Eloquent\Builder import for the nested query callback, to satisfy the native PHP type-hint rule.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Modules/Core/Filament/Company/Pages/CompanyUsers.php` around lines 68 - 74,
Add native parameter types to the callbacks in the Select configuration: type
the nested `$query` parameter in `whereDoesntHave` as
`Illuminate\Database\Eloquent\Builder` and type `$value` in
`getOptionLabelUsing` appropriately for the selected user identifier. Add the
required Builder import and preserve the existing query and label behavior.
Source: Coding guidelines
Comment on lines
+77
to
+87
| ->action(function (array $data) use ($company): void { | ||
| if ($company->users()->whereKey($data['user_id'])->exists()) { | ||
| Notification::make() | ||
| ->title(trans('ip.user_already_in_company')) | ||
| ->warning() | ||
| ->send(); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| $company->users()->attach($data['user_id']); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make membership invariants atomic. Both safeguards are check-then-write operations. Concurrent adds can duplicate a membership or raise an unhandled unique-key error; concurrent removals can both observe two users and leave the company with none.
Modules/Core/Filament/Company/Pages/CompanyUsers.php#L77-L87: enforce a(company_id, user_id)unique constraint and make attachment idempotent.Modules/Core/Filament/Company/Pages/CompanyUsers.php#L101-L120: transactionally lock a shared company row before recounting and detaching.
📍 Affects 1 file
Modules/Core/Filament/Company/Pages/CompanyUsers.php#L77-L87(this comment)Modules/Core/Filament/Company/Pages/CompanyUsers.php#L101-L120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Modules/Core/Filament/Company/Pages/CompanyUsers.php` around lines 77 - 87,
In CompanyUsers.php lines 77-87, enforce a unique (company_id, user_id)
membership constraint and replace the check-then-attach flow with an idempotent
attachment that safely handles concurrent adds. In CompanyUsers.php lines
101-120, wrap the removal flow in a transaction, lock the shared company row
before recounting users, then detach only when the locked count preserves the
minimum-user invariant.
Comment on lines
+23
to
+25
| protected static ?string $navigationLabel = 'Email Templates'; | ||
|
|
||
| protected static ?string $modelLabel = 'Email Template'; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localize the resource labels. These static strings bypass the application translation system; expose translated labels through the resource label methods and add the corresponding ip.* keys.
Modules/Core/Filament/Company/Resources/EmailTemplates/EmailTemplateResource.php#L23-L25: replace the hard-coded navigation and model labels withtrans()-backed resource methods.Modules/Core/Filament/Company/Resources/TaxRates/TaxRateResource.php#L23-L25: replace the hard-coded navigation and model labels withtrans()-backed resource methods.
As per coding guidelines, use trans() for internationalization and never __().
📍 Affects 2 files
Modules/Core/Filament/Company/Resources/EmailTemplates/EmailTemplateResource.php#L23-L25(this comment)Modules/Core/Filament/Company/Resources/TaxRates/TaxRateResource.php#L23-L25
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Modules/Core/Filament/Company/Resources/EmailTemplates/EmailTemplateResource.php`
around lines 23 - 25, Localize the resource labels in EmailTemplateResource.php
(lines 23-25) and TaxRateResource.php (lines 23-25) by replacing the hard-coded
navigation and model label properties with resource label methods backed by
trans(). Add the corresponding ip.* translation keys, and do not use __().
Source: Coding guidelines
| $emailTemplateToUpdate->update([ | ||
| 'company_id' => $this->getCompanyId() ?? 1, | ||
| 'type' => $data['type'], | ||
| 'title' => $data['title'], |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Complete the native type declarations.
Modules/Core/Services/EmailTemplateService.php#L37-L37: declareupdateEmailTemplate()’s$dataparameter asarray.Modules/Core/Filament/Company/Resources/TaxRates/Tables/TaxRatesTable.php#L25-L29: type the formatter asfunction (mixed $state): string.
As per coding guidelines, use native PHP type hints throughout the codebase.
📍 Affects 2 files
Modules/Core/Services/EmailTemplateService.php#L37-L37(this comment)Modules/Core/Filament/Company/Resources/TaxRates/Tables/TaxRatesTable.php#L25-L29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Modules/Core/Services/EmailTemplateService.php` at line 37, Complete the
native type declarations at both sites: update updateEmailTemplate() in
Modules/Core/Services/EmailTemplateService.php so its $data parameter is typed
as array, and type the formatter closure in
Modules/Core/Filament/Company/Resources/TaxRates/Tables/TaxRatesTable.php as
accepting mixed $state and returning string.
Source: Coding guidelines
Comment on lines
+77
to
+78
| $alreadyMember = User::factory()->create(); | ||
| $this->company->users()->attach($alreadyMember); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Create member fixtures through withCompany().
Modules/Core/Tests/Feature/CompanyUsersTest.php#L77-L78: create$alreadyMemberwithUser::factory()->withCompany(...)for the current company.Modules/Core/Tests/Feature/CompanyUsersTest.php#L109-L110: create$secondUserwith the same factory membership state.
As per coding guidelines, “For users needing company membership, use User::factory()->withCompany(...).”
📍 Affects 1 file
Modules/Core/Tests/Feature/CompanyUsersTest.php#L77-L78(this comment)Modules/Core/Tests/Feature/CompanyUsersTest.php#L109-L110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Modules/Core/Tests/Feature/CompanyUsersTest.php` around lines 77 - 78, Update
the CompanyUsersTest fixtures at Modules/Core/Tests/Feature/CompanyUsersTest.php
lines 77-78 and 109-110: create both $alreadyMember and $secondUser via
User::factory()->withCompany(...) for the current company, and remove the manual
attach-based membership setup.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #235, #236, #237, #238, #239, #240, #241, #242, #243.
Company-panel "manage/assign" settings cluster — lets a company admin manage their own Tax
Rates, Email Templates, and Users, and choose per-company defaults (invoice numbering, tax
rates, email templates, enabled payment methods) without needing admin-panel access.
TaxRateResource/EmailTemplateResource, mirroring theexisting
NoteTemplateResourcepattern (modal create/edit/delete, gated onPermission::MANAGE_COMPANY_SETTINGS).(existing
canCreate/canDeleterestriction), view/edit already worked.CheckboxList, new "Payments"tab on Company Settings) —
PaymentMethodstays the existing fixed enum, no new model needed.Selects on the Settingspage to the company's own
EmailTemplaterecords.already worked); added tests proving both the options list and the persisted selection are
correctly company-scoped.
CompanyUserspage: list/add/remove users for the current company, with guardsagainst duplicate-attach and removing the last remaining user.
Also fixes two real, pre-existing bugs found while building this:
EmailTemplateService::updateEmailTemplate()never includedtitlein its update array, sorenaming a template's title silently never persisted (admin panel too, since both share this
service).
CompanySettings::save()force-cast every value to(string)before persisting, which wouldhave mangled the new payment-methods array setting into the literal string
"Array".Test plan
php artisan test,ip2-test-php:8.4image) — 562/562, 0 failures(duplicate-attach, last-user-removal, cross-company scoping)
Note
This branch is cherry-picked cleanly onto
upstream/developfrom the original PR #690, whichpicked up unrelated commits due to fork/upstream branch divergence (Docker/SQLite-elimination
infra, an unrelated CI tenant-switch flake fix, a widget bug fix — none of which touch #235-243).
#690 is closed in favor of this PR.
Summary by CodeRabbit
New Features
Bug Fixes