split db_classes.py #79
Conversation
studio_app/models/appointment.py
Outdated
| canceled_by_client: Mapped[Optional[bool]] = mapped_column(default = False) | ||
|
|
||
| @staticmethod | ||
| def create(user_id, service, date_time, slot_id, description): |
There was a problem hiding this comment.
You shouldn't keep any logic in the model, even this.
studio_app/models/service.py
Outdated
| description: Mapped[str] = mapped_column(default = "") | ||
| deleted: Mapped[bool] = mapped_column(default = False) | ||
|
|
||
| def create(self, name, description): |
There was a problem hiding this comment.
Same applies here. You shouldn't add it here. You'd better use a repository for it. But you can also create model without repositories anywhere in your code but models shouldn't contain these methods
studio_app/models/slot.py
Outdated
| count_slots_created = 0 | ||
|
|
||
| count_for_cycle = how_many_days_for_advance_to_populate_slot_table + 1 | ||
| for x in reversed(range(how_many_days_for_advance_to_populate_slot_table + 1)): |
There was a problem hiding this comment.
Same here. You are keeping the logic in the model. This is a bad practice. It's better to keep it separated in things like services and repositories.
studio_app/models/user.py
Outdated
| deleted_at: Mapped[Optional[datetime.datetime]] | ||
| deleted_by_id = mapped_column(ForeignKey("user.id"), nullable=True) | ||
|
|
||
| @staticmethod |
|
You should also keep migrations separate. Don't create one migration for all tables |
migrations/versions/26c692788161_.py
Outdated
| depends_on = None | ||
|
|
||
|
|
||
| def upgrade(): |
There was a problem hiding this comment.
all tables in the migration. not the best
| count_slots_created = 0 | ||
|
|
||
| count_for_cycle = self.config.HOW_FAR_IN_FUTURE_CREATE_SLOTS + 1 | ||
| for x in reversed(range(self.config.HOW_FAR_IN_FUTURE_CREATE_SLOTS + 1)): |
There was a problem hiding this comment.
Repository methods should only handle database operations to maintain clean architecture:
-
Current issues:
- Business logic (time calculations, slot generation rules) mixed with db operations
- Hard to test business rules independently of database
- Complex method handling multiple responsibilities
- Difficult to reuse slot creation logic elsewhere
-
Better approach:
- Move business logic to service layer
- Keep repository focused on CRUD operations
- Separate slot generation rules from database access
- Make code more testable and maintainable
|
|
||
| class AppointmentRepository(BaseRepository): | ||
| def create(self, **kwargs): | ||
| new_appointment = AppointmentModel(sms_confirmation_code=randrange(1000, 9999, 11), **kwargs) |
There was a problem hiding this comment.
Using **kwargs in repository methods creates several problems:
- Lack of type safety - no validation or type hints for required fields
- Hidden dependencies - unclear what fields are needed for appointment creation
- Potential errors - typos in field names won't be caught until runtime
- Poor documentation - developers need to look at model to know required params
Better approach would be explicit params like:
def create(self, customer_id: int, slot_id: int, service_id: int):
This makes the contract clear and catches issues at compile time
split db_classes into dedicated files
each file contains one model
database init and migration are done
seeding functions to be changed accordingly
related PR #71
that was not touched yet