Пользовательская ручка view #38 - #40
Conversation
|
💩 Code linting failed, use |
Coverage Report
Summary
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
пофиксил формат, вроде теперь норм |
petrCher
left a comment
petrCher
left a comment
There was a problem hiding this comment.
тесты еще не смотрел, сперва надо поправить мои комменты
плюс еще дополнение небольшое, возможно непонятно:
visit_count в другой таблице (ты с ней здесь не работаешь) просто считает вообще все заходы в сервис пользователя
shown_count считает сколько раз показали именно эту модалку
last_visit_number это копия visit_count на момент последнего показа
|
|
||
| if not notes: | ||
| raise ObjectNotFound(Note, 'all') | ||
| raise ObjectNotFound(Note, "all") |
There was a problem hiding this comment.
похоже что случайно форматнулось пока я пытался подгонать нужное форматирование для пуша. Поправлю обратно
| """ | ||
| Отмечает, что модалка реально была отрисована пользователю. | ||
|
|
||
| Увеличивает shown_count в note_view и запоминает номер захода |
There was a problem hiding this comment.
уточни, что note_view это таблица
| user=Depends(UnionAuth()), | ||
| ) -> StatusResponseModel: | ||
| """ | ||
| Отмечает, что модалка реально была отрисована пользователю. |
There was a problem hiding this comment.
была "показана", "отрисована" не слишком литературно
| Отмечает, что модалка реально была отрисована пользователю. | ||
|
|
||
| Увеличивает shown_count в note_view и запоминает номер захода | ||
| (last_visit_number), от которого потом считается frequency. |
There was a problem hiding this comment.
эта строка должна быть по идее на прошлой, если линтинг сам не переносит строку то надо на одной написать (но там не должен переносить он ничего, 120 вроде символов ограничение)
|
|
||
| Повторный вызов не ошибка | ||
|
|
||
| Исключение ObjectNotFound(404), если модалки с `id` не существует |
There was a problem hiding this comment.
про 404 и 403 можно не писать, это и так известно из названий исключений
| user_id=user_id, | ||
| shown_count=1, | ||
| last_visit_number=visit_count, | ||
| rejected_count=0, |
There was a problem hiding this comment.
это можно не писать, там же в структуре бд по дефолту 0
| note_id=note_id, | ||
| user_id=user_id, | ||
| shown_count=1, | ||
| last_visit_number=visit_count, |
There was a problem hiding this comment.
а откуда visit_count у юзера, у которого только создается запись? просто при беглом взгляде можно упустить момент с тем что выше ты задал равным 0
There was a problem hiding this comment.
здесь лучше будет прописать last_visit_number=1 так как при создании нового уже пользователь точно видел одно оповещение
|
|
||
| if user_visit is not None: | ||
| visit_count = user_visit.visit_count | ||
| else: |
There was a problem hiding this comment.
https://github.com/profcomff/modal-service-api/pull/40/changes#r3861487114 по этому комменту смотри
лучше просто без else
| note = Note.get(session=db.session, id=note_id) | ||
| if note.status != ModalStatus.ACTIVE: | ||
| raise ForbiddenAction(Note) | ||
|
|
There was a problem hiding this comment.
сюда еще добавить проверку на существование сервиса (для этого есть даже отдельная таблица в бд)
There was a problem hiding this comment.
и еще проверка не истекла ли модалка
те end_ts не превышен ли
| """ | ||
|
|
||
| @classmethod | ||
| async def mark_view(cls, db: Session, note_id: int, user_id: int, service_id: int) -> NoteView: |
There was a problem hiding this comment.
честно говоря не вижу смысла в аннотации NoteView
просто все равно возвращается именно он
если бы работали со схемами pydantic то это ок, но с схемой в бд плохо
да и return в конце не нужен, так как классметод в ручку ничего не возвращает по текущей логике
|
Вроде все пофиксил, жду ревью |
|
@Aiz0r когда исправляешь мой коммент, надо его резолвить, чтобы я видел что он действительно исправлен |
| assert view is not None | ||
| assert view.shown_count == 1 | ||
|
|
||
| dbsession.delete(view) |
There was a problem hiding this comment.
всю работу с бд выносим в фикстуры, там можно yield использовать для передачи, потом удаляем
| dbsession.commit() | ||
|
|
||
|
|
||
| def test_second_view_increments_shown_count(client, dbsession, notes, authlib_user_data): |
There was a problem hiding this comment.
плохо когда один и тот же тест разделен на 2, где разница - числовое значение
| def test_second_view_increments_shown_count(client, dbsession, notes, authlib_user_data): | ||
| note = notes[0] | ||
|
|
||
| client.post(f"{url}/{note.id}/view", params={"service_id": 1}) |
There was a problem hiding this comment.
почему есть привязка к сервису с айди 1?
| dbsession.commit() | ||
|
|
||
|
|
||
| def test_nonexistent_note_returns_404(client): |
There was a problem hiding this comment.
вообще надо один бы тест сделать, так как одна ручка, надо через mark.parametrize прописать тест-кейсы и на разные статусы ответов разная логика
| assert response.status_code == status.HTTP_404_NOT_FOUND | ||
|
|
||
|
|
||
| def test_archived_note_returns_403(client, notes): |
There was a problem hiding this comment.
это все тоже в один тест общий можно сделать
2 participants
Изменения
Добавлена ручка POST /user/{id}/view, она фиксирует реальный показ модалки пользователю. Показ засчитывается в таблице note_view: при первом показе создаётся запись, при повторных увеличивается счётчик показов. Если модалки не существует - ошибка 404, если модалка не активна - ошибка 403
Детали реализации
NoteViewService.mark_view (utils/services.py) - основная логика
Тесты в tests/test_routes/test_user.py: первый показ, повторный показ (инкремент), несуществующая модалка (404), неактивная модалка (403)
Check-List
blackиisortдля Back-End илиPrettierдля Front-End?