From 2eedd2fd20a0142645b0edd097f84c2d4c353bf2 Mon Sep 17 00:00:00 2001 From: Claudio Schaad Date: Thu, 2 Jul 2026 20:56:20 +0200 Subject: [PATCH] PR E: ChartService cleanup Remove the settings-mutating year-hopping in GetAccountExpensesPerMonth that discarded the user's header year selection and silently forced the Spendings-over-time chart back to DateTime.Now.Year. The chart now honours settingsService.GetYear(), skips accounts with no transactions instead of returning null, and no longer depends on IAccountRepository or ISubclassRepository. Also reorder the plan: async becomes PR F, tests PR G. Co-Authored-By: Claude Opus 4.7 --- IMPROVEMENT_PLAN.md | 5 +- Schaad.Accounting.Services/ChartService.cs | 130 +++++++++------------ 2 files changed, 55 insertions(+), 80 deletions(-) diff --git a/IMPROVEMENT_PLAN.md b/IMPROVEMENT_PLAN.md index ab9f5ae..84ae1ed 100644 --- a/IMPROVEMENT_PLAN.md +++ b/IMPROVEMENT_PLAN.md @@ -99,5 +99,6 @@ Analysis and phased plan produced 2026-07-02. See conversation history for full - **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 (async). -- **PR F** — Phase 4 (tests) — done alongside PR D to lock in behavior. +- **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 items 19 + 21: async I/O across the repository stack, and precomputed dictionaries for view assembly. +- **PR G** — Phase 4 (tests) — locks in behavior for PR F. diff --git a/Schaad.Accounting.Services/ChartService.cs b/Schaad.Accounting.Services/ChartService.cs index 6ab109d..169cdd3 100644 --- a/Schaad.Accounting.Services/ChartService.cs +++ b/Schaad.Accounting.Services/ChartService.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using Schaad.Accounting.Datasets; using Schaad.Accounting.Datasets.Charts; using Schaad.Accounting.Interfaces; @@ -8,115 +9,88 @@ namespace Schaad.Accounting.Services { public class ChartService : IChartService { - private readonly IAccountRepository accountRepository; private readonly ISettingsService settingsService; - private readonly ISubclassRepository subclassRepository; private readonly IViewService viewService; - public ChartService( - ISettingsService settingsService, - IViewService viewService, - IAccountRepository accountRepository, - ISubclassRepository subclassRepository) + public ChartService(ISettingsService settingsService, IViewService viewService) { this.settingsService = settingsService; this.viewService = viewService; - this.accountRepository = accountRepository; - this.subclassRepository = subclassRepository; } public IReadOnlyList GetExpensesPerMonth() { - var transactions = viewService.GetTransactionViewList().Where(a => a.TargetAccount.Class == ClassIds.Expenses); - - // More than one subclass xx - if (transactions.Select(t => t.TargetAccount.SubClass).Distinct().Count() > 1) - { - return GetSubClassExpensesPerMonth(); - } - - // only one subclass xx (z.B. Mandant Mannenbach) - return GetAccountExpensesPerMonth(); + var expenseTransactions = viewService.GetTransactionViewList() + .Where(t => t.TargetAccount.Class == ClassIds.Expenses) + .ToList(); + + var distinctSubClasses = expenseTransactions + .Select(t => t.TargetAccount.SubClass) + .Distinct() + .Count(); + + return distinctSubClasses > 1 + ? GetSubClassExpensesPerMonth(expenseTransactions) + : GetAccountExpensesPerMonth(); } - private List GetSubClassExpensesPerMonth() + private List GetSubClassExpensesPerMonth(List expenseTransactions) { - //var subclasses = subclassRepository.GetSubClassList(); - var transactions = viewService.GetTransactionViewList().Where(a => a.TargetAccount.Class == ClassIds.Expenses).ToList(); - var list = new List(); - - var newestTransaction = transactions.OrderByDescending(t => t.ValueDate).FirstOrDefault(); - var maxMonth = newestTransaction?.ValueDate.Month ?? 12; var year = settingsService.GetYear(); + var maxMonth = expenseTransactions + .OrderByDescending(t => t.ValueDate) + .FirstOrDefault()?.ValueDate.Month ?? 12; - // Group by subclass - foreach (var grp in transactions.GroupBy(a => a.TargetAccount.SubClass).Select(a => new {Key = a.Key, List = a.ToList()})) + var list = new List(); + foreach (var grp in expenseTransactions.GroupBy(t => t.TargetAccount.SubClass)) { - // Sum subclass transactions per month - var groupedByMonth = grp.List.GroupBy(g => g.ValueDate.Month).ToDictionary(g => g.Key, g => g.ToList().Sum(s => s.Value)); + var groupedByMonth = grp + .GroupBy(t => t.ValueDate.Month) + .ToDictionary(g => g.Key, g => g.Sum(t => t.Value)); EnsureEntryForEveryMonth(groupedByMonth, maxMonth); - //var subClass = subclasses.FirstOrDefault(s => s.Number == grp.List.First().TargetAccount.SubClass); - list.Add( - new DataSerie( - Id: grp.List.First().TargetAccount.SubClass.ToString(), - Name: grp.List.First().TargetAccount.Name, - X: groupedByMonth.OrderBy(g => g.Key).Select(g => new DateOnly(year, g.Key, 1)).ToList(), - Y: groupedByMonth.OrderBy(g => g.Key).Select(g => g.Value).ToList() - ) - ); + list.Add(new DataSerie( + Id: grp.Key.ToString(), + Name: grp.First().TargetAccount.Name, + X: groupedByMonth.OrderBy(g => g.Key).Select(g => new DateOnly(year, g.Key, 1)).ToList(), + Y: groupedByMonth.OrderBy(g => g.Key).Select(g => g.Value).ToList() + )); } return list; } private List GetAccountExpensesPerMonth() { + var year = settingsService.GetYear(); + var allTransactions = viewService.GetTransactionViewList(); + var expenseAccounts = viewService.GetAccountViewList().Where(a => a.Class == ClassIds.Expenses); + var list = new List(); - var expensesAccounts = viewService.GetAccountViewList().Where(a => a.Class == ClassIds.Expenses); - foreach (var account in expensesAccounts) + foreach (var account in expenseAccounts) { - var serie = GetAccountExpensesPerMonth(account.Id, DateTime.Now.Year); - if (serie != null) + var transactions = allTransactions.Where(t => t.TargetAccountId == account.Id).ToList(); + if (transactions.Count == 0) { - list.Add(serie); + continue; } + + var maxMonth = transactions.Max(t => t.ValueDate.Month); + var groupedByMonth = transactions + .GroupBy(t => t.ValueDate.Month) + .ToDictionary(g => g.Key, g => g.Sum(t => t.Value)); + EnsureEntryForEveryMonth(groupedByMonth, maxMonth); + + list.Add(new DataSerie( + Id: account.Id, + Name: account.Name, + X: groupedByMonth.OrderBy(g => g.Key).Select(g => new DateOnly(year, g.Key, 1)).ToList(), + Y: groupedByMonth.OrderBy(g => g.Key).Select(g => g.Value).ToList() + )); } return list; } - private DataSerie GetAccountExpensesPerMonth(string accountId, int year) - { - if (settingsService.TrySetYear(year)) - { - var accountList = accountRepository.GetAccountList(); - var account = accountList.SingleOrDefault(a => a.Id == accountId); - // perhaps we dont have the account for last year - if (account == null) - { - return null; - } - var transactions = viewService.GetTransactionViewList().Where(t => t.TargetAccountId == accountId).ToList(); - - var newestTransaction = transactions.OrderByDescending(t => t.ValueDate).FirstOrDefault(); - var maxMonth = newestTransaction?.ValueDate.Month ?? 12; - - var groupedByMonth = transactions.GroupBy(g => g.ValueDate.Month).ToDictionary(g => g.Key, g => g.ToList().Sum(s => s.Value)); - EnsureEntryForEveryMonth(groupedByMonth, maxMonth); - - settingsService.SetYear(DateTime.Now.Year); - - return new DataSerie( - Id: accountId, - Name: account.Name, - X: groupedByMonth.OrderBy(g => g.Key).Select(g => new DateOnly(year, g.Key, 1)).ToList(), - Y: groupedByMonth.OrderBy(g => g.Key).Select(g => g.Value).ToList() - ); - } - - return null; - } - - private void EnsureEntryForEveryMonth(Dictionary values, int maxMonth = 12) + private static void EnsureEntryForEveryMonth(Dictionary values, int maxMonth = 12) { for (int i = 1; i <= maxMonth; i++) { @@ -127,4 +101,4 @@ namespace Schaad.Accounting.Services } } } -} \ No newline at end of file +}