-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
logginable deleting bug fix #198
Conversation
💩 Code linting failed, use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
И можно не делать такой огромный запрос в базу, а, например, сравнить длину списка ответа на запрос по всем логинейбл аутх методам с длиной списка ответа запроса, который выше(в 74 строке) кидался(это был запрос просто по данному методу)
Получилось даже с одним запросом поидее все сделать. Но возможно это не слишком читаемо очевидно, не знаю как лучше, но так точно быстрее
auth_backend/auth_method/oauth.py
Outdated
.filter( | ||
AuthMethod.user_id == user.id, | ||
AuthMethod.auth_method.in_( | ||
[cls.get_name()] + [method.get_name() for method in AUTH_METHODS.values() if method.loginable] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не совсем понимаю, что ты хотел этой строчкой сделать
сложно читаемо, в любом случае
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ну такой запрос тут сделан он сам по себе довольно абстрактен, там ниже в проверке его просто использую, но я и написал, что возможно это ге очень читаемо, и стоит сделать 2 запроса, но код будет более читаем
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я не совсем понимаю, что ты хочешь этим запросом получить
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Хотя, вроде понял, а зачем туда добавлять cls.get_name() его разве там не будет в запросе?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Хотя, вроде понял, а зачем туда добавлять cls.get_name() его разве там не будет в запросе?
Потому что я хочу чтобы там удаляемый метод даже если он не loginable, а такое возможно
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Типа ситуация, когда это последний метод и он не логинейбл? Ну со стороны пользователя мы такое никогда не увидим, так как доступа к удалению последнего логинейбл нет
Тут просто тогда может возникнуть проблема, когда это будет логинейбл, и тогда длина списка будет на 1 больше, и, возможно, получится удалить. Я так вижу
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Теоретически ты прав я думаю да, не подумал об этом, сейчас потестим
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Типа ситуация, когда это последний метод и он не логинейбл? Ну со стороны пользователя мы такое никогда не увидим, так как доступа к удалению последнего логинейбл нет Тут просто тогда может возникнуть проблема, когда это будет логинейбл, и тогда длина списка будет на 1 больше, и, возможно, получится удалить. Я так вижу
Ты был прав ничего не меняется, перемудрил)
auth_backend/auth_method/oauth.py
Outdated
@@ -74,13 +75,15 @@ async def _delete_auth_methods(cls, user: User, *, db_session) -> list[AuthMetho | |||
AuthMethod.query(session=db_session) | |||
.filter( | |||
AuthMethod.user_id == user.id, | |||
AuthMethod.auth_method == cls.get_name(), | |||
AuthMethod.auth_method.in_([method.get_name() for method in AUTH_METHODS.values() if method.loginable]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
так зачем ты старую реализацию поменял, теперь нельзя будет удалить не логинейбл
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
так, давай еще раз разберемся
допустим у меня 3 метода, из которых 1 логинейбл, а один не логинейбл
Я пытаюсь удалить логинейбл. Тогда нужно получить все логинейбл методы мои, увидеть там список из 1 объекта, а затем сравнить со списком, который просто прокидывается через поиск по cls.get_name() в данный момент.
|
@grigoriev-semyon я знаю что так можно делать, но не очень понимаю к чему этот тут |
@Temmmmmo я поменял вообще немного логику чека, да теперь мы делаем 2 query запроса, но зато код как по мне теперь намного более читаемый и прозрачный |
Вот так это делается, определить можно в абстрактном классе каком нибудь |
This comment has been minimized.
This comment has been minimized.
@Temmmmmo Я посмотрел, там уже есть такой класс, так что да нет смысла добавлять эту переменную в классы, перепишу сегодня |
2b3e9b0
to
88c296d
Compare
💩 Code linting failed, use |
Изменения
#192
Детали реализации
Check-List
black
иisort
для Back-End илиPrettier
для Front-End?