BlazingSolutions / TimeKeeper

A project for learning Blazor
9 stars 0 forks source link

Retrieve categories from database when adding time entry #66

Closed DotNetDublin closed 3 years ago

DotNetDublin commented 3 years ago

Hi @jonhilt

As outlined in this discussion I've attempted to follow your approach with mediatR/CQRS in order to retrieve categories from the database.

As you'll see I have a few questions and have also managed to break Swagger even if the actual application still works.

Once the Swagger issue is resolved and we're happy I'm following your approach correctly my next goal is to update ListTimeEntries.razor to remove the GetCategoryName method.

We'll remove GetClientName in a separate pull request dealing with reading clients from the database.

Finally, the mediatr-categories has been brought up to date with all the latest changes in main.

jonhilt commented 3 years ago

As you'll see I have a few questions and have also managed to break Swagger even if the actual application still works.

I've pushed a fix for this.

To diagnose the issue I attempted to look at the swagger file itself: https://localhost:44331/swagger/v1/swagger.json

This then showed a more helpful error about issues with the Model classes.

I've had this before. Because we adopt naming conventions with MediatR, like using Model, Query and Command for nested class names,, Swagger attempts to generate a schema using these names, but then falls over because there's more than one Model class.

My commit introduces a little code to handle this and basically use more verbose (and specific) schema Ids in the event that the class name is Command, Model or Query.

DotNetDublin commented 3 years ago

Thanks @jonhilt.

I would never have got there, well not without a lot of trial and error and swearing :)

Not strictly related however I was wondering whether another small job for me might be to rename CreateTimeEntry.cs to Create.cs?

jonhilt commented 3 years ago

Hi @jonhilt

As outlined in this discussion I've attempted to follow your approach with mediatR/CQRS in order to retrieve categories from the database.

Looks great to me!

Regarding your question about create a new Api class for categories, I think the approach of sticking to ITimeEntryApi for now is a good call.

One thing I try to consider is what DDD refers to as bounded contexts. The idea is that certain parts of the system are cohesive and should effectively be maintained together.

In a company for example, you might have a Warehouse and an Accounts department.

These could well be separate bounded contexts. So anything to do with managing stock, moving things around in the warehouse, dispatching items from the warehouse etc. would be represented in the Warehouse bounded context (in code terms, this could be a separate project in the solution, or even an entire microservice just for warehouse operations).

Then the accounting team might need to request certain things from the warehouse (maybe a daily report about stock movements or whatever) but otherwise have an entirely separate series of things they need to work with (ledgers, invoices etc.) They may even have data that looks like a duplicate of what's in the warehouse, but that's by design, so they can work on their own data without suddenly finding things being changed because the warehouse did something!

In our case, I think time entry is a fairly clear, cohesive set of functions which all belong together. Categorising time logged feels part of this, so having a single API for time management functionality seems fine to me (and the simplest thing to start with, which could always be refactored later).

The key, in my experience, is thinking about this in terms of business domain, and what makes sense to live/change/work together (less so about dividing up the code based on technical concerns like which entity we're dealing with).

The MediatR handler etc. all look good to me 😃

jonhilt commented 3 years ago

Not strictly related however I was wondering whether another small job for me might be to rename CreateTimeEntry.cs to Create.cs?

Yeah I think so, the context is already there from its namespace/folder (plus this would make things more consistent, easily discoverable when jumping around between features)