refactor: streamline BibItem and BibRecord creation logic, introduce domain event handling #2068
Conversation
…domain event handling - Removed `Status` from `BibItem` creation, validators, and associated tests for simplification. - Introduced `BookRecordCreatedEventHandler` to handle automatic BibItem creation upon BibRecord initialization. - Updated `BibItem` domain model with default acquisition details and methods for condition/note updates. - Enhanced validation and creation logic in `BibItem` and `BibRecord` for better error handling and default management. - Adjusted unit tests to align with the new creation paradigm and replaced redundant initialization methods. - Optimized repository `AddAsync` method to efficiently handle batch inserts and introduced logging for diagnostics.
|
WalkthroughRemoves Status from AddBibItemCommand and BibItem.Create/Update signatures; introduces ConditionNote/Notes with dedicated mutators; adds batch AddAsync to repositories; refactors QuickAdd to emit BookRecordCreatedEvent and moves item creation to an event handler; updates BibRecord to raise domain events; adjusts GraphQL message and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant API as GraphQL/API
participant QuickAdd as BookQuickAddCommandHandler
participant BibRecord as BibRecord (Domain)
participant RepoR as BibRecordRepository
participant DomainBus as DomainEventBus
participant EH as BookRecordCreatedEventHandler
participant RepoI as BibItemRepository
participant Outbox as Outbox
User->>API: BookQuickAdd(request)
API->>QuickAdd: Handle(request)
QuickAdd->>BibRecord: CreateBookRecord(..., numberOfCopies)
BibRecord-->>QuickAdd: KnResult<BibRecord> (adds BookRecordCreatedEvent)
QuickAdd->>RepoR: AddAsync(bibRecord)
RepoR->>Outbox: Persist domain events/outbox
QuickAdd-->>API: Success(createdBibRecord)
note over DomainBus,EH: async event handling (new/changed)
DomainBus-->>EH: BookRecordCreatedEvent(bibRecordId, numberOfCopies)
EH->>EH: loop 1..NumberOfCopies (generate barcode/callNo)
EH->>RepoI: AddAsync(IEnumerable<BibItem>)
RepoI->>Outbox: Persist outbox messages
EH-->>DomainBus: Done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Potential focus areas:
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/Kathanika.Domain/Aggregates/BibRecordAggregate/BibRecord.cs (1)
86-95: Validate numberOfPages and numberOfCopiesGuard against invalid inputs at creation time to protect downstream handlers and data integrity.
public static KnResult<BibRecord> CreateBookRecord( @@ - long numberOfPages, - int numberOfCopies = 0) + long numberOfPages, + int numberOfCopies = 0) { + if (numberOfPages <= 0) + return KnResult.Failure<BibRecord>(new KnError("BibRecord.InvalidNumberOfPages", + "Number of pages must be positive.")); + if (numberOfCopies < 0) + return KnResult.Failure<BibRecord>(new KnError("BibRecord.InvalidNumberOfCopies", + "Number of copies cannot be negative."));
🧹 Nitpick comments (23)
tests/Kathanika.Domain.Tests/ArchTests/ArchTests.cs (1)
7-39: Don’t comment out tests; mark them skipped or delete.Keep the intent visible and runnable by the test runner.
Apply within this block:
- // [Fact] - // public void PublicMethod_MustReturnResult() - // { - // List<Type> aggregatesAndEntities = Types.InAssembly(typeof(Entity).Assembly) - // .That() - // .Inherit(typeof(Entity)) - // .Or() - // .Inherit(typeof(AggregateRoot)) - // .And() - // .ArePublic() - // .GetTypes() - // .ToList(); - // - // var aggregateMethods = typeof(AggregateRoot) - // .GetMethods() - // .Where(m => m.IsPublic) - // .Select(m => m.Name) - // .ToArray(); - // - // var nonResultReturnTypes = aggregatesAndEntities - // .SelectMany(t => t.GetMethods()) - // .Where( - // m => !(m.IsSpecialName && m.Name.StartsWith("get_")) - // && !(m.IsSpecialName && m.Name.StartsWith("set_")) - // && !(m.ReturnType == typeof(KnResult) - // || (m.ReturnType.IsGenericType && - // m.ReturnType.GetGenericTypeDefinition().BaseType == typeof(KnResult))) - // && aggregateMethods.All(am => am != m.Name)) - // .Select(x => new { MethodName = x.Name, Type = x.DeclaringType?.Name }) - // .ToList(); - // - // Assert.Empty(nonResultReturnTypes); - // } + [Fact(Skip = "Disabled during domain-event refactor. Update rule/expectations, then re-enable.")] + public void PublicMethod_MustReturnResult() + { + List<Type> aggregatesAndEntities = Types.InAssembly(typeof(Entity).Assembly) + .That() + .Inherit(typeof(Entity)) + .Or() + .Inherit(typeof(AggregateRoot)) + .And() + .ArePublic() + .GetTypes() + .ToList(); + + var aggregateMethods = typeof(AggregateRoot) + .GetMethods() + .Where(m => m.IsPublic) + .Select(m => m.Name) + .ToArray(); + + var nonResultReturnTypes = aggregatesAndEntities + .SelectMany(t => t.GetMethods()) + .Where( + m => !(m.IsSpecialName && m.Name.StartsWith("get_")) + && !(m.IsSpecialName && m.Name.StartsWith("set_")) + && !(m.ReturnType == typeof(KnResult) + || (m.ReturnType.IsGenericType && + m.ReturnType.GetGenericTypeDefinition().BaseType == typeof(KnResult))) + && aggregateMethods.All(am => am != m.Name)) + .Select(x => new { MethodName = x.Name, Type = x.DeclaringType?.Name }) + .ToList(); + + Assert.Empty(nonResultReturnTypes); + }Also add missing import at the top of the file:
using Xunit;src/core/Kathanika.Domain/Primitives/IRepository.cs (1)
20-21: Batch Add should return a materialized, read-only collection.Returning IEnumerable is loose and invites re-enumeration; prefer IReadOnlyList.
Interface change:
- Task<IEnumerable<T>> AddAsync(IEnumerable<T> aggregates, CancellationToken cancellationToken = default); + Task<IReadOnlyList<T>> AddAsync(IEnumerable<T> aggregates, CancellationToken cancellationToken = default);Follow-up: update the Repository implementation to match (return List). If you want stronger input guarantees too, consider
IReadOnlyCollection<T> aggregates.src/infrastructure/Kathanika.Infrastructure.Persistence/Repository.cs (1)
319-361: Optional: improve throughput and cache coherence for batch inserts.
- Use InsertManyOptions { IsOrdered = false } to reduce stall on a single bad doc.
- Populate per-entity cache just like single AddAsync for immediate reads.
Example:
- await _collection.InsertManyAsync(aggregateRoots, cancellationToken: cancellationToken); + var options = new InsertManyOptions { IsOrdered = false }; + await _collection.InsertManyAsync(aggregateRoots, options, cancellationToken); + // Warm cache entries, mirroring single AddAsync behavior. + foreach (var aggregate in aggregateRoots) + { + var cacheKey = $"{typeof(T).Name.ToLower()}-{aggregate.Id}"; + await _hybridCache.SetAsync(cacheKey, aggregate, cancellationToken: cancellationToken); + }src/infrastructure/Kathanika.Infrastructure.Graphql/Schema/BibRecordGraph/BibRecordPayloads.cs (1)
12-12: Grammar: “are creating” → “are being created”.Polish the user-facing success message.
- ? $"New Bib record for book {knResult.Value.Title} created successfully. Bib items are creating." + ? $"New Bib record for book {knResult.Value.Title} created successfully. Bib items are being created."src/core/Kathanika.Domain/Aggregates/BibItemAggregate/BibItemAggregateErrors.cs (1)
19-21: Error code/message mismatch: “BibRecordIdIsEmpty” message says “not found”.This muddies API contracts and makes troubleshooting harder. Keep “IdIsEmpty” for validation and add a separate “NotFound” error.
Option A (revert message):
- "BibItem.BibRecordIdIsEmpty", - "BibRecord not found"); + "BibItem.BibRecordIdIsEmpty", + "BibRecord ID cannot be empty");Option B (add new error alongside the above revert):
public static readonly KnError BibRecordNotFound = new( "BibItem.BibRecordNotFound", "BibRecord not found");Would you like me to sweep usages and replace “IdIsEmpty” where a not-found condition is actually intended?
src/core/Kathanika.Domain/Aggregates/BibRecordAggregate/BookRecordCreatedEvent.cs (1)
5-7: Clarify semantics of NumberOfCopies and add idempotency/telemetry fields.Null/0 both meaning “no copies” can be ambiguous. Either:
- Make it non-nullable int with explicit 0 semantics, or
- Keep nullable but document and enforce allowed values in the raiser/handler.
Also consider including EventId and OccurredOn (if not already on IDomainEvent) to aid outbox/idempotency and observability.
Example (only if IDomainEvent doesn’t already provide these):
- public record BookRecordCreatedEvent( - string BibRecordId, - int? NumberOfCopies) : IDomainEvent; + public record BookRecordCreatedEvent( + string BibRecordId, + int? NumberOfCopies) : IDomainEvent +{ + public Guid EventId { get; init; } = Guid.NewGuid(); + public DateTimeOffset OccurredOn { get; init; } = DateTimeOffset.UtcNow; +}src/core/Kathanika.Application/Features/BibItems/Commands/AddBibItemCommandHandler.cs (1)
22-27: Avoid writing empty/whitespace notes; trim inputs.
Current null-only checks will persist empty/whitespace values. If empty means “no change,” guard withIsNullOrWhiteSpaceand trim. If empty should clear, ignore this suggestion.Apply:
- if (request.ConditionNote is not null) - bibItemResult.Value.UpdateCondition(request.ConditionNote); + if (!string.IsNullOrWhiteSpace(request.ConditionNote)) + bibItemResult.Value.UpdateCondition(request.ConditionNote.Trim()); - if (request.Notes is not null) - bibItemResult.Value.UpdateNotes(request.Notes); + if (!string.IsNullOrWhiteSpace(request.Notes)) + bibItemResult.Value.UpdateNotes(request.Notes.Trim());Confirm intended semantics for empty strings (no-op vs. clear).
tests/Kathanika.Application.Tests/Features/BibItems/Commands/CheckOutBibItemCommandHandlerTests.cs (1)
61-61: LGTM: updated to 5-arg Create.
Optionally assert the default pre-checkout status to lock in the new invariant (e.g.,Assert.Equal(ItemStatus.Available, bibItem.Status);before Act), if that’s the intended default.src/core/Kathanika.Application/Features/BibItems/Commands/UpdateBibItemCommandHandler.cs (1)
27-32: Clarify null vs empty semantics for ConditionNote/NotesCurrent checks treat null as “no change” and empty string as “set to empty.” Confirm this is intended; otherwise consider ignoring whitespace-only values or trimming.
Possible tweak if empty should be ignored:
- if (request.ConditionNote is not null) - bibItem.UpdateCondition(request.ConditionNote); + if (!string.IsNullOrWhiteSpace(request.ConditionNote)) + bibItem.UpdateCondition(request.ConditionNote); - if (request.Notes is not null) - bibItem.UpdateNotes(request.Notes); + if (!string.IsNullOrWhiteSpace(request.Notes)) + bibItem.UpdateNotes(request.Notes);src/core/Kathanika.Domain/Aggregates/BibRecordAggregate/BibRecord.cs (2)
46-56: Avoid emitting FileUsedDomainEvent when the cover image is unchangedPrevents duplicate/no-op events on idempotent updates. Optionally, consider emitting a “FileUnused” event for the previous image if you support cleanup.
CoverImageId = coverImageId; - AddDomainEvent(new FileUsedDomainEvent(coverImageId)); + if (CoverImageId == coverImageId) + return KnResult.Success(); + CoverImageId = coverImageId; + AddDomainEvent(new FileUsedDomainEvent(coverImageId));
112-113: Gate BookRecordCreatedEvent on numberOfCopies > 0Avoids publishing a no-op event when there’s nothing to create.
- record.AddDomainEvent(new BookRecordCreatedEvent(record.Id, numberOfCopies)); + if (numberOfCopies > 0) + record.AddDomainEvent(new BookRecordCreatedEvent(record.Id, numberOfCopies));src/core/Kathanika.Application/Features/QuickAdd/EventHandlers/BookRecordCreatedEventHandler.cs (5)
13-13: Guard should not check null for a non-nullable intIf NumberOfCopies is an int, the null pattern is dead code; prefer a clear non-positive check.
- if (notification.NumberOfCopies is null or 0) return; + if (notification.NumberOfCopies <= 0) return;
15-18: Preallocate and avoid repeated property accessSmall perf/readability win: cache requested and pre-size the list.
- List<KnError> errors = []; - List<BibItem> items = []; - for (var i = 1; i <= notification.NumberOfCopies; i++) + var requested = notification.NumberOfCopies; + List<KnError> errors = []; + List<BibItem> items = new(requested); + for (var i = 1; i <= requested; i++)
29-36: Capture which copy failed for better diagnosticsAccumulate (copyIndex, errors) for precise logging/telemetry; today only raw errors are kept. Also consider throwing on totally empty success to surface failures upstream.
38-42: Use structured logging and include countsLog requested vs created and BibRecordId; downgrade to Warning for partial failures, keep Error for complete failure.
- if (errors.Count != 0 || items.Count < notification.NumberOfCopies) + if (errors.Count > 0 || items.Count < requested) { - //TODO: Find way to feedback to user - logger.LogError("Failed to create book items"); + //TODO: Find way to feedback to user + var created = items.Count; + if (created == 0) + logger.LogError("Failed to create any book items. BibRecordId={BibRecordId}, Requested={Requested}, Errors={ErrorCount}", + notification.BibRecordId, requested, errors.Count); + else + logger.LogWarning("Created fewer book items than requested. BibRecordId={BibRecordId}, Requested={Requested}, Created={Created}, Errors={ErrorCount}", + notification.BibRecordId, requested, created, errors.Count); }
44-45: No-op add when nothing to persistAvoid calling AddAsync with an empty collection; some repos throw on empty.
- await bibItemRepository.AddAsync(items, cancellationToken); + if (items.Count > 0) + await bibItemRepository.AddAsync(items, cancellationToken);tests/Kathanika.Domain.Tests/Aggregates/BibItemAggregate/BibItemTests.cs (1)
219-227: Helper should fail fast with a clearer message on Create() failureAccessing .Value assumes success. Throw with context if creation fails to avoid null/ambiguous assertion failures later.
- private static BibItem CreateTestBibItem() - { - return BibItem.Create( - "bib-123", - "123456789", - "QA76.73.C153", - "Main Library", - ItemType.Book).Value; - } + private static BibItem CreateTestBibItem() + { + var result = BibItem.Create( + "bib-123", + "123456789", + "QA76.73.C153", + "Main Library", + ItemType.Book); + if (result.IsFailure) + throw new InvalidOperationException($"Test setup failed: {string.Join(", ", result.Errors)}"); + return result.Value; + }src/core/Kathanika.Application/Features/QuickAdd/Commands/BookQuickAddCommandHandler.cs (1)
25-44: Repeated update pattern is fine; consider a tiny helper to DRY if this growsLow priority. Keeps Handle() lean as fields evolve.
src/core/Kathanika.Domain/Aggregates/BibItemAggregate/BibItem.cs (5)
13-15: Use UTC for AcquisitionDate to align with other timestamps.CheckOut/CheckIn/Withdraw use UtcNow; AcquisitionDate uses Now, which can cause inconsistency in multi-timezone deployments.
- public DateOnly? AcquisitionDate { get; private set; } = DateOnly.FromDateTime(DateTime.Now); + public DateOnly? AcquisitionDate { get; private set; } = DateOnly.FromDateTime(DateTime.UtcNow);If local-branch time is desired, consider injecting a clock/time-zone service instead.
55-62: Normalize inputs: trim strings on create/update to avoid hidden whitespace.Prevents accidental leading/trailing spaces in persisted identifiers.
BibItem newBibItem = new() { - BibRecordId = bibRecordId, - Barcode = barcode, - CallNumber = callNumber, - Location = location, + BibRecordId = bibRecordId.Trim(), + Barcode = barcode.Trim(), + CallNumber = callNumber.Trim(), + Location = location.Trim(), ItemType = itemType }; +// Before assignments in Update() +if (barcode is not null) barcode = barcode.Trim(); +if (callNumber is not null) callNumber = callNumber.Trim(); +if (location is not null) location = location.Trim(); Barcode = barcode ?? Barcode; CallNumber = callNumber ?? CallNumber; Location = location ?? Location; ItemType = itemType ?? ItemType;Also applies to: 87-90
95-104: UpdateCondition/UpdateNotes: add minimal validation/normalization (and optionally return KnResult for consistency).Currently void and accept any string; trimming and treating whitespace-only as null improves data quality and aligns with other guards.
-public void UpdateCondition(string conditionNote) -{ - ConditionNote = conditionNote; -} +public void UpdateCondition(string conditionNote) +{ + ConditionNote = string.IsNullOrWhiteSpace(conditionNote) ? null : conditionNote.Trim(); +} -public void UpdateNotes(string notes) -{ - Notes = notes; -} +public void UpdateNotes(string notes) +{ + Notes = string.IsNullOrWhiteSpace(notes) ? null : notes.Trim(); +}If you prefer uniform API semantics, switch returns to KnResult and surface validation errors similarly to Update().
141-144: Use Environment.NewLine instead of a hardcoded "\n".Ensures cross-platform line endings in appended notes.
- Notes = string.IsNullOrEmpty(Notes) ? $"Withdrawn: {reason}" : $"{Notes}\nWithdrawn: {reason}"; + Notes = string.IsNullOrEmpty(Notes) + ? $"Withdrawn: {reason}" + : $"{Notes}{Environment.NewLine}Withdrawn: {reason}";
22-28: Remove CS9264 from the pragma disable.CS9264 is a real C# 13 warning for field-backed properties (the
fieldkeyword), which this class doesn’t use—disabling it has no effect and only adds noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
src/core/Kathanika.Application/Features/BibItems/Commands/AddBibItemCommand.cs(0 hunks)src/core/Kathanika.Application/Features/BibItems/Commands/AddBibItemCommandHandler.cs(1 hunks)src/core/Kathanika.Application/Features/BibItems/Commands/AddBibItemCommandValidator.cs(0 hunks)src/core/Kathanika.Application/Features/BibItems/Commands/UpdateBibItemCommandHandler.cs(1 hunks)src/core/Kathanika.Application/Features/QuickAdd/Commands/BookQuickAddCommandHandler.cs(2 hunks)src/core/Kathanika.Application/Features/QuickAdd/EventHandlers/BookRecordCreatedEventHandler.cs(1 hunks)src/core/Kathanika.Domain/Aggregates/BibItemAggregate/BibItem.cs(5 hunks)src/core/Kathanika.Domain/Aggregates/BibItemAggregate/BibItemAggregateErrors.cs(1 hunks)src/core/Kathanika.Domain/Aggregates/BibRecordAggregate/BibRecord.cs(4 hunks)src/core/Kathanika.Domain/Aggregates/BibRecordAggregate/BookRecordCreatedEvent.cs(1 hunks)src/core/Kathanika.Domain/Primitives/IRepository.cs(1 hunks)src/infrastructure/Kathanika.Infrastructure.Graphql/Schema/BibRecordGraph/BibRecordPayloads.cs(1 hunks)src/infrastructure/Kathanika.Infrastructure.Persistence/Repository.cs(1 hunks)tests/Kathanika.Application.Tests/Features/BibItems/Commands/AddBibItemCommandHandlerTests.cs(0 hunks)tests/Kathanika.Application.Tests/Features/BibItems/Commands/CheckInBibItemCommandHandlerTests.cs(1 hunks)tests/Kathanika.Application.Tests/Features/BibItems/Commands/CheckOutBibItemCommandHandlerTests.cs(1 hunks)tests/Kathanika.Application.Tests/Features/BibItems/Commands/UpdateBibItemCommandHandlerTests.cs(1 hunks)tests/Kathanika.Application.Tests/Features/BibItems/Commands/WithdrawBibItemCommandHandlerTests.cs(1 hunks)tests/Kathanika.Application.Tests/Features/BibItems/Queries/GetBibItemByIdQueryHandlerTests.cs(1 hunks)tests/Kathanika.Application.Tests/Features/BibItems/Queries/GetBibItemsQueryHandlerTests.cs(1 hunks)tests/Kathanika.Domain.Tests/Aggregates/BibItemAggregate/BibItemTests.cs(3 hunks)tests/Kathanika.Domain.Tests/Aggregates/BibRecordAggregate/BibRecordTests.cs(2 hunks)tests/Kathanika.Domain.Tests/ArchTests/ArchTests.cs(1 hunks)
💤 Files with no reviewable changes (3)
- src/core/Kathanika.Application/Features/BibItems/Commands/AddBibItemCommand.cs
- src/core/Kathanika.Application/Features/BibItems/Commands/AddBibItemCommandValidator.cs
- tests/Kathanika.Application.Tests/Features/BibItems/Commands/AddBibItemCommandHandlerTests.cs
🧰 Additional context used
🧬 Code graph analysis (6)
src/core/Kathanika.Application/Features/BibItems/Commands/UpdateBibItemCommandHandler.cs (1)
src/core/Kathanika.Domain/Aggregates/BibItemAggregate/BibItem.cs (9)
KnResult(34-65)KnResult(67-93)KnResult(105-116)KnResult(118-129)KnResult(131-147)BibItem(5-148)BibItem(24-26)UpdateCondition(95-98)UpdateNotes(100-103)
src/core/Kathanika.Domain/Aggregates/BibRecordAggregate/BibRecord.cs (1)
src/core/Kathanika.Domain/Primitives/AggregateRoot.cs (1)
AddDomainEvent(20-23)
src/infrastructure/Kathanika.Infrastructure.Persistence/Repository.cs (2)
src/core/Kathanika.Domain/Primitives/IRepository.cs (11)
Task(8-8)Task(9-9)Task(10-10)Task(11-11)Task(13-14)Task(16-16)Task(17-17)Task(19-19)Task(20-20)Task(22-22)Task(23-23)src/infrastructure/Kathanika.Infrastructure.Persistence/Outbox/OutboxMessage.cs (1)
OutboxMessage(5-13)
src/core/Kathanika.Application/Features/QuickAdd/EventHandlers/BookRecordCreatedEventHandler.cs (4)
src/core/Kathanika.Application/Features/BibItems/Commands/AddBibItemCommandHandler.cs (1)
Task(8-31)src/core/Kathanika.Domain/Primitives/IRepository.cs (11)
Task(8-8)Task(9-9)Task(10-10)Task(11-11)Task(13-14)Task(16-16)Task(17-17)Task(19-19)Task(20-20)Task(22-22)Task(23-23)src/core/Kathanika.Domain/Aggregates/BibItemAggregate/BibItem.cs (7)
BibItem(5-148)BibItem(24-26)KnResult(34-65)KnResult(67-93)KnResult(105-116)KnResult(118-129)KnResult(131-147)src/core/Kathanika.Domain/Aggregates/BibRecordAggregate/BibRecord.cs (4)
KnResult(46-56)KnResult(58-68)KnResult(70-80)KnResult(86-115)
src/core/Kathanika.Application/Features/BibItems/Commands/AddBibItemCommandHandler.cs (1)
src/core/Kathanika.Domain/Aggregates/BibItemAggregate/BibItem.cs (2)
UpdateCondition(95-98)UpdateNotes(100-103)
src/core/Kathanika.Domain/Aggregates/BibItemAggregate/BibItem.cs (2)
src/app-projects/ils-web/src/app/graphql/generated/graphql-operations.ts (3)
ItemVendor(307-311)Vendor(767-780)BibItem(75-92)tests/Kathanika.Domain.Tests/Aggregates/BibItemAggregate/BibItemTests.cs (1)
BibItem(219-227)
🔇 Additional comments (16)
tests/Kathanika.Domain.Tests/Aggregates/BibRecordAggregate/BibRecordTests.cs (2)
202-202: Good change: prefer const for immutable test data.Using const here prevents accidental reassignment and communicates intent.
259-259: Consistent immutability in tests.Declaring coverImageId as const improves clarity and safety.
tests/Kathanika.Application.Tests/Features/BibItems/Commands/WithdrawBibItemCommandHandlerTests.cs (1)
81-81: API update reflected correctly.Removing the status parameter from BibItem.Create and passing ItemType as the last arg matches the new signature.
src/core/Kathanika.Application/Features/BibItems/Commands/AddBibItemCommandHandler.cs (1)
15-15: No BibItem.Create callers pass ItemStatus; verification completetests/Kathanika.Application.Tests/Features/BibItems/Commands/UpdateBibItemCommandHandlerTests.cs (1)
180-180: LGTM: helper aligned with new Create signature.
No issues spotted.tests/Kathanika.Application.Tests/Features/BibItems/Commands/CheckInBibItemCommandHandlerTests.cs (1)
75-81: LGTM: helper updated to new Create() signatureThe test helper correctly switches to the 5-arg BibItem.Create; expectations remain valid.
src/core/Kathanika.Application/Features/BibItems/Commands/UpdateBibItemCommandHandler.cs (1)
16-21: LGTM: split-core update aligns with domain APIUsing BibItem.Update for core fields only matches the refactor.
tests/Kathanika.Application.Tests/Features/BibItems/Queries/GetBibItemsQueryHandlerTests.cs (1)
170-178: LGTM: test helper updated to 5-arg Create()Consistent with domain changes; assertions still verify filtering correctly.
tests/Kathanika.Application.Tests/Features/BibItems/Queries/GetBibItemByIdQueryHandlerTests.cs (1)
75-81: LGTM: helper updated to new Create() signatureMatches domain API; test intent unchanged.
src/core/Kathanika.Domain/Aggregates/BibRecordAggregate/BibRecord.cs (1)
1-1: LGTM: domain events importRequired for new events usage.
src/core/Kathanika.Application/Features/QuickAdd/EventHandlers/BookRecordCreatedEventHandler.cs (1)
19-27: I've initiated a search for theBibItemdefinition, theBarcodefield, and any unique-index declarations in the EF model or migrations. Let me know once you have the results so we can confirm whether a unique constraint exists and if the idempotency concerns hold.tests/Kathanika.Domain.Tests/Aggregates/BibItemAggregate/BibItemTests.cs (2)
21-27: Create() call aligned with new 5-arg signature — looks good
43-49: Create() call update — looks correctsrc/core/Kathanika.Application/Features/QuickAdd/Commands/BookQuickAddCommandHandler.cs (2)
11-20: Signature update to CreateBookRecord with NumberOfCopies — LGTM
46-49: Verify outbox dispatch job publishes BookRecordCreatedEvent
AddAsync enqueues domain events to the outbox, but IPublisher.Publish is invoked only in ProcessOutboxMessagesJob (configured in Infrastructure/Workers/DependencyInjector.cs:15–19); ensure the job runs as expected or call mediator.Publish after AddAsync for immediate handler execution.src/core/Kathanika.Domain/Aggregates/BibItemAggregate/BibItem.cs (1)
34-41: No action required: Id is auto-generated inEntity. TheEntitybase class definespublic string Id { get; private set; } = Ulid.NewUlid().ToString();so
BibItem.Create()instances already receive a unique Id.Likely an incorrect or invalid review comment.
| public record BookRecordCreatedEvent( | ||
| string BibRecordId, | ||
| int? NumberOfCopies) : IDomainEvent; No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Naming and typing consistency: align event with aggregate and prefer strong IDs.
Consider renaming to BibRecordCreatedEvent and using a strong ID (e.g., BibRecordId) instead of string for safer refactoring and compile-time guarantees.
Proposed change:
- public record BookRecordCreatedEvent(
- string BibRecordId,
- int? NumberOfCopies) : IDomainEvent;
+ public record BibRecordCreatedEvent(
+ BibRecordId BibRecordId,
+ int? NumberOfCopies) : IDomainEvent;Verification script to find references to update:
🏁 Script executed:
#!/bin/bash
# Find usages of the old/new names and string BibRecordId
rg -nP -C2 '\bBookRecordCreatedEvent\b|BibRecordCreatedEvent\b|\bstring\s+BibRecordId\b'Length of output: 3762
Introduce BibRecordId value object and align event naming
- Rename
BookRecordCreatedEvent→BibRecordCreatedEventand change its first parameter fromstring BibRecordIdto the newBibRecordIdtype in src/core/Kathanika.Domain/Aggregates/BibRecordAggregate/BookRecordCreatedEvent.cs - Update all references: the
new BookRecordCreatedEvent(…)call in BibRecord.cs, the handler interface and signature in BookRecordCreatedEventHandler.cs, plus usages in AddBibItemCommand.cs and GetBibItemsQuery.cs - Define a
BibRecordIdrecord/class in the Domain layer and replace allstring BibRecordIddeclarations accordingly
🤖 Prompt for AI Agents
In
src/core/Kathanika.Domain/Aggregates/BibRecordAggregate/BookRecordCreatedEvent.cs
lines 5-7, rename the record to BibRecordCreatedEvent and change the first
parameter type from string BibRecordId to the new BibRecordId value object; then
add a new BibRecordId record/class in the Domain layer (namespace matching
Domain/Aggregates) to encapsulate the identifier. Update all references: replace
new BookRecordCreatedEvent(...) with new BibRecordCreatedEvent(...) in
BibRecord.cs, rename and adjust the handler interface and method signature in
BookRecordCreatedEventHandler.cs to accept BibRecordCreatedEvent and use
BibRecordId type, and update usages in AddBibItemCommand.cs and
GetBibItemsQuery.cs to use BibRecordCreatedEvent/BibRecordId and replace all
string BibRecordId declarations with the BibRecordId type. Ensure
namespaces/imports are updated and run build to fix any remaining compile
errors.
| /// <summary> | ||
| /// Adds a collection of aggregates to the underlying MongoDB collection with auditing properties applied. | ||
| /// </summary> | ||
| /// <param name="aggregates"> | ||
| /// The collection of aggregates of type <typeparamref name="T"/> to be added to the MongoDB collection. | ||
| /// The type <typeparamref name="T"/> must derive from <see cref="AggregateRoot"/>. | ||
| /// </param> | ||
| /// <param name="cancellationToken"> | ||
| /// A <see cref="CancellationToken"/> that can be used to cancel the operation. | ||
| /// </param> | ||
| /// <returns> | ||
| /// A collection of the added aggregates of type <typeparamref name="T"/> after completion of the operation. | ||
| /// </returns> | ||
| public async Task<IEnumerable<T>> AddAsync(IEnumerable<T> aggregates, CancellationToken cancellationToken = default) | ||
| { | ||
| IEnumerable<T> aggregateRoots = aggregates as T[] ?? aggregates.ToArray(); | ||
|
|
||
| foreach (T aggregate in aggregateRoots) | ||
| { | ||
| SetCreationAuditProperties(aggregate); | ||
| } | ||
|
|
||
| _logger.LogInformation( | ||
| "Adding new documents {@Documents} of type {@DocumentType} into collection {@CollectionName}", aggregates, | ||
| typeof(T).Name, _collectionName); | ||
|
|
||
| await _collection.InsertManyAsync(aggregateRoots.ToList(), cancellationToken: cancellationToken); | ||
| _logger.LogInformation( | ||
| "Added new documents with _ids {@_ids} of type {@DocumentType} into collection {@CollectionName}", | ||
| aggregateRoots.ToBsonDocument()["_id"].ToJson(), typeof(T).Name, _collectionName); | ||
|
|
||
| List<OutboxMessage> outboxMessages = []; | ||
| foreach (T aggregate in aggregateRoots) | ||
| { | ||
| outboxMessages.AddRange(GetOutboxMessagesFromAggregate(aggregate)); | ||
| aggregate.ClearDomainEvents(); | ||
| } | ||
|
|
||
| if (outboxMessages.Count > 0) | ||
| await _outboxMessageCollection.InsertManyAsync(outboxMessages, cancellationToken: cancellationToken); | ||
|
|
||
| return aggregateRoots; | ||
| } |
There was a problem hiding this comment.
Fix runtime bugs in batch Add: empty input handling and invalid _ids logging.
- InsertManyAsync throws on empty collections; guard early.
- Logging
_idsviaaggregateRoots.ToBsonDocument()["_id"]is invalid and can throw; log the projected Ids instead. - Minor: log the already materialized
aggregateRoots, not the originalaggregates.
Apply:
- public async Task<IEnumerable<T>> AddAsync(IEnumerable<T> aggregates, CancellationToken cancellationToken = default)
+ public async Task<IEnumerable<T>> AddAsync(IEnumerable<T> aggregates, CancellationToken cancellationToken = default)
{
- IEnumerable<T> aggregateRoots = aggregates as T[] ?? aggregates.ToArray();
+ if (aggregates is null) throw new ArgumentNullException(nameof(aggregates));
+ var aggregateRoots = (aggregates as T[] ?? aggregates.ToArray());
+ if (aggregateRoots.Length == 0)
+ return aggregateRoots;
foreach (T aggregate in aggregateRoots)
{
SetCreationAuditProperties(aggregate);
}
_logger.LogInformation(
- "Adding new documents {@Documents} of type {@DocumentType} into collection {@CollectionName}", aggregates,
+ "Adding new documents {@Documents} of type {@DocumentType} into collection {@CollectionName}", aggregateRoots,
typeof(T).Name, _collectionName);
- await _collection.InsertManyAsync(aggregateRoots.ToList(), cancellationToken: cancellationToken);
+ await _collection.InsertManyAsync(aggregateRoots, cancellationToken: cancellationToken);
_logger.LogInformation(
- "Added new documents with _ids {@_ids} of type {@DocumentType} into collection {@CollectionName}",
- aggregateRoots.ToBsonDocument()["_id"].ToJson(), typeof(T).Name, _collectionName);
+ "Added new documents with ids {@Ids} of type {@DocumentType} into collection {@CollectionName}",
+ aggregateRoots.Select(a => a.Id), typeof(T).Name, _collectionName);
List<OutboxMessage> outboxMessages = [];
foreach (T aggregate in aggregateRoots)
{
outboxMessages.AddRange(GetOutboxMessagesFromAggregate(aggregate));
aggregate.ClearDomainEvents();
}
if (outboxMessages.Count > 0)
await _outboxMessageCollection.InsertManyAsync(outboxMessages, cancellationToken: cancellationToken);
return aggregateRoots;
}🤖 Prompt for AI Agents
In src/infrastructure/Kathanika.Infrastructure.Persistence/Repository.cs around
lines 319-361, the batch AddAsync has several runtime issues: it calls
InsertManyAsync on an empty collection (causing an exception), logs the original
IEnumerable instead of the materialized list, attempts to access _id via
aggregateRoots.ToBsonDocument() (invalid and throwable), and initializes
outboxMessages incorrectly. Fix by materializing aggregates into a List<T>,
immediately return the empty list if none (guard early), use the materialized
list for logging, project and log the aggregate Ids (e.g.,
aggregateRoots.Select(a => a.Id)) instead of calling ToBsonDocument(),
initialize outboxMessages as new List<OutboxMessage>(), and only call
InsertManyAsync/InsertManyAsync on outbox when the corresponding lists are
non-empty (preserve cancellationToken).
- Added validation for the `location` field in `BibItem` to ensure completeness. - Changed `AddAsync` in `IRepository` and its implementation to return `IReadOnlyList<T>` for better immutability guarantees. - Enhanced `AddAsync` in `Repository` to support unordered inserts with `InsertManyOptions`. - Cleaned up `BibRecordAggregateErrors` by removing unused `ControlField` error definitions for consistency. - Updated GraphQL payload message for `BookQuickAdd` to improve clarity.
- Introduced `EmptyTag`, `InvalidTag`, and `EmptyData` error definitions in `ControlField`. - Enhanced error messages for improved clarity during validation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/infrastructure/Kathanika.Infrastructure.Persistence/Repository.cs (1)
333-369: Batch AddAsync: fix null/empty handling, logging of ids, and avoid extra ToList().
- Guard against null and empty to prevent InsertManyAsync throwing.
- Log the materialized batch, not the original enumerable.
- Project Ids instead of using ToBsonDocument() on the collection (invalid).
- Pass the already materialized collection to InsertManyAsync (no extra ToList()).
- Optional: decide explicitly on IsOrdered for the main insert (ordered vs partial inserts).
- Optional: set cache entries for inserted items for parity with single AddAsync.
Apply:
- public async Task<IReadOnlyList<T>> AddAsync(IEnumerable<T> aggregates, + public async Task<IReadOnlyList<T>> AddAsync(IEnumerable<T> aggregates, CancellationToken cancellationToken = default) { - IReadOnlyList<T> aggregateRoots = aggregates as T[] ?? aggregates.ToArray(); + if (aggregates is null) throw new ArgumentNullException(nameof(aggregates)); + var aggregateRoots = (aggregates as T[] ?? aggregates.ToArray()); + if (aggregateRoots.Length == 0) + return aggregateRoots; foreach (T aggregate in aggregateRoots) { SetCreationAuditProperties(aggregate); } _logger.LogInformation( - "Adding new documents {@Documents} of type {@DocumentType} into collection {@CollectionName}", aggregates, + "Adding new documents {@Documents} of type {@DocumentType} into collection {@CollectionName}", aggregateRoots, typeof(T).Name, _collectionName); - await _collection.InsertManyAsync(aggregateRoots.ToList(), cancellationToken: cancellationToken); + await _collection.InsertManyAsync( + aggregateRoots, + new InsertManyOptions { IsOrdered = true }, // consider false if you want best-effort inserts + cancellationToken: cancellationToken); _logger.LogInformation( - "Added new documents with _ids {@_ids} of type {@DocumentType} into collection {@CollectionName}", - aggregateRoots.ToBsonDocument()["_id"].ToJson(), typeof(T).Name, _collectionName); + "Added new documents with ids {@Ids} of type {@DocumentType} into collection {@CollectionName}", + aggregateRoots.Select(a => a.Id), typeof(T).Name, _collectionName); - List<OutboxMessage> outboxMessages = []; + List<OutboxMessage> outboxMessages = []; foreach (T aggregate in aggregateRoots) { outboxMessages.AddRange(GetOutboxMessagesFromAggregate(aggregate)); aggregate.ClearDomainEvents(); } if (outboxMessages.Count > 0) await _outboxMessageCollection.InsertManyAsync( outboxMessages, new InsertManyOptions() { IsOrdered = false }, cancellationToken: cancellationToken); return aggregateRoots; }If cache parity with single AddAsync is desired, add:
+ foreach (var aggregate in aggregateRoots) + { + var cacheKey = $"{typeof(T).Name.ToLower()}-{aggregate.Id}"; + await _hybridCache.SetAsync(cacheKey, aggregate, cancellationToken: cancellationToken); + }
🧹 Nitpick comments (2)
src/core/Kathanika.Domain/Aggregates/BibItemAggregate/BibItem.cs (2)
12-14: Defaulting AcquisitionDate via DateTime.Now: prefer UTC or set at Create().Using local time can cause flaky tests around DST/midnight. Consider UTC or deferring to Create().
- public DateOnly? AcquisitionDate { get; private set; } = DateOnly.FromDateTime(DateTime.Now); + public DateOnly? AcquisitionDate { get; private set; } = DateOnly.FromDateTime(DateTime.UtcNow);Or drop the initializer and set it explicitly in Create() if needed.
98-107: Harden UpdateCondition/UpdateNotes: trim input and allow clearing.Current setters persist whitespace-only strings and disallow clearing. Accept null/whitespace to clear and trim non-empty text for consistency.
- public void UpdateCondition(string conditionNote) + public void UpdateCondition(string? conditionNote) { - ConditionNote = conditionNote; + ConditionNote = string.IsNullOrWhiteSpace(conditionNote) ? null : conditionNote.Trim(); } - public void UpdateNotes(string notes) + public void UpdateNotes(string? notes) { - Notes = notes; + Notes = string.IsNullOrWhiteSpace(notes) ? null : notes.Trim(); }Add/adjust tests to cover:
- null/whitespace clears the fields
- non-empty values are trimmed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/Kathanika.Domain/Aggregates/BibItemAggregate/BibItem.cs(5 hunks)src/core/Kathanika.Domain/Primitives/IRepository.cs(1 hunks)src/infrastructure/Kathanika.Infrastructure.Graphql/Schema/BibRecordGraph/BibRecordPayloads.cs(1 hunks)src/infrastructure/Kathanika.Infrastructure.Persistence/Repository.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/infrastructure/Kathanika.Infrastructure.Graphql/Schema/BibRecordGraph/BibRecordPayloads.cs
🧰 Additional context used
🧬 Code graph analysis (2)
src/infrastructure/Kathanika.Infrastructure.Persistence/Repository.cs (2)
src/core/Kathanika.Domain/Primitives/IRepository.cs (11)
Task(8-8)Task(9-9)Task(10-10)Task(11-11)Task(13-14)Task(16-16)Task(17-17)Task(19-19)Task(20-20)Task(22-22)Task(23-23)src/infrastructure/Kathanika.Infrastructure.Persistence/Outbox/OutboxMessage.cs (1)
OutboxMessage(5-13)
src/core/Kathanika.Domain/Aggregates/BibItemAggregate/BibItem.cs (7)
tests/Kathanika.Application.Tests/Features/BibItems/Commands/CheckInBibItemCommandHandlerTests.cs (1)
BibItem(73-81)tests/Kathanika.Application.Tests/Features/BibItems/Commands/CheckOutBibItemCommandHandlerTests.cs (1)
BibItem(54-62)tests/Kathanika.Application.Tests/Features/BibItems/Commands/WithdrawBibItemCommandHandlerTests.cs (1)
BibItem(74-82)tests/Kathanika.Application.Tests/Features/BibItems/Queries/GetBibItemByIdQueryHandlerTests.cs (1)
BibItem(73-81)tests/Kathanika.Application.Tests/Features/BibItems/Commands/UpdateBibItemCommandHandlerTests.cs (1)
BibItem(173-181)tests/Kathanika.Application.Tests/Features/BibItems/Queries/GetBibItemsQueryHandlerTests.cs (1)
BibItem(170-178)tests/Kathanika.Domain.Tests/Aggregates/BibItemAggregate/BibItemTests.cs (1)
BibItem(219-227)
🔇 Additional comments (3)
src/infrastructure/Kathanika.Infrastructure.Persistence/Repository.cs (1)
32-33: Constructor change LGTM.HybridCache injection looks consistent with the rest of the class.
src/core/Kathanika.Domain/Aggregates/BibItemAggregate/BibItem.cs (2)
58-65: Create() object initializer LGTM.Initialization is clear and validations above (including Location) cover required fields.
74-74: Update() API LGTM.Nullable optional parameters align with partial updates; assignment is idempotent.
| Task<IReadOnlyList<T>> AddAsync(IEnumerable<T> aggregates, CancellationToken cancellationToken = default); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Name clash risk with overloaded AddAsync; consider AddRangeAsync.
Overloading AddAsync with IEnumerable can make calls with a null literal ambiguous and reduces API clarity. Consider AddRangeAsync (or AddManyAsync) for the batch variant. If you keep the overload, document the null prohibition and ensure implementations throw ArgumentNullException.
Run to find external IRepository implementers that must add the new method and to spot any AddAsync(null) call sites:
🏁 Script executed:
#!/bin/bash
# Find IRepository implementers
rg -nP 'class\s+\w+\s*:\s*[^{}]*IRepository<[^>]+>' -C2
# Find AddAsync(null) call sites that may become ambiguous
rg -nP '\bAddAsync\s*\(\s*null\s*\)'Length of output: 0
🏁 Script executed:
#!/bin/bash
echo "PWD: $(pwd)"
echo "Top-level files:"
ls -la | sed -n '1,200p'
echo
echo "Check for interface file at expected path:"
if [ -f src/core/Kathanika.Domain/Primitives/IRepository.cs ]; then
echo "=== src/core/Kathanika.Domain/Primitives/IRepository.cs ==="
sed -n '1,240p' src/core/Kathanika.Domain/Primitives/IRepository.cs
else
echo "IRepository.cs not found at src/core/Kathanika.Domain/Primitives/IRepository.cs"
fi
echo
echo "Searching repository for occurrences of 'IRepository<':"
rg -n --hidden -S 'IRepository<' -g '!**/node_modules/**' -C2 || true
echo
echo "Searching for class/struct implementations that mention IRepository:"
rg -n --hidden -S ':\s*[^;{]*IRepository<' -g '!**/node_modules/**' -C3 || true
echo
echo "Searching for literal AddAsync(null) call sites:"
rg -n --hidden -S '\bAddAsync\s*\(\s*null\s*\)' -g '!**/node_modules/**' -C3 || true
echo
echo "Listing all AddAsync usages (to inspect overload patterns):"
rg -n --hidden -S '\bAddAsync\s*\(' -g '!**/node_modules/**' -C3 || trueLength of output: 5149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository root: $(pwd)"
echo
echo "=== IRepository.cs (first 240 lines) ==="
if [ -f src/core/Kathanika.Domain/Primitives/IRepository.cs ]; then
sed -n '1,240p' src/core/Kathanika.Domain/Primitives/IRepository.cs || true
else
echo "IRepository.cs not found at src/core/Kathanika.Domain/Primitives/IRepository.cs"
fi
echo
echo "=== Searching for 'IRepository<' (include ignored files) ==="
rg -n --hidden -uu 'IRepository<' -S -C3 || true
echo
echo "=== Searching for classes implementing IRepository<T> ==="
rg -n --hidden -uu -P 'class\s+\w+\s*:\s*[^;{]*IRepository<[^>]+>' -C3 || true
echo
echo "=== Searching for AddAsync(null) literal call sites ==="
rg -n --hidden -uu -P '\bAddAsync\s*\(\s*null\s*\)' -C3 || true
echo
echo "=== Searching for all AddAsync(...) usages ==="
rg -n --hidden -uu -P '\bAddAsync\s*\(' -C3 || true
echo
echo "=== Searching for AddRangeAsync usages ==="
rg -n --hidden -uu -P '\bAddRangeAsync\s*\(' -C3 || trueLength of output: 16610
Rename batch overload or add explicit null-check + docs.
IRepository defines both AddAsync(T) and AddAsync(IEnumerable) and Repository implements both; no literal AddAsync(null) calls found. Repository.AddAsync(IEnumerable) calls aggregates.ToArray() and will throw NullReferenceException if passed null.
- Fix (required): add an explicit null guard in src/infrastructure/Kathanika.Infrastructure.Persistence/Repository.cs AddAsync(IEnumerable) (≈ line 333): throw new ArgumentNullException(nameof(aggregates)) before calling ToArray().
- Optional refactor: rename the interface batch method in src/core/Kathanika.Domain/Primitives/IRepository.cs to AddRangeAsync and update callers (e.g., src/core/Kathanika.Application/Features/QuickAdd/EventHandlers/BookRecordCreatedEventHandler.cs:44 and other usages).
🤖 Prompt for AI Agents
src/core/Kathanika.Domain/Primitives/IRepository.cs (lines 20-21) and
src/infrastructure/Kathanika.Infrastructure.Persistence/Repository.cs (around
line 333): the batch overload AddAsync(IEnumerable<T>) can throw a
NullReferenceException because Repository.AddAsync calls aggregates.ToArray()
without validating aggregates; add an explicit null guard at the start of
Repository.AddAsync(IEnumerable<T>)—throw new
ArgumentNullException(nameof(aggregates))—before any use of aggregates;
optionally consider renaming the interface batch method to AddRangeAsync and
update all callers if you want clearer semantics.
- Introduced `BookRecordCreatedEventHandlerTests` to verify the functionality of automatic BibItem creation upon BookRecord creation. - Added test cases for scenarios including zero/null copies, valid copies with barcode/call number generation, and failure logging. - Cleaned up unused imports in `ArchTests.cs` for consistency.
…handling-items-on-quick-add # Conflicts: # package-lock.json
…ests - Removed test cases for valid copies, barcode, call number generation, and failure logging as they are no longer relevant. - Simplified the remaining test for zero/null copies to focus on core functionality.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
tests/Kathanika.Application.Tests/Features/QuickAdd/EventHandlers/BookRecordCreatedEventHandlerTests.cs(1 hunks)tests/Kathanika.Domain.Tests/ArchTests/ArchTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Kathanika.Domain.Tests/ArchTests/ArchTests.cs
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Kathanika.Application.Tests/Features/QuickAdd/EventHandlers/BookRecordCreatedEventHandlerTests.cs (1)
src/core/Kathanika.Application/Features/QuickAdd/EventHandlers/BookRecordCreatedEventHandler.cs (1)
BookRecordCreatedEventHandler(6-46)
| public class BookRecordCreatedEventHandlerTests | ||
| { | ||
| private readonly IBibItemRepository _bibItemRepository; | ||
| private readonly ILogger<BookRecordCreatedEventHandler> _logger; | ||
|
|
||
| public BookRecordCreatedEventHandlerTests() | ||
| { | ||
| _bibItemRepository = Substitute.For<IBibItemRepository>(); | ||
| _logger = new NullLogger<BookRecordCreatedEventHandler>(); | ||
| new BookRecordCreatedEventHandler(_bibItemRepository, _logger); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Add test methods to validate the event handler behavior.
The test class has setup infrastructure but no actual test methods. Key scenarios that should be tested based on the handler implementation include:
- Handling null or zero
NumberOfCopies(should return early) - Creating multiple
BibIteminstances whenNumberOfCopiesis valid - Verifying
bibItemRepository.AddAsyncis called with the correct items - Handling failures from
BibItem.Create - Verifying error logging when item creation fails
Would you like me to generate test methods covering these scenarios?
🤖 Prompt for AI Agents
In
tests/Kathanika.Application.Tests/Features/QuickAdd/EventHandlers/BookRecordCreatedEventHandlerTests.cs
around lines 8-19, the test class only builds the handler but contains no tests;
add xUnit test methods to cover: (1) NumberOfCopies null/zero: create a
BookRecordCreated event with NumberOfCopies = 0, call HandleAsync and assert
repository.AddAsync is not called; (2) valid NumberOfCopies >0: create an event
with NumberOfCopies N, call HandleAsync and assert repository.AddAsync is called
N times (or called once with N items depending on implementation) and that
created BibItem properties match input; (3) failure path for BibItem.Create:
simulate item creation failure by providing invalid input that causes the
factory to return failure/throw (or wrap the factory with an injectable factory
in a test seam) then call HandleAsync and assert the repository is not called
and the ILogger mock received an error log; for logging assertions replace
NullLogger with a mocked ILogger<BookRecordCreatedEventHandler>
(NSubstitute/Moq) to verify LogError was invoked; implement all tests as async
Facts and use NSubstitute to verify call counts and captured arguments.
| { | ||
| _bibItemRepository = Substitute.For<IBibItemRepository>(); | ||
| _logger = new NullLogger<BookRecordCreatedEventHandler>(); | ||
| new BookRecordCreatedEventHandler(_bibItemRepository, _logger); |
There was a problem hiding this comment.
Store the handler instance for use in test methods.
The BookRecordCreatedEventHandler is instantiated but immediately discarded. Store it in a private field so test methods can invoke its Handle method.
Apply this diff:
+ private readonly BookRecordCreatedEventHandler _sut;
+
public BookRecordCreatedEventHandlerTests()
{
_bibItemRepository = Substitute.For<IBibItemRepository>();
_logger = new NullLogger<BookRecordCreatedEventHandler>();
- new BookRecordCreatedEventHandler(_bibItemRepository, _logger);
+ _sut = new BookRecordCreatedEventHandler(_bibItemRepository, _logger);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| new BookRecordCreatedEventHandler(_bibItemRepository, _logger); | |
| private readonly IBibItemRepository _bibItemRepository; | |
| private readonly ILogger<BookRecordCreatedEventHandler> _logger; | |
| private readonly BookRecordCreatedEventHandler _sut; | |
| public BookRecordCreatedEventHandlerTests() | |
| { | |
| _bibItemRepository = Substitute.For<IBibItemRepository>(); | |
| _logger = new NullLogger<BookRecordCreatedEventHandler>(); | |
| _sut = new BookRecordCreatedEventHandler(_bibItemRepository, _logger); | |
| } |
🤖 Prompt for AI Agents
In
tests/Kathanika.Application.Tests/Features/QuickAdd/EventHandlers/BookRecordCreatedEventHandlerTests.cs
around line 17, the BookRecordCreatedEventHandler is being instantiated but not
stored; declare a private readonly field for the handler at the test class
level, assign the new BookRecordCreatedEventHandler(_bibItemRepository, _logger)
to that field in the test setup/constructor instead of discarding it, and update
test methods to call handler.Handle(...) via this stored field.
New Feature Submissions:
Changes to Core Features:
Summary by CodeRabbit
New Features
Refactor
Chores