Split remaining LiveView god-modules (global_settings, fees_component) #540

New issue

Closed

opened 2026-06-15 22:22:52 +02:00 by moritz · 1 comment

moritz commented

2026-06-15 22:22:52 +02:00

Owner

Motivation

Two large LiveViews remain after the index split: global_settings_live.ex (1658 LOC, ~27 deps, coordinating
three unrelated integration-config surfaces) and membership_fees_component.ex (1334 LOC). At the upper size
bound for a single issue, but kept together to keep the issue list small.

Scope (ordered steps, CI green each)

R-027 — split global_settings_live.ex. Extract per-integration sections (SMTP, OIDC, Vereinfacht) into
focused components/LiveComponents. One section per step.
R-028 — split membership_fees_component.ex. Isolate the cycle CRUD and related surfaces. One surface per step.

Acceptance criteria

Each settings/fees surface isolated into its own module; save paths tested.
Behavior parity on the settings and member-detail-fees screens.
just ci-dev green after each step.

Dependencies

R-028 depends on the "Correctness …" issue (R-006 hardening) and the "Import/Export …" issue (R-026 rule moved
into Ash) landing first.
R-027 pairs with Track A of the "Untangle compile cycles" issue (global_settings consumes the split config).

## Motivation Two large LiveViews remain after the index split: `global_settings_live.ex` (1658 LOC, ~27 deps, coordinating three unrelated integration-config surfaces) and `membership_fees_component.ex` (1334 LOC). At the upper size bound for a single issue, but kept together to keep the issue list small. ## Scope (ordered steps, CI green each) - **R-027 — split `global_settings_live.ex`.** Extract per-integration sections (SMTP, OIDC, Vereinfacht) into focused components/LiveComponents. One section per step. - **R-028 — split `membership_fees_component.ex`.** Isolate the cycle CRUD and related surfaces. One surface per step. ## Acceptance criteria - Each settings/fees surface isolated into its own module; save paths tested. - Behavior parity on the settings and member-detail-fees screens. - `just ci-dev` green after each step. ## Dependencies - R-028 depends on the "Correctness …" issue (R-006 hardening) and the "Import/Export …" issue (R-026 rule moved into Ash) landing first. - R-027 pairs with Track A of the "Untangle compile cycles" issue (global_settings consumes the split config).

moritz added this to the Code and Test Refactoring | TI I milestone

2026-06-15 22:22:52 +02:00

moritz added this to the Sprint 18: Oktober 2026 project

2026-06-15 22:22:52 +02:00

simon referenced this issue

2026-10-15 20:01:07 +02:00

Update Design Guidelines #592

simon referenced this issue

2026-10-17 10:46:50 +02:00

Remove dead code and tests that cannot fail #605

simon referenced this issue

2026-10-19 11:26:36 +02:00

Remove dead code and tests that cannot fail closes #605 #608

simon referenced this issue

2026-10-19 11:37:34 +02:00

Run more of the test suite in parallel #610

simon referenced this issue

2026-10-19 13:47:38 +02:00

Run the LiveView test files in parallel #611

simon commented

2026-10-19 13:55:25 +02:00

Owner

Synergy note: #546 (club name not saved in settings) lives in global_settings_live.ex, which this issue splits. Proposal: fix it here as the first step (failing regression test, then the fix, as its own commit), before the split.

Test isolation, so the settings tests can finally run in parallel (#610 measured the cost of the serial suite):

The Settings singleton is why several test files must run async: false. When splitting, make each section's state isolatable per test (e.g. the section reads settings it is given instead of the global singleton), so global_settings_live_test.exs (5.1 s serial) and the settings-dependent LiveView tests can run async.
The same applies outside the LiveView. #610 left these files serial because every test in them needs global state: test/mv/membership_fees/cycle_generator_edge_cases_test.exs (Settings include_joining_cycle), test/mv/release_test.exs (ADMIN_EMAIL/ADMIN_PASSWORD* OS env), test/mv/vereinfacht/sync_contact_async_test.exs (Vereinfacht OS env), test/mv/accounts/user_last_admin_property_test.exs (mutates the committed system user and roles). The full list with reasons is in the #610 PR. Either cover the Settings part here or file a small chore for "inject settings/env instead of reading globals" that #540 references.
The test "shows warning when SMTP is not configured in production" (global_settings_live_test.exs:139) does not test the warning and should be rewritten.

ENV-over-Settings precedence (follow-up from #538, same settings UI — do it together with or right after this split):
Configuration values resolve "ENV first, then Settings", but with two divergent, undocumented rules. OIDC, Vereinfacht and mail-from resolve per field, and a set-but-empty ENV variable counts as set (value nil, no fallback). SMTP uses group mode (SMTP_HOST set ⇒ all SMTP fields from ENV) and treats an empty variable as unset.
Bug in the per-field rule: the settings page decides read-only fields via *_env_set?, which treats an empty variable as unset (field editable), while value resolution treats it as set and returns nil. Example: docker-compose passes OIDC_CLIENT_ID: ${OIDC_CLIENT_ID} with the host variable empty ⇒ the admin enters a client ID, it is saved, the effective value stays nil, and SSO silently does not work.
Proposal: (1) empty ENV = unset everywhere; (2) group mode per integration (SMTP, OIDC, Vereinfacht), secrets (*_PASSWORD, *_SECRET, *_FILE) may additionally come from ENV individually, mail-from stays per field; (3) the settings page marks an ENV-managed group read-only with a hint naming the controlling variable. Acceptance: rules in one place (Mv.Config.Source) and documented; property tests replace #538's behaviour-pinning properties; for every field "UI shows ENV-managed" ⇔ "effective value comes from ENV"; CHANGELOG entry under Changed (deployments with empty ENV variables now pick up Settings values).

Synergy note: #546 (club name not saved in settings) lives in `global_settings_live.ex`, which this issue splits. Proposal: fix it here as the first step (failing regression test, then the fix, as its own commit), before the split. Test isolation, so the settings tests can finally run in parallel (#610 measured the cost of the serial suite): - The Settings singleton is why several test files must run `async: false`. When splitting, make each section's state isolatable per test (e.g. the section reads settings it is given instead of the global singleton), so `global_settings_live_test.exs` (5.1 s serial) and the settings-dependent LiveView tests can run async. - The same applies outside the LiveView. #610 left these files serial because every test in them needs global state: `test/mv/membership_fees/cycle_generator_edge_cases_test.exs` (Settings `include_joining_cycle`), `test/mv/release_test.exs` (`ADMIN_EMAIL`/`ADMIN_PASSWORD*` OS env), `test/mv/vereinfacht/sync_contact_async_test.exs` (Vereinfacht OS env), `test/mv/accounts/user_last_admin_property_test.exs` (mutates the committed system user and roles). The full list with reasons is in the #610 PR. Either cover the Settings part here or file a small chore for "inject settings/env instead of reading globals" that #540 references. - The test "shows warning when SMTP is not configured in production" (`global_settings_live_test.exs:139`) does not test the warning and should be rewritten. ENV-over-Settings precedence (follow-up from #538, same settings UI — do it together with or right after this split): Configuration values resolve "ENV first, then Settings", but with two divergent, undocumented rules. OIDC, Vereinfacht and mail-from resolve per field, and a set-but-empty ENV variable counts as set (value `nil`, no fallback). SMTP uses group mode (`SMTP_HOST` set ⇒ all SMTP fields from ENV) and treats an empty variable as unset. Bug in the per-field rule: the settings page decides read-only fields via `*_env_set?`, which treats an empty variable as unset (field editable), while value resolution treats it as set and returns `nil`. Example: docker-compose passes `OIDC_CLIENT_ID: ${OIDC_CLIENT_ID}` with the host variable empty ⇒ the admin enters a client ID, it is saved, the effective value stays `nil`, and SSO silently does not work. Proposal: (1) empty ENV = unset everywhere; (2) group mode per integration (SMTP, OIDC, Vereinfacht), secrets (`*_PASSWORD`, `*_SECRET`, `*_FILE`) may additionally come from ENV individually, mail-from stays per field; (3) the settings page marks an ENV-managed group read-only with a hint naming the controlling variable. Acceptance: rules in one place (`Mv.Config.Source`) and documented; property tests replace #538's behaviour-pinning properties; for every field "UI shows ENV-managed" ⇔ "effective value comes from ENV"; CHANGELOG entry under *Changed* (deployments with empty ENV variables now pick up Settings values).

simon referenced this issue

2026-10-19 21:03:16 +02:00

Run more of the test suite in parallel closes #610 #612

simon referenced this issue

2026-10-20 23:23:17 +02:00

Run the LiveView test files in parallel closes #611 #616

simon referenced this issue

2026-10-24 15:00:18 +02:00

Page sections, danger zone, callouts and form controls #618

simon referenced this issue from a pull request that will close it,

2026-10-25 11:15:29 +01:00

Split the settings page and the membership fee tab into focused modules and apply one ENV rule set closes #540 #619