From 857b312f9f445a8562c852dc634688e056639d44 Mon Sep 17 00:00:00 2001 From: Claudio Schaad Date: Thu, 2 Jul 2026 21:51:44 +0200 Subject: [PATCH] PR K: hide IFxService and FixerIo key behind IFxConverter 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 --- IMPROVEMENT_PLAN.md | 1 + Schaad.Accounting.Services/FxConverter.cs | 21 ++++++++++++ .../Interfaces/IFxConverter.cs | 11 ++++++ Schaad.Accounting.Services/ViewService.cs | 20 ++++------- .../FxConverterTestShould.cs | 34 +++++++++++++++++++ .../ViewServiceTestShould.cs | 12 +++---- Schaad.Accounting.UI/Extensions.cs | 1 + 7 files changed, 79 insertions(+), 21 deletions(-) create mode 100644 Schaad.Accounting.Services/FxConverter.cs create mode 100644 Schaad.Accounting.Services/Interfaces/IFxConverter.cs create mode 100644 Schaad.Accounting.Tests/FxConverterTestShould.cs diff --git a/IMPROVEMENT_PLAN.md b/IMPROVEMENT_PLAN.md index b48d461..6340fb7 100644 --- a/IMPROVEMENT_PLAN.md +++ b/IMPROVEMENT_PLAN.md @@ -106,3 +106,4 @@ Analysis and phased plan produced 2026-07-02. See conversation history for full - **PR H** — Phase 2 item 18 (first slice): add `ILogger` 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` `` 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` 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.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. diff --git a/Schaad.Accounting.Services/FxConverter.cs b/Schaad.Accounting.Services/FxConverter.cs new file mode 100644 index 0000000..d7a1e59 --- /dev/null +++ b/Schaad.Accounting.Services/FxConverter.cs @@ -0,0 +1,21 @@ +using Schaad.Accounting.Datasets; +using Schaad.Accounting.Interfaces; +using Schaad.Finance.Api; + +namespace Schaad.Accounting.Services +{ + public class FxConverter : IFxConverter + { + private readonly IFxService fxService; + private readonly SettingsDataset settings; + + public FxConverter(IFxService fxService, SettingsDataset settings) + { + this.fxService = fxService; + this.settings = settings; + } + + public decimal ConvertToChf(decimal amount, string fromCurrency) + => fxService.ConvertCurrency(amount, fromCurrency, "CHF", settings.FixerIoApiKey); + } +} diff --git a/Schaad.Accounting.Services/Interfaces/IFxConverter.cs b/Schaad.Accounting.Services/Interfaces/IFxConverter.cs new file mode 100644 index 0000000..15a8fba --- /dev/null +++ b/Schaad.Accounting.Services/Interfaces/IFxConverter.cs @@ -0,0 +1,11 @@ +namespace Schaad.Accounting.Interfaces +{ + /// + /// Thin wrapper over the vendor IFxService that hides the FixerIo API key + /// and the base currency, so callers just ask "convert this amount to CHF". + /// + public interface IFxConverter + { + decimal ConvertToChf(decimal amount, string fromCurrency); + } +} diff --git a/Schaad.Accounting.Services/ViewService.cs b/Schaad.Accounting.Services/ViewService.cs index e4fb22c..067a4e2 100644 --- a/Schaad.Accounting.Services/ViewService.cs +++ b/Schaad.Accounting.Services/ViewService.cs @@ -5,7 +5,6 @@ using Schaad.Accounting.Datasets; using Schaad.Accounting.Datasets.Reports; using Schaad.Accounting.Interfaces; using Schaad.Accounting.Models; -using Schaad.Finance.Api; using Schaad.Finance.Api.Datasets; namespace Schaad.Accounting.Services @@ -17,8 +16,7 @@ namespace Schaad.Accounting.Services private readonly IBookingRuleRepository bookingRuleRepository; private readonly ISubclassRepository subclassRepository; private readonly ITransactionRepository transactionRepository; - private readonly IFxService fxService; - private readonly ISettingsService settingsService; + private readonly IFxConverter fxConverter; public ViewService( IAccountRepository accountRepository, @@ -26,16 +24,14 @@ namespace Schaad.Accounting.Services ITransactionRepository transactionRepository, ISubclassRepository subclassRepository, IBookingRuleRepository bookingRuleRepository, - IFxService fxService, - ISettingsService settingsService) + IFxConverter fxConverter) { this.accountRepository = accountRepository; this.bankTransactionRepository = bankTransactionRepository; this.transactionRepository = transactionRepository; this.subclassRepository = subclassRepository; this.bookingRuleRepository = bookingRuleRepository; - this.fxService = fxService; - this.settingsService = settingsService; + this.fxConverter = fxConverter; } /// @@ -55,7 +51,6 @@ namespace Schaad.Accounting.Services var accounts = accountRepository.GetAccountList(); var transactionList = GetTransactionViewList(); var subClassNameByNumber = subclassRepository.GetSubClassList().ToDictionary(s => s.Number, s => s.Name); - var settings = settingsService.GetSettings(); // Group transactions by account so per-account balance is O(k) instead of O(M). var creditsByAccount = transactionList.GroupBy(t => t.TargetAccountId).ToDictionary(g => g.Key, g => g.ToList()); @@ -67,8 +62,8 @@ namespace Schaad.Accounting.Services return new AccountDataset( account: a, balance: balance, - balanceCHF: fxService.ConvertCurrency(balance, a.Currency, "CHF", settings.FixerIoApiKey), - startBalanceCHF: fxService.ConvertCurrency(a.StartBalance, a.Currency, "CHF", settings.FixerIoApiKey), + balanceCHF: fxConverter.ConvertToChf(balance, a.Currency), + startBalanceCHF: fxConverter.ConvertToChf(a.StartBalance, a.Currency), className: subclassRepository.GetClass(a.Class), subClassName: subClassNameByNumber[a.SubClass]); }) @@ -111,9 +106,8 @@ namespace Schaad.Accounting.Services { var accountList = GetAccountViewList(); - var settings = settingsService.GetSettings(); - var profit = Math.Abs(accountList.Where(m => m.Class == ClassIds.Income).Sum(m =>m.BalanceCHF)); - profit += Math.Abs(accountList.Where(m => m.Class == ClassIds.Activa).Sum(m => fxService.ConvertCurrency(m.StartBalance, m.Currency, "CHF", settings.FixerIoApiKey))); + var profit = Math.Abs(accountList.Where(m => m.Class == ClassIds.Income).Sum(m => m.BalanceCHF)); + profit += Math.Abs(accountList.Where(m => m.Class == ClassIds.Activa).Sum(m => fxConverter.ConvertToChf(m.StartBalance, m.Currency))); var loss = Math.Abs(accountList.Where(m => m.Class == ClassIds.Expenses).Sum(m => m.BalanceCHF)); var balanceView = new BalanceSheetDataset( diff --git a/Schaad.Accounting.Tests/FxConverterTestShould.cs b/Schaad.Accounting.Tests/FxConverterTestShould.cs new file mode 100644 index 0000000..2b3e0a6 --- /dev/null +++ b/Schaad.Accounting.Tests/FxConverterTestShould.cs @@ -0,0 +1,34 @@ +using NSubstitute; +using Schaad.Accounting.Datasets; +using Schaad.Accounting.Services; +using Schaad.Finance.Api; +using Shouldly; + +namespace Schaad.Accounting.Tests; + +public class FxConverterTestShould +{ + private readonly IFxService fxService = Substitute.For(); + private readonly SettingsDataset settings = new() { DataPath = "", FixerIoApiKey = "test-key" }; + + private FxConverter BuildConverter() => new(fxService, settings); + + [Fact] + public void DelegateToFxServiceWithChfAsTargetAndConfiguredApiKeyWhenConverting() + { + fxService.ConvertCurrency(100m, "USD", "CHF", "test-key").Returns(90m); + + var result = BuildConverter().ConvertToChf(100m, "USD"); + + result.ShouldBe(90m); + fxService.Received(1).ConvertCurrency(100m, "USD", "CHF", "test-key"); + } + + [Fact] + public void PassNegativeAmountsThroughWhenConverting() + { + fxService.ConvertCurrency(-50m, "EUR", "CHF", Arg.Any()).Returns(-48m); + + BuildConverter().ConvertToChf(-50m, "EUR").ShouldBe(-48m); + } +} diff --git a/Schaad.Accounting.Tests/ViewServiceTestShould.cs b/Schaad.Accounting.Tests/ViewServiceTestShould.cs index d8aa4de..7637ce3 100644 --- a/Schaad.Accounting.Tests/ViewServiceTestShould.cs +++ b/Schaad.Accounting.Tests/ViewServiceTestShould.cs @@ -1,9 +1,7 @@ using NSubstitute; -using Schaad.Accounting.Datasets; using Schaad.Accounting.Interfaces; using Schaad.Accounting.Models; using Schaad.Accounting.Services; -using Schaad.Finance.Api; using Shouldly; namespace Schaad.Accounting.Tests; @@ -15,8 +13,7 @@ public class ViewServiceTestShould private readonly IBankTransactionRepository bankTransactionRepo = Substitute.For(); private readonly IBookingRuleRepository bookingRuleRepo = Substitute.For(); private readonly ISubclassRepository subclassRepo = Substitute.For(); - private readonly IFxService fxService = Substitute.For(); - private readonly ISettingsService settingsService = Substitute.For(); + private readonly IFxConverter fxConverter = Substitute.For(); public ViewServiceTestShould() { @@ -24,15 +21,14 @@ public class ViewServiceTestShould subclassRepo.GetSubClassList().Returns( Enumerable.Range(10, 50).Select(n => new SubClass { Number = n, Name = "sub-" + n }).ToList()); subclassRepo.GetClass(Arg.Any()).Returns(""); - settingsService.GetSettings().Returns(new SettingsDataset { DataPath = "", FixerIoApiKey = "" }); // Passthrough FX by default (CHF-only). Individual tests can override. - fxService.ConvertCurrency(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + fxConverter.ConvertToChf(Arg.Any(), Arg.Any()) .Returns(ci => ci.ArgAt(0)); } private ViewService BuildService() => - new(accountRepo, bankTransactionRepo, transactionRepo, subclassRepo, bookingRuleRepo, fxService, settingsService); + new(accountRepo, bankTransactionRepo, transactionRepo, subclassRepo, bookingRuleRepo, fxConverter); // --- Balance math --------------------------------------------------------- @@ -82,7 +78,7 @@ public class ViewServiceTestShould transactionRepo.GetTransactionList().Returns(new List()); // 1 USD = 0.90 CHF - fxService.ConvertCurrency(Arg.Any(), "USD", "CHF", Arg.Any()) + fxConverter.ConvertToChf(Arg.Any(), "USD") .Returns(ci => ci.ArgAt(0) * 0.9m); var account = BuildService().GetAccountViewList().Single(); diff --git a/Schaad.Accounting.UI/Extensions.cs b/Schaad.Accounting.UI/Extensions.cs index 384173b..5605f6a 100644 --- a/Schaad.Accounting.UI/Extensions.cs +++ b/Schaad.Accounting.UI/Extensions.cs @@ -31,6 +31,7 @@ namespace Schaad.Accounting.UI services.AddScoped(); services.AddScoped(); services.AddSingleton(); + services.AddScoped(); services.AddSingleton(); return services;