Feature/financial years

[BerendWouters]

I'm still in doubt about the domain model of the FinancialYear.

I've added some unit tests nonetheless, so we can still change the implementation.

[vyruz1986]

I'm also missing an EFCore migration. I'm also wondering whether FInancialYear should become an AggregateRoot whjch owns many Transaction objects. The it is now, they would become 2 aggregate roots with relations between them, which is generally frowned upon in DDD.
A better would be to make FinanalYear the aggregate root, and make the current Transaction aggregate root, a value object of a financial year instead. In this case the TransactionRepository would need to be removed, and each interaction with a transaction would need to take please through the FinancialYear.
In this an EFCore configuration using OwnsMany(y => y.Transactions) would be needed from the FinancialYear config

[BerendWouters]

I'm also missing an EFCore migration. I'm also wondering whether FInancialYear should become an AggregateRoot whjch owns many Transaction objects. The it is now, they would become 2 aggregate roots with relations between them, which is generally frowned upon in DDD. A better would be to make FinanalYear the aggregate root, and make the current Transaction aggregate root, a value object of a financial year instead. In this case the TransactionRepository would need to be removed, and each interaction with a transaction would need to take please through the FinancialYear. In this an EFCore configuration using OwnsMany(y => y.Transactions) would be needed from the FinancialYear config

I was opting for that last path too, but I did want to have this discussed. From a domain point of view, this approach makes to most sense. So: FinancialYear becomes the new aggregate root, Transaction is removed and becomes a value object of that newly created aggregate root.
I do wonder about the following though:
The existing transaction handlers are unaware of the Financial year concept. Am I correct in letting them exist, but instead of interacting with the Transaction (which it can't since we removed the ITransactionRepository), it should first figure out what FinancialYear the transaction belongs to, using the ReceivedDate of the transaction? This would imply that we could move transactions from one FinancialYear to another (as longs as they're not closed and so on).
The cool thing about using the FinancialYear as the aggregate root is that we only have to check if the transaction can be edited if the FinancialYear isn't closed, and not the transaction themselves. As a consequence, I don't think we'd need to add a Locked property on the Transaction.

The reason I didnt yet create a migration was to prevent too many up and down migrations during development.

[vyruz1986]

I'm also missing an EFCore migration. I'm also wondering whether FInancialYear should become an AggregateRoot whjch owns many Transaction objects. The it is now, they would become 2 aggregate roots with relations between them, which is generally frowned upon in DDD. A better would be to make FinanalYear the aggregate root, and make the current Transaction aggregate root, a value object of a financial year instead. In this case the TransactionRepository would need to be removed, and each interaction with a transaction would need to take please through the FinancialYear. In this an EFCore configuration using OwnsMany(y => y.Transactions) would be needed from the FinancialYear config

I was opting for that last path too, but I did want to have this discussed. From a domain point of view, this approach makes to most sense. So: FinancialYear becomes the new aggregate root, Transaction is removed and becomes a value object of that newly created aggregate root. I do wonder about the following though: The existing transaction handlers are unaware of the Financial year concept. Am I correct in letting them exist, but instead of interacting with the Transaction (which it can't since we removed the ITransactionRepository), it should first figure out what FinancialYear the transaction belongs to, using the ReceivedDate of the transaction? This would imply that we could move transactions from one FinancialYear to another (as longs as they're not closed and so on). The cool thing about using the FinancialYear as the aggregate root is that we only have to check if the transaction can be edited if the FinancialYear isn't closed, and not the transaction themselves. As a consequence, I don't think we'd need to add a Locked property on the Transaction.

The reason I didnt yet create a migration was to prevent too many up and down migrations during development.

Indeed that would impose a breaking change in that transactions could not be accessed by themselves, but would need to be accessed through their FY aggregate root.
Which means that would likely translate back to UI changes as well, as you would first need to select the financial year before being able to see/add/edit transactions...

[BerendWouters]

I've updated the PR.

No migrations yet, but I think the domain change has been done.

However, some other issues I'm not sure if the current implementation is the best:

• As you mentioned, handling transactions should be done through the aggregate root. I've removed the TransactionRepository, but allowed to find the FY via a transactionId. I'm not really sure if this goes against any practices. It feels off, but there's a difference between the UI and the actual domain model (which I think is allowed in DDD).
• However, by introducing the FY as the aggregate root and DDD stating you should always start at the aggregate root, we will always track all transactions (and attachments) under one FY. This might introduce a performance penalty, but unsure. It's not quite clear to me to what DDD says about including a certain child item, but it feels like a bad practice.
• Querying. DDD is aimed at managing the state of the object you're working on, but is if I remember correctly quite loose about getting data. For that reason, we still have a DbSet of Transactions, but only for querying purposes. I think this is fine, but we should always adhere the no-tracking configuration, when querying the DbContext. In the past I've used a query facade pattern to make this more strict, but that's just convention imho.

[vyruz1986]

• Querying. DDD is aimed at managing the state of the object you're working on, but is if I remember correctly quite loose about getting data. For that reason, we still have a DbSet of Transactions, but only for querying purposes. I think this is fine, but we should always adhere the no-tracking configuration, when querying the DbContext. In the past I've used a query facade pattern to make this more strict, but that's just convention imho.

I think the DbSet will have to go anyway, because iirc an entity cannot have both it's own DbSet as well as be owned by another entity.
Offcourse you could expose an IQueryable from the DbContext which does something like FinancialYears.SelectMany(y => t>Transactions).AsNoTracking(), or go all the way as you mentioned and create an I<Entity>QueryStore next to I<Entity>Repository for query purposes

[BerendWouters]

I knew this would happen: owned types don't support hierarchy (https://learn.microsoft.com/en-us/ef/core/modeling/owned-entities#current-shortcomings).