feat: Bring the architecture to a clean archeticture. In the structur…#29
Merged
Conversation
…e, repeat the project structure for easier navigation. Separate the models into different files.
Contributor
Руководство для рецензентаРефакторинг проекта в сторону «чистой архитектуры»: ORM‑модели перенесены в выделенный пакет доменных моделей, сервисы и тесты обновлены для использования этого пакета, добавлены более богатые доменные модели (users, bans, services, appointments, magic tokens), скрипт входной точки для деплоя и небольшие доработки тестов. Диаграмма взаимоотношений сущностей для новых доменных моделейerDiagram
User {
int id
string role
}
Service {
int id
int entrepreneur_id
}
Appointment {
int id
datetime date
int service_id
int entrepreneur_id
int user_id
}
MagicToken {
int id
int telegram_id
int user_id
}
Ban {
int id
int user_id
int banned_by
int revoked_by
}
User ||--o{ Service : offers
User ||--o{ Appointment : makes
User ||--o{ Appointment : serves_as_entrepreneur
User ||--o{ MagicToken : owns
User ||--o{ Ban : is_banned_by
User ||--o{ Ban : issues_ban
User ||--o{ Ban : revokes_ban
Service ||--o{ Appointment : booked_for
Изменения на уровне файлов
Подсказки и командыВзаимодействие с Sourcery
Настройка работыЗайдите в свою панель управления, чтобы:
Получение помощи
Original review guide in EnglishReviewer's GuideRefactors the project toward a clean architecture by moving ORM models into a dedicated domain models package, updating services/tests to use it, and adding richer domain models (users, bans, services, appointments, magic tokens) plus a deployment entrypoint script and minor test cleanups. Entity relationship diagram for new domain modelserDiagram
User {
int id
string role
}
Service {
int id
int entrepreneur_id
}
Appointment {
int id
datetime date
int service_id
int entrepreneur_id
int user_id
}
MagicToken {
int id
int telegram_id
int user_id
}
Ban {
int id
int user_id
int banned_by
int revoked_by
}
User ||--o{ Service : offers
User ||--o{ Appointment : makes
User ||--o{ Appointment : serves_as_entrepreneur
User ||--o{ MagicToken : owns
User ||--o{ Ban : is_banned_by
User ||--o{ Ban : issues_ban
User ||--o{ Ban : revokes_ban
Service ||--o{ Appointment : booked_for
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - я нашёл 3 проблемы и оставил несколько общих замечаний:
- Новые модули доменной модели (
user_model,appointment_model,service_model,ban_model,magic_token_model) импортируют друг друга напрямую во время выполнения, что легко может привести к циклическим импортам; по возможности используйтеTYPE_CHECKINGи строковые типы дляrelationshipвместо взаимных импортов. - В
User.validate_create_dateвы сравниваетеcreated_atсdatetime.utcnow(), тогда как значение по умолчанию для столбца задаётся черезdatetime.now(timezone.utc), то есть смешиваются наивные и привязанные к таймзоне объектыdatetime; приведите обе стороны к единому варианту с таймзоной, чтобы избежать ошибок при сравнении во время выполнения. - Новый тест
test_decode_tokenвtests/presentation/api/v1/auth/test_jwt_handler.pyсоздаёт и декодирует токен, но никак не проверяет результат декодирования, поэтому фактически не валидирует поведение; добавьте проверки (assert) на декодированную полезную нагрузку или ожидаемые ошибки.
Prompt for AI Agents
Пожалуйста, исправьте замечания из этого code review:
## Общие комментарии
- Новые модули доменной модели (`user_model`, `appointment_model`, `service_model`, `ban_model`, `magic_token_model`) импортируют друг друга напрямую во время выполнения, что легко может привести к циклическим импортам; по возможности используйте `TYPE_CHECKING` и строковые типы для `relationship` вместо взаимных импортов.
- В `User.validate_create_date` вы сравниваете `created_at` с `datetime.utcnow()`, тогда как значение по умолчанию для столбца задаётся через `datetime.now(timezone.utc)`, то есть смешиваются наивные и привязанные к таймзоне объекты `datetime`; приведите обе стороны к единому варианту с таймзоной, чтобы избежать ошибок при сравнении во время выполнения.
- Новый тест `test_decode_token` в `tests/presentation/api/v1/auth/test_jwt_handler.py` создаёт и декодирует токен, но никак не проверяет результат декодирования, поэтому фактически не валидирует поведение; добавьте проверки (assert) на декодированную полезную нагрузку или ожидаемые ошибки.
## Отдельные комментарии
### Комментарий 1
<location path="src/domain/models/user_model.py" line_range="82" />
<code_context>
+ if not isinstance(value, datetime) and value is not None:
+ logger.warning("Некорректная дата")
+ raise TypeError("Некорректная дата")
+ elif value > datetime.utcnow():
+ logger.warning("Дата регистрации не может быть в будущем")
+ raise ValueError("Дата регистрации не может быть в будущем")
</code_context>
<issue_to_address>
**issue (bug_risk):** Избегайте сравнения значения `created_at` с таймзоной и наивного `datetime.utcnow()`.
Так как `created_at` — это `DateTime(timezone=True)` со значением по умолчанию `datetime.now(timezone.utc)`, он будет привязан к таймзоне. Сравнение его с `datetime.utcnow()` (наивным) приведёт к `TypeError`. Используйте `datetime.now(timezone.utc)` для сравнения или иным образом убедитесь, что обе стороны имеют согласованные настройки таймзоны, чтобы избежать падения валидатора.
</issue_to_address>
### Комментарий 2
<location path="src/domain/models/ban_model.py" line_range="96" />
<code_context>
+
+ if self.banned_at is not None:
+ if value < self.banned_at:
+ raise ValueError("banned_at не должно быть позже revoked_at")
+
+ return value
</code_context>
<issue_to_address>
**issue (bug_risk):** Сообщение об ошибке в `validate_revoked_at` не соответствует фактическому условию валидации.
Проверка использует `if value < self.banned_at:`, но сообщение говорит, что `banned_at` не должно быть позже `revoked_at`, что является обратным условием. Это будет вводить в заблуждение при отладке. Пожалуйста, либо измените сравнение (например, на `value <= self.banned_at`), чтобы оно соответствовало сообщению, либо обновите сообщение, указав, что `revoked_at` не должно быть раньше `banned_at`.
</issue_to_address>
### Комментарий 3
<location path="src/domain/models/ban_model.py" line_range="75" />
<code_context>
+ def validate_expires_at(self, key, value):
+ if value is not None:
+ if value <= datetime.now(timezone.utc):
+ raise ValueError("expires_at не должно быть в прошлом или None")
+ if value <= self.banned_at:
+ raise ValueError("expires_at не может быть меньше banned_at")
</code_context>
<issue_to_address>
**nitpick:** Сообщение валидации для `expires_at` противоречит логике, которая допускает `None`.
Условие `if value is not None:` разрешает `expires_at=None`, но текст ошибки говорит `"... не должно быть в прошлом или None"`, что подразумевает, что `None` недопустимо. Пожалуйста, приведите сообщение в соответствие с фактическим поведением (например, просто "expires_at не должно быть в прошлом") или обновите валидацию, если по бизнес-правилам `None` действительно должен быть запрещён.
</issue_to_address>Sourcery бесплатно для open source — если вам нравятся наши обзоры, пожалуйста, расскажите о нас ✨
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- The new domain model modules (
user_model,appointment_model,service_model,ban_model,magic_token_model) import each other directly at runtime, which can easily create circular import issues; consider relying onTYPE_CHECKINGand string-basedrelationshiptype hints instead of mutual imports wherever possible. - In
User.validate_create_dateyou comparecreated_atagainstdatetime.utcnow()while the column default usesdatetime.now(timezone.utc), mixing naive and timezone-aware datetimes; align both sides to be consistently timezone-aware to avoid runtime comparison errors. - The new
test_decode_tokenintests/presentation/api/v1/auth/test_jwt_handler.pyconstructs and decodes a token but never asserts anything about the decoded result, so it currently doesn't verify behavior; add assertions on the decoded payload or expected failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new domain model modules (`user_model`, `appointment_model`, `service_model`, `ban_model`, `magic_token_model`) import each other directly at runtime, which can easily create circular import issues; consider relying on `TYPE_CHECKING` and string-based `relationship` type hints instead of mutual imports wherever possible.
- In `User.validate_create_date` you compare `created_at` against `datetime.utcnow()` while the column default uses `datetime.now(timezone.utc)`, mixing naive and timezone-aware datetimes; align both sides to be consistently timezone-aware to avoid runtime comparison errors.
- The new `test_decode_token` in `tests/presentation/api/v1/auth/test_jwt_handler.py` constructs and decodes a token but never asserts anything about the decoded result, so it currently doesn't verify behavior; add assertions on the decoded payload or expected failures.
## Individual Comments
### Comment 1
<location path="src/domain/models/user_model.py" line_range="82" />
<code_context>
+ if not isinstance(value, datetime) and value is not None:
+ logger.warning("Некорректная дата")
+ raise TypeError("Некорректная дата")
+ elif value > datetime.utcnow():
+ logger.warning("Дата регистрации не может быть в будущем")
+ raise ValueError("Дата регистрации не может быть в будущем")
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid comparing a timezone-aware `created_at` value with a naive `datetime.utcnow()` instance.
Since `created_at` is `DateTime(timezone=True)` with default `datetime.now(timezone.utc)`, it will be timezone-aware. Comparing it to `datetime.utcnow()` (naive) will raise `TypeError`. Use `datetime.now(timezone.utc)` for the comparison, or otherwise ensure both sides have matching timezone awareness to avoid crashing the validator.
</issue_to_address>
### Comment 2
<location path="src/domain/models/ban_model.py" line_range="96" />
<code_context>
+
+ if self.banned_at is not None:
+ if value < self.banned_at:
+ raise ValueError("banned_at не должно быть позже revoked_at")
+
+ return value
</code_context>
<issue_to_address>
**issue (bug_risk):** The error message in `validate_revoked_at` does not match the actual validation condition.
The check uses `if value < self.banned_at:`, but the message says `banned_at` must not be later than `revoked_at`, which is the inverse of the condition. This will mislead debugging. Please either change the comparison (e.g. `value <= self.banned_at`) to match the message, or update the message to state that `revoked_at` must not be earlier than `banned_at`.
</issue_to_address>
### Comment 3
<location path="src/domain/models/ban_model.py" line_range="75" />
<code_context>
+ def validate_expires_at(self, key, value):
+ if value is not None:
+ if value <= datetime.now(timezone.utc):
+ raise ValueError("expires_at не должно быть в прошлом или None")
+ if value <= self.banned_at:
+ raise ValueError("expires_at не может быть меньше banned_at")
</code_context>
<issue_to_address>
**nitpick:** Validation message for `expires_at` conflicts with the logic that allows `None`.
The guard `if value is not None:` allows `expires_at=None`, but the error text says `"... не должно быть в прошлом или None"`, which suggests `None` is invalid. Please align the message with the actual behavior (e.g. just "expires_at не должно быть в прошлом") or update the validation if `None` should be disallowed by business rules.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Coverage Report for CI Build 26688250472Coverage increased (+2.5%) to 30.351%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Coverage Report for CI Build 26688593824Coverage increased (+2.5%) to 30.351%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
added 2 commits
May 30, 2026 19:09
…1riX/SteelTime into feat/architecture-improvement
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…e, repeat the project structure for easier navigation. Separate the models into different files.
Summary by Sourcery
Выделены доменные модели в отдельный доменный слой и обновлены ссылки во всех сервисах, тестах и инфраструктуре в соответствии с более чистой архитектурой.
Новые возможности:
Улучшения:
Деплой:
Тесты:
Original summary in English
Summary by Sourcery
Extract domain models into a dedicated domain layer and update references across services, tests, and infrastructure to align with a cleaner architecture.
New Features:
Enhancements:
Deployment:
Tests: