diff --git a/IMPROVEMENT_PLAN.md b/IMPROVEMENT_PLAN.md index 6340fb7..796a181 100644 --- a/IMPROVEMENT_PLAN.md +++ b/IMPROVEMENT_PLAN.md @@ -107,3 +107,4 @@ Analysis and phased plan produced 2026-07-02. See conversation history for full - **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. +- **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. diff --git a/Schaad.Accounting.Common/Models/Account.cs b/Schaad.Accounting.Common/Models/Account.cs index a6cb99c..62460e3 100644 --- a/Schaad.Accounting.Common/Models/Account.cs +++ b/Schaad.Accounting.Common/Models/Account.cs @@ -59,19 +59,5 @@ namespace Schaad.Accounting.Models { get { return string.IsNullOrEmpty(Currency) == false && Currency != "CHF"; } } - - /// - /// Makes a copy - /// - public void Copy(Account target) - { - target.LastBankBalance = LastBankBalance; - target.StartBalance = StartBalance; - target.BankAccountNumber = BankAccountNumber; - target.Currency = string.IsNullOrEmpty(Currency) ? "CHF" : Currency; - target.Id = Id; - target.Name = Name; - target.Number = Number; - } } } \ No newline at end of file diff --git a/Schaad.Accounting.Common/Models/BookingRule.cs b/Schaad.Accounting.Common/Models/BookingRule.cs index df09f22..7990cd6 100644 --- a/Schaad.Accounting.Common/Models/BookingRule.cs +++ b/Schaad.Accounting.Common/Models/BookingRule.cs @@ -20,18 +20,5 @@ namespace Schaad.Accounting.Models [Display(Name = "Konto")] [Required] public string AccountId { get; set; } - - - /// - /// Makes a copy - /// - public void Copy(BookingRule target) - { - target.LookupText = LookupText; - target.LookupValue = LookupValue; - target.BookingText = BookingText; - target.Id = Id; - target.AccountId = AccountId; - } } } \ No newline at end of file diff --git a/Schaad.Accounting.Common/Models/BookingText.cs b/Schaad.Accounting.Common/Models/BookingText.cs index 8eb1343..3f1b6e0 100644 --- a/Schaad.Accounting.Common/Models/BookingText.cs +++ b/Schaad.Accounting.Common/Models/BookingText.cs @@ -10,14 +10,5 @@ namespace Schaad.Accounting.Models [Required] [MinLength(3)] public string Text { get; set; } - - /// - /// Makes a copy - /// - public void Copy(BookingText target) - { - target.Id = Id; - target.Text = Text; - } } } \ No newline at end of file diff --git a/Schaad.Accounting.Common/Models/SplitPredefinition.cs b/Schaad.Accounting.Common/Models/SplitPredefinition.cs index 38e30c4..837eb1b 100644 --- a/Schaad.Accounting.Common/Models/SplitPredefinition.cs +++ b/Schaad.Accounting.Common/Models/SplitPredefinition.cs @@ -9,17 +9,5 @@ public decimal BookingValue { get; set; } public string AccountId { get; set; } - - - /// - /// Makes a copy - /// - public void Copy(SplitPredefinition target) - { - target.BookingText = BookingText; - target.BookingValue = BookingValue; - target.Id = Id; - target.AccountId = AccountId; - } } } \ No newline at end of file diff --git a/Schaad.Accounting.Common/Models/SubClass.cs b/Schaad.Accounting.Common/Models/SubClass.cs index 57e2e67..4584ade 100644 --- a/Schaad.Accounting.Common/Models/SubClass.cs +++ b/Schaad.Accounting.Common/Models/SubClass.cs @@ -14,15 +14,5 @@ namespace Schaad.Accounting.Models [Required] [MinLength(3)] public string Name { get; set; } - - /// - /// Makes a copy - /// - public void Copy(SubClass target) - { - target.Id = Id; - target.Name = Name; - target.Number = Number; - } } } \ No newline at end of file diff --git a/Schaad.Accounting.Common/Models/Transaction.cs b/Schaad.Accounting.Common/Models/Transaction.cs index bd91f84..e662b1a 100644 --- a/Schaad.Accounting.Common/Models/Transaction.cs +++ b/Schaad.Accounting.Common/Models/Transaction.cs @@ -74,20 +74,22 @@ namespace Schaad.Accounting.Models } /// - /// Makes a copy + /// Returns a shallow independent copy — used to hand out defensive copies + /// (e.g. from repository reads) without exposing the caller to later mutation. /// - public void Copy(Transaction target) + public Transaction Clone() => new() { - target.BankTransactionId = BankTransactionId; - target.BankTransactionText = BankTransactionText; - target.Id = Id; - target.OriginAccountId = OriginAccountId; - target.TargetAccountId = TargetAccountId; - target.Text = Text; - target.Value = Value; - target.ValueDate = ValueDate; - target.BookingDate = BookingDate > DateTime.MinValue ? BookingDate : ValueDate; - target.FxRate = FxRate; - } + Id = Id, + BankTransactionId = BankTransactionId, + BankTransactionText = BankTransactionText, + RelatedParty = RelatedParty, + OriginAccountId = OriginAccountId, + TargetAccountId = TargetAccountId, + Text = Text, + Value = Value, + ValueDate = ValueDate, + BookingDate = BookingDate, + FxRate = FxRate + }; } } \ No newline at end of file diff --git a/Schaad.Accounting.Db/Repositories/AccountRepository.cs b/Schaad.Accounting.Db/Repositories/AccountRepository.cs index cbe9ac8..4c7f032 100644 --- a/Schaad.Accounting.Db/Repositories/AccountRepository.cs +++ b/Schaad.Accounting.Db/Repositories/AccountRepository.cs @@ -28,16 +28,22 @@ namespace Schaad.Accounting.Repositories /// public void SaveAccount(Account account) { - var accounts = GetAccountList(); - var existingAccount = accounts.FirstOrDefault(a => a.Id == account.Id); - - if (existingAccount == null) + if (string.IsNullOrEmpty(account.Currency)) { - existingAccount = new Account(); - accounts.Add(existingAccount); - account.Id = Guid.NewGuid().ToString(); + account.Currency = "CHF"; + } + + var accounts = GetAccountList(); + var idx = accounts.FindIndex(a => a.Id == account.Id); + if (idx >= 0) + { + accounts[idx] = account; + } + else + { + account.Id = Guid.NewGuid().ToString(); + accounts.Add(account); } - account.Copy(existingAccount); Save(accounts, ACCOUNTS); } diff --git a/Schaad.Accounting.Db/Repositories/BookingRuleRepository.cs b/Schaad.Accounting.Db/Repositories/BookingRuleRepository.cs index 623c92d..c5b5110 100644 --- a/Schaad.Accounting.Db/Repositories/BookingRuleRepository.cs +++ b/Schaad.Accounting.Db/Repositories/BookingRuleRepository.cs @@ -28,15 +28,16 @@ namespace Schaad.Accounting.Repositories public void SaveBookingRule(BookingRule bookingRule) { var bookingRules = GetBookingRuleList(); - var existingRule = bookingRules.FirstOrDefault(a => a.Id == bookingRule.Id); - - if (existingRule == null) + var idx = bookingRules.FindIndex(r => r.Id == bookingRule.Id); + if (idx >= 0) { - existingRule = new BookingRule(); - bookingRules.Add(existingRule); - bookingRule.Id = Guid.NewGuid().ToString(); + bookingRules[idx] = bookingRule; + } + else + { + bookingRule.Id = Guid.NewGuid().ToString(); + bookingRules.Add(bookingRule); } - bookingRule.Copy(existingRule); Save(bookingRules, BOOKING_RULES); } diff --git a/Schaad.Accounting.Db/Repositories/BookingTextRepository.cs b/Schaad.Accounting.Db/Repositories/BookingTextRepository.cs index e98fe45..7c399fe 100644 --- a/Schaad.Accounting.Db/Repositories/BookingTextRepository.cs +++ b/Schaad.Accounting.Db/Repositories/BookingTextRepository.cs @@ -28,15 +28,16 @@ namespace Schaad.Accounting.Repositories public void SaveBookingText(BookingText bookingText) { var bookingTexts = GetBookingTextList(); - var existingText = bookingTexts.FirstOrDefault(a => a.Id == bookingText.Id); - - if (existingText == null) + var idx = bookingTexts.FindIndex(t => t.Id == bookingText.Id); + if (idx >= 0) { - existingText = new BookingText(); - bookingTexts.Add(existingText); - bookingText.Id = Guid.NewGuid().ToString(); + bookingTexts[idx] = bookingText; + } + else + { + bookingText.Id = Guid.NewGuid().ToString(); + bookingTexts.Add(bookingText); } - bookingText.Copy(existingText); Save(bookingTexts, BOOKING_TEXTS); } diff --git a/Schaad.Accounting.Db/Repositories/SplitPredefinitionRepository.cs b/Schaad.Accounting.Db/Repositories/SplitPredefinitionRepository.cs index 1cc276d..2c12fec 100644 --- a/Schaad.Accounting.Db/Repositories/SplitPredefinitionRepository.cs +++ b/Schaad.Accounting.Db/Repositories/SplitPredefinitionRepository.cs @@ -28,15 +28,16 @@ namespace Schaad.Accounting.Repositories public void SaveSplitPredefinition(SplitPredefinition splitPredefinition) { var definitions = GetSplitPredefinitionList(); - var existingDefinition = definitions.FirstOrDefault(a => a.Id == splitPredefinition.Id); - - if (existingDefinition == null) + var idx = definitions.FindIndex(d => d.Id == splitPredefinition.Id); + if (idx >= 0) { - existingDefinition = new SplitPredefinition(); - definitions.Add(existingDefinition); - splitPredefinition.Id = Guid.NewGuid().ToString(); + definitions[idx] = splitPredefinition; + } + else + { + splitPredefinition.Id = Guid.NewGuid().ToString(); + definitions.Add(splitPredefinition); } - splitPredefinition.Copy(existingDefinition); Save(definitions, SPLIT_PREDEFINITION); } } diff --git a/Schaad.Accounting.Db/Repositories/SubclassRepository.cs b/Schaad.Accounting.Db/Repositories/SubclassRepository.cs index 3117592..f9935c0 100644 --- a/Schaad.Accounting.Db/Repositories/SubclassRepository.cs +++ b/Schaad.Accounting.Db/Repositories/SubclassRepository.cs @@ -29,20 +29,21 @@ namespace Schaad.Accounting.Repositories public List GetSubClassList() => LoadList(SUBCLASSES); /// - /// Save a booking text (insert/update) + /// Save a subclass (insert/update) /// public void SaveSubClass(SubClass subClass) { var subclasses = GetSubClassList(); - var existingSubClass = subclasses.FirstOrDefault(a => a.Id == subClass.Id); - - if (existingSubClass == null) + var idx = subclasses.FindIndex(s => s.Id == subClass.Id); + if (idx >= 0) { - existingSubClass = new SubClass(); - subclasses.Add(existingSubClass); - subClass.Id = Guid.NewGuid().ToString(); + subclasses[idx] = subClass; + } + else + { + subClass.Id = Guid.NewGuid().ToString(); + subclasses.Add(subClass); } - subClass.Copy(existingSubClass); Save(subclasses, SUBCLASSES); } diff --git a/Schaad.Accounting.Db/Repositories/TransactionRepository.cs b/Schaad.Accounting.Db/Repositories/TransactionRepository.cs index e1f5c91..d7aa5e2 100644 --- a/Schaad.Accounting.Db/Repositories/TransactionRepository.cs +++ b/Schaad.Accounting.Db/Repositories/TransactionRepository.cs @@ -45,17 +45,22 @@ namespace Schaad.Accounting.Repositories transaction.FxRate = null; } + if (transaction.BookingDate <= DateTime.MinValue) + { + transaction.BookingDate = transaction.ValueDate; + } var transactionList = GetTransactionList(); - var existingTransaction = transactionList.FirstOrDefault(a => a.Id == transaction.Id); - - if (existingTransaction == null) + var idx = transactionList.FindIndex(t => t.Id == transaction.Id); + if (idx >= 0) { - existingTransaction = new Transaction(); - transactionList.Add(existingTransaction); - transaction.Id = Guid.NewGuid().ToString(); + transactionList[idx] = transaction; + } + else + { + transaction.Id = Guid.NewGuid().ToString(); + transactionList.Add(transaction); } - transaction.Copy(existingTransaction); Save(transactionList, TRANSACTIONS); } @@ -71,8 +76,7 @@ namespace Schaad.Accounting.Repositories return null; } - var result = new Transaction(); - stored.Copy(result); + var result = stored.Clone(); // value is stored in CHF -> convert back to foreign currency for display/editing var isFxAccount = accountRepository.GetAccount(result.OriginAccountId).IsFxAccount diff --git a/Schaad.Accounting.Services/ViewService.cs b/Schaad.Accounting.Services/ViewService.cs index 067a4e2..481e0f7 100644 --- a/Schaad.Accounting.Services/ViewService.cs +++ b/Schaad.Accounting.Services/ViewService.cs @@ -159,8 +159,7 @@ namespace Schaad.Accounting.Services var originAccount = accountsById[t.OriginAccountId]; if (originAccount.Class == ClassIds.Activa && accountId == t.OriginAccountId) { - var copy = new Transaction(); - t.Copy(copy); + var copy = t.Clone(); copy.Value *= -1; return copy; } diff --git a/Schaad.Accounting.Tests/TransactionRepositoryTestShould.cs b/Schaad.Accounting.Tests/TransactionRepositoryTestShould.cs index ba7efee..1e6960e 100644 --- a/Schaad.Accounting.Tests/TransactionRepositoryTestShould.cs +++ b/Schaad.Accounting.Tests/TransactionRepositoryTestShould.cs @@ -136,4 +136,43 @@ public class TransactionRepositoryTestShould : IDisposable sut.GetTransactionList().ShouldBeEmpty(); } + + [Fact] + public void PreserveRelatedPartyWhenRoundTrippingTransaction() + { + accountRepo.GetAccount(Arg.Any()).Returns(new Account { Currency = "CHF" }); + + sut.SaveTransaction(new Transaction + { + OriginAccountId = "a", + TargetAccountId = "b", + Value = 10m, + Text = "Rent", + RelatedParty = "ACME Property AG", + ValueDate = new DateTime(2026, 4, 1), + BookingDate = new DateTime(2026, 4, 1) + }); + + // Regression: earlier Transaction.Copy dropped RelatedParty on every save, + // so the round-trip lost it. + sut.GetTransactionList().Single().RelatedParty.ShouldBe("ACME Property AG"); + } + + [Fact] + public void DefaultBookingDateToValueDateWhenBookingDateIsUnset() + { + accountRepo.GetAccount(Arg.Any()).Returns(new Account { Currency = "CHF" }); + + sut.SaveTransaction(new Transaction + { + OriginAccountId = "a", + TargetAccountId = "b", + Value = 10m, + Text = "x", + ValueDate = new DateTime(2026, 4, 15) + // BookingDate left at default (DateTime.MinValue) + }); + + sut.GetTransactionList().Single().BookingDate.ShouldBe(new DateTime(2026, 4, 15)); + } }