From 3ffe4578c28d6c0d8df4efb9a4e33d5c76dbcb5d Mon Sep 17 00:00:00 2001 From: Claudio Schaad Date: Thu, 2 Jul 2026 21:57:17 +0200 Subject: [PATCH] PR L: drop Copy(target) methods in favour of index-based replace Replace the hand-rolled model.Copy(target) pattern in every SaveXxx with FindIndex -> in-place replace (or Add on new). Move the two hidden defaults out of the deleted Copy bodies: Currency = "CHF" default now lives in AccountRepository.SaveAccount; BookingDate = ValueDate default now lives in TransactionRepository.SaveTransaction. Delete Copy from Account, BookingRule, BookingText, SubClass, and SplitPredefinition. Keep Transaction.Clone() (renamed from Copy, and now includes RelatedParty) for the defensive copy in GetTransaction and ViewService.WithDisplaySign. Fixes a pre-existing bug where Transaction.Copy silently dropped RelatedParty on every update save. 42 tests total, all passing. Co-Authored-By: Claude Opus 4.7 --- IMPROVEMENT_PLAN.md | 1 + Schaad.Accounting.Common/Models/Account.cs | 14 ------- .../Models/BookingRule.cs | 13 ------- .../Models/BookingText.cs | 9 ----- .../Models/SplitPredefinition.cs | 12 ------ Schaad.Accounting.Common/Models/SubClass.cs | 10 ----- .../Models/Transaction.cs | 28 ++++++------- .../Repositories/AccountRepository.cs | 22 +++++++---- .../Repositories/BookingRuleRepository.cs | 15 +++---- .../Repositories/BookingTextRepository.cs | 15 +++---- .../SplitPredefinitionRepository.cs | 15 +++---- .../Repositories/SubclassRepository.cs | 17 ++++---- .../Repositories/TransactionRepository.cs | 22 ++++++----- Schaad.Accounting.Services/ViewService.cs | 3 +- .../TransactionRepositoryTestShould.cs | 39 +++++++++++++++++++ 15 files changed, 116 insertions(+), 119 deletions(-) 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)); + } }