Feat/mub roles+permissions #6
Conversation
waldemarX
force-pushed
the
feat/mub-roles
branch
2 times, most recently
from
5e07b27
to
5d36bd5
Compare
| permissions: Mapped[list["RolePermission"]] = relationship( | ||
| passive_deletes=True, cascade="all, delete-orphan", lazy="selectin" | ||
| ) |
There was a problem hiding this comment.
issue: ORM-каскады не совместимы с constraints на ForeignKey (ondelete="cascade")
| community: Mapped[Community] = relationship(passive_deletes=True) | ||
| permissions: Mapped[list["RolePermission"]] = relationship( |
There was a problem hiding this comment.
polish: Раз разделяем отношения пустыми строчками, перед permissions тоже
| @property | ||
| def permission_list(self) -> list[Permission]: # noqa |
There was a problem hiding this comment.
issue:
- Никогда нельзя использовать noqa без указания ошибки
- Линтер абсолютно прав, название фигня, списки списками не зовут
| summary="Provide a role to a participant", | ||
| ) | ||
| async def provide_role_to_participant( |
There was a problem hiding this comment.
issue: некорректный и неконсистентный неминг (а было лучше с assign)
There was a problem hiding this comment.
suggestion: Раз уж в participants точно такая же предзагрузка ролей, что в ролях для разрешений, можно и API совместить с participants_mub. Assign и deassign можно оставить отдельными методами, но получение добавить в модель для retrieve
| exclude={ | ||
| "permissions", | ||
| } |
There was a problem hiding this comment.
polish: без запятой (или с запятой после }) компактнее
| if role_data.permissions: | ||
| role.permissions.clear() | ||
| role.permissions.extend( | ||
| [ | ||
| RolePermission(role_id=role.id, permission=permission) | ||
| for permission in role_data.permissions | ||
| ] | ||
| ) |
There was a problem hiding this comment.
issue (repeat): сначала убрать всё, затем добавить всё. Нет
There was a problem hiding this comment.
Мы, кстати, обычно отношениями ORM не пользуемся
There was a problem hiding this comment.
Зачем эта логика в контроллере тоже особо не понимаю (может и меет место быть, но странно)
| ) | ||
| if role_data.permissions: |
There was a problem hiding this comment.
issue (ну камон): if на непонятное условие. permissions это не bool, обязательно уточнять, что ты проверяешь. А тут это даже ведёт к некорректному поведению: невозможно отчистить список разрешений
| permissions: list[Permission] | None | ||
|
|
There was a problem hiding this comment.
issue: Не используется PatchDefault, а должен. None тут не при делах
| router = APIRouterExt(tags=["roles meta mub"]) | ||
|
|
There was a problem hiding this comment.
issue: теперь meta не имеет смысла
niqzart
force-pushed
the
staging
branch
3 times, most recently
from
1c99138
to
96c5d21
Compare
|
Closed in favour of #25 |
No description provided.