Claudio Schaad
857b312f9f
New IFxConverter.ConvertToChf(amount, fromCurrency) and FxConverter wrap the vendor IFxService and the FixerIo API key so callers stop threading the key through every conversion. ViewService drops IFxService and ISettingsService from its constructor and takes IFxConverter instead; GetAccountViewList and GetBalanceSheetView no longer read settingsService.GetSettings() per method. FxConverterTestShould locks in the target-currency + API-key routing. 40 tests total, all passing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
109 lines
13 KiB
Markdown
109 lines
13 KiB
Markdown
# 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)** — deferred. After PR D, each XML file is loaded at most once per SignalR circuit, and this app is single-user local Blazor Server. Converting every repository/service method to async would touch ~40 files for negligible user-visible benefit and real regression risk. Revisit if the app is ever hosted for multiple concurrent users.
|
||
- **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.
|