refactor: fewer response models #146

eoaksnes · 2022-11-14T10:12:06Z

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!

api/src/features/todo/use_cases/delete_todo_by_id.py

api/src/features/todo/use_cases/get_todo_by_id.py

sebastianvitterso · 2022-11-14T10:25:51Z

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.

web/src/api/generated/models/add-todo.ts

web/src/api/generated/models/delete-todo-by-id-response-model.ts

web/src/api/generated/models/todo-item.ts

web/src/api/generated/models/update-todo-request.ts

web/src/api/generated/models/update-todo.ts

web/src/hooks/useTodos.tsx

web/src/api/generated/.openapi-generator/FILES

eoaksnes · 2022-11-14T17:16:06Z

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 · 2022-11-14T17:48:53Z

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 · 2022-11-15T09:00:11Z

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 · 2022-11-16T08:08:55Z

I think things are okay for now:

@olavis please go over any comments again

olavis · 2022-11-18T08:49:02Z

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 · 2022-11-18T12:46:44Z

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.

web/src/hooks/useTodos.tsx

olavis · 2022-11-22T13:18:51Z

@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 · 2022-11-22T13:41:08Z

@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.

sebastianvitterso reviewed Nov 14, 2022

View reviewed changes

api/src/features/todo/use_cases/delete_todo_by_id.py Outdated Show resolved Hide resolved

api/src/features/todo/use_cases/get_todo_by_id.py Outdated Show resolved Hide resolved

olavis suggested changes Nov 14, 2022

View reviewed changes

eoaksnes force-pushed the feature/map-request-response-model branch from 6696e56 to a61051c Compare November 14, 2022 17:13

sebastianvitterso mentioned this pull request Nov 15, 2022

build(ci/cd): update flake8 repo location #152

Merged

sebastianvitterso approved these changes Nov 15, 2022

View reviewed changes

eoaksnes requested a review from olavis November 18, 2022 07:33

eoaksnes changed the title ~~refactor: map entity fields to request and response models~~ refactor: fewer response models Nov 18, 2022

eoaksnes added 8 commits November 18, 2022 14:43

refactor: map entity fields to request and response models

609fe76

test: add include, exclude and rename for filter_fields decorator

4c94e47

refactor: add request model for add todo

318ab24

refactor: remove entities (need to rebase after PR finished)

944d6d2

refactor: remove console log (need to rebase after PR)

da0d4d1

refactor: re-generate API again (must rebase after PR)

8a9fcc5

refactor: remove delete response (need to rebase after PR)

357f1eb

refactor: wip

4413603

eoaksnes force-pushed the feature/map-request-response-model branch from b0649de to 4413603 Compare November 18, 2022 13:43

eoaksnes added 3 commits November 18, 2022 14:44

refactor: wip

c3c77ee

refactor: wip

956ba13

refactor: wip

c44f9cd

olavis reviewed Nov 22, 2022

View reviewed changes

web/src/hooks/useTodos.tsx Show resolved Hide resolved

olavis closed this Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: fewer response models #146

refactor: fewer response models #146

eoaksnes commented Nov 14, 2022 •

edited

Loading

sebastianvitterso left a comment

sebastianvitterso commented Nov 14, 2022

olavis left a comment

eoaksnes commented Nov 14, 2022

eoaksnes commented Nov 14, 2022 •

edited

Loading

olavis commented Nov 15, 2022

sebastianvitterso left a comment •

edited

Loading

eoaksnes commented Nov 16, 2022

olavis commented Nov 18, 2022

eoaksnes commented Nov 18, 2022

olavis commented Nov 22, 2022

olavis commented Nov 22, 2022

refactor: fewer response models #146

refactor: fewer response models #146

Conversation

eoaksnes commented Nov 14, 2022 • edited Loading

Why is this pull request needed?

What does this pull request change?

Issues related to this change:

sebastianvitterso left a comment

Choose a reason for hiding this comment

sebastianvitterso commented Nov 14, 2022

olavis left a comment

Choose a reason for hiding this comment

eoaksnes commented Nov 14, 2022

eoaksnes commented Nov 14, 2022 • edited Loading

olavis commented Nov 15, 2022

sebastianvitterso left a comment • edited Loading

Choose a reason for hiding this comment

Looks good!

To respond to some of @olavis points:

eoaksnes commented Nov 16, 2022

olavis commented Nov 18, 2022

eoaksnes commented Nov 18, 2022

olavis commented Nov 22, 2022

olavis commented Nov 22, 2022

eoaksnes commented Nov 14, 2022 •

edited

Loading

eoaksnes commented Nov 14, 2022 •

edited

Loading

sebastianvitterso left a comment •

edited

Loading