Repository navigation
Conversation
Coverage Report
Summary
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Вроде все поправил, жду ревью @petrCher. |
|
ну или от @Georgon тоже жду ревью |
|
git rebase сделай, так как залил твой прошлый пр |
|
если что только надо меня и Гошу не в assigners добавлять, а в reviews) |
Ой, сорри, поправлю завтра |
Ок, поправлю |
bce5048 to
17ef578
Compare
|
если ревью ждешь, не забывай просить ревью во вкладке reviewers) |
Мне не дает вас выбрать в ревьюверы, если что |
Могу на рандомного кого-то повесить. Но на тебя или Georgon не могу. Тупо в списке нет |
petrCher
left a comment
petrCher
left a comment
There was a problem hiding this comment.
https://github.com/profcomff/modal-service-api/blob/main/modal_backend/exceptions.py#L35
ValueError у тебя используется кастомный и получается что ты в него передаешь не совсем то, что было ранее запланировано для него и получается бред)
поэтому надо бы создать свое новое исключение и в него передавать данные
| type_id: int | ||
| header: str | ||
| end_ts: datetime.datetime | ||
| end_ts: datetime.datetime | None # по тз может быть null, если is_always = true |
There was a problem hiding this comment.
коммент можно удалить
если для меня пр коммент нужен, то его лучше оставлять прям в гитхабе, как я оставляю текущий коммент тебе
| dbsession.delete(note) | ||
|
|
||
|
|
||
| VALID_NOTE_BODIES: dict[str, dict] = { |
There was a problem hiding this comment.
все тесты не то что нужно, ты делаешь тесты ради тестов
а надо тесты ради понимания кода
есть предложение исправить все проблемы этого пр по фалидации и потом отдельным сделаешь норм тестики (на созваны объясню что хочу увидеть)
There was a problem hiding this comment.
если что тесты ниже этого коммента удалить надо, те небольшие правки выше правильные
| ): | ||
| """Валидация полей при создании note""" | ||
| if note.is_always: | ||
| note.end_ts = None |
There was a problem hiding this comment.
а зачем такая проверка если в схеме прописано что дефолт none
There was a problem hiding this comment.
и тогда сразу вопрос почему start_ts не делал?) только для конца
| """Валидация полей при создании note""" | ||
| if note.is_always: | ||
| note.end_ts = None | ||
| elif note.end_ts is None or (note.start_ts is not None and note.start_ts >= note.end_ts): |
There was a problem hiding this comment.
лучше просто or все сделать
| note.end_ts = None | ||
| elif note.end_ts is None or (note.start_ts is not None and note.start_ts >= note.end_ts): | ||
| raise ValueError("Invalid end_ts value") | ||
|
|
There was a problem hiding this comment.
нужна кстати проверка что start и end_ts>now иначе я могу сейчас создать модалку которая просрочена
| raise ValueError("Service ids must not be empty") | ||
|
|
||
| if isinstance(note, NoteRatingPost): | ||
| if note.rating_max is None or not (2 <= note.rating_max <= 10): |
There was a problem hiding this comment.
https://github.com/profcomff/modal-service-api/blob/main/modal_backend/settings.py#L11
хардкор с числом 10 это плохо
надо сделать отдельной переменной для variables в settings и оттуда использовать здесь и в будущих тестах
также как сделаешь переменную надо в github/build_and_publish.yml прописать в джобе эту переменную как ту, которую надо взять из github secrets(с ними я сам разберусь, тебе только желательно в описании жирным пометить что мне надо это сделать)
| elif isinstance(note, NoteImagePost): | ||
| if not note.images: | ||
| raise ValueError("Images must not be empty") | ||
| elif isinstance(note, NoteTextPost): |
There was a problem hiding this comment.
хочется в порядке следования type_id прописать валидацию каждого объекта
| if not note.images: | ||
| raise ValueError("Images must not be empty") | ||
| elif isinstance(note, NoteTextPost): | ||
| if note.max_length is None or not (1 <= note.max_length <= 5000): |
There was a problem hiding this comment.
давай для 5000 тоже сделаем как с максимальным рейтингом
| raise ValueError("Choice options must have unique ids") | ||
| if not all( | ||
| opt.text for opt in note.choice_options | ||
| ): # я так понял, что текст из пробелов это тоже текст. Поправьте если нет |
There was a problem hiding this comment.
opt.text.strip()
коммент всегда в гитхабе писать, не в коде
There was a problem hiding this comment.
пробелы в тексте лишние убрать
|
|
||
| if note.frequency < 1: | ||
| raise ValueError("Frequency must be greater than 0") | ||
| if not note.group_ids: |
There was a problem hiding this comment.
хочется для групп и сервисов сделать проверку на их существование реальное, надо в бд здесь будет обращаться
Изменения
Валидация модалок при создании в зависимости от типа
Детали реализации
Check-List
blackиisortдля Back-End илиPrettierдля Front-End?