Loaders for auth #415
Loaders for auth #415
Conversation
Pull Request Test Coverage Report for Build 2858
💛 - Coveralls |
oleksiyVeretiuk
force-pushed
the
loaders_for_auth
branch
from
ab4a7d5
to
89f5f0b
Compare
src/openprocurement/api/auth.py
Outdated
| 'level': ( | ||
| value.split(',', 1)[1] if | ||
| ',' in value else | ||
| ALL_ACCREDITATIONS_GRANTED |
There was a problem hiding this comment.
Дуже незрозумілий код. Чи не можна його винести в окрему функцію і трохи більше прокоментувати? Бо мені геть незрозуміло що відбувається і навіщо.
Подальший код цієї функції також незрозумілий.
| create_users_from_group, | ||
| _ini_auth, | ||
| _yaml_auth, | ||
| _json_auth |
There was a problem hiding this comment.
Underscore на початку назви означає приватність. Тому в імпорті воно дивиться дуже погано. Рекомендую просто прибрати underscore з назви тих функцій, бо не бачу в них потребу щось приховувати.
There was a problem hiding this comment.
В перший раз не помітив виправлю в наступних комітах
| self.assertEqual(self.mock_get_path.call_count, 1) | ||
| self.mock_get_path.assert_called_with(self.app_meta) | ||
|
|
||
| self.assertEqual(self.mock_config_parser_class.call_count, 1) |
There was a problem hiding this comment.
Тут і далі в тестах бачу багато перевірок на кількість викликів та взагалі фактів викликів.
Яку практичну користь вони переслідують?
Рекомендую перевірити себе так:
У всіх assert-методах unittest-у останнім параметром можна передати стрічку, яка буде виведена при неуспішності умови.
Яке повідомлення можна було б виводити при неуспішності твердження на кількість виклику методу? Наскільки розумію, шось типу: той метод мав викликатись і відпрацювати 1 раз, але не відпрацював жодного. Як на мене, в цьому мало практичної користі. Тому раджу ті assert-и, до яких не можна придумати змістовні пояснення, просто видалити. Бо при подальших змінах в коді перевірки на кількість викликів падають, і незрозуміло чому і навіщо вони були зроблені.
Якщо ж вони там важливі - додай комент, будь ласка, а ще краще - допиши їм такі повідомлення про неуспішність. Тоді все набагато зрозумілішим стає.
🎩
There was a problem hiding this comment.
Це потрібно по великому рахунку, щоб перевірити коректність роботи скрипта відносно його flow. Тут використовуються моки, отже певний функціонал емулюється в цьому випадку нам не важливо як відпрацює та функція нам є важливим те, що вона була викликана і повернула певний результат від якого може залежити виклик інакшої функції і.т.д. Стосовно коментарів ти маєш на увазі під кожен assert проставити внаслідок чого він викликаєтсья?
There was a problem hiding this comment.
Так, щоб було якесь пояснення його причини.
…penprocurement.api into loaders_for_auth
| create_users_from_group, | ||
| _ini_auth, | ||
| _yaml_auth, | ||
| _json_auth |
| self.assertEqual(self.mock_create_users.call_count, 2) | ||
| self.mock_create_users.assert_called_with(config_sections[-1], config_items) | ||
| self.assertEqual(self.mock_create_users_ini.call_count, 2) | ||
| self.mock_create_users_ini.assert_called_with(config_sections[-1], config_items) |
There was a problem hiding this comment.
Можна якісь коменти або еррор меседжі по цих асертах? Бо незрозуміло що вони тестують.
oleksiyVeretiuk
force-pushed
the
loaders_for_auth
branch
from
f6139fc
to
321d2af
Compare
src/openprocurement/api/auth.py
Outdated
| if not config: | ||
| LOGGER.warning("Auth file '%s' was empty, no user will be added", | ||
| file_path) | ||
| for item in config: |
There was a problem hiding this comment.
Тут іменування item i config потрібно змінити на доцільніші.
src/openprocurement/api/auth.py
Outdated
| LOGGER.warning("Auth file '%s' was empty, no user will be added", | ||
| file_path) | ||
| for item in config: | ||
| users.update(create_users_from_group(item, config[item].items())) |
There was a problem hiding this comment.
config[item].items() треба "впихнути" в якусь зрозумілу змінну, що ж ми тут витягнули.
Cаму змінну users вартувало б тут присвоїти, а не просто апдейтнути.
There was a problem hiding this comment.
users не вийде переприсвоїти, адже функція create_users_from_group повертає словник з користувачами для певної групи і єдиний спосіб то внести в users це оновити через update
There was a problem hiding this comment.
Та, ти правий. Давай перенесемо визначення users перед цим for-ом. а значення create_users_from_group присвоюватимемо у читабельну проміжну змінну.
src/openprocurement/api/auth.py
Outdated
|
|
||
|
|
||
| # Backward compatibility for `file` type | ||
| auth_mapping['file'] = ini_auth |
There was a problem hiding this comment.
Явне навішування @auth(auth_type="ini") на ini_auth не дасть аналогічного ефекту?
There was a problem hiding this comment.
Хочеш зробити декоратор на декоруєму функцію?
This change is