diff --git a/database/migrations/relax_custom_fields_unique_key.php b/database/migrations/relax_custom_fields_unique_key.php new file mode 100644 index 00000000..37c9b5a9 --- /dev/null +++ b/database/migrations/relax_custom_fields_unique_key.php @@ -0,0 +1,184 @@ +swapUniqueKey( + from: $this->narrowColumns(), + fromIndexName: null, + to: $this->wideColumns(), + toIndexName: $this->wideIndexName(), + ); + } + + public function down(): void + { + $this->assertNoDuplicatesUnderNarrowKey(); + + $this->swapUniqueKey( + from: $this->wideColumns(), + fromIndexName: $this->wideIndexName(), + to: $this->narrowColumns(), + toIndexName: null, + ); + } + + /** + * MySQL runs each ALTER TABLE as its own auto-committing DDL statement, so dropping + * the wide key and adding the narrow one are not transactional together. If rows exist + * that share (code, entity_type[, tenant]) across different sections — exactly the + * shape the wide key exists to allow — the DROP succeeds and the subsequent ADD fails + * on the duplicate, leaving the table with neither unique key. Check first and abort + * before touching anything. + */ + private function assertNoDuplicatesUnderNarrowKey(): void + { + $table = config('custom-fields.database.table_names.custom_fields'); + + if (! Schema::hasColumn($table, 'custom_field_section_id')) { + return; + } + + $columns = $this->narrowColumns(); + + $duplicateCodes = DB::table($table) + ->select($columns) + ->groupBy($columns) + ->havingRaw('count(*) > 1') + ->pluck('code'); + + if ($duplicateCodes->isEmpty()) { + return; + } + + throw new RuntimeException(sprintf( + 'Cannot roll back the custom_fields unique key: %d code(s) — including "%s" — are shared by more than one row for the same (%s), only differing by custom_field_section_id. onlySections() allows this under the wide key, but the narrow key being restored cannot. Resolve or remove the duplicate rows before rolling back this migration.', + $duplicateCodes->count(), + $duplicateCodes->first(), + implode(', ', $columns) + )); + } + + /** + * Drops $from's unique key if present and adds $to's if absent. Shared by both + * directions: up() widens (code, entity_type[, tenant]) to also include + * custom_field_section_id; down() narrows it back to the original key. + * + * @param array $from + * @param array $to + */ + private function swapUniqueKey(array $from, ?string $fromIndexName, array $to, ?string $toIndexName): void + { + $table = config('custom-fields.database.table_names.custom_fields'); + + if (! Schema::hasColumn($table, 'custom_field_section_id')) { + return; + } + + $fromIndexName ??= $this->defaultUniqueIndexName($table, $from); + $toIndexName ??= $this->defaultUniqueIndexName($table, $to); + $existingIndexes = collect(Schema::getIndexes($table))->pluck('name'); + + Schema::table($table, function (Blueprint $blueprint) use ($existingIndexes, $fromIndexName, $to, $toIndexName): void { + if ($existingIndexes->contains($fromIndexName)) { + $blueprint->dropUnique($fromIndexName); + } + + if (! $existingIndexes->contains($toIndexName)) { + $blueprint->unique($to, $toIndexName); + } + }); + } + + /** + * @return array + */ + private function narrowColumns(): array + { + $columns = ['code', 'entity_type']; + + if (FeatureManager::isEnabled(CustomFieldsFeature::SYSTEM_MULTI_TENANCY)) { + $columns[] = config('custom-fields.database.column_names.tenant_foreign_key'); + } + + return $columns; + } + + /** + * @return array + */ + private function wideColumns(): array + { + return [...$this->narrowColumns(), 'custom_field_section_id']; + } + + private function wideIndexName(): string + { + return FeatureManager::isEnabled(CustomFieldsFeature::SYSTEM_MULTI_TENANCY) + ? 'cf_code_entity_tenant_section_unique' + : 'cf_code_entity_section_unique'; + } + + /** + * Mirrors Laravel's own auto-generated unique-index name (Blueprint::createIndexName()) + * so the drop target matches exactly what create_custom_fields_table.php produced, + * without hardcoding a name that would drift if a configured column name changes. + * + * Laravel's shipped config/database.php enables `prefix_indexes` for mysql and pgsql, + * which makes Blueprint::createIndexName() fold the connection's table prefix into the + * name it generates. Skipping that step here would compute a drop target that never + * matches the real index name on a prefixed install, so the drop would silently no-op. + * + * @param array $columns + */ + private function defaultUniqueIndexName(string $table, array $columns): string + { + $connection = Schema::getConnection(); + + $prefixedTable = $connection->getConfig('prefix_indexes') + ? $this->applyTablePrefix($table, $connection->getTablePrefix()) + : $table; + + $index = strtolower($prefixedTable.'_'.implode('_', $columns).'_unique'); + + return str_replace(['-', '.'], '_', $index); + } + + private function applyTablePrefix(string $table, string $prefix): string + { + return str_contains($table, '.') + ? substr_replace($table, '.'.$prefix, strrpos($table, '.'), 1) + : $prefix.$table; + } +}; diff --git a/docs/content/1.getting-started/1.installation.md b/docs/content/1.getting-started/1.installation.md index 90cba90a..a4ed1f49 100644 --- a/docs/content/1.getting-started/1.installation.md +++ b/docs/content/1.getting-started/1.installation.md @@ -84,3 +84,18 @@ php artisan vendor:publish --tag="custom-fields-translations" ```bash php artisan vendor:publish --tag="custom-fields-views" ``` + +## Picking Up New Migrations After an Upgrade + +Package migrations are not run automatically by `php artisan migrate` after a version +bump — they're only copied into your app the first time you install, or when you +explicitly republish them. If a release adds or changes a migration, republish and run +it: + +```bash +php artisan vendor:publish --tag="custom-fields-migrations" +php artisan migrate +``` + +See the [upgrade guide](/getting-started/upgrade-guide#picking-up-new-migrations) for a +concrete example of a release that requires this step. diff --git a/docs/content/1.getting-started/3.upgrade-guide.md b/docs/content/1.getting-started/3.upgrade-guide.md index e1a7bf93..75dbb6d3 100644 --- a/docs/content/1.getting-started/3.upgrade-guide.md +++ b/docs/content/1.getting-started/3.upgrade-guide.md @@ -45,6 +45,35 @@ php artisan custom-fields:upgrade --skip=clear-caches php artisan custom-fields:upgrade --skip=email-format,phone-format ``` +## Picking Up New Migrations + +Package migrations are not run automatically by `php artisan migrate` after a version +bump — they're only copied into your app on first install, or when you explicitly +republish them: + +```bash +php artisan vendor:publish --tag="custom-fields-migrations" +php artisan migrate +``` + +For example, a release may relax the `custom_fields` unique key from +`(code, entity_type[, tenant])` to `(code, entity_type[, tenant], custom_field_section_id)` +so a field code can be reused across different sections — the shape `onlySections()` (see +[Builder Scoping](/essentials/builder-scoping)) relies on. Existing installs need the +republish-and-migrate step above to pick that change up; it is not applied automatically. + +**Before rolling that migration back**, resolve any rows that ended up sharing a code +across sections. The migration checks for this first and aborts with a clear error rather +than dropping the wide key and then failing to recreate the narrow one, which would leave +the table with no unique key at all. + +**NULL is not unique-constrained.** `custom_field_section_id` is nullable, and both MySQL +and Postgres treat `NULL` as distinct from every other value in a unique index — including +one that includes it. So after this migration, two sectionless fields +(`custom_field_section_id IS NULL`) can still share a code for the same entity type, a +collision the narrow key used to prevent. There is no schema-level fix for this; keep +sectionless codes unique at the application layer if you rely on that guarantee. + ## Breaking Changes ### High Impact diff --git a/docs/content/2.essentials/6.data-model.md b/docs/content/2.essentials/6.data-model.md index f130878d..62f90c62 100644 --- a/docs/content/2.essentials/6.data-model.md +++ b/docs/content/2.essentials/6.data-model.md @@ -27,7 +27,7 @@ The Custom Fields plugin employs a **Hybrid Entity-Attribute-Value (EAV) with Ty |--------|------|-------------| | `id` | bigint | Primary key | | `entity_type` | string | Polymorphic entity class | - | `code` | string | Unique identifier | + | `code` | string | Unique per entity type (+ tenant, when multi-tenancy is enabled) | | `name` | string | Display name | | `type` | string | Section type | | `width` | string | Section layout width (`CustomFieldWidth` enum: 25/33/50/66/75/100). Requires the `UI_SECTION_WIDTH_CONTROL` feature. | @@ -44,7 +44,7 @@ The Custom Fields plugin employs a **Hybrid Entity-Attribute-Value (EAV) with Ty | `id` | bigint | Primary key | | `custom_field_section_id` | bigint | Parent section | | `entity_type` | string | Polymorphic entity class | - | `code` | string | Unique identifier | + | `code` | string | Unique per entity type and section (+ tenant, when multi-tenancy is enabled) — not globally unique, so the same code can exist in two different sections. See [Builder Scoping](/essentials/builder-scoping) | | `name` | string | Display name | | `type` | string | Field type (text, number, etc.) | | `lookup_type` | string | For lookup fields | diff --git a/docs/content/2.essentials/7.builder-scoping.md b/docs/content/2.essentials/7.builder-scoping.md new file mode 100644 index 00000000..51dc6389 --- /dev/null +++ b/docs/content/2.essentials/7.builder-scoping.md @@ -0,0 +1,83 @@ +--- +title: Builder Scoping +description: Scope custom-field resolution to specific sections, and hook into code-uniqueness resolution +navigation: + icon: i-lucide-filter +--- + +## onlySections() + +By default, every builder (`CustomFields::form()`, `::infolist()`, `::table()`, +`::exporter()`, `::importer()`) resolves fields for the whole entity type. `onlySections()` +constrains that resolution to a specific set of `custom_field_sections` rows: + +```php +use Relaticle\CustomFields\Facades\CustomFields; + +CustomFields::form() + ->forModel($model) + ->onlySections([$sectionA->id, $sectionB->id]) + ->build(); +``` + +Passing `[]` (the default) means "no scope" — existing call sites that never call +`onlySections()` are unaffected. + +This exists for consumers that version their form definitions — for example, cloning a +section (and its fields) per form version. Field codes are normally unique per entity +type, so two versions of "the same field" would collide on `code` unless resolution is +scoped by section. `onlySections()` lets each version's builder only see its own +section(s), so the same code can live in more than one section without one bleeding into +the other's schema. + +`onlySections()` is inherited by every builder from `BaseBuilder`, including through +`FormBuilder::build()` / `InfolistBuilder::build()` — the scope is threaded through +`FormContainer` / `InfolistContainer` as well, so it applies whether you call `->build()` +or `->values()`. + +### The persistence contract — read this before relying on section-scoped codes + +`onlySections()` narrows *resolution* (which fields get loaded onto a form, infolist, or +table). It does not change how values are *saved*. `UsesCustomFields::saveCustomFields()` +iterates the model's custom-field-values relationship and writes each submitted value by +**field code**. If two sections share a code and that relationship isn't scoped to match +`onlySections()`, `saveCustomFields()` will silently write the same value to **both** +field rows — a data-corrupting outcome that has nothing to do with whether resolution +scoping itself is working correctly. + +If you use `onlySections()`, scope your model's custom-field-values relationship +(`customFields()` by default, or your override) to the same section(s). It is +entity-scoped by default and overridable per model. + +## CodeGenerator::resolveUniquenessScopeUsing() + +Auto-generated codes (`FIELD_CODE_AUTO_GENERATE`) are checked for collisions before use. +Register a callback to narrow that check the same way `onlySections()` narrows +resolution: + +```php +use Illuminate\Database\Eloquent\Builder; +use Relaticle\CustomFields\Support\CodeGenerator; + +CodeGenerator::resolveUniquenessScopeUsing( + fn (string $entityType, string $type, int|string|null $sectionId): ?Closure => $sectionId !== null + ? fn (Builder $query): Builder => $query->where('custom_field_section_id', $sectionId) + : null +); +``` + +The callback receives: + +- `$entityType` — the entity the field or section belongs to. +- `$type` — `'field'` or `'section'`, so you can scope differently per kind of code. +- `$sectionId` — the section the code is being generated within, or `null` when there + isn't one (for example, the sectionless field-management table). + +Return `null` to leave the uniqueness check global — the default, backward-compatible +behavior. Return a closure to narrow it: the closure receives the in-progress `Builder` +and must **return** the query to apply. `where()`-style mutation also works (it returns +the same instance), but a closure that hands back a different instance — e.g. +`$query->clone()->where(...)` — is honored too, since the return value is always what's +used. + +Register the callback once, typically in a service provider's `boot()` method. diff --git a/resources/lang/en/custom-fields.php b/resources/lang/en/custom-fields.php index 2fed43ef..1de8f52f 100644 --- a/resources/lang/en/custom-fields.php +++ b/resources/lang/en/custom-fields.php @@ -29,6 +29,7 @@ 'default_section_name' => 'Default', 'notifications' => [ 'created' => 'Section created', + 'duplicate_field_code' => 'This section already has a field with that code. Rename one of them before moving it here.', ], 'actions' => [ 'activate' => 'Activate', diff --git a/src/CustomFieldsServiceProvider.php b/src/CustomFieldsServiceProvider.php index 538e58c2..df9c83d1 100644 --- a/src/CustomFieldsServiceProvider.php +++ b/src/CustomFieldsServiceProvider.php @@ -206,6 +206,7 @@ private function getMigrations(): array { return [ 'create_custom_fields_table', + 'relax_custom_fields_unique_key', ]; } } diff --git a/src/Enums/VisibilityOperator.php b/src/Enums/VisibilityOperator.php index 1d4e57b8..3269e6a9 100644 --- a/src/Enums/VisibilityOperator.php +++ b/src/Enums/VisibilityOperator.php @@ -62,9 +62,13 @@ public function evaluate(mixed $fieldValue, mixed $expectedValue): bool }; } + /** + * The arm order here mirrors the JavaScript emitted by FrontendVisibilityService, so a + * condition resolves the same way on the server and in the live form. Reordering an arm + * desyncs the two engines. + */ private function evaluateEquals(mixed $fieldValue, mixed $expectedValue): bool { - // Handle null values if ($fieldValue === null && $expectedValue === null) { return true; } @@ -73,20 +77,69 @@ private function evaluateEquals(mixed $fieldValue, mixed $expectedValue): bool return false; } - // Handle arrays + // Two collections compare as sets, matching the client's sorted-JSON equality. + if (is_array($fieldValue) && is_array($expectedValue)) { + return $this->normalizeSet($fieldValue) === $this->normalizeSet($expectedValue); + } + + // A multi-value field against a single condition value means membership. Compared on the + // string form so a field holding [42] still matches the condition a text input stored + // as '42' - the same coercion the client applies. if (is_array($fieldValue)) { - return in_array($expectedValue, $fieldValue, true); + return in_array($this->stringifyScalar($expectedValue), $this->normalizeSet($fieldValue), true); + } + + if (is_array($expectedValue)) { + return false; } - // Handle strings (case-insensitive) + // formatJsValue() emits booleans and the literals 'true'/'false' alike as JS booleans, + // so the client cannot tell a real bool from its spelling. Compare on the spelling. + if (is_bool($fieldValue) || is_bool($expectedValue)) { + return strtolower($this->stringifyScalar($fieldValue)) === strtolower($this->stringifyScalar($expectedValue)); + } + + // Two strings compare as strings, so '42.5' and '42.50' stay distinct. if (is_string($fieldValue) && is_string($expectedValue)) { return strtolower($fieldValue) === strtolower($expectedValue); } - // Handle numeric values + // Mixed number/numeric-string compares numerically, matching the client's parseFloat + // branches. A numeric custom field reads back as an int while its condition is stored + // as the string a text input produced, so strict identity never matched. + if (is_numeric($fieldValue) && is_numeric($expectedValue)) { + return (float) $fieldValue === (float) $expectedValue; + } + return $fieldValue === $expectedValue; } + /** + * @param array $values + * @return array + */ + private function normalizeSet(array $values): array + { + $normalized = array_map(fn (mixed $value): string => $this->stringifyScalar($value), array_values($values)); + + sort($normalized); + + return $normalized; + } + + private function stringifyScalar(mixed $value): string + { + if (is_bool($value)) { + return $value ? 'true' : 'false'; + } + + if (is_scalar($value)) { + return (string) $value; + } + + return serialize($value); + } + private function evaluateContains(mixed $fieldValue, mixed $expectedValue): bool { if ($fieldValue === null || $expectedValue === null) { diff --git a/src/Filament/Integration/Builders/BaseBuilder.php b/src/Filament/Integration/Builders/BaseBuilder.php index dfa9d26f..d8817249 100644 --- a/src/Filament/Integration/Builders/BaseBuilder.php +++ b/src/Filament/Integration/Builders/BaseBuilder.php @@ -29,6 +29,9 @@ abstract class BaseBuilder protected array $only = []; + /** @var array */ + protected array $onlySections = []; + public function forSchema(Schema $schema): static { /** @var Model & HasCustomFields $model */ @@ -82,6 +85,30 @@ public function only(array $fieldCodes): static return $this; } + /** + * Constrain resolution to the given custom field sections. + * + * Field codes are unique per section, not globally, in consumers that version their + * sections. Scoping structurally lets two sections carry the same code without one + * bleeding into the other's schema. + * + * IMPORTANT: this only narrows resolution (what gets loaded onto the form/infolist). + * It does not change how UsesCustomFields::saveCustomFields() saves — that method + * writes by code against the model's customFields() relationship. If two sections + * share a code and customFields() isn't scoped to match, saveCustomFields() will + * write the same submitted value to both field rows. Scoping resolution alone is not + * sufficient; the model's customFields() relation must be scoped to the same + * section(s) too. See the "Builder Scoping" docs page. + * + * @param array $sectionIds + */ + public function onlySections(array $sectionIds): static + { + $this->onlySections = $sectionIds; + + return $this; + } + /** * @return Collection */ @@ -94,6 +121,10 @@ protected function getFilteredSections(): Collection /** @var Collection $sections */ $sections = $this->sections + ->when($this->onlySections !== [], fn (Builder $query): Builder => $query->whereIn( + $this->sections->getModel()->getQualifiedKeyName(), + $this->onlySections + )) ->with(['fields' => function (mixed $query): mixed { return $query ->when($this instanceof TableBuilder, fn (CustomFieldQueryBuilder $q, bool $condition): CustomFieldQueryBuilder => $q->visibleInList()) @@ -122,6 +153,17 @@ protected function getFilteredSections(): Collection */ protected function getFieldsDirectly(): Collection { + /* + * custom_field_section_id only exists on the table when SYSTEM_SECTIONS was + * enabled at migration time, and this method is exclusively the sections-disabled + * path (see getAllFields()). A section scope can never match anything here — there + * is no section table to resolve it against — so return empty rather than filter + * by a column that may not exist. + */ + if ($this->onlySections !== [] && ! FeatureManager::isEnabled(CustomFieldsFeature::SYSTEM_SECTIONS)) { + return collect(); + } + return CustomFields::newCustomFieldModel()::forMorphEntity($this->model::class) ->when($this instanceof TableBuilder, fn (CustomFieldQueryBuilder $q): CustomFieldQueryBuilder => $q->visibleInList()) ->when($this instanceof InfolistBuilder, fn (CustomFieldQueryBuilder $q): CustomFieldQueryBuilder => $q->visibleInView()) diff --git a/src/Filament/Integration/Builders/FormBuilder.php b/src/Filament/Integration/Builders/FormBuilder.php index 4e1c4a7e..62bf6e46 100644 --- a/src/Filament/Integration/Builders/FormBuilder.php +++ b/src/Filament/Integration/Builders/FormBuilder.php @@ -24,7 +24,8 @@ public function build(): Grid $container = FormContainer::make() ->forModel($this->explicitModel ?? null) ->only($this->only) - ->except($this->except); + ->except($this->except) + ->onlySections($this->onlySections); // Only set withoutSections if explicitly configured if ($this->withoutSections !== null) { diff --git a/src/Filament/Integration/Builders/FormContainer.php b/src/Filament/Integration/Builders/FormContainer.php index d628ad71..66c1c1f4 100644 --- a/src/Filament/Integration/Builders/FormContainer.php +++ b/src/Filament/Integration/Builders/FormContainer.php @@ -15,6 +15,9 @@ final class FormContainer extends Grid private array $only = []; + /** @var array */ + private array $onlySections = []; + private ?bool $withoutSections = null; public static function make(array|int|null $columns = 12): static @@ -52,6 +55,16 @@ public function only(array $fieldCodes): static return $this; } + /** + * @param array $sectionIds + */ + public function onlySections(array $sectionIds): static + { + $this->onlySections = $sectionIds; + + return $this; + } + public function withoutSections(bool $withoutSections = true): static { $this->withoutSections = $withoutSections; @@ -79,6 +92,7 @@ private function generateSchema(): array ->withoutSections($withoutSections) ->only($this->only) ->except($this->except) + ->onlySections($this->onlySections) ->values() ->toArray(); } diff --git a/src/Filament/Integration/Builders/InfolistBuilder.php b/src/Filament/Integration/Builders/InfolistBuilder.php index 4d5973d8..972d8eb8 100644 --- a/src/Filament/Integration/Builders/InfolistBuilder.php +++ b/src/Filament/Integration/Builders/InfolistBuilder.php @@ -31,7 +31,8 @@ public function build(): InfolistContainer ->hiddenLabels($this->hiddenLabels) ->visibleWhenFilled($this->visibleWhenFilled) ->only($this->only) - ->except($this->except); + ->except($this->except) + ->onlySections($this->onlySections); // Only set withoutSections if explicitly configured if ($this->withoutSections !== null) { diff --git a/src/Filament/Integration/Builders/InfolistContainer.php b/src/Filament/Integration/Builders/InfolistContainer.php index 1e5942c8..7f7a37d8 100644 --- a/src/Filament/Integration/Builders/InfolistContainer.php +++ b/src/Filament/Integration/Builders/InfolistContainer.php @@ -16,6 +16,9 @@ final class InfolistContainer extends Grid private array $only = []; + /** @var array */ + private array $onlySections = []; + private bool $hiddenLabels = false; private bool $visibleWhenFilled = false; @@ -57,6 +60,16 @@ public function only(array $fieldCodes): static return $this; } + /** + * @param array $sectionIds + */ + public function onlySections(array $sectionIds): static + { + $this->onlySections = $sectionIds; + + return $this; + } + public function hiddenLabels(bool $hiddenLabels = true): static { $this->hiddenLabels = $hiddenLabels; @@ -98,6 +111,7 @@ private function generateSchema(): array ->forModel($model) ->only($this->only) ->except($this->except) + ->onlySections($this->onlySections) ->hiddenLabels($this->hiddenLabels) ->visibleWhenFilled($this->visibleWhenFilled) ->withoutSections($withoutSections); diff --git a/src/Filament/Management/Schemas/FieldForm.php b/src/Filament/Management/Schemas/FieldForm.php index c05ead07..7e7d7d0f 100644 --- a/src/Filament/Management/Schemas/FieldForm.php +++ b/src/Filament/Management/Schemas/FieldForm.php @@ -42,6 +42,9 @@ class FieldForm implements FormInterface /** @var ?Closure(?CustomFieldSection): ?Closure */ private static ?Closure $uniqueNameRuleModifierResolver = null; + /** @var ?Closure(?CustomFieldSection): ?Closure */ + private static ?Closure $uniqueCodeRuleModifierResolver = null; + /** * Register a resolver that scopes the field-name uniqueness rule beyond the default * entity-type (+ tenant) scope. The resolver receives the section the field belongs to @@ -56,6 +59,22 @@ public static function resolveUniqueRuleModifierUsing(?Closure $resolver): void self::$uniqueNameRuleModifierResolver = $resolver; } + /** + * Register a resolver that scopes the field-code uniqueness rule beyond the default + * entity-type (+ tenant) scope. Same contract as resolveUniqueRuleModifierUsing(), kept + * as a separate hook because code and name typically need different scoping: name is a + * cosmetic label a consumer may want unique per parent form only, while code is a + * stable identity other systems (e.g. reporting) key off of, so its scope is usually + * "everything except a defined set of related forms" rather than "this one form". + * Register once. + * + * @param ?Closure(?CustomFieldSection $section): ?Closure $resolver + */ + public static function resolveUniqueCodeRuleModifierUsing(?Closure $resolver): void + { + self::$uniqueCodeRuleModifierResolver = $resolver; + } + private static function resolveUniqueNameRuleModifier(?CustomFieldSection $section): ?Closure { if (self::$uniqueNameRuleModifierResolver instanceof Closure) { @@ -65,6 +84,15 @@ private static function resolveUniqueNameRuleModifier(?CustomFieldSection $secti return null; } + private static function resolveUniqueCodeRuleModifier(?CustomFieldSection $section): ?Closure + { + if (self::$uniqueCodeRuleModifierResolver instanceof Closure) { + return (self::$uniqueCodeRuleModifierResolver)($section); + } + + return null; + } + /** * Disable field when editing a system-defined custom field. */ @@ -137,6 +165,7 @@ private static function getValidationSchema(): array public static function schema(bool $withOptionsRelationship = true, ?CustomFieldSection $section = null): array { $uniqueNameRuleModifier = self::resolveUniqueNameRuleModifier($section); + $uniqueCodeRuleModifier = self::resolveUniqueCodeRuleModifier($section); $optionsRepeater = Repeater::make('options') ->table([ @@ -293,15 +322,23 @@ public static function schema(bool $withOptionsRelationship = true, ?CustomField table: CustomFields::customFieldModel(), column: 'code', ignoreRecord: true, - modifyRuleUsing: fn (Unique $rule, Get $get) => $rule - ->where('entity_type', $get('entity_type')) - ->when( - FeatureManager::isEnabled(CustomFieldsFeature::SYSTEM_MULTI_TENANCY), - fn (Unique $rule) => $rule->where( - config('custom-fields.database.column_names.tenant_foreign_key'), - TenantContextService::getCurrentTenantId() - ) - ) + modifyRuleUsing: function (Unique $rule, Get $get) use ($uniqueCodeRuleModifier): Unique { + $rule = $rule + ->where('entity_type', $get('entity_type')) + ->when( + FeatureManager::isEnabled(CustomFieldsFeature::SYSTEM_MULTI_TENANCY), + fn (Unique $rule) => $rule->where( + config('custom-fields.database.column_names.tenant_foreign_key'), + TenantContextService::getCurrentTenantId() + ) + ); + + if ($uniqueCodeRuleModifier instanceof Closure) { + return $uniqueCodeRuleModifier($rule, $get); + } + + return $rule; + } ) ->afterStateUpdated(function (Set $set, ?string $state): void { $set('code', Str::of($state)->slug('_')->toString()); diff --git a/src/Livewire/Concerns/CreatesCustomFields.php b/src/Livewire/Concerns/CreatesCustomFields.php index 8c64245b..e67e03ea 100644 --- a/src/Livewire/Concerns/CreatesCustomFields.php +++ b/src/Livewire/Concerns/CreatesCustomFields.php @@ -20,7 +20,7 @@ protected function mutateFieldData(array $data, string $entityType, int|string|n } if (FeatureManager::isEnabled(CustomFieldsFeature::FIELD_CODE_AUTO_GENERATE) && blank($data['code'] ?? null)) { - $data['code'] = CodeGenerator::generateUniqueFieldCode($data['name'], $entityType); + $data['code'] = CodeGenerator::generateUniqueFieldCode($data['name'], $entityType, sectionId: $sectionId); } $result = [ diff --git a/src/Livewire/ManageCustomFieldSection.php b/src/Livewire/ManageCustomFieldSection.php index 8d75c973..98cb829f 100644 --- a/src/Livewire/ManageCustomFieldSection.php +++ b/src/Livewire/ManageCustomFieldSection.php @@ -11,9 +11,11 @@ use Filament\Actions\Contracts\HasActions; use Filament\Forms\Concerns\InteractsWithForms; use Filament\Forms\Contracts\HasForms; +use Filament\Notifications\Notification; use Filament\Support\Enums\Size; use Filament\Support\Enums\Width; use Illuminate\Contracts\View\View; +use Illuminate\Database\Eloquent\Model; use Livewire\Component; use Relaticle\CustomFields\CustomFields; use Relaticle\CustomFields\CustomFieldsPlugin; @@ -45,6 +47,22 @@ public static function resolveUniqueRuleModifierUsing(?Closure $callback): void public function updateFieldsOrder(int|string $sectionId, array $fields): void { $model = CustomFields::newCustomFieldModel(); + + /* + * Before the unique key was relaxed to include custom_field_section_id, two fields + * could never share a code anywhere in the entity type, so a drop target could never + * already hold a colliding code. Now it can — check before writing, or the second + * update() in the loop below throws an unhandled QueryException. + */ + if ($this->fieldsHaveDuplicateCode($model, $fields)) { + Notification::make() + ->danger() + ->title(__('custom-fields::custom-fields.section.notifications.duplicate_field_code')) + ->send(); + + return; + } + foreach ($fields as $index => $field) { $model->query() ->withDeactivated() @@ -59,6 +77,20 @@ public function updateFieldsOrder(int|string $sectionId, array $fields): void $this->dispatch('fields-reordered')->to('manage-custom-field-section'); } + /** + * @param array $fieldIds + */ + private function fieldsHaveDuplicateCode(Model $model, array $fieldIds): bool + { + return $model->query() + ->withDeactivated() + ->whereIn($model->getKeyName(), $fieldIds) + ->select('code') + ->groupBy('code') + ->havingRaw('count(*) > 1') + ->exists(); + } + public function actions(): ?ActionGroup { if ($this->section->hasSystemDefinedFields()) { diff --git a/src/Services/Visibility/FrontendVisibilityService.php b/src/Services/Visibility/FrontendVisibilityService.php index 91eb9ca5..e868d0b6 100644 --- a/src/Services/Visibility/FrontendVisibilityService.php +++ b/src/Services/Visibility/FrontendVisibilityService.php @@ -347,6 +347,10 @@ private function buildNotEqualsExpression( /** * Build standard equals expression for non-optionable fields. + * + * Two strings are compared case-insensitively to mirror VisibilityOperator::evaluateEquals(), + * which folds both sides through strtolower(). Without this the server shows a field for + * "active" vs "Active" while the client hides it. */ private function buildStandardEqualsExpression( string $fieldValue, @@ -359,28 +363,39 @@ private function buildStandardEqualsExpression( const fieldVal = {$fieldValue}; const compareVal = {$jsValue}; if (!Array.isArray(fieldVal) || !Array.isArray(compareVal)) return false; - return JSON.stringify(fieldVal.sort()) === JSON.stringify(compareVal.sort()); + const norm = a => JSON.stringify(a.map(v => String(v)).sort()); + return norm(fieldVal) === norm(compareVal); })()"; } return "(() => { const fieldVal = {$fieldValue}; const compareVal = {$jsValue}; + const isBlank = v => v === null || v === undefined; + const isNumericLike = v => typeof v !== 'boolean' && String(v).trim() !== '' && !isNaN(Number(v)); - if (typeof fieldVal === typeof compareVal) { - return fieldVal === compareVal; + if (isBlank(fieldVal) && isBlank(compareVal)) { + return true; } - if ((fieldVal === null || fieldVal === undefined) && (compareVal === null || compareVal === undefined)) { - return true; + if (isBlank(fieldVal) || isBlank(compareVal)) { + return false; + } + + if (Array.isArray(fieldVal)) { + return fieldVal.map(v => String(v)).includes(String(compareVal)); + } + + if (typeof fieldVal === 'boolean' || typeof compareVal === 'boolean') { + return String(fieldVal).toLowerCase() === String(compareVal).toLowerCase(); } - if (typeof fieldVal === 'number' && typeof compareVal === 'string' && !isNaN(parseFloat(compareVal))) { - return fieldVal === parseFloat(compareVal); + if (typeof fieldVal === 'string' && typeof compareVal === 'string') { + return fieldVal.toLowerCase() === compareVal.toLowerCase(); } - if (typeof fieldVal === 'string' && typeof compareVal === 'number' && !isNaN(parseFloat(fieldVal))) { - return parseFloat(fieldVal) === compareVal; + if (isNumericLike(fieldVal) && isNumericLike(compareVal)) { + return Number(fieldVal) === Number(compareVal); } return String(fieldVal) === String(compareVal); @@ -605,9 +620,6 @@ private function formatJsValue(mixed $value): string is_string($value) => $this->toJsString($value), is_int($value) => (string) $value, is_float($value) => number_format($value, 10, '.', ''), - is_numeric($value) => str_contains($value, '.') - ? number_format((float) $value, 10, '.', '') - : (string) ((int) $value), is_array($value) => collect($value) ->map(fn (mixed $item): string => $this->formatJsValue($item)) ->pipe( diff --git a/src/Support/CodeGenerator.php b/src/Support/CodeGenerator.php index 0cdeced4..88857a16 100644 --- a/src/Support/CodeGenerator.php +++ b/src/Support/CodeGenerator.php @@ -4,6 +4,8 @@ namespace Relaticle\CustomFields\Support; +use Closure; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Support\Str; use Relaticle\CustomFields\CustomFields; use Relaticle\CustomFields\Enums\CustomFieldsFeature; @@ -15,6 +17,9 @@ */ final class CodeGenerator { + /** @var (Closure(string, string, int|string|null): (Closure(Builder): Builder)|null)|null */ + private static ?Closure $uniquenessScopeResolver = null; + /** * Generate a slug-style code from a name. */ @@ -23,10 +28,24 @@ public static function generateFromName(string $name): string return Str::of($name)->slug('_')->toString(); } + /** + * Narrow the uniqueness check to a subset of rows. + * + * The callback receives the entity type, either 'field' or 'section', and the section + * the code is being generated within (null when there is none). It returns a query + * scope closure, or null to leave the check global. + * + * @param (Closure(string, string, int|string|null): (Closure(Builder): Builder)|null)|null $callback + */ + public static function resolveUniquenessScopeUsing(?Closure $callback): void + { + self::$uniquenessScopeResolver = $callback; + } + /** * Generate a unique code for a custom field within an entity type. */ - public static function generateUniqueFieldCode(string $name, string $entityType, ?int $ignoreId = null): string + public static function generateUniqueFieldCode(string $name, string $entityType, ?int $ignoreId = null, int|string|null $sectionId = null): string { $baseCode = self::generateFromName($name); @@ -34,7 +53,8 @@ public static function generateUniqueFieldCode(string $name, string $entityType, $baseCode, $entityType, 'field', - $ignoreId + $ignoreId, + $sectionId ); } @@ -60,12 +80,13 @@ private static function ensureUniqueCode( string $baseCode, string $entityType, string $type, - ?int $ignoreId = null + ?int $ignoreId = null, + int|string|null $sectionId = null ): string { $code = $baseCode; $counter = 1; - while (self::codeExists($code, $entityType, $type, $ignoreId)) { + while (self::codeExists($code, $entityType, $type, $ignoreId, $sectionId)) { $code = sprintf('%s_%d', $baseCode, $counter); $counter++; } @@ -80,7 +101,8 @@ private static function codeExists( string $code, string $entityType, string $type, - ?int $ignoreId = null + ?int $ignoreId = null, + int|string|null $sectionId = null ): bool { $model = $type === 'field' ? CustomFields::newCustomFieldModel() @@ -95,6 +117,14 @@ private static function codeExists( $query->where($model->getKeyName(), '!=', $ignoreId); } + if (self::$uniquenessScopeResolver instanceof Closure) { + $scope = (self::$uniquenessScopeResolver)($entityType, $type, $sectionId); + + if ($scope !== null) { + $query = $scope($query); + } + } + if (FeatureManager::isEnabled(CustomFieldsFeature::SYSTEM_MULTI_TENANCY)) { $query->where( config('custom-fields.database.column_names.tenant_foreign_key'), diff --git a/tests/Feature/Admin/Pages/CustomFieldsFieldManagementTest.php b/tests/Feature/Admin/Pages/CustomFieldsFieldManagementTest.php index 3fdd2c73..1aa53c33 100644 --- a/tests/Feature/Admin/Pages/CustomFieldsFieldManagementTest.php +++ b/tests/Feature/Admin/Pages/CustomFieldsFieldManagementTest.php @@ -188,6 +188,41 @@ ]); }); + it('notifies instead of 500ing when a drag would create a duplicate code within the target section', function (): void { + $targetSection = $this->section; + $sourceSection = CustomFieldSection::factory() + ->forEntityType($this->userEntityType) + ->create(); + + $existingField = CustomField::factory() + ->ofType('text') + ->create([ + 'custom_field_section_id' => $targetSection->getKey(), + 'entity_type' => $this->userEntityType, + 'code' => 'shared_code', + 'sort_order' => 0, + ]); + + $draggedField = CustomField::factory() + ->ofType('text') + ->create([ + 'custom_field_section_id' => $sourceSection->getKey(), + 'entity_type' => $this->userEntityType, + 'code' => 'shared_code', + 'sort_order' => 0, + ]); + + livewire(ManageCustomFieldSection::class, [ + 'section' => $targetSection, + 'entityType' => $this->userEntityType, + ]) + ->call('updateFieldsOrder', $targetSection->getKey(), [$existingField->getKey(), $draggedField->getKey()]) + ->assertNotified(); + + expect($draggedField->fresh()) + ->custom_field_section_id->toBe($sourceSection->getKey()); + }); + }); describe('ManageCustomField - Field Actions', function (): void { diff --git a/tests/Feature/ConsumerScopeHooksTest.php b/tests/Feature/ConsumerScopeHooksTest.php index af0c3cff..1c2fdd48 100644 --- a/tests/Feature/ConsumerScopeHooksTest.php +++ b/tests/Feature/ConsumerScopeHooksTest.php @@ -2,15 +2,29 @@ declare(strict_types=1); +use Filament\Forms\Components\Field; +use Filament\Infolists\Components\Entry; use Filament\Schemas\Components\Component; use Filament\Schemas\Components\Utilities\Get; +use Filament\Schemas\Schema as FilamentSchema; +use Illuminate\Database\Eloquent\Builder; +use Illuminate\Database\Events\QueryExecuted; +use Illuminate\Database\Schema\Blueprint; +use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Schema; use Relaticle\CustomFields\Enums\CustomFieldsFeature; +use Relaticle\CustomFields\Facades\CustomFields; use Relaticle\CustomFields\FeatureSystem\FeatureConfigurator; use Relaticle\CustomFields\Filament\Management\Forms\Components\VisibilityComponent; use Relaticle\CustomFields\Filament\Management\Schemas\FieldForm; +use Relaticle\CustomFields\Livewire\ManageCustomField; +use Relaticle\CustomFields\Livewire\ManageCustomFieldSection; use Relaticle\CustomFields\Models\CustomField; use Relaticle\CustomFields\Models\CustomFieldSection; +use Relaticle\CustomFields\Support\CodeGenerator; use Relaticle\CustomFields\Tests\Fixtures\Models\Post; +use Relaticle\CustomFields\Tests\Fixtures\Models\User; +use Relaticle\CustomFields\Tests\Fixtures\Resources\Posts\Pages\CreatePost; /** * Consumer scoping hooks let an app (e.g. a versioned-form builder) constrain the @@ -27,6 +41,8 @@ afterEach(function (): void { VisibilityComponent::resolveAvailableFieldsScopeUsing(null); FieldForm::resolveUniqueRuleModifierUsing(null); + FieldForm::resolveUniqueCodeRuleModifierUsing(null); + CodeGenerator::resolveUniquenessScopeUsing(null); }); function nullGet(): Get @@ -50,6 +66,23 @@ function availableFields(VisibilityComponent $component): array return $method->invoke($component, nullGet()); } +/** + * With SYSTEM_SECTIONS enabled, FormBuilder::values()/InfolistBuilder::values() return one + * component per section, not one per field, so asserting a count on the outer collection + * can't tell "the section kept N fields" from "there are N sections" apart. Reach into the + * section component's raw childComponents (set by ->schema()) to count what it actually + * carries, without needing the full Livewire-attached schema tree just to read it back. + * + * @return array + */ +function sectionFieldComponents(Component $section): array +{ + $property = new ReflectionProperty($section, 'childComponents'); + $property->setAccessible(true); + + return $property->getValue($section)['default'] ?? []; +} + describe('VisibilityComponent available-fields scope resolver', function (): void { it('lists all entity fields when no resolver is registered (backward compatible)', function (): void { $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'A', 'code' => 'a']); @@ -110,3 +143,468 @@ function availableFields(VisibilityComponent $component): array expect(FieldForm::schema(section: $section))->toBeArray()->not->toBeEmpty(); }); }); + +describe('FieldForm unique-code modifier resolver', function (): void { + beforeEach(function (): void { + config()->set('custom-fields.features', FeatureConfigurator::configure() + ->enable(CustomFieldsFeature::FIELD_CONDITIONAL_VISIBILITY, CustomFieldsFeature::SYSTEM_SECTIONS) + ); + + $this->actingAs(User::factory()->create()); + }); + + it('builds the field schema unchanged when no resolver is registered (backward compatible)', function (): void { + $schema = FieldForm::schema(); + + expect($schema)->toBeArray()->not->toBeEmpty(); + }); + + it('rejects a duplicate code across sections with no resolver registered (backward compatible)', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'A', 'code' => 'a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'B', 'code' => 'b']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionA->id, + 'entity_type' => Post::class, + 'name' => 'Existing', + 'code' => 'shared_code', + 'type' => 'text', + ]); + + livewire(ManageCustomFieldSection::class, ['entityType' => Post::class, 'section' => $sectionB]) + ->callAction('createField', data: [ + 'name' => 'New Field', + 'code' => 'shared_code', + 'type' => 'text', + ]) + ->assertHasActionErrors(['code']); + }); + + it('allows editing a field to reuse a code excluded by the registered resolver', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'A', 'code' => 'a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'B', 'code' => 'b']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionA->id, + 'entity_type' => Post::class, + 'name' => 'Original', + 'code' => 'reused_code', + 'type' => 'text', + ]); + + $clone = CustomField::factory()->create([ + 'custom_field_section_id' => $sectionB->id, + 'entity_type' => Post::class, + 'name' => 'Cloned', + 'code' => 'reused_code', + 'type' => 'text', + ]); + + /* + * Mirrors a consumer that scopes code uniqueness to "everything outside this set of + * related sections" (e.g. a version lineage) rather than "just this one section" -- + * the shape CRITICAL-1 in the whole-branch review required. + */ + FieldForm::resolveUniqueCodeRuleModifierUsing( + fn (?CustomFieldSection $s): ?Closure => $s instanceof CustomFieldSection + ? fn ($rule) => $rule->whereNotIn('custom_field_section_id', [$sectionA->id, $sectionB->id]) + : null + ); + + livewire(ManageCustomField::class, ['field' => $clone]) + ->callAction('edit', data: [ + 'name' => 'Cloned', + 'code' => 'reused_code', + 'type' => 'text', + ]) + ->assertHasNoActionErrors(); + }); + + it('still rejects a code collision outside the resolver-defined scope', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'A', 'code' => 'a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'B', 'code' => 'b']); + $sectionOutside = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Outside', 'code' => 'outside']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionOutside->id, + 'entity_type' => Post::class, + 'name' => 'Outside field', + 'code' => 'outside_code', + 'type' => 'text', + ]); + + $target = CustomField::factory()->create([ + 'custom_field_section_id' => $sectionB->id, + 'entity_type' => Post::class, + 'name' => 'Target', + 'code' => 'target_code', + 'type' => 'text', + ]); + + FieldForm::resolveUniqueCodeRuleModifierUsing( + fn (?CustomFieldSection $s): ?Closure => $s instanceof CustomFieldSection + ? fn ($rule) => $rule->whereNotIn('custom_field_section_id', [$sectionA->id, $sectionB->id]) + : null + ); + + livewire(ManageCustomField::class, ['field' => $target]) + ->callAction('edit', data: [ + 'name' => 'Target', + 'code' => 'outside_code', + 'type' => 'text', + ]) + ->assertHasActionErrors(['code']); + }); +}); + +describe('CodeGenerator uniqueness scope resolver', function (): void { + it('suffixes a colliding code when no resolver is registered (backward compatible)', function (): void { + $section = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Scope Default', 'code' => 'scope_default']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'entity_type' => Post::class, + 'name' => 'HMIS ID', + 'code' => 'hmis_id', + 'type' => 'text', + ]); + + expect(CodeGenerator::generateUniqueFieldCode('HMIS ID', Post::class)) + ->toBe('hmis_id_1'); + }); + + it('returns the base code when the collision is outside the registered scope', function (): void { + $outside = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Scope Outside', 'code' => 'scope_outside']); + $inside = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Scope Inside', 'code' => 'scope_inside']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $outside->id, + 'entity_type' => Post::class, + 'name' => 'HMIS ID', + 'code' => 'hmis_id', + 'type' => 'text', + ]); + + CodeGenerator::resolveUniquenessScopeUsing( + fn (string $entityType, string $type, int|string|null $sectionId): ?Closure => $type === 'field' && $sectionId !== null + ? fn (Builder $query): Builder => $query->where('custom_field_section_id', $sectionId) + : null + ); + + expect(CodeGenerator::generateUniqueFieldCode('HMIS ID', Post::class, sectionId: $inside->id)) + ->toBe('hmis_id'); + }); + + it('still suffixes when the collision is inside the registered scope', function (): void { + $inside = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Scope Inside Only', 'code' => 'scope_inside_only']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $inside->id, + 'entity_type' => Post::class, + 'name' => 'HMIS ID', + 'code' => 'hmis_id', + 'type' => 'text', + ]); + + CodeGenerator::resolveUniquenessScopeUsing( + fn (string $entityType, string $type, int|string|null $sectionId): ?Closure => $type === 'field' && $sectionId !== null + ? fn (Builder $query): Builder => $query->where('custom_field_section_id', $sectionId) + : null + ); + + expect(CodeGenerator::generateUniqueFieldCode('HMIS ID', Post::class, sectionId: $inside->id)) + ->toBe('hmis_id_1'); + }); + + it('applies a scope closure that returns a new builder instance instead of mutating in place', function (): void { + $outside = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Cloned Outside', 'code' => 'cloned_outside']); + $inside = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Cloned Inside', 'code' => 'cloned_inside']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $outside->id, + 'entity_type' => Post::class, + 'name' => 'HMIS ID', + 'code' => 'hmis_id', + 'type' => 'text', + ]); + + /* + * Builder::where() mutates and returns the same instance, so a mutation-style scope + * closure would pass this even if the code discarded its return value. clone() is + * what actually discriminates: it hands back a distinct instance, so only code that + * reassigns $query to the closure's return value picks the narrowed clone up. + */ + CodeGenerator::resolveUniquenessScopeUsing( + fn (string $entityType, string $type, int|string|null $sectionId): ?Closure => $type === 'field' && $sectionId !== null + ? fn (Builder $query): Builder => $query->clone()->where('custom_field_section_id', $sectionId) + : null + ); + + expect(CodeGenerator::generateUniqueFieldCode('HMIS ID', Post::class, sectionId: $inside->id)) + ->toBe('hmis_id'); + }); + + it('passes null as the section id to the resolver when generateUniqueFieldCode() is called without one', function (): void { + $section = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Sectionless', 'code' => 'sectionless']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'entity_type' => Post::class, + 'name' => 'HMIS ID', + 'code' => 'hmis_id', + 'type' => 'text', + ]); + + $receivedSectionId = 'not-called'; + + CodeGenerator::resolveUniquenessScopeUsing( + function (string $entityType, string $type, int|string|null $sectionId) use (&$receivedSectionId): ?Closure { + $receivedSectionId = $sectionId; + + return null; + } + ); + + CodeGenerator::generateUniqueFieldCode('HMIS ID', Post::class); + + expect($receivedSectionId)->toBeNull(); + }); +}); + +describe('BaseBuilder onlySections() scope', function (): void { + beforeEach(function (): void { + config()->set('custom-fields.features', FeatureConfigurator::configure() + ->enable(CustomFieldsFeature::FIELD_CONDITIONAL_VISIBILITY, CustomFieldsFeature::SYSTEM_SECTIONS) + ); + }); + + it('scopes resolution to the given sections when two sections share a field code', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Qualifying A', 'code' => 'qualifying_a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Qualifying B', 'code' => 'qualifying_b']); + + foreach ([$sectionA, $sectionB] as $section) { + CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'entity_type' => Post::class, + 'name' => 'Shared', + 'code' => 'shared_code', + 'type' => 'text', + ]); + } + + $scoped = CustomFields::form() + ->forModel(Post::class) + ->onlySections([$sectionA->id]) + ->values(); + + $unscoped = CustomFields::form() + ->forModel(Post::class) + ->values(); + + expect($scoped)->toHaveCount(1) + ->and($unscoped)->toHaveCount(2); + }); + + it('treats an empty section scope as no scope', function (): void { + $section = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Only Section', 'code' => 'only_section']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'entity_type' => Post::class, + 'name' => 'A Field', + 'code' => 'a_field', + 'type' => 'text', + ]); + + expect( + CustomFields::form() + ->forModel(Post::class) + ->onlySections([]) + ->values() + )->toHaveCount(1); + }); + + it('composes section scope with only() field codes', function (): void { + $section = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Combo', 'code' => 'combo']); + + foreach (['keep_me', 'drop_me'] as $index => $code) { + CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'entity_type' => Post::class, + 'name' => ucfirst($code), + 'code' => $code, + 'type' => 'text', + 'sort_order' => $index, + ]); + } + + /* + * With SYSTEM_SECTIONS enabled, values() returns one component per section (there is + * only ever the one section here), so asserting a count on the outer collection can't + * tell "only() dropped a field" from "only() dropped nothing" apart — it stays 1 + * either way. Assert on the section's own field count instead so this discriminates. + */ + $sections = CustomFields::form() + ->forModel(Post::class) + ->onlySections([$section->id]) + ->only(['keep_me']) + ->values(); + + expect($sections)->toHaveCount(1) + ->and(sectionFieldComponents($sections->first()))->toHaveCount(1); + }); +}); + +describe('BaseBuilder onlySections() scope on a sections-disabled install', function (): void { + /* + * custom_field_section_id only exists on the table when SYSTEM_SECTIONS was enabled at + * migration time. Toggling the feature flag at runtime does not remove an already + * migrated column, so the column is dropped here to reproduce a genuine + * sections-were-never-enabled install rather than merely flipping the config flag. + */ + beforeEach(function (): void { + config()->set('custom-fields.features', FeatureConfigurator::configure() + ->enable(CustomFieldsFeature::FIELD_CONDITIONAL_VISIBILITY) + ); + + collect(Schema::getIndexes('custom_fields')) + ->filter(fn (array $index): bool => in_array('custom_field_section_id', $index['columns'], true)) + ->each(fn (array $index) => DB::statement("DROP INDEX \"{$index['name']}\"")); + + Schema::table('custom_fields', fn (Blueprint $table) => $table->dropColumn('custom_field_section_id')); + }); + + it('returns fields unchanged when the section scope is empty', function (): void { + CustomField::factory()->create([ + 'entity_type' => Post::class, + 'name' => 'Alpha', + 'code' => 'alpha_flat', + 'type' => 'text', + ]); + + expect(CustomFields::form()->forModel(Post::class)->onlySections([])->values()) + ->toHaveCount(1); + }); + + it('returns an empty collection instead of throwing when a section scope is requested', function (): void { + CustomField::factory()->create([ + 'entity_type' => Post::class, + 'name' => 'Alpha', + 'code' => 'alpha_flat', + 'type' => 'text', + ]); + + /* + * SQLite falls back to treating an unresolvable double-quoted identifier as a + * string literal instead of raising "no such column" (the error MySQL, the + * project's production driver, actually raises), so a dropped-column WHERE clause + * silently matches zero rows here either way. A query-log assertion is the + * driver-agnostic way to prove the guard short-circuits before any query runs, + * rather than merely happening to agree with the guarded result by accident. + */ + $customFieldsTableQueried = false; + + DB::listen(function (QueryExecuted $query) use (&$customFieldsTableQueried): void { + if (str_contains($query->sql, 'custom_fields')) { + $customFieldsTableQueried = true; + } + }); + + $result = CustomFields::form()->forModel(Post::class)->onlySections([1])->values(); + + expect($customFieldsTableQueried)->toBeFalse() + ->and($result)->toHaveCount(0); + }); +}); + +describe('FormContainer/InfolistContainer onlySections() scope', function (): void { + beforeEach(function (): void { + config()->set('custom-fields.features', FeatureConfigurator::configure() + ->enable(CustomFieldsFeature::FIELD_CONDITIONAL_VISIBILITY, CustomFieldsFeature::SYSTEM_SECTIONS) + ); + + $this->actingAs(User::factory()->create()); + }); + + /* + * Filament's getFlatComponents() keys its result by component statePath, so two + * fields sharing a code would collapse into a single array entry regardless of + * onlySections() — a false negative unrelated to scoping. Distinct codes per + * section are what let the assertion actually discriminate scoped vs. unscoped. + */ + it('honors the section scope through FormBuilder::build(), not just values()', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Built A', 'code' => 'built_a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Built B', 'code' => 'built_b']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionA->id, + 'entity_type' => Post::class, + 'name' => 'Built A Field', + 'code' => 'built_a_field', + 'type' => 'text', + ]); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionB->id, + 'entity_type' => Post::class, + 'name' => 'Built B Field', + 'code' => 'built_b_field', + 'type' => 'text', + ]); + + $container = CustomFields::form() + ->forModel(Post::class) + ->onlySections([$sectionA->id]) + ->build(); + + $schema = FilamentSchema::make(livewire(CreatePost::class)->instance()) + ->model(Post::class) + ->components([$container]); + + $fieldNames = collect($schema->getFlatComponents()) + ->filter(fn (object $component): bool => $component instanceof Field) + ->map(fn (Field $component): string => $component->getName()); + + expect($fieldNames)->toHaveCount(1) + ->and($fieldNames)->toContain('custom_fields.built_a_field') + ->and($fieldNames)->not->toContain('custom_fields.built_b_field'); + }); + + it('honors the section scope through InfolistBuilder::build(), not just values()', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Info Built A', 'code' => 'info_built_a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Info Built B', 'code' => 'info_built_b']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionA->id, + 'entity_type' => Post::class, + 'name' => 'Info Built A Field', + 'code' => 'info_built_a_field', + 'type' => 'text', + ]); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionB->id, + 'entity_type' => Post::class, + 'name' => 'Info Built B Field', + 'code' => 'info_built_b_field', + 'type' => 'text', + ]); + + $container = CustomFields::infolist() + ->forModel(Post::class) + ->onlySections([$sectionA->id]) + ->build(); + + $schema = FilamentSchema::make(livewire(CreatePost::class)->instance()) + ->model(Post::class) + ->components([$container]); + + $entryNames = collect($schema->getFlatComponents()) + ->filter(fn (object $component): bool => $component instanceof Entry) + ->map(fn (Entry $component): string => $component->getName()); + + expect($entryNames)->toHaveCount(1) + ->and($entryNames)->toContain('custom_fields.info_built_a_field') + ->and($entryNames)->not->toContain('custom_fields.info_built_b_field'); + }); +}); diff --git a/tests/Feature/FieldVisibilityIntegrationTest.php b/tests/Feature/FieldVisibilityIntegrationTest.php index 48e78cd2..ad7b0b4d 100644 --- a/tests/Feature/FieldVisibilityIntegrationTest.php +++ b/tests/Feature/FieldVisibilityIntegrationTest.php @@ -228,3 +228,124 @@ ->and($visibleJs)->toBe($expectedJs); }); }); + +$buildEqualsJs = function (string|int|float $conditionValue, VisibilityOperator $operator = VisibilityOperator::EQUALS): string { + $section = CustomFieldSection::factory()->create([ + 'name' => 'Text Condition Section', + 'entity_type' => Post::class, + 'active' => true, + ]); + + $triggerField = CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'name' => 'Status', + 'code' => 'status', + 'type' => 'text', + 'entity_type' => Post::class, + ]); + + $conditionalField = CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'name' => 'Gated', + 'code' => 'gated', + 'type' => 'text', + 'entity_type' => Post::class, + 'settings' => [ + 'visibility' => [ + 'mode' => VisibilityMode::SHOW_WHEN, + 'logic' => VisibilityLogic::ALL, + 'conditions' => [[ + 'field_code' => 'status', + 'operator' => $operator, + 'value' => $conditionValue, + 'source' => ConditionSource::CustomField, + ]], + ], + ], + ]); + + return (string) app(FrontendVisibilityService::class) + ->buildVisibilityExpression($conditionalField, collect([$triggerField, $conditionalField])); +}; + +describe('Numeric-string condition values in the generated visibility JS', function () use ($buildEqualsJs): void { + it('emits a numeric string as a JS string literal, and still coerces when the field holds a number', function () use ($buildEqualsJs): void { + expect($buildEqualsJs('42')) + ->toContain("const compareVal = '42';") + ->toContain('isNumericLike(fieldVal) && isNumericLike(compareVal)') + ->and(VisibilityOperator::EQUALS->evaluate(42, '42'))->toBeTrue(); + }); + + it('keeps a numeric string comparable as a string, matching the backend operator', function () use ($buildEqualsJs): void { + // VisibilityOperator::evaluateEquals() compares two strings as strings, so '42.5' and + // '42.50' are NOT equal on the server. Emitting the condition as a JS number would make + // parseFloat('42.5') === 42.5 true on the client and silently desync the two engines. + expect($buildEqualsJs('42.50'))->toContain("const compareVal = '42.50';") + ->and(VisibilityOperator::EQUALS->evaluate('42.5', '42.50'))->toBeFalse(); + }); + + it('emits genuine int and float condition values as JS numbers', function () use ($buildEqualsJs): void { + expect($buildEqualsJs(42))->toContain('const compareVal = 42;') + ->and($buildEqualsJs(42.5))->toContain('const compareVal = 42.5000000000;'); + }); +}); + +describe('Case-insensitive string equality parity between the two visibility engines', function () use ($buildEqualsJs): void { + it('folds both sides before comparing, matching the backend operator', function () use ($buildEqualsJs): void { + // Server: strtolower('active') === strtolower('Active') is true. A case-sensitive client + // comparison hides the field the server would show. + expect(VisibilityOperator::EQUALS->evaluate('active', 'Active'))->toBeTrue() + ->and($buildEqualsJs('Active')) + ->toContain("typeof fieldVal === 'string' && typeof compareVal === 'string'") + ->toContain('fieldVal.toLowerCase() === compareVal.toLowerCase()'); + }); + + it('applies the same folding to not_equals, which negates the equals expression', function () use ($buildEqualsJs): void { + expect(VisibilityOperator::NOT_EQUALS->evaluate('active', 'Active'))->toBeFalse() + ->and($buildEqualsJs('Active', VisibilityOperator::NOT_EQUALS)) + ->toContain('fieldVal.toLowerCase() === compareVal.toLowerCase()'); + }); + + it('still distinguishes genuinely different strings', function () use ($buildEqualsJs): void { + expect(VisibilityOperator::EQUALS->evaluate('inactive', 'Active'))->toBeFalse() + ->and($buildEqualsJs('Active'))->toContain("const compareVal = 'Active';"); + }); +}); + +describe('VisibilityOperator equality mirrors the client expression', function (): void { + it('matches a numeric field value against the string a text input stored', function (): void { + // A NUMERIC field reads back from integer_value as an int, while its condition is + // whatever the text input persisted. Strict identity meant such a condition never matched. + expect(VisibilityOperator::EQUALS->evaluate(42, '42'))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate('42', 42))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate(42.5, '42.50'))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate(42, '43'))->toBeFalse(); + }); + + it('keeps two strings on string comparison so trailing zeros still differ', function (): void { + expect(VisibilityOperator::EQUALS->evaluate('42.5', '42.50'))->toBeFalse(); + }); + + it('compares a boolean against its spelling, which is all the client can see', function (): void { + expect(VisibilityOperator::EQUALS->evaluate(true, 'true'))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate('TRUE', true))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate(false, 'false'))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate(true, 'false'))->toBeFalse() + ->and(VisibilityOperator::EQUALS->evaluate(true, '42'))->toBeFalse(); + }); + + it('compares two collections as sets rather than looking for one inside the other', function (): void { + expect(VisibilityOperator::EQUALS->evaluate(['a', 'b'], ['b', 'a']))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate(['a', 'b'], ['a']))->toBeFalse(); + }); + + it('treats a single condition value against a multi-value field as membership', function (): void { + expect(VisibilityOperator::EQUALS->evaluate(['a', 'b'], 'a'))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate([42], '42'))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate(['a', 'b'], 'c'))->toBeFalse(); + }); + + it('never matches a scalar field against a collection condition', function (): void { + expect(VisibilityOperator::EQUALS->evaluate('a', ['a', 'b']))->toBeFalse(); + }); +}); diff --git a/tests/Feature/RelaxCustomFieldsUniqueKeyMigrationTest.php b/tests/Feature/RelaxCustomFieldsUniqueKeyMigrationTest.php new file mode 100644 index 00000000..304b7968 --- /dev/null +++ b/tests/Feature/RelaxCustomFieldsUniqueKeyMigrationTest.php @@ -0,0 +1,172 @@ +migration = require __DIR__.'/../../database/migrations/relax_custom_fields_unique_key.php'; + $this->table = config('custom-fields.database.table_names.custom_fields'); +}); + +function customFieldsIndexNames(string $table): Collection +{ + return collect(Schema::getIndexes($table))->pluck('name'); +} + +it('down() restores the narrow key and drops the wide one', function (): void { + expect(customFieldsIndexNames($this->table)) + ->toContain('cf_code_entity_section_unique') + ->not->toContain('custom_fields_code_entity_type_unique'); + + $this->migration->down(); + + $indexes = customFieldsIndexNames($this->table); + + expect($indexes) + ->toContain('custom_fields_code_entity_type_unique') + ->not->toContain('cf_code_entity_section_unique'); +}); + +it('up() restores the wide key after a down() round trip', function (): void { + $this->migration->down(); + $this->migration->up(); + + $indexes = customFieldsIndexNames($this->table); + + expect($indexes) + ->toContain('cf_code_entity_section_unique') + ->not->toContain('custom_fields_code_entity_type_unique'); +}); + +it('up() is idempotent when the wide index already exists', function (): void { + expect(fn () => $this->migration->up())->not->toThrow(Throwable::class); + + expect(customFieldsIndexNames($this->table)->filter( + fn (string $name): bool => $name === 'cf_code_entity_section_unique' + ))->toHaveCount(1); +}); + +it('up() is idempotent when the narrow index is already gone, the state a consumer who hand-applied an equivalent change is in', function (): void { + Schema::table($this->table, fn (Blueprint $table) => $table->dropUnique('cf_code_entity_section_unique')); + + expect(customFieldsIndexNames($this->table)) + ->not->toContain('cf_code_entity_section_unique') + ->not->toContain('custom_fields_code_entity_type_unique'); + + expect(fn () => $this->migration->up())->not->toThrow(Throwable::class); + + expect(customFieldsIndexNames($this->table))->toContain('cf_code_entity_section_unique'); +}); + +it('down() aborts before dropping anything when rows share a code across sections', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'code' => 'section_a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'code' => 'section_b']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionA->id, + 'entity_type' => Post::class, + 'code' => 'duplicate_code', + 'type' => 'text', + ]); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionB->id, + 'entity_type' => Post::class, + 'code' => 'duplicate_code', + 'type' => 'text', + ]); + + expect(fn () => $this->migration->down()) + ->toThrow(RuntimeException::class, 'duplicate_code'); + + $indexes = customFieldsIndexNames($this->table); + + expect($indexes) + ->toContain('cf_code_entity_section_unique') + ->not->toContain('custom_fields_code_entity_type_unique'); +}); + +it('down() succeeds once the duplicate rows are resolved', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'code' => 'section_a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'code' => 'section_b']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionA->id, + 'entity_type' => Post::class, + 'code' => 'duplicate_code', + 'type' => 'text', + ]); + + $duplicate = CustomField::factory()->create([ + 'custom_field_section_id' => $sectionB->id, + 'entity_type' => Post::class, + 'code' => 'duplicate_code', + 'type' => 'text', + ]); + + $duplicate->update(['code' => 'no_longer_duplicate']); + + expect(fn () => $this->migration->down())->not->toThrow(Throwable::class); + + expect(customFieldsIndexNames($this->table)) + ->toContain('custom_fields_code_entity_type_unique') + ->not->toContain('cf_code_entity_section_unique'); +}); + +it('honors prefix_indexes and the connection table prefix when computing the drop-target index name', function (): void { + config()->set('database.connections.prefixed_for_test', [ + 'driver' => 'sqlite', + 'database' => ':memory:', + 'prefix' => 'wp_', + 'prefix_indexes' => true, + ]); + + $originalDefault = config('database.default'); + config()->set('database.default', 'prefixed_for_test'); + + try { + $connection = DB::connection('prefixed_for_test'); + $connection->useDefaultSchemaGrammar(); + $columns = ['code', 'entity_type', 'custom_field_section_id']; + + $defaultUniqueIndexName = new ReflectionMethod($this->migration, 'defaultUniqueIndexName'); + $defaultUniqueIndexName->setAccessible(true); + $actual = $defaultUniqueIndexName->invoke($this->migration, 'custom_fields', $columns); + + $blueprint = new Blueprint($connection, 'custom_fields'); + $createIndexName = new ReflectionMethod($blueprint, 'createIndexName'); + $createIndexName->setAccessible(true); + $expected = $createIndexName->invoke($blueprint, 'unique', $columns); + + expect($actual)->toBe($expected) + ->and($actual)->toStartWith('wp_custom_fields_'); + } finally { + config()->set('database.default', $originalDefault); + DB::purge('prefixed_for_test'); + } +}); + +it('computes the same index name with no prefix configured, matching current behavior', function (): void { + $columns = ['code', 'entity_type']; + + $defaultUniqueIndexName = new ReflectionMethod($this->migration, 'defaultUniqueIndexName'); + $defaultUniqueIndexName->setAccessible(true); + + $actual = $defaultUniqueIndexName->invoke($this->migration, 'custom_fields', $columns); + + expect($actual)->toBe('custom_fields_code_entity_type_unique'); +});