Skip to content

[Core]: Go over settings and make sure all settings are implemented correctly #137

Open
2 of 2 issues completed

Description

@nielsdrost7

Settings pages contain dozens of options that directly influence system behavior. Incorrect or missing settings can break workflows.


Specific

As a reviewer, I want to verify that all settings are implemented, functional, and correctly mapped to behavior.

Measurable

100% coverage of settings with test validation.

Achievable

By cross-checking each setting with its actual system effect.

Relevant

Settings control core functions like taxes, dates, numbering, emails.

Time-Bound

Review completed by Q3 2025.


Arrange

  • All settings pages are accessible.
  • Expected behavior is documented.

Act

  • The reviewer adjusts settings and checks corresponding outputs.

Assert

  • Each setting is functional and not ignored.
  • Invalid values trigger validation.
  • Each setting updates the underlying configuration.
  • Defaults work when no value is provided.

Activity

  1. added this to the v2.0.0-alpha milestone on Nov 27, 2025
  2. 14 remaining items

  3. nielsdrost7 commented on Jul 16, 2026

    @nielsdrost7
    CollaboratorAuthor

    SMART Plan — Fix settings persistence and audit setting-to-behaviour mapping

    Context/Epic: Core

    Specific

    Verify every setting on the admin Settings page actually persists and affects real system behaviour. Current state, and the headline finding: it doesn't persist at all. Modules\Core\Filament\Admin\Pages\Settings::submit() reads:

    public function submit(): void
    {
        $this->settings = $this->form->getState();
        // Here you would save settings to database or config
        // For now, just keep them in memory
    }

    Every setting on that page — currency, date format, language, theme, number formatting, update-channel preferences, etc. — is discarded on the next page load. This is despite Modules\Core\Models\Setting::saveByKey() and Setting::setAll() already existing and working (they write to the settings table and push values into config('ip.*')) — they're simply never called from the Settings page. Separately, the theme setting (Select::make('settings.theme')) only offers a single hard-coded 'default' option with a // TODO: Make it automatically grab themes from themes dir. comment, even though 5 real theme CSS files exist on disk (see #81/#516).

    Measurable

    • Saving the Settings form persists every changed value to the settings table via Setting::saveByKey().
      • PHPUnit: it_persists_a_changed_setting_to_the_database()
        #[Test]
        public function it_persists_a_changed_setting_to_the_database(): void
        {
            /* Arrange */
            /* superAdmin provided by AbstractAdminPanelTestCase */
    
            /* Act */
            Livewire::actingAs($this->superAdmin())
                ->test(\Modules\Core\Filament\Admin\Pages\Settings::class)
                ->fillForm(['settings.currency_code' => 'GBP'])
                ->call('submit');
    
            /* Assert */
            $this->assertDatabaseHas('settings', ['setting_key' => 'currency_code', 'setting_value' => 'GBP']);
        }
    • A saved setting survives a fresh page load (i.e. mount() reads it back from the database, not just its in-memory default).
      • PHPUnit: it_reloads_a_previously_saved_setting_on_next_mount()
        #[Test]
        public function it_reloads_a_previously_saved_setting_on_next_mount(): void
        {
            /* Arrange */
            \Modules\Core\Models\Setting::saveByKey('currency_code', 'GBP');
    
            /* Act */
            $component = Livewire::actingAs($this->superAdmin())
                ->test(\Modules\Core\Filament\Admin\Pages\Settings::class);
    
            /* Assert */
            $component->assertSet('settings.currency_code', 'GBP');
        }
    • Saving pushes the value into config('ip.*') immediately, matching Setting::saveByKey()'s existing behaviour, so the rest of the app sees the change without a restart.
      • PHPUnit: it_updates_the_runtime_config_when_a_setting_is_saved()
        #[Test]
        public function it_updates_the_runtime_config_when_a_setting_is_saved(): void
        {
            /* Arrange */
            /* superAdmin provided by AbstractAdminPanelTestCase */
    
            /* Act */
            Livewire::actingAs($this->superAdmin())
                ->test(\Modules\Core\Filament\Admin\Pages\Settings::class)
                ->fillForm(['settings.date_format' => 'd-m-Y'])
                ->call('submit');
    
            /* Assert */
            $this->assertSame('d-m-Y', config('ip.date_format'));
        }
    • An invalid value (e.g. a non-numeric update_check_interval) is rejected by form validation rather than silently saved.
      • PHPUnit: it_rejects_a_non_numeric_update_check_interval()
        #[Test]
        public function it_rejects_a_non_numeric_update_check_interval(): void
        {
            /* Arrange */
            /* superAdmin provided by AbstractAdminPanelTestCase */
    
            /* Act */
            $component = Livewire::actingAs($this->superAdmin())
                ->test(\Modules\Core\Filament\Admin\Pages\Settings::class)
                ->fillForm(['settings.update_check_interval' => 'not-a-number'])
                ->call('submit');
    
            /* Assert */
            $component->assertHasFormErrors(['settings.update_check_interval']);
        }
    • The Settings save action is only reachable by users with the manage-settings permission (existing canAccess() gate), confirmed still enforced after the persistence fix.
      • PHPUnit: it_forbids_a_user_without_manage_settings_permission_from_accessing_the_page()
        #[Test]
        public function it_forbids_a_user_without_manage_settings_permission_from_accessing_the_page(): void
        {
            /* Arrange */
            $user = User::factory()->create(['is_active' => true]);
    
            /* Act */
            $response = $this->actingAs($user)->get('/admin/settings');
    
            /* Assert */
            $response->assertForbidden();
        }

    Achievable

    1. Replace the submit() body: iterate $this->form->getState() and call Setting::saveByKey($key, $value) per entry (mirroring Setting::setAll()'s existing config-push behaviour), instead of the current no-op.
    2. Update mount() to read defaults from Setting::getByKey() first, falling back to the current hard-coded ??= defaults only when no row exists.
    3. Fix the theme select to list the real theme files (ties into [Core]: Finish edit profile settings with UI layout themes #81/Theme designer so customers can design their own theme #516) instead of the single hard-coded 'default' option.
    4. Go setting-by-setting through the ~25 keys in mount() (currency_symbol_placement, number_format, first_day_of_the_week, etc.) and confirm each one is actually read somewhere in the app — several may be display-only today with no consuming code, which is a second, separate kind of gap from the persistence bug.
    5. Add PHPUnit coverage in Modules/Core/Tests/Feature/.

    Relevant

    A settings page that silently discards every change is worse than no settings page — admins will assume their configuration took effect and won't discover otherwise until something downstream behaves unexpectedly.

    Time-bound

    Sprint 5 — the persistence fix (steps 1-2) is small and should ship on its own before the broader setting-by-setting audit (step 4).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    refinedRefined with SMART plan / test suggestions

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions