AccountingNext/IMPROVEMENT_PLAN.md

AccountingNext — Technical Improvement Plan

Analysis and phased plan produced 2026-07-02. See conversation history for full context.

Findings

1. Correctness bugs (must fix)

• Shared mutable state across users. SettingsService is registered as Singleton (Schaad.Accounting.UI/Extensions.cs:21) but holds mutable year/mandator fields (Schaad.Accounting.Services/SettingsService.cs:13-14). Any user switching year/mandator changes it for all connected Blazor Server users.
• TrySetYear rollback is broken. SettingsService.cs:33 captures var oldYear = year; — that's the parameter, not this.year. The "rollback" restores the same value that just failed.
• ChartService mutates a shared service. GetAccountExpensesPerMonth(accountId, year) (ChartService.cs:87) calls settingsService.TrySetYear(year) then SetYear(DateTime.Now.Year) to reload prior-year data. Global mutation on a singleton — guaranteed to race in production.
• DummyFxService has a typo. DummyFxService.cs:13 — currencies.Contains(fromCurrency) && currencies.Contains(fromCurrency) (both sides check fromCurrency). Also it silently returns the raw amount for supported currencies, so all "CHF conversions" are no-ops.
• TransactionRepository.GetTransaction mutates the entity. TransactionRepository.cs:67-81 divides Value by FxRate on read, and SaveTransaction multiplies by FxRate on write. Also silently NREs when the id is not found (line 73 dereferences transaction).
• ViewService.GetTransactionViewList(accountId) mutates transactions. ViewService.cs:172 flips t.Value *= -1 on the loaded transaction. FileService.GetTransactionListCsv does the same (FileService.cs:137). These are the shared objects returned by the repository — later reads see wrong signs.
• DI double-registration. Extensions.cs registers IChartService, IViewService, IFileService, statement services, and IFxService in both AddRepositories and AddServices. Also AddServices registers PdfParsingService as its own key (services.AddSingleton<PdfParsingService, PdfParsingService>()) instead of IPdfParsingService.
• Magic numbers instead of ClassIds. ProfitLossReport.razor.cs:25-26 uses .Class == 3 / .Class == 4.
• ClassIds doc/code mismatch. CLAUDE.md states Activa=1000, Passiva=2000, …, but ClassIds.cs defines them as 1,2,3,4 (matches Account.Class = Number / 1000).
• Non-atomic XML writes, no locking. BaseRepository.Save writes directly to the target path. Interrupted writes corrupt data. Two concurrent saves interleave. No temp-file+rename, no FileShare lock.
• AccountRepository.EnsureFileExisits calls SaveAccount in a loop. Each call re-loads and re-serializes the whole account list.
• ProfitLossReport / BalanceSheetReport compute totals by re-summing balances that ViewService already computes — but with subtle differences. Two sources of truth for the same number.

2. Reliability / maintainability

• No async I/O. Every XML read/write is synchronous, blocking the SignalR hub thread in Blazor Server.
• No caching. A single page render calls GetAccountList() and GetTransactionList() many times. Every one is a fresh file read + XML deserialization.
• O(N·M) lookups. ViewService.cs:138-139 — accountList.Single(...) inside a Select over all transactions. Should be a dictionary keyed by id.
• No logging. ILogger is not used anywhere.
• No error surface. Repositories throw or return null; UI dereferences with !.
• Anemic domain model + Copy methods. Account.Copy, Transaction.Copy, SubClass.Copy etc. are hand-rolled property copies used to merge edits into loaded entities.
• Vendor coupling in the Domain project. Schaad.Accounting.Common references Schaad.Finance.Api.dll. IChartService, IFileService, and IViewService live in Common and depend on Schaad.Finance.Api types.
• Duplicated formatting logic. ToFormattedString(decimal) exists in Extensions.cs and again in FileService.cs.
• Culture setup in Program.cs runs after MapRazorComponents and just sets DefaultThreadCurrentCulture globally. Should be RequestLocalizationOptions middleware.
• Constructors doing I/O. AccountRepository, SubclassRepository, SplitPredefinitonRepository, BookingRuleRepository call EnsureFileExisits in the constructor.
• Typos leak into public API. EnsureFileExisits, SplitPredefinitonRepository.
• Dead / redundant code. AccountRepository.EnsureFileExisits uses new to hide the base method; its file.IndexOf("Accounts") > -1 guard is redundant. Home.razor.cs:50 uses a loaded flag though OnInitializedAsync already runs once per instance. Commented-out //var subclasses = subclassRepository.GetSubClassList(); in ChartService.
• Legacy nuget packages. All three library projects reference System.Text.RegularExpressions 4.3.1, System.Xml.XmlSerializer 4.3.0, System.ComponentModel.Annotations 5.0.0 — legacy .NET Standard packages, redundant on net9.0.
• Common.csproj has <Folder Include="Interfaces\Extensions\" /> for a folder that does not exist.
• ClassIds should be static class; currently instantiable.

3. Testability

• Zero tests, zero test project.
• Business rules (balance calculation, FX conversion, bank-transaction matching, CSV export, split logic) are entangled with mutable singletons and side effects on returned entities.

---

Phased plan

Phase 1 — Stop the bleeding (correctness, low churn)

1. Change ISettingsService registration from Singleton to Scoped. Deferred. MyHeader.YearChanged uses NavigateTo(..., forceLoad: true) after mutating settings, which tears down the SignalR circuit — a Scoped instance would be recreated with default values on the new circuit. Proper fix: persist the year/mandator selection to a cookie or query string, then Scoped becomes safe. Tracked as a new Phase 2 item.
2. Fix TrySetYear (capture this.year before overwriting). Defer the ChartService prior-year-loading pattern to Phase 3.
3. Fix DummyFxService typo (or delete the class and replace with a real IFxService implementation from Schaad.Finance.Api).
4. Remove mutation-on-read in TransactionRepository.GetTransaction, ViewService.GetTransactionViewList(accountId), and FileService.GetTransactionListCsv.
5. Add a not-found guard to TransactionRepository.GetTransaction.
6. De-duplicate the DI registrations. One AddAccounting() extension called once in Program.cs.
7. Replace .Class == 3/4 magic numbers with ClassIds.*. Update CLAUDE.md's incorrect ClassIds section.
8. Make XML writes atomic: write to foo.xml.tmp then File.Move(..., overwrite: true). Wrap Load/Save in a per-file SemaphoreSlim (or a simple lock).

Phase 2 — Structural cleanup

9. Introduce a per-request unit of work / cache. A scoped IAccountingContext that loads each XML file at most once per request and holds the deserialized lists.
10. Replace Copy(target) methods with a single merge-in-place pattern (or record with).
11. Extract shared formatting to a single Formatting helper.
12. Move IFileService, IChartService, IViewService out of Common (they depend on Schaad.Finance.Api). Common should have no vendor dependency.
13. Fix typos (EnsureFileExists, SplitPredefinitionRepository).
14. Move file existence bootstrapping out of constructors into a startup step (IHostedService or lazy first-use).
15. Fix AccountRepository.EnsureFileExists to compute the start-balance updates in memory and save once.
16. Remove legacy NuGet packages. Remove the stale Interfaces\Extensions\ folder entry.
17. Set culture via RequestLocalizationOptions middleware.
18. Add ILogger<T> to services and repositories. 18b. Persist selected year and mandator across page reloads (cookie, query string, or ProtectedLocalStorage). Prerequisite for making ISettingsService Scoped (Phase 1 item 1, deferred).

Phase 3 — Async & performance

19. Convert repository interfaces to async.
20. Cache the current view's data behind the scoped unit-of-work; invalidate on save.
21. Precompute accountsById and subclassNameByNumber dictionaries once per request.
22. Clean up ChartService prior-year loading pattern: introduce an explicit "load year data" helper instead of mutating ISettingsService.

Phase 4 — Testability & safety net

23. Add a Schaad.Accounting.Tests xUnit project.
24. First tests: ViewService.GetBalanceView, MatchBankTransactionByBookingRule / SameAccountsLastMonth, TransactionRepository FX round-trip, FileService.GetTransactionListCsv.
25. Abstract the XML store (IEntityStore<T>) so tests use an in-memory store.

Phase 5 — Nice-to-haves

26. Replace hand-written XML models with record types + source-generated serializers.
27. Inject Fixer.io key via IOptions<FxSettings> instead of threading through method params.
28. Prune unused Fluent UI packages.
29. Add a health/backup admin page.

---

PR breakdown

• PR A — Phase 1 items 1–7 + CLAUDE.md fix.
• PR B — Phase 1 item 8 (atomic writes + lock).
• PR C — Phase 2 mechanical cleanup: items 11, 13, 15, 16, 17.
• PR D — Phase 2 item 9: per-request unit of work / cache. Split off from PR C because it is invasive enough to warrant its own review.
• PR E — Phase 3 item 22: ChartService cleanup. Promoted ahead of async because it fixes an active correctness bug — GetAccountExpensesPerMonth(accountId, year) was mutating ISettingsService on the singleton to hop years, with the year parameter always hardcoded to DateTime.Now.Year, so the Spendings-over-time chart discarded the user's header year selection.
• PR F — Phase 3 item 21: precomputed dictionaries in ViewService to eliminate O(N·M) Single(...) scans and per-account transaction filtering.
• Item 19 (async I/O) — landed as PR P after being explicitly requested. See PR P entry below.
• PR G — Phase 4 items 23 + 24: Schaad.Accounting.Tests xUnit + NSubstitute + Shouldly project. Adopts the xxxTestShould.DoThisWhenThat naming convention with Shouldly assertions (no xUnit Assert.*). Covers ViewService, TransactionRepository, Formatting, RepositoryCache, AccountRepository, ChartService, and FileService.GetTransactionListCsv — 38 tests locking in the earlier PRs' behavior. Note: originally shipped as three separate commits (initial project, expanded coverage, Shouldly + naming conversion) and later squashed into one commit at the user's request.
• PR H — Phase 2 item 18 (first slice): add ILogger<FileService> to FileService.ImportAccountStatementFile so bank-statement imports emit Information for the file being processed and each account's import count, Warning when an account is skipped because it belongs to a different mandator, and Error when the vendor parser reports a failure. Rest of the logging (BaseRepository save failures, MatchOpenBankTransactions summary) tracked as a follow-up because it requires threading loggers through all seven repositories.
• PR I — Phase 2 item 12: move the service interfaces (IViewService, IFileService, IChartService) from Schaad.Accounting.Common into Schaad.Accounting.Services/Interfaces/. Namespaces are unchanged (Schaad.Accounting.Interfaces), so no consumer needs a using update. Drops the Schaad.Finance.Api <Reference> from Common.csproj — Common is now vendor-free and matches its documented role as the "shared models, DTOs, interfaces" layer.
• PR J — Phase 2 item 18 (second slice): thread ILogger<T> through BaseRepository and every concrete repository so BaseRepository.Save's catch block logs the failing file path and exception before rethrowing (previously the exception's origin was silently swallowed and only the stack trace at the callsite survived). Pulls Microsoft.Extensions.Logging.Abstractions into the Db project. TransactionRepositoryTestShould and AccountRepositoryTestShould use NullLogger<T>.Instance. Remaining item 18 work (MatchOpenBankTransactions match-count summary in ViewService) still open.
• PR K — Phase 5 item 27: introduce IFxConverter.ConvertToChf(amount, fromCurrency) and its FxConverter implementation. The vendor IFxService and the FixerIo API key are now hidden inside FxConverter; callers stop threading the API key through every method call. ViewService drops both IFxService and ISettingsService from its constructor and takes IFxConverter instead. Simplifies GetAccountViewList (no more per-call settingsService.GetSettings() reads) and GetBalanceSheetView. Two-test FxConverterTestShould locks in the target-currency and API-key routing.
• PR L — Phase 2 item 10: drop the hand-rolled Copy(target) methods from Account, BookingRule, BookingText, SubClass, and SplitPredefinition; keep only a Transaction.Clone() for defensive-copy needs in GetTransaction / ViewService.WithDisplaySign. Every SaveXxx now uses FindIndex → in-place replace (or Add for new entries), moving the two "hidden" defaults (Account.Currency = "CHF" when unset, Transaction.BookingDate = ValueDate when unset) into the corresponding SaveXxx method where they belong. Fixes a pre-existing bug: Transaction.Copy never copied RelatedParty, so update-saves silently dropped it. Added PreserveRelatedPartyWhenRoundTrippingTransaction and DefaultBookingDateToValueDateWhenBookingDateIsUnset as regression tests. 42 tests total.
• PR M — Phase 2 item 18 (final slice): add ILogger<ViewService> and log a match-count summary from MatchOpenBankTransactions ("Matched {Matched} of {Total} open bank transactions"). Completes item 18 — statement imports (PR H), repository save failures (PR J), and match runs now all emit structured logs. Also removes a dead ISettingsService injection from Home.razor.cs (declared with [Inject] but never used anywhere in the file).
• PR N — Small polish pass covering three long-noted rough edges from the original analysis: ClassIds becomes a static class (was instantiable); Home.razor.cs loses the redundant loaded guard in OnInitializedAsync (Blazor already runs that lifecycle hook once per component instance); the hardcoded mandator = "Claudio Schaad" in SettingsService moves to a DefaultMandator field on SettingsDataset, plumbed through appsettings.Development.json.
• PR O — Phase 5 item 26 (partial): convert BalanceDataset and BalanceSheetDataset to positional records. Both were constructor-initialised value carriers with read-only usage. The rest of item 26 was descoped: the XML-serialised domain models (Account, Transaction, ...) need mutable public setters for XmlSerializer, and value-equality on mutable data is a footgun (hash changes on mutation); source-generated XML serialisers do not exist without switching file formats. DataSerie was already a record; MessageDataset has a real mutation method (Add) and stays a class; AccountDataset / TransactionDataset / BookingRuleDataset inherit from the mutable domain models and can't cleanly become records without a bigger refactor.
• PR P — Phase 3 item 19: full async I/O top to bottom. BaseRepository.LoadAsync/SaveAsync/LoadListAsync return Task<T>; the per-file lock (obj) becomes SemaphoreSlim so it can be await-ed. Save serialises to a MemoryStream synchronously (XmlSerializer has no async form) then writes bytes with File.WriteAllBytesAsync; File.Move (atomic rename) has no async counterpart in .NET 9 and stays sync. Every repository interface + implementation, every service interface + implementation (except the vendor-owned IFxService), every Razor page/dialog OnInitializedAsync, and every test-file assertion becomes async / await. 51 files touched in one atomic diff — the codebase does not compile in intermediate states. AccountRepository.EnsureAccountsFile keeps two .GetAwaiter().GetResult() bridges because it runs from the constructor (constructors can't be async). The observed async payoff is one File.ReadAllBytesAsync and one File.WriteAllBytesAsync per Load/Save; after PR D each XML file is deserialised at most once per SignalR circuit, so on this single-user local Blazor Server app this is architectural-hygiene work, not a measurable perf win.