Implement BusinessDocumentBase combining BusinessBase and BusinessListBase#4815
Implement BusinessDocumentBase combining BusinessBase and BusinessListBase#4815rockfordlhotka wants to merge 8 commits intomainfrom
Conversation
Document the design for a new class that combines BusinessBase and BusinessListBase capabilities using composition. This enables the "Document Pattern" where a business object has its own properties AND contains a collection of child items. The plan includes: - Complete interface analysis from both base classes - Conflict resolution strategies for shared interfaces - 10-phase implementation plan with specific tasks - Risk assessment and success criteria Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
I looked for a related discussion or issue for this PR, but didn't find one. What is the intended utility for this kind of class? Looking at this initial plan, I wonder how more complex documents would be handled. By more complex, I mean having multiple child lists. In the invoice example, the editable Invoice class in my project would also have a comments collection, a contacts collection. Thinking across the full project, I don't have any documents with only a single collection. |
|
You are describing a business (base) object. I think the purpose of this PR is to get the whole rules stuff etc. into a business list class. |
|
I get that use case. Those business rules currently have to be handled in the parent class. We typically use properties on the child classes to indicate their error state. For example, the child will have an IsDuplicate property with a rule that ensures the property is false. I'm all for simplifying this scenario, but it wasn't clear in the description and the generated plan. Sorry for butting in without understanding the full context. |
|
…BusinessListBase Adds BusinessDocumentBase<T,C>, a new base class that inherits from BusinessBase<T> and adds full BusinessListBase<T,C> collection capabilities. This enables the "document pattern" where a single business object has its own managed properties, validation rules, and authorization AND contains a collection of child items. Key features: - Full IList<C> collection support via composition (MobileList<C>) - Deleted child tracking (IContainsDeletedList) - N-level undo cascading to collection children - IsDirty/IsValid/IsBusy aggregation across properties and children - MobileFormatter serialization of collection items - CollectionChanged/PropertyChanged events - LoadListMode for bulk loading without events - Child data access (Child_Update/Child_UpdateAsync) Includes IBusinessDocumentBase<C> interface and 29 unit tests covering create/fetch, collection operations, status aggregation, events, clone, and n-level undo. Implements #1830 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rockfordlhotka
changed the title
- Add 30 new tests covering: NotUndoable, AddNew/Async, advanced undo (nested BeginEdit/ApplyEdit, deleted list tracking), equality, event suppression, save workflow, advanced clone/undo state, WaitForIdle, and serialization roundtrip parent reference preservation - Add metastate PropertyChanged event tests (Xaml mode, SkipOnCIServer) mirroring BasicModernTests: MakeOld, MarkDeleted, property/child changes - Expose public API gaps: AddNew(), AddNewAsync(), SuppressListChangedEvents, RaiseListChangedEvents (public read) - Fix InsertItem/RemoveItem to call OnChildChanged so IsDirty/IsValid/ IsSavable PropertyChanged fires when collection items are added/removed - Add test infrastructure: NotUndoableData, MakeOld(), DataPortal_DeleteSelf, DeletedCount on TestDocument; AsyncRuleText+rule on DocumentLineItem; MetastateDocument and MetastateLineItem for metastate tests - Delete BusinessDocumentBase-PLAN.md (planning artifact, not for shipping) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR implements BusinessDocumentBase<T,C>, a new base class that combines the capabilities of BusinessBase<T> and BusinessListBase<T,C> to support the "document pattern" where a single business object has both its own managed properties and a collection of child items. This addresses issue #1830 regarding adding rules engine support to collection-based types.
Changes:
- New
BusinessDocumentBase<T,C>class providing full BusinessBase property management and BusinessListBase collection capabilities - New
IBusinessDocumentBase<C>interface consolidating BusinessBase and BusinessListBase contracts - Comprehensive test suite with 29 unit tests covering CRUD operations, status aggregation, N-level undo, serialization, and metastate events
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Source/Csla/BusinessDocumentBase.cs | Main implementation combining BusinessBase inheritance with IList collection management, deleted child tracking, undo cascading, and child data access |
| Source/Csla/IBusinessDocumentBase.cs | Interface definition consolidating IBusinessBase and IBusinessListBase |
| Source/tests/Csla.test/BusinessDocumentBase/BusinessDocumentBaseTests.cs | Comprehensive test suite (29 tests) covering collection operations, status aggregation, undo, serialization, and async behavior |
| Source/tests/Csla.test/BusinessDocumentBase/BusinessDocumentBaseMetastateTests.cs | Tests for PropertyChanged metastate events (IsDirty, IsValid, etc.) when properties or children change |
| Source/tests/Csla.test/BusinessDocumentBase/TestDocument.cs | Test document class combining properties (DocumentNumber, DocumentDate) with child collection of DocumentLineItems |
| Source/tests/Csla.test/BusinessDocumentBase/DocumentLineItem.cs | Child business object for testing with Description, Amount properties and async rule support |
| Source/tests/Csla.test/BusinessDocumentBase/MetastateDocument.cs | Document class for metastate event testing with validation rules ([Required] on Name property) |
| Source/tests/Csla.test/BusinessDocumentBase/MetastateLineItem.cs | Minimal child object for metastate document tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- SetItem: add IsChild guard (throws InvalidOperationException for non-child items) to match InsertItem's enforcement of the child constraint; previously the indexer setter accepted any object - RegisterProperty: add four missing lambda-expression overloads (relationship, friendlyName, friendlyName+default, friendlyName+default+relationship) to match the full set present on BusinessBase Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Adds IndexerSet_NonChild_ThrowsInvalidOperationException to verify that doc[i] = nonChild throws InvalidOperationException, mirroring the existing InsertNonChild test and covering the SetItem fix from the Copilot review. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
StefanOssendorf
left a comment
StefanOssendorf
left a comment
There was a problem hiding this comment.
I skipped the tests for now until the implementation is discussed.
Source/Csla/BusinessDocumentBase.cs
Outdated
| { | ||
| /// <summary> | ||
| /// Base class for an editable business object that has its own | ||
| /// properties AND contains a collection of child items. |
There was a problem hiding this comment.
contains or is a collection of child items?
For me it should be an is?
Source/Csla/BusinessDocumentBase.cs
Outdated
| IList<C>, | ||
| IBusinessDocumentBase<C> | ||
| where T : BusinessDocumentBase<T, C> | ||
| where C : IEditableBusinessObject |
There was a problem hiding this comment.
should we add the notnull constraint here to make clear a
FooDocuments<FooDocuments, FooDocument?> isn't allowed?
|
|
||
| #region IContainsDeletedList | ||
|
|
||
| IEnumerable<IEditableBusinessObject> IContainsDeletedList.DeletedList |
There was a problem hiding this comment.
If we want to be memory efficient here we could to:
(IEnumerable<IEditableBusinessObject>)(_deletedList ?? Enumerable.Empty<IEditableBusinessObject>());
| /// <param name="item">Child object to check.</param> | ||
| /// <exception cref="ArgumentNullException"><paramref name="item"/> is <see langword="null"/>.</exception> | ||
| [EditorBrowsable(EditorBrowsableState.Advanced)] | ||
| public bool ContainsDeleted(C item) |
There was a problem hiding this comment.
Should this be public or better be protected and the user should implement functionality to check that? 🤔
| DeletedList.Add(child); | ||
| } | ||
|
|
||
| private void UnDeleteChild(C child) |
There was a problem hiding this comment.
Isn't there missing a UnDeleteChild or the like? I just tested this and Deleting+Readding still deletes the item on the server side ^^'
| /// <param name="index">Index of the item to insert.</param> | ||
| /// <param name="item">Item to insert.</param> | ||
| /// <exception cref="ArgumentNullException"><paramref name="item"/> is <see langword="null"/>.</exception> | ||
| protected virtual void InsertItem(int index, C item) |
There was a problem hiding this comment.
null guard missing?
| /// <param name="index">The zero-based index of the item to replace.</param> | ||
| /// <param name="item">The new value for the item at the specified index.</param> | ||
| /// <exception cref="ArgumentNullException"><paramref name="item"/> is <see langword="null"/>.</exception> | ||
| protected virtual void SetItem(int index, C item) |
There was a problem hiding this comment.
null guard missing?
Source/Csla/BusinessDocumentBase.cs
Outdated
|
|
||
| void IEditableCollection.SetParent(IParent? parent) | ||
| { | ||
| SetParent(parent!); |
There was a problem hiding this comment.
The ! is wrong here. We have to handle the IParent?.
Source/Csla/BusinessDocumentBase.cs
Outdated
| /// <exception cref="ArgumentNullException"><paramref name="propertyName"/> is <see langword="null"/>.</exception> | ||
| protected static new PropertyInfo<P> RegisterProperty<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] P>(string propertyName) | ||
| { | ||
| if (propertyName is null) |
There was a problem hiding this comment.
string.IsNullOrWhiteSpace should here be used. An empty property is also invalid.
| /// <param name="info">PropertyInfo object for the property.</param> | ||
| /// <returns>The provided IPropertyInfo object.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="info"/> is <see langword="null"/>.</exception> | ||
| protected static new PropertyInfo<P> RegisterProperty<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] P>(PropertyInfo<P> info) |
There was a problem hiding this comment.
Idea: Should we still support the "old way" registrations or skip them in newer types and only use the modern version/art?
- Fix doc comment: "contains" → "is" a collection - Add notnull constraint to C type parameter - Use Enumerable.Empty for IContainsDeletedList to avoid lazy allocation - Add UnDeleteChild logic in InsertItem for re-added deleted children - Add null guards on public collection methods (Add, Insert, Remove, Contains, IndexOf, CopyTo) - Remove null-forgiving operator on IEditableCollection.SetParent - Use string.IsNullOrWhiteSpace for RegisterProperty/RegisterMethod name validation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed the review feedback from @StefanOssendorf and Copilot:
Not changed:
All 29 existing tests pass. |
Summary
BusinessDocumentBase<T,C>, a new base class that inherits fromBusinessBase<T>and adds fullBusinessListBase<T,C>collection capabilitiesIBusinessDocumentBase<C>consolidated interfaceKey Features
IList<C>collection support via composition (MobileList<C>)IContainsDeletedList)IsDirty/IsValid/IsBusyaggregation across properties and childrenMobileFormatterserialization of collection itemsCollectionChanged/PropertyChangedeventsLoadListModefor bulk loading without eventsChild_Update/Child_UpdateAsync)RegisterProperty/RegisterMethodwith correct type resolutionTest plan
Closes #1830
🤖 Generated with Claude Code