[IP-247]: company-scoped Settings page (Invoices/Quotes/Taxes/Email/System/Dashboard) - #686
Merged
nielsdrost7 merged 7 commits intoJul 24, 2026
Conversation
Ran vendor/bin/pint on develop: import ordering, binary-operator spacing, and empty-body brace style. Purely cosmetic, no behavior change.
…global settings getByKey() explicitly queries for company_id IS NULL rows, but Setting now uses BelongsToCompany, which adds a global scope filtering by the current tenant's company_id whenever one is resolved. The two conditions are mutually exclusive, so getByKey() silently returned null for every legacy global setting (e.g. generate_invoice_number_for_draft) whenever a company was active -- which is effectively always in the running app. saveByKey() already guarded against this with withoutGlobalScopes(); getByKey() didn't. Also apply Pint formatting to the two changed files.
Contributor
📝 WalkthroughWalkthroughThe change introduces company-scoped settings storage and APIs, a permission-protected Filament settings page, migration support, translations, factories, and feature tests. It also updates Laravel and PHP constraints, project metadata, ignore patterns, formatting, and one flaky test. ChangesCompany settings
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CompanySettings
participant Setting
participant SettingsTable
User->>CompanySettings: Open company settings
CompanySettings->>Setting: Load company-scoped values
Setting->>SettingsTable: Read company row or global fallback
User->>CompanySettings: Submit form
CompanySettings->>Setting: Normalize and save values
Setting->>SettingsTable: Upsert company-scoped rows
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ 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
🧹 Nitpick comments (3)
Modules/Invoices/Tests/Feature/RecurringInvoicesTest.php (1)
34-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove or use the unused test fixtures.
PHPMD reports
$user,$customer, and$productas unused in each arrange block. Remove them, or pass them into the relevant factories/payload if they are intended to establish relationships.Also applies to: 69-73, 109-113, 137-141, 176-180, 210-214
🤖 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/Invoices/Tests/Feature/RecurringInvoicesTest.php` around lines 34 - 38, Remove the unused $user, $customer, and $product fixtures from each arrange block in RecurringInvoicesTest, or explicitly pass them into the relevant invoice factories or request payloads when their relationships are required. Keep only fixtures consumed by the test scenarios, including the repeated blocks identified by the review.Source: Linters/SAST tools
Modules/Core/Database/Migrations/2026_07_19_000001_add_company_id_to_settings_table.php (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
$driveris assigned here but never used withinup()(the helpers resolve the driver independently). Safe to remove.🤖 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/Database/Migrations/2026_07_19_000001_add_company_id_to_settings_table.php` at line 22, Remove the unused `$driver` assignment from the `up()` method; leave the migration helpers and their independent driver resolution unchanged.Modules/Core/Models/Setting.php (1)
223-238: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDocblock/behavior mismatch in
getBoolForCompany.The docblock says
$defaultis returned "when the value isn't a parseable boolean string", but the code only returns$defaultfornull; any non-boolean string flows throughfilter_var(...)and yieldsfalse, not$default. Either update the docblock or short-circuit unparseable values. Low impact today since toggles persist'0'/'1'.🤖 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/Models/Setting.php` around lines 223 - 238, The docblock for Setting::getBoolForCompany promises the default for unparseable boolean values, but the implementation returns false; update the method to detect filter_var’s unparseable result and return $default while preserving valid boolean parsing and the existing null handling.
🤖 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 `@composer.json`:
- Around line 20-26: Align composer.json dependencies with the repository’s
Laravel 11 and PHP 8.1+ compatibility policy, replacing the PHP ^8.3 and Laravel
^13.0 requirements with supported constraints while preserving the other package
requirements. Update CLAUDE.md to reflect the same supported versions; do not
formally change the governing policy unless also updating the lockfile, CI, and
documentation consistently. Affected sites: composer.json lines 20-26 require
dependency changes; CLAUDE.md lines 5-13 require matching documentation updates.
In
`@Modules/Core/Database/Migrations/2026_07_19_000001_add_company_id_to_settings_table.php`:
- Around line 14-18: Update the migration docblock describing the `(company_id,
setting_key)` index to state that it is a regular non-unique composite index and
uniqueness is enforced by the application layer. Remove claims about partial
indexing, database-enforced uniqueness, and `WHERE company_id IS NOT NULL`,
keeping the actual schema behavior represented by the index definition.
In `@Modules/Core/Filament/Company/Pages/CompanySettings.php`:
- Around line 146-148: Update the TextInput for Setting::KEY_COMPANY_NAME to use
trans() with a dedicated translation key for the company-name label instead of
the hardcoded English string; ensure the key resolves to the intended “Company
Name” text rather than reusing ip.company_name if it maps to “Customer Name.”
- Around line 231-271: Populate the options for each invoice template Select in
the invoice templates Section using the existing seeded or persisted template
source, matching the approach used by quote template selectors elsewhere in
CompanySettings. Replace the empty options for the PDF, paid, overdue, public,
email, paid-email, and overdue-email fields while preserving their labels and
placeholders; if no template source is currently available, add a TODO instead
of leaving the selectors silently unusable.
In `@Modules/Core/Models/Setting.php`:
- Around line 178-191: Update Setting::saveForCompany to encrypt values for the
existing sensitive-key allowlist, including SMTP and invoice/quote PDF
passwords, before persistence, while retaining current serialization for other
settings; update getForCompany to decrypt those sensitive values, and remove
password prefill/reveal behavior from mount() so secrets are not sent to the
browser.
In `@Modules/Core/Tests/Feature/CompanySettingsTest.php`:
- Around line 108-133: Rename the affected test methods, including
get_for_company_falls_back_to_global_when_no_company_row,
get_for_company_returns_default_when_nothing_set,
company_scoped_value_wins_over_global, and
partial_unique_index_allows_same_key_across_companies, to snake_case names
following the it_<verb>_<subject> convention; ensure the index test name
describes a non-unique index. Add explicit /* Arrange */, /* Act */, and /*
Assert */ blocks to each affected test while preserving their existing behavior.
---
Nitpick comments:
In
`@Modules/Core/Database/Migrations/2026_07_19_000001_add_company_id_to_settings_table.php`:
- Line 22: Remove the unused `$driver` assignment from the `up()` method; leave
the migration helpers and their independent driver resolution unchanged.
In `@Modules/Core/Models/Setting.php`:
- Around line 223-238: The docblock for Setting::getBoolForCompany promises the
default for unparseable boolean values, but the implementation returns false;
update the method to detect filter_var’s unparseable result and return $default
while preserving valid boolean parsing and the existing null handling.
In `@Modules/Invoices/Tests/Feature/RecurringInvoicesTest.php`:
- Around line 34-38: Remove the unused $user, $customer, and $product fixtures
from each arrange block in RecurringInvoicesTest, or explicitly pass them into
the relevant invoice factories or request payloads when their relationships are
required. Keep only fixtures consumed by the test scenarios, including the
repeated blocks identified by the review.
🪄 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: abe0f6b2-a70f-4821-b7e7-bd30d1186416
📒 Files selected for processing (22)
.gitignoreCLAUDE.mdModules/Core/Database/Factories/CompanyUserFactory.phpModules/Core/Database/Factories/CustomFieldValueFactory.phpModules/Core/Database/Factories/NoteFactory.phpModules/Core/Database/Factories/SettingFactory.phpModules/Core/Database/Migrations/2026_07_19_000001_add_company_id_to_settings_table.phpModules/Core/Filament/Company/Pages/CompanySettings.phpModules/Core/Models/Setting.phpModules/Core/Providers/CompanyPanelProvider.phpModules/Core/Tests/Feature/CompanySettingsTest.phpModules/Core/Tests/Feature/Seeders/RoleHasPermissionsSeederTest.phpModules/Core/Tests/Unit/DateFieldAutoPopulationTest.phpModules/Core/resources/views/filament/company/pages/company-settings.blade.phpModules/Expenses/Observers/ExpenseObserver.phpModules/Invoices/Tests/Feature/InvoiceDuplicateNumberPreventionTest.phpModules/Invoices/Tests/Feature/RecurringInvoicesTest.phpModules/Payments/Observers/PaymentObserver.phpModules/Quotes/Tests/Feature/QuoteDuplicateNumberPreventionTest.phpModules/Quotes/Tests/Unit/QuoteModelTest.phpcomposer.jsonresources/lang/en/ip.php
💤 Files with no reviewable changes (2)
- Modules/Invoices/Tests/Feature/InvoiceDuplicateNumberPreventionTest.php
- Modules/Quotes/Tests/Feature/QuoteDuplicateNumberPreventionTest.php
Comment on lines
+20
to
+26
| "php": "^8.3", | ||
| "awcodes/mason": "^3.1", | ||
| "doctrine/dbal": ">=4.4", | ||
| "dompdf/dompdf": "^3.1", | ||
| "filament/actions": ">=5.6", | ||
| "filament/filament": ">=5.6", | ||
| "laravel/framework": ">=12.46", | ||
| "laravel/framework": "^13.0", |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n '"php"|"laravel/framework"' composer.json
rg -n 'Laravel (11|13)|PHP (8\.1|8\.3)' CLAUDE.md
composer validate --strict
composer check-platform-reqsRepository: InvoicePlane/InvoicePlane-v2
Length of output: 1397
Align the manifest and documentation with the Laravel 11/PHP 8.1+ compatibility policy.
composer.json declares PHP ^8.3 and Laravel ^13.0, while the repository’s **/*.{php,json} requirement mandates Laravel 11 with PHP 8.1+. Keep CLAUDE.md matching the supported versions, or formally update the governing compatibility policy, lockfile, CI, and documentation together.
📍 Affects 2 files
composer.json#L20-L26(this comment)CLAUDE.md#L5-L13
🤖 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 `@composer.json` around lines 20 - 26, Align composer.json dependencies with
the repository’s Laravel 11 and PHP 8.1+ compatibility policy, replacing the PHP
^8.3 and Laravel ^13.0 requirements with supported constraints while preserving
the other package requirements. Update CLAUDE.md to reflect the same supported
versions; do not formally change the governing policy unless also updating the
lockfile, CI, and documentation consistently. Affected sites: composer.json
lines 20-26 require dependency changes; CLAUDE.md lines 5-13 require matching
documentation updates.
Source: Coding guidelines
Comment on lines
+14
to
+18
| * The unique index is a *partial* index (Postgres / MariaDB / SQLite all | ||
| * support `WHERE`): the `(company_id, setting_key)` pair is unique only | ||
| * when `company_id IS NOT NULL`. Two companies may each have their own | ||
| * `currency_code`; a global `currency_code` and a company-scoped one | ||
| * may coexist. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Docblock describes a partial UNIQUE index that the migration never creates.
Lines 46–56 create a plain, non-unique composite index (->index([...])) and the inline comment states uniqueness is enforced only at the application layer. This docblock claims a WHERE company_id IS NOT NULL partial unique index, which contradicts the actual schema and could mislead maintainers into assuming DB-level uniqueness. Please align the docblock with the soft-constraint reality.
🤖 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/Database/Migrations/2026_07_19_000001_add_company_id_to_settings_table.php`
around lines 14 - 18, Update the migration docblock describing the `(company_id,
setting_key)` index to state that it is a regular non-unique composite index and
uniqueness is enforced by the application layer. Remove claims about partial
indexing, database-enforced uniqueness, and `WHERE company_id IS NOT NULL`,
keeping the actual schema behavior represented by the index definition.
Comment on lines
+146
to
+148
| TextInput::make(Setting::KEY_COMPANY_NAME) | ||
| ->label('Company Name') | ||
| ->maxLength(255), |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use trans() for this label.
->label('Company Name') hardcodes an English string; every other field here uses trans('ip....'). As per coding guidelines ("Use trans() function for internationalization"), route it through a translation key (note ip.company_name currently resolves to "Customer Name", so a distinct key may be needed).
🤖 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` around lines 146 -
148, Update the TextInput for Setting::KEY_COMPANY_NAME to use trans() with a
dedicated translation key for the company-name label instead of the hardcoded
English string; ensure the key resolves to the intended “Company Name” text
rather than reusing ip.company_name if it maps to “Customer Name.”
Source: Coding guidelines
Comment on lines
+231
to
+271
| Section::make(trans('ip.invoice_templates'))->columns(2)->schema([ | ||
| Select::make(Setting::KEY_INVOICE_PDF_TEMPLATE) | ||
| ->label(trans('ip.default_pdf_template')) | ||
| ->options([]) | ||
| ->placeholder(trans('ip.none')), | ||
|
|
||
| Select::make(Setting::KEY_INVOICE_PAID_PDF_TEMPLATE) | ||
| ->label(trans('ip.pdf_template_paid')) | ||
| ->options([]) | ||
| ->placeholder(trans('ip.none')), | ||
|
|
||
| Select::make(Setting::KEY_INVOICE_OVERDUE_PDF_TEMPLATE) | ||
| ->label(trans('ip.pdf_template_overdue')) | ||
| ->options([]) | ||
| ->placeholder(trans('ip.none')), | ||
|
|
||
| Select::make(Setting::KEY_INVOICE_PUBLIC_TEMPLATE) | ||
| ->label(trans('ip.default_public_template')) | ||
| ->options([]) | ||
| ->placeholder(trans('ip.none')), | ||
|
|
||
| Select::make(Setting::KEY_INVOICE_EMAIL_TEMPLATE) | ||
| ->label(trans('ip.default_email_template')) | ||
| ->options([]) | ||
| ->placeholder(trans('ip.none')), | ||
|
|
||
| Select::make(Setting::KEY_INVOICE_PAID_EMAIL_TEMPLATE) | ||
| ->label(trans('ip.email_template_paid')) | ||
| ->options([]) | ||
| ->placeholder(trans('ip.none')), | ||
|
|
||
| Select::make(Setting::KEY_INVOICE_OVERDUE_EMAIL_TEMPLATE) | ||
| ->label(trans('ip.email_template_overdue')) | ||
| ->options([]) | ||
| ->placeholder(trans('ip.none')), | ||
|
|
||
| Textarea::make(Setting::KEY_INVOICE_PDF_FOOTER) | ||
| ->label(trans('ip.pdf_invoice_footer')) | ||
| ->rows(3) | ||
| ->columnSpanFull(), | ||
| ]), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo files matching CompanySettings.php:\n'
fd -a 'CompanySettings\.php$' . || true
file="$(fd 'CompanySettings\.php$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
printf '\nFile: %s\n' "$file"
wc -l "$file"
sed -n '200,340p' "$file" | nl -ba -v200
fi
printf '\nSearch for template option sourcing / methods:\n'
rg -n "KEY_INVOICE_(PDF|EMAIL|PUBLIC|PAID|OVERDUE)_TEMPLATE|KEY_QUOTE|template|Template|templates|getOptions|options\(" -S . --glob '*.php' --glob '*.json' \
| head -n 200 || trueRepository: InvoicePlane/InvoicePlane-v2
Length of output: 450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='./Modules/Core/Filament/Company/Pages/CompanySettings.php'
printf 'CompanySettings.php lines 1-80:\n'
sed -n '1,80p' "$file"
printf '\nTemplate-related keys and options in CompanySettings.php:\n'
rg -n "KEY_INVOICE_(PDF|EMAIL|PUBLIC|PAID|OVERDUE)_TEMPLATE|KEY_QUOTE|template|templates|options\(|getOptions" "$file"
printf '\nLines 220-340:\n'
sed -n '220,340p' "$file"
printf '\nTemplate option/model definitions in repository:\n'
rg -n "KEY_INVOICE_(PDF|EMAIL|PUBLIC|PAID|OVERDUE)_TEMPLATE|KEY_QUOTE|Invoice.*Template|Quote.*Template|template|templates|getOptions|options\(" -S . --glob '*.php' --glob '*.json' \
| sed -n '1,240p'Repository: InvoicePlane/InvoicePlane-v2
Length of output: 43115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Email template seeder:\n'
sed -n '1,90p' ./Modules/Core/Database/Seeders/EmailTemplatesSeeder.php
printf '\nEmailTemplate model outline and source relevant methods:\n'
sed -n '1,120p' ./Modules/Core/Models/EmailTemplate.php
printf '\nInvoice HTML helper relevant source:\n'
sed -n '1,80p' ./Modules/Core/Support/HTML.php
printf '\nQuote HTML helper relevant source:\n'
sed -n '30,70p' ./Modules/Core/Support/HTML.php
printf '\nInvoice template references in invoice views/routes:\n'
rg -n "invoice_template|pdf_invoice_template|invoice-pdf|templates\\.invoices|HTML::invoice|HTML::quote|quote_template|templates\\.quotes" -S . --glob '*.php' --glob '*.blade.php' | sed -n '1,220p'Repository: InvoicePlane/InvoicePlane-v2
Length of output: 10832
Fill the template selector options.
The invoice and quote template selects all call ->options([]), so admins can’t choose a template and these settings will remain empty. Use the seeded/persisted templates here, or add a TODO if this is intentionally deferred.
🤖 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` around lines 231 -
271, Populate the options for each invoice template Select in the invoice
templates Section using the existing seeded or persisted template source,
matching the approach used by quote template selectors elsewhere in
CompanySettings. Replace the empty options for the PDF, paid, overdue, public,
email, paid-email, and overdue-email fields while preserving their labels and
placeholders; if no template source is currently available, add a TODO instead
of leaving the selectors silently unusable.
Comment on lines
+178
to
+191
| public static function saveForCompany(int $companyId, string $key, mixed $value): void | ||
| { | ||
| $setting = self::query() | ||
| ->withoutGlobalScopes() | ||
| ->where('company_id', $companyId) | ||
| ->where('setting_key', $key) | ||
| ->first() | ||
| ?? new self(); | ||
|
|
||
| $setting->company_id = $companyId; | ||
| $setting->setting_key = $key; | ||
| $setting->setting_value = is_scalar($value) || $value === null ? (string) $value : json_encode($value); | ||
| $setting->save(); | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Sensitive settings (SMTP/PDF passwords) are persisted in plaintext.
saveForCompany stores every value as a raw string, including KEY_SMTP_PASSWORD, KEY_INVOICE_PDF_PASSWORD, and KEY_QUOTE_PDF_PASSWORD. The linked issues call for encrypted SMTP credentials. Because mount() also prefills these into form state, the plaintext secrets are additionally shipped to the browser on every page load and revealed via ->revealable().
Consider encrypting known-sensitive keys on write (e.g. Crypt::encryptString(...) gated on a sensitive-key allowlist) and decrypting in getForCompany, and avoid prefilling password fields.
🤖 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/Models/Setting.php` around lines 178 - 191, Update
Setting::saveForCompany to encrypt values for the existing sensitive-key
allowlist, including SMTP and invoice/quote PDF passwords, before persistence,
while retaining current serialization for other settings; update getForCompany
to decrypt those sensitive values, and remove password prefill/reveal behavior
from mount() so secrets are not sent to the browser.
Comment on lines
+108
to
+133
| #[Test] | ||
| #[Group('per-company')] | ||
| public function get_for_company_falls_back_to_global_when_no_company_row(): void | ||
| { | ||
| /* Arrange */ | ||
| Setting::saveByKey('legacy_key', 'global-value'); | ||
|
|
||
| /* Assert */ | ||
| $this->assertSame('global-value', Setting::getForCompany($this->company->id, 'legacy_key')); | ||
| } | ||
|
|
||
| #[Test] | ||
| #[Group('per-company')] | ||
| public function get_for_company_returns_default_when_nothing_set(): void | ||
| { | ||
| $this->assertSame('fallback', Setting::getForCompany($this->company->id, 'unrelated_key', 'fallback')); | ||
| } | ||
|
|
||
| #[Test] | ||
| #[Group('per-company')] | ||
| public function get_for_company_company_only_skips_global_fallback(): void | ||
| { | ||
| Setting::saveByKey('legacy_key', 'global-value'); | ||
|
|
||
| $this->assertNull(Setting::getForCompany($this->company->id, 'legacy_key', null, true)); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test method naming/AAA don't match guidelines.
Methods like get_for_company_falls_back_to_global_when_no_company_row, get_for_company_returns_default_when_nothing_set, company_scoped_value_wins_over_global, and partial_unique_index_allows_same_key_across_companies don't follow the it_<verb>_<subject> pattern, and a few of these lack the /* Arrange */ /* Act */ /* Assert */ blocks. Also partial_unique_index_... is misleading since the migration creates a non-unique index. As per coding guidelines ("Use snake_case method names in tests following the pattern it_<verb>_<subject>" and structure tests with AAA blocks).
🤖 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/CompanySettingsTest.php` around lines 108 - 133,
Rename the affected test methods, including
get_for_company_falls_back_to_global_when_no_company_row,
get_for_company_returns_default_when_nothing_set,
company_scoped_value_wins_over_global, and
partial_unique_index_allows_same_key_across_companies, to snake_case names
following the it_<verb>_<subject> convention; ensure the index test name
describes a non-unique index. Add explicit /* Arrange */, /* Act */, and /*
Assert */ blocks to each affected test while preserving their existing behavior.
Source: Coding guidelines
nielsdrost7
changed the title
2 tasks
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
Adds a company-scoped Settings page (
Modules/Core/Filament/Company/Pages/CompanySettings.php) covering ~40 settings across 8 tabs (General, Amounts, Dashboard, System, Invoices, Quotes, Taxes, Email), backed by acompany_id-scoped rewrite of theSettingmodel.This branch existed locally, unmerged, for several days while a separate issue-refinement pass was independently recommending fresh implementations of settings already built here. This PR rebases it onto current
develop, fixes a real bug found while verifying it, and opens it for review so the duplicate-effort risk goes away.What's included
Modules/Core/Models/Setting.php— addscompany_idscoping (BelongsToCompany), per-company read/write (getForCompany/saveForCompany/getBoolForCompany), keeps backward-compatible global read/write (getByKey/saveByKey) for existing callers, adds oneKEY_*constant per new setting.Modules/Core/Filament/Company/Pages/CompanySettings.php— the settings page itself, tabbed by area.company_idtosettings.CompanySettingsTest(12 tests) covering persistence, scoping, and authorization.Bug found and fixed during review
Setting::getByKey()explicitly filteredWHERE company_id IS NULL, butSettingnow carriesBelongsToCompany's global scope, which addsWHERE company_id = <current tenant>whenever a company is active. Those two conditions are mutually exclusive, so every legacy global setting read viagetByKey()silently returnednullwhenever a company was active — which is effectively always, in the running app. This brokegenerate_invoice_number_for_drafthandling for both invoices and quotes (verified via 4 previously-passing tests that started failing against this branch:InvoiceNumberGenerationOnCreateTest,InvoicesTest×2,QuoteNumberGenerationOnCreateTest).saveByKey()already guarded against this withwithoutGlobalScopes();getByKey()didn't. Fixed by adding the same guard. All 4 tests pass again after the fix.Testing
ip2-test-php:8.4per project convention (host PHP lacks thesqliteextension).Closes
This branch implements the specific setting(s) each of the following issues asked for (verified by reading each issue body against the actual
Setting::KEY_*fields present inCompanySettings.php, not just title similarity):Closes #251 — Date & Time formats
Closes #254 — Invoices PDF Settings (mark-as-sent, watermark, password, footer, template)
Closes #255 — Invoice Template (PDF/email template selects per lifecycle state)
Closes #256 — QR Code Settings
Closes #257 — Email Settings
Closes #259 — Default invoice footer
Closes #260 — Quote validity days
Closes #261 — Quotes PDF Settings (mark-as-sent, password)
Closes #262 — Quote Template
Closes #263 — Default invoice tax rate
Closes #264 — Default quote tax rate
Closes #265 — SMTP configuration
Closes #266 — Default email sender / send method
Partially addresses (not closing — real remaining scope)
Numberingmodule instead — worth a separate look).Test plan
CompanySettingsTestpasses (12/12)Summary by CodeRabbit
New Features
Bug Fixes
Chores