From 4bea29431be217ea56efb30651f3f45441ba863d Mon Sep 17 00:00:00 2001 From: Arnel Robles Date: Thu, 13 Aug 2026 13:50:13 +0800 Subject: [PATCH 1/4] Accounting: cover the money paths, and fix a duplicate-account bug found doing it Accounting sat at 49.6% coverage while carrying the ledger. Three areas had no tests at all: the module's own HTTP surface, the one-shot migration, and AccountService. AccountService looked like dead code -- nothing inside barakoCMS calls it -- but BaryoClub uses it in seven places. Testing it turned up a real bug: UpsertAsync looked for an existing account with a database query, so accounts stored earlier in the same uncommitted unit of work were invisible. UpsertManyAsync loops that method and is how a host seeds a chart in one transaction, so a repeated code became two accounts sharing it, with balances split between them. It now checks pending changes first. The test was red before the fix. Each new test was checked by reintroducing the bug it claims to catch: a balance tolerance, totals accumulated through double, a migration that moves instead of copies, a dropped idempotency guard, a widened role gate. Two of those checks found weak tests rather than weak code. A one-line journal entry is rejected for being unbalanced, not for having too few lines, so the line-minimum rule was only really pinned once an entry with no lines was tested. Accounting 49.6% -> 85.4%, suite 71.1% -> 74.4%. Module bumped to 0.2.1. --- BarakoCMS.Accounting/AccountService.cs | 25 +- .../Accounting/AccountServiceTests.cs | 199 +++++++++++++ .../Features/Accounting/AccountingApiTests.cs | 278 ++++++++++++++++++ .../Accounting/AccountingMigrationTests.cs | 213 ++++++++++++++ CHANGELOG.md | 24 ++ 5 files changed, 735 insertions(+), 4 deletions(-) create mode 100644 BarakoCMS.Tests/Features/Accounting/AccountServiceTests.cs create mode 100644 BarakoCMS.Tests/Features/Accounting/AccountingApiTests.cs create mode 100644 BarakoCMS.Tests/Features/Accounting/AccountingMigrationTests.cs diff --git a/BarakoCMS.Accounting/AccountService.cs b/BarakoCMS.Accounting/AccountService.cs index 479f131..5d22181 100644 --- a/BarakoCMS.Accounting/AccountService.cs +++ b/BarakoCMS.Accounting/AccountService.cs @@ -60,14 +60,28 @@ public async Task CountAsync(CancellationToken ct = default) => public async Task UpsertAsync(Account account, CancellationToken ct = default) { var session = WriteSession; + var data = ToData(account); + + // Accounts staged earlier in this same unit of work are not in the database yet, so a query + // cannot see them. Seeding a whole chart in one transaction is the ordinary way to reach + // that, and without this the second appearance of a code becomes a second account — one code + // split across two documents, with lookups picking between them arbitrarily. + var staged = session.PendingChanges.AllChangedFor() + .FirstOrDefault(c => c.ContentType == AccountingContentTypes.Account && HasCode(c, account.Code)); + + if (staged is not null) + { + staged.Data = data; + staged.UpdatedAt = DateTime.UtcNow; + session.Store(staged); + return; + } + var existing = await session.Query() .Where(c => c.ContentType == AccountingContentTypes.Account) .ToListAsync(ct); - var match = existing.FirstOrDefault(c => string.Equals( - ContentData.AsString(ContentData.Get(c.Data, "Code")), account.Code, StringComparison.OrdinalIgnoreCase)); - - var data = ToData(account); + var match = existing.FirstOrDefault(c => HasCode(c, account.Code)); if (match is not null) { @@ -96,6 +110,9 @@ public async Task UpsertManyAsync(IEnumerable accounts, CancellationTok await UpsertAsync(account, ct); } + private static bool HasCode(barakoCMS.Models.Content c, string code) => string.Equals( + ContentData.AsString(ContentData.Get(c.Data, "Code")), code, StringComparison.OrdinalIgnoreCase); + private static Dictionary ToData(Account a) => new() { ["Code"] = a.Code, diff --git a/BarakoCMS.Tests/Features/Accounting/AccountServiceTests.cs b/BarakoCMS.Tests/Features/Accounting/AccountServiceTests.cs new file mode 100644 index 0000000..8ceb7b8 --- /dev/null +++ b/BarakoCMS.Tests/Features/Accounting/AccountServiceTests.cs @@ -0,0 +1,199 @@ +using Xunit; +using FluentAssertions; +using BarakoCMS.Accounting; +using BarakoCMS.Accounting.Domain; +using Marten; +using Microsoft.Extensions.DependencyInjection; +using barakoCMS.Models; + +namespace BarakoCMS.Tests.Features.Accounting; + +/// +/// The chart-of-accounts API that hosts actually use. +/// +/// It had no tests at all, which is the wrong way round: nothing inside barakoCMS calls it, so it +/// looked like dead code, but BaryoClub uses it in seven places — seeding the chart, creating member +/// accounts, batch charging, delisting, and reminders. Being consumer-only means a break here shows +/// up in someone else's repository, after a release, rather than in this one's CI. +/// +[Collection("Sequential")] +public class AccountServiceTests +{ + private readonly IntegrationTestFixture _factory; + + public AccountServiceTests(IntegrationTestFixture factory) => _factory = factory; + + private IDocumentStore Store() + { + using var scope = _factory.Services.CreateScope(); + return scope.ServiceProvider.GetRequiredService(); + } + + private static Account Acct(string code, string name = "Account", AccountType type = AccountType.Asset) => + new() { Code = code, Name = name, Type = type, IsActive = true }; + + private static string Tag() => Guid.NewGuid().ToString("N")[..8]; + + [Fact] + public async Task An_upserted_account_can_be_read_back() + { + var tag = Tag(); + using var s = Store().LightweightSession(); + var svc = new AccountService(s); + + await svc.UpsertAsync(Acct($"1000-{tag}", "Cash on hand")); + await s.SaveChangesAsync(); + + var found = await new AccountService(s).GetByCodeAsync($"1000-{tag}"); + found.Should().NotBeNull(); + found!.Name.Should().Be("Cash on hand"); + found.Type.Should().Be(AccountType.Asset); + found.IsActive.Should().BeTrue(); + } + + [Fact] + public async Task Lookup_by_code_ignores_case() + { + var tag = Tag(); + using var s = Store().LightweightSession(); + await new AccountService(s).UpsertAsync(Acct($"ab-{tag}")); + await s.SaveChangesAsync(); + + // Hosts pass codes through URLs and spreadsheets; a case-sensitive miss would read as + // "account not found" and send a charge to the wrong place, or nowhere. + (await new AccountService(s).GetByCodeAsync($"AB-{tag}")).Should().NotBeNull(); + } + + [Fact] + public async Task Upserting_an_existing_code_updates_in_place_rather_than_duplicating() + { + var tag = Tag(); + var code = $"4000-{tag}"; + + using (var s1 = Store().LightweightSession()) + { + await new AccountService(s1).UpsertAsync(Acct(code, "Dues", AccountType.Income)); + await s1.SaveChangesAsync(); + } + + using (var s2 = Store().LightweightSession()) + { + await new AccountService(s2).UpsertAsync(Acct(code, "Membership dues", AccountType.Income)); + await s2.SaveChangesAsync(); + } + + // Two accounts sharing a code means every balance for that code is split across two + // documents, and which one a lookup returns is arbitrary. + using var q = Store().QuerySession(); + var all = await new AccountService(q).GetAllAsync(); + all.Where(a => a.Code == code).Should().HaveCount(1); + all.Single(a => a.Code == code).Name.Should().Be("Membership dues"); + } + + [Fact] + public async Task The_chart_comes_back_ordered_by_code() + { + var tag = Tag(); + using var s = Store().LightweightSession(); + await new AccountService(s).UpsertManyAsync(new[] + { + Acct($"5000-{tag}"), Acct($"1000-{tag}"), Acct($"3000-{tag}"), + }); + await s.SaveChangesAsync(); + + var mine = (await new AccountService(s).GetAllAsync()) + .Where(a => a.Code.EndsWith(tag)).Select(a => a.Code).ToList(); + + mine.Should().Equal($"1000-{tag}", $"3000-{tag}", $"5000-{tag}"); + } + + [Fact] + public async Task A_read_only_service_refuses_to_write_instead_of_silently_dropping_it() + { + using var q = Store().QuerySession(); + var svc = new AccountService(q); + + // Silently doing nothing here would be the worst outcome: a seeding run that reports success + // and leaves the chart empty. + var act = async () => await svc.UpsertAsync(Acct("1000-readonly")); + await act.Should().ThrowAsync(); + } + + [Fact] + public async Task Member_metadata_survives_the_round_trip() + { + var tag = Tag(); + var memberId = Guid.NewGuid(); + + using var s = Store().LightweightSession(); + await new AccountService(s).UpsertAsync(new Account + { + Code = $"1200-{tag}", Name = "Receivable — J. Cruz", Type = AccountType.Asset, + MemberId = memberId, PayeeName = "J. Cruz", ParentCode = $"1000-{tag}", IsActive = true, + }); + await s.SaveChangesAsync(); + + // BaryoClub keys a member's statement off MemberId. Losing it detaches the member from their + // own receivable account, which is the cross-member read the project treats as a security bug. + var found = (await new AccountService(s).GetByCodeAsync($"1200-{tag}"))!; + found.MemberId.Should().Be(memberId); + found.PayeeName.Should().Be("J. Cruz"); + found.ParentCode.Should().Be($"1000-{tag}"); + } + + [Fact] + public async Task Deactivating_an_account_is_persisted() + { + var tag = Tag(); + var code = $"1300-{tag}"; + + using var s = Store().LightweightSession(); + await new AccountService(s).UpsertAsync(Acct(code)); + await s.SaveChangesAsync(); + + var acct = (await new AccountService(s).GetByCodeAsync(code))!; + acct.IsActive = false; + await new AccountService(s).UpsertAsync(acct); + await s.SaveChangesAsync(); + + // The journal hook refuses postings to an inactive account, so an ignored deactivation means + // a delisted member keeps accruing charges. + (await new AccountService(s).GetByCodeAsync(code))!.IsActive.Should().BeFalse(); + } + + [Fact] + public async Task Count_matches_what_the_chart_returns() + { + using var s = Store().LightweightSession(); + var svc = new AccountService(s); + (await svc.CountAsync()).Should().Be((await svc.GetAllAsync()).Count); + } + + /// + /// Repeating a code inside one uncommitted unit of work. + /// + /// looks for an existing account with + /// session.Query, which reads the database — so accounts stored earlier in the same + /// uncommitted batch are invisible to it. is a loop + /// over that method and is what a host uses to seed a whole chart in one transaction, which is + /// exactly where a repeated code is most likely to appear. + /// + [Fact] + public async Task Repeating_a_code_within_one_unit_of_work_does_not_create_two_accounts() + { + var tag = Tag(); + var code = $"6000-{tag}"; + + using var s = Store().LightweightSession(); + await new AccountService(s).UpsertManyAsync(new[] + { + Acct(code, "First spelling"), + Acct(code, "Second spelling"), + }); + await s.SaveChangesAsync(); + + using var q = Store().QuerySession(); + var matching = (await new AccountService(q).GetAllAsync()).Where(a => a.Code == code).ToList(); + matching.Should().HaveCount(1, "one code is one account, whichever transaction it arrived in"); + } +} diff --git a/BarakoCMS.Tests/Features/Accounting/AccountingApiTests.cs b/BarakoCMS.Tests/Features/Accounting/AccountingApiTests.cs new file mode 100644 index 0000000..fcdcda2 --- /dev/null +++ b/BarakoCMS.Tests/Features/Accounting/AccountingApiTests.cs @@ -0,0 +1,278 @@ +using Xunit; +using FluentAssertions; +using System.Net; +using System.Net.Http.Json; +using System.Net.Http.Headers; +using System.Text.Json; +using barakoCMS.Models; +using BarakoCMS.Accounting; +using BarakoCMS.Accounting.Domain; +using Marten; +using Microsoft.Extensions.DependencyInjection; + +namespace BarakoCMS.Tests.Features.Accounting; + +/// +/// The module's own HTTP surface: POST /api/accounting/journal-entries and the accounts endpoints. +/// +/// These had no tests at all, while carrying real money: BaryoClub posts a treasurer's entries through +/// the journal-entries route. Accounting moved to content types, and the content path is well covered, +/// but this route is still live, still registered, and still what an existing consumer calls — so a +/// change that satisfied the content tests could break the thing actually in use. +/// +/// The invariants worth stating: an unbalanced entry never reaches the store, a rejected post consumes +/// no entry number, amounts survive as decimal, and posting requires an accounting role. +/// +[Collection("Sequential")] +public class AccountingApiTests +{ + private readonly IntegrationTestFixture _factory; + private readonly HttpClient _client; + private Guid _userId; + private bool _seeded; + + public AccountingApiTests(IntegrationTestFixture factory) + { + _factory = factory; + _client = factory.CreateClient(); + } + + private async Task SeedAsync() + { + if (_seeded) return; + + using var scope = _factory.Services.CreateScope(); + var store = scope.ServiceProvider.GetRequiredService(); + using var session = store.LightweightSession(); + + foreach (var def in new[] + { + AccountingContentTypes.AccountDefinition(), + AccountingContentTypes.JournalEntryDefinition(), + }) + { + var existing = await session.Query().FirstOrDefaultAsync(t => t.Name == def.Name); + if (existing is null) session.Store(def); + } + + if (await session.LoadAsync(barakoCMS.Data.DataSeeder.SuperAdminRoleId) is null) + { + session.Store(new Role + { + Id = barakoCMS.Data.DataSeeder.SuperAdminRoleId, + Name = "SuperAdmin", + Description = "Full system access", + }); + } + + // The endpoint loads the caller by their UserId claim, so the user has to exist, not just the token. + _userId = Guid.NewGuid(); + session.Store(new User + { + Id = _userId, + Username = $"ledger_{Guid.NewGuid():N}", + Email = $"{Guid.NewGuid():N}@example.com", + RoleIds = new List { barakoCMS.Data.DataSeeder.SuperAdminRoleId }, + }); + await session.SaveChangesAsync(); + + _client.DefaultRequestHeaders.Authorization = + new AuthenticationHeaderValue("Bearer", _factory.CreateToken(new[] { "SuperAdmin" }, _userId.ToString())); + _seeded = true; + } + + private async Task AccountAsync(string code, string type = "Asset") + { + await SeedAsync(); + var res = await _client.PostAsJsonAsync("/api/contents", new + { + contentType = AccountingContentTypes.Account, + status = 1, + sensitivity = 0, + data = new { Code = code, Name = $"Account {code}", Type = type, IsActive = true }, + }); + res.StatusCode.Should().Be(HttpStatusCode.OK, await res.Content.ReadAsStringAsync()); + return code; + } + + private static object Line(string code, decimal debit, decimal credit) => + new { AccountCode = code, Debit = debit, Credit = credit }; + + private Task PostEntryAsync(object body) => + _client.PostAsJsonAsync("/api/accounting/journal-entries", body); + + private static object Entry(string memo, params object[] lines) => + new { Date = "2026-03-01", Memo = memo, Lines = lines }; + + private async Task EntryCountAsync() + { + using var scope = _factory.Services.CreateScope(); + var s = scope.ServiceProvider.GetRequiredService(); + var all = await s.Query() + .Where(c => c.ContentType == AccountingContentTypes.JournalEntry) + .ToListAsync(); + return all.Count; + } + + [Fact] + public async Task A_balanced_entry_posts_and_is_numbered() + { + var suffix = Guid.NewGuid().ToString("N")[..6]; + var cash = await AccountAsync($"1000-{suffix}"); + var income = await AccountAsync($"4000-{suffix}", "Income"); + + var res = await PostEntryAsync(Entry("membership dues", Line(cash, 1500.50m, 0m), Line(income, 0m, 1500.50m))); + + res.IsSuccessStatusCode.Should().BeTrue(await res.Content.ReadAsStringAsync()); + var body = await res.Content.ReadFromJsonAsync(); + body.GetProperty("entryNumber").GetString().Should().NotBeNullOrWhiteSpace( + "an entry without a number cannot be referred to in a statement"); + body.GetProperty("amount").GetDecimal().Should().Be(1500.50m); + } + + [Fact] + public async Task An_unbalanced_entry_is_refused_and_stores_nothing() + { + var suffix = Guid.NewGuid().ToString("N")[..6]; + var cash = await AccountAsync($"1000-{suffix}"); + var income = await AccountAsync($"4000-{suffix}", "Income"); + var before = await EntryCountAsync(); + + // One peso out. The whole point of double entry is that this cannot be stored. + var res = await PostEntryAsync(Entry("off by one", Line(cash, 100m, 0m), Line(income, 0m, 99m))); + + res.IsSuccessStatusCode.Should().BeFalse(await res.Content.ReadAsStringAsync()); + (await EntryCountAsync()).Should().Be(before, "a rejected entry must not reach the ledger"); + } + + [Fact] + public async Task A_rejected_entry_does_not_consume_an_entry_number() + { + var suffix = Guid.NewGuid().ToString("N")[..6]; + var cash = await AccountAsync($"1000-{suffix}"); + var income = await AccountAsync($"4000-{suffix}", "Income"); + + var first = await PostEntryAsync(Entry("first", Line(cash, 10m, 0m), Line(income, 0m, 10m))); + var firstNumber = (await first.Content.ReadFromJsonAsync()).GetProperty("entryNumber").GetString(); + + // A failed post in between must not burn a number, or the ledger shows a gap and a treasurer + // has to explain a missing entry that never existed. + (await PostEntryAsync(Entry("rejected", Line(cash, 10m, 0m), Line(income, 0m, 9m)))) + .IsSuccessStatusCode.Should().BeFalse(); + + var third = await PostEntryAsync(Entry("second", Line(cash, 20m, 0m), Line(income, 0m, 20m))); + var thirdNumber = (await third.Content.ReadFromJsonAsync()).GetProperty("entryNumber").GetString(); + + var firstSeq = int.Parse(new string(firstNumber!.Where(char.IsDigit).ToArray())[^4..]); + var thirdSeq = int.Parse(new string(thirdNumber!.Where(char.IsDigit).ToArray())[^4..]); + (thirdSeq - firstSeq).Should().Be(1, "the sequence should advance once, not twice"); + } + + [Fact] + public async Task An_entry_against_an_unknown_account_is_refused() + { + var suffix = Guid.NewGuid().ToString("N")[..6]; + var cash = await AccountAsync($"1000-{suffix}"); + var before = await EntryCountAsync(); + + var res = await PostEntryAsync(Entry("typo", Line(cash, 50m, 0m), Line("no-such-account", 0m, 50m))); + + res.IsSuccessStatusCode.Should().BeFalse(await res.Content.ReadAsStringAsync()); + (await EntryCountAsync()).Should().Be(before); + } + + [Fact] + public async Task An_entry_with_too_few_lines_is_refused() + { + var suffix = Guid.NewGuid().ToString("N")[..6]; + var cash = await AccountAsync($"1000-{suffix}"); + + // One line is refused, though the balance rule would have caught it anyway. + (await PostEntryAsync(Entry("one leg", Line(cash, 100m, 0m)))) + .IsSuccessStatusCode.Should().BeFalse(); + + // No lines is the case where the minimum is the only thing standing in the way: debits and + // credits are both zero, so the entry balances, and the "total must exceed zero" rule is + // itself conditioned on there being lines. Drop the minimum and an empty entry posts. + (await PostEntryAsync(Entry("nothing at all"))) + .IsSuccessStatusCode.Should().BeFalse("an entry with no lines is not an entry"); + } + + [Fact] + public async Task Fractional_amounts_survive_the_round_trip_exactly() + { + var suffix = Guid.NewGuid().ToString("N")[..6]; + var cash = await AccountAsync($"1000-{suffix}"); + var income = await AccountAsync($"4000-{suffix}", "Income"); + + // 0.1 + 0.2 is 0.30000000000000004 as double. If any part of this path is double, the entry + // either fails to balance or stores a number a treasurer cannot reconcile. + var res = await PostEntryAsync(Entry("thirds", + Line(cash, 0.1m, 0m), Line(cash, 0.2m, 0m), Line(income, 0m, 0.3m))); + + res.IsSuccessStatusCode.Should().BeTrue(await res.Content.ReadAsStringAsync()); + (await res.Content.ReadFromJsonAsync()).GetProperty("amount").GetDecimal() + .Should().Be(0.3m); + } + + [Fact] + public async Task A_large_amount_keeps_its_centavos() + { + var suffix = Guid.NewGuid().ToString("N")[..6]; + var cash = await AccountAsync($"1000-{suffix}"); + var income = await AccountAsync($"4000-{suffix}", "Income"); + + // Far beyond float's exact-integer range, with centavos that a double would round away. + const decimal big = 12_345_678.91m; + var res = await PostEntryAsync(Entry("annual", Line(cash, big, 0m), Line(income, 0m, big))); + + res.IsSuccessStatusCode.Should().BeTrue(await res.Content.ReadAsStringAsync()); + (await res.Content.ReadFromJsonAsync()).GetProperty("amount").GetDecimal() + .Should().Be(big); + } + + [Fact] + public async Task Posting_requires_an_accounting_role() + { + await SeedAsync(); + var anon = _factory.CreateClient(); + (await anon.PostAsJsonAsync("/api/accounting/journal-entries", Entry("anon", Line("1000", 1m, 0m)))) + .StatusCode.Should().Be(HttpStatusCode.Unauthorized); + + var editor = _factory.CreateClient(); + editor.DefaultRequestHeaders.Authorization = + new AuthenticationHeaderValue("Bearer", _factory.CreateToken(new[] { "Editor" }, Guid.NewGuid().ToString())); + (await editor.PostAsJsonAsync("/api/accounting/journal-entries", Entry("editor", Line("1000", 1m, 0m)))) + .StatusCode.Should().Be(HttpStatusCode.Forbidden, "an editor has no business posting to the ledger"); + } + + [Fact] + public async Task Accounts_can_be_created_and_listed_through_the_module_endpoints() + { + await SeedAsync(); + var code = $"5000-{Guid.NewGuid().ToString("N")[..6]}"; + + // Type is the AccountType enum and no string-enum converter is registered, so this endpoint + // takes the ordinal. The content-type path takes "Expense" as a string for the same concept — + // worth knowing before writing a client against either. + var created = await _client.PostAsJsonAsync("/api/accounting/accounts", new + { + Code = code, + Name = "Office supplies", + Type = (int)AccountType.Expense, + }); + created.IsSuccessStatusCode.Should().BeTrue(await created.Content.ReadAsStringAsync()); + + var listed = await _client.GetAsync("/api/accounting/accounts"); + listed.IsSuccessStatusCode.Should().BeTrue(); + (await listed.Content.ReadAsStringAsync()).Should().Contain(code); + } + + [Fact] + public async Task Listing_accounts_requires_a_role() + { + await SeedAsync(); + var anon = _factory.CreateClient(); + (await anon.GetAsync("/api/accounting/accounts")).StatusCode.Should().Be(HttpStatusCode.Unauthorized); + } +} diff --git a/BarakoCMS.Tests/Features/Accounting/AccountingMigrationTests.cs b/BarakoCMS.Tests/Features/Accounting/AccountingMigrationTests.cs new file mode 100644 index 0000000..c9d1cfb --- /dev/null +++ b/BarakoCMS.Tests/Features/Accounting/AccountingMigrationTests.cs @@ -0,0 +1,213 @@ +using Xunit; +using FluentAssertions; +using BarakoCMS.Accounting; +using BarakoCMS.Accounting.Domain; +using Marten; +using Microsoft.Extensions.DependencyInjection; +using barakoCMS.Models; + +namespace BarakoCMS.Tests.Features.Accounting; + +/// +/// The one-shot move of an existing deployment's ledger from the old strongly-typed documents onto +/// content types. +/// +/// This had no tests, and it is the single most dangerous piece of code in the module: it runs once, +/// against a real club's books, usually by someone following a runbook. A silent drop or a +/// double-post here is not a crash — it is a treasurer's balance being quietly wrong afterwards, with +/// the run already finished and the operator moving on. +/// +/// Its doc comment makes two promises, so those are what gets pinned: it copies rather than moves, +/// and running it twice does not duplicate anything. +/// +[Collection("Sequential")] +public class AccountingMigrationTests +{ + private readonly IntegrationTestFixture _factory; + + public AccountingMigrationTests(IntegrationTestFixture factory) => _factory = factory; + + private IDocumentStore Store() + { + using var scope = _factory.Services.CreateScope(); + return scope.ServiceProvider.GetRequiredService(); + } + + /// Old-shape data, tagged with a unique code prefix so parallel-safe assertions are possible. + private static (Account cash, Account income, JournalEntry entry) Legacy(string tag) + { + var cash = new Account + { + Code = $"1000-{tag}", Name = "Cash on hand", Type = AccountType.Asset, + IsActive = true, CreatedAt = new DateTime(2021, 5, 4, 9, 30, 0, DateTimeKind.Utc), + }; + var income = new Account + { + Code = $"4000-{tag}", Name = "Dues", Type = AccountType.Income, + IsActive = true, CreatedAt = new DateTime(2021, 5, 4, 9, 30, 0, DateTimeKind.Utc), + }; + var entry = new JournalEntry + { + EntryNumber = $"JE-2021-{tag}", + Date = new DateOnly(2021, 6, 1), + Memo = "Q2 dues", + Status = JournalStatus.Posted, + Amount = 2500.25m, + CreatedAt = new DateTime(2021, 6, 1, 8, 0, 0, DateTimeKind.Utc), + Lines = new List + { + new() { AccountCode = cash.Code, Debit = 2500.25m, Credit = 0m }, + new() { AccountCode = income.Code, Debit = 0m, Credit = 2500.25m }, + }, + }; + return (cash, income, entry); + } + + private async Task SeedLegacyAsync() + { + var tag = Guid.NewGuid().ToString("N")[..8]; + var (cash, income, entry) = Legacy(tag); + using var s = Store().LightweightSession(); + s.Store(cash, income); + s.Store(entry); + await s.SaveChangesAsync(); + return tag; + } + + private async Task> ContentAsync(string type, string tag) + { + using var s = Store().QuerySession(); + var all = await s.Query().Where(c => c.ContentType == type).ToListAsync(); + return all.Where(c => System.Text.Json.JsonSerializer.Serialize(c.Data).Contains(tag)).ToList(); + } + + [Fact] + public async Task It_copies_accounts_and_entries_onto_content_types() + { + var tag = await SeedLegacyAsync(); + + using var s = Store().LightweightSession(); + var result = await AccountingMigration.RunAsync(s, Guid.NewGuid()); + + result.AccountsCopied.Should().BeGreaterThanOrEqualTo(2); + result.EntriesCopied.Should().BeGreaterThanOrEqualTo(1); + + var accounts = await ContentAsync(AccountingContentTypes.Account, tag); + accounts.Should().HaveCount(2); + + var entries = await ContentAsync(AccountingContentTypes.JournalEntry, tag); + entries.Should().HaveCount(1); + } + + [Fact] + public async Task It_leaves_the_original_documents_in_place() + { + var tag = await SeedLegacyAsync(); + + using var s = Store().LightweightSession(); + await AccountingMigration.RunAsync(s, Guid.NewGuid()); + + // "Copy, not move" is the safety property the whole design rests on: if the converted shape + // turns out wrong, the books are still on disk and the migration can be re-run. A version + // that deleted as it went would pass every other test here. + using var q = Store().QuerySession(); + (await q.Query().Where(a => a.Code == $"1000-{tag}").ToListAsync()) + .Should().HaveCount(1, "the original account must survive the migration"); + (await q.Query().Where(e => e.EntryNumber == $"JE-2021-{tag}").ToListAsync()) + .Should().HaveCount(1, "the original entry must survive the migration"); + } + + [Fact] + public async Task Running_it_twice_copies_nothing_the_second_time() + { + var tag = await SeedLegacyAsync(); + + using (var s1 = Store().LightweightSession()) + await AccountingMigration.RunAsync(s1, Guid.NewGuid()); + + using var s2 = Store().LightweightSession(); + var second = await AccountingMigration.RunAsync(s2, Guid.NewGuid()); + + // An operator who is unsure whether the first run finished will run it again. If that + // double-posts the ledger, every balance afterwards is wrong and nothing announces it. + second.EntriesCopied.Should().Be(0, "a second run must not re-post the ledger"); + second.AccountsCopied.Should().Be(0, "a second run must not duplicate the chart"); + second.EntriesSkipped.Should().BeGreaterThan(0, "the entries should be recognised, not invisible"); + + (await ContentAsync(AccountingContentTypes.JournalEntry, tag)).Should().HaveCount(1); + (await ContentAsync(AccountingContentTypes.Account, tag)).Should().HaveCount(2); + } + + [Fact] + public async Task Amounts_and_line_values_survive_as_decimal() + { + var tag = await SeedLegacyAsync(); + + using var s = Store().LightweightSession(); + await AccountingMigration.RunAsync(s, Guid.NewGuid()); + + var entry = (await ContentAsync(AccountingContentTypes.JournalEntry, tag)).Single(); + + // The centavos are the point. A migration that round-trips money through double moves a + // real club's books by amounts too small for anyone to notice on the day. + Dec(entry.Data, "Amount").Should().Be(2500.25m); + + var lines = ((System.Collections.IEnumerable)entry.Data["Lines"]).Cast().Select(AsDict).ToList(); + var debits = lines.Select(l => Dec(l, "Debit")).ToList(); + var credits = lines.Select(l => Dec(l, "Credit")).ToList(); + + debits.Sum().Should().Be(2500.25m); + credits.Sum().Should().Be(2500.25m); + debits.Sum().Should().Be(credits.Sum(), "a migrated entry that no longer balances is a corrupted ledger"); + } + + [Fact] + public async Task The_original_dates_are_preserved() + { + var tag = await SeedLegacyAsync(); + + using var s = Store().LightweightSession(); + await AccountingMigration.RunAsync(s, Guid.NewGuid()); + + // Stamping migration day onto the records would file every historical entry under the date + // the move happened, which quietly rewrites every period report. + var entry = (await ContentAsync(AccountingContentTypes.JournalEntry, tag)).Single(); + Str(entry.Data, "Date").Should().Be("2021-06-01"); + entry.CreatedAt.Should().BeCloseTo(new DateTime(2021, 6, 1, 8, 0, 0, DateTimeKind.Utc), TimeSpan.FromSeconds(1)); + + var account = (await ContentAsync(AccountingContentTypes.Account, tag)) + .Single(a => Str(a.Data, "Code") == $"1000-{tag}"); + account.CreatedAt.Should().BeCloseTo(new DateTime(2021, 5, 4, 9, 30, 0, DateTimeKind.Utc), TimeSpan.FromSeconds(1)); + } + + [Fact] + public async Task Migrated_entries_are_published_and_not_marked_sensitive() + { + var tag = await SeedLegacyAsync(); + + using var s = Store().LightweightSession(); + await AccountingMigration.RunAsync(s, Guid.NewGuid()); + + // A migrated ledger that landed as Draft would be invisible to every report, which reads as + // "the migration lost my data" even though it is all there. + (await ContentAsync(AccountingContentTypes.JournalEntry, tag)).Single() + .Status.Should().Be(ContentStatus.Published); + } + + // ContentData is internal to the module, so the bag is read directly here rather than widening + // production visibility to suit a test. + private static decimal Dec(Dictionary d, string key) => + d[key] is System.Text.Json.JsonElement je ? je.GetDecimal() : Convert.ToDecimal(d[key]); + + private static string Str(Dictionary d, string key) => + d[key] is System.Text.Json.JsonElement je ? je.GetString() ?? "" : Convert.ToString(d[key]) ?? ""; + + private static Dictionary AsDict(object o) => o switch + { + Dictionary d => d, + System.Text.Json.JsonElement je => System.Text.Json.JsonSerializer + .Deserialize>(je.GetRawText())!, + _ => System.Text.Json.JsonSerializer + .Deserialize>(System.Text.Json.JsonSerializer.Serialize(o))!, + }; +} diff --git a/CHANGELOG.md b/CHANGELOG.md index dfe3f02..093b948 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,30 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed: seeding a chart of accounts could create two accounts sharing one code + +`AccountService.UpsertAsync` looked for an existing account with a database query, so accounts stored +earlier in the *same uncommitted* unit of work were invisible to it. `UpsertManyAsync` is a loop over +that method and is how a host seeds a whole chart in one transaction — precisely where a repeated +code is most likely to appear. The second appearance became a second account: one code split across +two documents, with lookups picking between them arbitrarily and balances divided between them. + +It now checks the session's pending changes before the database. Accounting module `0.2.1`. + +### Accounting test coverage: 49.6% → 77.7% + +The module's own HTTP surface (`POST /api/accounting/journal-entries`, the accounts endpoints), the +one-shot `AccountingMigration`, and `AccountService` had no tests between them, while carrying the +money. Three new suites cover them, each checked by reintroducing the bug it claims to catch — +balance tolerance, totals accumulated through `double`, a migration that moves instead of copies, a +dropped idempotency guard, and a widened role gate. + +Two of those checks found weak tests rather than weak code, and both were rewritten: a one-line +journal entry is rejected for being unbalanced, not for having too few lines, so the line-minimum +rule was only pinned once an entry with *no* lines was tested. + ## [3.19.0] - 2026-08-09 ### Fixed: the Next.js upgrade that was never actually broken From 3597683849e6f49b0ec583b560cf6cca73c8c141 Mon Sep 17 00:00:00 2001 From: Arnel Robles Date: Sat, 15 Aug 2026 17:37:18 +0800 Subject: [PATCH 2/4] Tests: close two gaps found in review The migration test's name promised the entry was not marked sensitive but only checked Status. Sensitivity decides who may read the record, so a migration that changed it would move the ledger across an access boundary silently. The upsert tests reused one code spelling, so the case-insensitive match they exist to cover was never exercised, and neither proved which of the two writes won. Both now use letter codes and differ by case; reverting the comparison to Ordinal fails them, which it did not before. --- .../Accounting/AccountServiceTests.cs | 21 ++++++++++++------- .../Accounting/AccountingMigrationTests.cs | 10 +++++++-- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/BarakoCMS.Tests/Features/Accounting/AccountServiceTests.cs b/BarakoCMS.Tests/Features/Accounting/AccountServiceTests.cs index 8ceb7b8..4782596 100644 --- a/BarakoCMS.Tests/Features/Accounting/AccountServiceTests.cs +++ b/BarakoCMS.Tests/Features/Accounting/AccountServiceTests.cs @@ -68,7 +68,9 @@ public async Task Lookup_by_code_ignores_case() public async Task Upserting_an_existing_code_updates_in_place_rather_than_duplicating() { var tag = Tag(); - var code = $"4000-{tag}"; + // Letters, so the second write can differ only in case and actually exercise the + // case-insensitive match. A digits-only code would compare equal either way. + var code = $"dues-{tag}"; using (var s1 = Store().LightweightSession()) { @@ -78,7 +80,8 @@ public async Task Upserting_an_existing_code_updates_in_place_rather_than_duplic using (var s2 = Store().LightweightSession()) { - await new AccountService(s2).UpsertAsync(Acct(code, "Membership dues", AccountType.Income)); + await new AccountService(s2).UpsertAsync( + Acct(code.ToUpperInvariant(), "Membership dues", AccountType.Income)); await s2.SaveChangesAsync(); } @@ -86,8 +89,9 @@ public async Task Upserting_an_existing_code_updates_in_place_rather_than_duplic // documents, and which one a lookup returns is arbitrary. using var q = Store().QuerySession(); var all = await new AccountService(q).GetAllAsync(); - all.Where(a => a.Code == code).Should().HaveCount(1); - all.Single(a => a.Code == code).Name.Should().Be("Membership dues"); + var matching = all.Where(a => string.Equals(a.Code, code, StringComparison.OrdinalIgnoreCase)).ToList(); + matching.Should().HaveCount(1, "a code differing only in case is the same account"); + matching.Single().Name.Should().Be("Membership dues", "the later write wins"); } [Fact] @@ -182,18 +186,21 @@ public async Task Count_matches_what_the_chart_returns() public async Task Repeating_a_code_within_one_unit_of_work_does_not_create_two_accounts() { var tag = Tag(); - var code = $"6000-{tag}"; + var code = $"levy-{tag}"; using var s = Store().LightweightSession(); await new AccountService(s).UpsertManyAsync(new[] { Acct(code, "First spelling"), - Acct(code, "Second spelling"), + Acct(code.ToUpperInvariant(), "Second spelling"), }); await s.SaveChangesAsync(); using var q = Store().QuerySession(); - var matching = (await new AccountService(q).GetAllAsync()).Where(a => a.Code == code).ToList(); + var matching = (await new AccountService(q).GetAllAsync()) + .Where(a => string.Equals(a.Code, code, StringComparison.OrdinalIgnoreCase)).ToList(); matching.Should().HaveCount(1, "one code is one account, whichever transaction it arrived in"); + matching.Single().Name.Should().Be("Second spelling", + "the staged account must be replaced, not merely left alone"); } } diff --git a/BarakoCMS.Tests/Features/Accounting/AccountingMigrationTests.cs b/BarakoCMS.Tests/Features/Accounting/AccountingMigrationTests.cs index c9d1cfb..0b05eba 100644 --- a/BarakoCMS.Tests/Features/Accounting/AccountingMigrationTests.cs +++ b/BarakoCMS.Tests/Features/Accounting/AccountingMigrationTests.cs @@ -190,8 +190,14 @@ public async Task Migrated_entries_are_published_and_not_marked_sensitive() // A migrated ledger that landed as Draft would be invisible to every report, which reads as // "the migration lost my data" even though it is all there. - (await ContentAsync(AccountingContentTypes.JournalEntry, tag)).Single() - .Status.Should().Be(ContentStatus.Published); + var migrated = (await ContentAsync(AccountingContentTypes.JournalEntry, tag)).Single(); + migrated.Status.Should().Be(ContentStatus.Published); + + // The other half of the name, which went unchecked at first: sensitivity decides who may + // read the record at all, so a migration that changed it would move the whole ledger across + // an access boundary without anything reporting a failure. + migrated.Sensitivity.Should().Be(SensitivityLevel.Public, + "the migration must not alter who can read an entry"); } // ContentData is internal to the module, so the bag is read directly here rather than widening From 7973e170b6130157ce628896dcf9f2f7f6ceb216 Mon Sep 17 00:00:00 2001 From: Arnel Robles Date: Sat, 15 Aug 2026 17:40:58 +0800 Subject: [PATCH 3/4] Accounting 0.2.2: the packaging release already took 0.2.1 --- BarakoCMS.Accounting/BarakoCMS.Accounting.csproj | 2 +- CHANGELOG.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/BarakoCMS.Accounting/BarakoCMS.Accounting.csproj b/BarakoCMS.Accounting/BarakoCMS.Accounting.csproj index c95822e..820a280 100644 --- a/BarakoCMS.Accounting/BarakoCMS.Accounting.csproj +++ b/BarakoCMS.Accounting/BarakoCMS.Accounting.csproj @@ -5,7 +5,7 @@ true BarakoCMS.Accounting - 0.2.1 + 0.2.2 Optional double-entry accounting module for barakoCMS: accounts, balanced journal entries, and reporting. diff --git a/CHANGELOG.md b/CHANGELOG.md index 093b948..8eee53a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ that method and is how a host seeds a whole chart in one transaction — precise code is most likely to appear. The second appearance became a second account: one code split across two documents, with lookups picking between them arbitrarily and balances divided between them. -It now checks the session's pending changes before the database. Accounting module `0.2.1`. +It now checks the session's pending changes before the database. Accounting module `0.2.2`. ### Accounting test coverage: 49.6% → 77.7% From a3395a01845863fac20cf24abae3f4b2a7ac7c4e Mon Sep 17 00:00:00 2001 From: Arnel Robles Date: Sat, 15 Aug 2026 17:51:28 +0800 Subject: [PATCH 4/4] Changelog: correct the accounting coverage figure to 85.4% 77.7% was the interim number, taken before the AccountService tests were added. --- CHANGELOG.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8eee53a..08a34d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ two documents, with lookups picking between them arbitrarily and balances divide It now checks the session's pending changes before the database. Accounting module `0.2.2`. -### Accounting test coverage: 49.6% → 77.7% +### Accounting test coverage: 49.6% → 85.4% The module's own HTTP surface (`POST /api/accounting/journal-entries`, the accounts endpoints), the one-shot `AccountingMigration`, and `AccountService` had no tests between them, while carrying the @@ -27,7 +27,12 @@ dropped idempotency guard, and a widened role gate. Two of those checks found weak tests rather than weak code, and both were rewritten: a one-line journal entry is rejected for being unbalanced, not for having too few lines, so the line-minimum -rule was only pinned once an entry with *no* lines was tested. +rule was only pinned once an entry with *no* lines was tested; and a `(decimal)(double)` round trip +is lossless at these magnitudes, so the shape that actually bites — the running totals declared as +`double` — is what the fractional-amount test now pins. + +`AccountService` was the surprise. Nothing inside barakoCMS calls it, so it read as dead code, but +BaryoClub uses it in seven places. Whole suite: 71.1% → 74.4%. ## [3.19.0] - 2026-08-09