refactor: fewer response models

[eoaksnes]

Why is this pull request needed?

• Make it easier to work with the API

  • Fewer response models, a common response model for todo item that will be used by the get, get_all, update and create endpoints

• Map entity fields to request and response models

  • Attempt to try to re-use entity code in the backend for use-case request and response models (not sure how good an idea this is, but let's try it as an experiment).

Relevant information

• https://stackoverflow.com/questions/52812337/clean-architecture-why-not-using-the-entity-as-request-model-of-the-use-case

What does this pull request change?

• Change response type for the update, to return the updated todo item (instead of success)
• Change response type for add, to return the created todo item (instead of success)
• Added filter_fields decorator that maps from entity to response and request models
  • (first version, should probably be made better if it works good)
• Moved validation logic to entities
  • (tried to use data class, but ended up using padantic, since it was too much work/hassle keeping core not aware of libraries, which is recommended)

Issues related to this change:

Resolved #155

[sebastianvitterso]

Two comments, then I'll approve!

[sebastianvitterso]

Also the tests fail!

[olavis]

I've only reviewed the web folder, but I found some "symptoms" in the generated models, that should be resolved in the backend.

[eoaksnes]

Sorry but I force-pushed, but have fixed several things.

How to use API in frontend:

How to create mappings to entities using class Config or as input to decorator (no demo for last):

So what should we do with the response for delete, should we keep the model or not:

[eoaksnes]

Not sure if it was a good idea to change the AddTodoRequestModel to title: str.

This makes the POST request look like this:

Query parameters... Maybe go back to using AddTodoRequestModel?

Also the test will not work as before:

Need to change to using query parameters there also.

[olavis]

I believe the points below are important to address in order to solve this. This PR is growing big, has several breaking changes, unresolved comments are outdated due to force-pushing and many important decision points needs conclusion in order to review it properly, like:

• What are the relations between the entities (User and TodoItem)?
• What is the response from DELETE?
• Communication between frontend and backend: what are the web client needs from the API and in which format do they communicate
• What properties should (and should not) the TodoItem type contain in the web client, aka what will be the returned response from the API?

[sebastianvitterso]

Looks good!

To respond to some of @olavis points:

• What are the relations between the entities (User and TodoItem)?

There is currently no User entity, is there? But each user can have n TodoItems, while each TodoItem has one user. (1-n)

• What is the response from DELETE?

Currently { success: boolean } which works fine.

• Communication between frontend and backend: what are the web client needs from the API and in which format do they communicate

I feel that currently, the communication works well, and since we have typescript in the frontend, it's not too critical how things are structured (details-wise), because we get autocomplete and type hints everywhere.

• What properties should (and should not) the TodoItem type contain in the web client, aka what will be the returned response from the API?

Ah this is a discussion for another day!

[eoaksnes]

I think things are okay for now:

@olavis please go over any comments again

[olavis]

I think things are okay for now:

@olavis please go over any comments again

@eoaksnes there's still unresolved comments from my last review, I can take a look again when you've gone through them? I see many of my points still holds up to this update.

[eoaksnes]

I think things are okay for now:
@olavis please go over any comments again

@eoaksnes there's still unresolved comments from my last review, I can take a look again when you've gone through them? I see many of my points still holds up to this update.

I comment them, please close those that are outdated and we need many of the request models. The camelcase should be done in new PR.

[olavis]

@eoaksnes the second round of reviewing had almost 40 updated files to go through.. Maybe you could focus on avoiding force-pushing and separating changes into smaller PR's next time? 🙏 It's hard to follow your work, and I'm less confident in my reviews as I fear that I easily could've missed something.

[olavis]

@eoaksnes the second round of reviewing had almost 40 updated files to go through.. Maybe you could focus on avoiding force-pushing and separating changes into smaller PR's next time? 🙏 It's hard to follow your work, and I'm less confident in my reviews as I fear that I easily could've missed something.

Okay, had a quick look. I'd say many of my arguments from the first round are still relevant (please explain why you mean they're not worth the attention and I'll appreciate to learn 😸). I also found some new code smells introduced with the update. I think I'll just leave it at this point for now - the changes between each review are big enough to keep this going even longer, and tbh I think it would be more effective to try to move some of this into separate branches and merge them bit by bit, to be able to keep up with the conversations and changes between each round. I see @sebastianvitterso have resolved many of my comments from the first round - if he can vouch for it, good! nothing's better. I think I've come close to the point where I'm not anymore able to review this confidently.