-
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
Subcribe #41
base: main
Are you sure you want to change the base?
Conversation
"INSERT INTO follows(folowee_id,folower_id) VALUES($1,$2)", | ||
user_id,user_authorized_id); | ||
|
||
return "Subcribed"; |
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.
Не уверен, что можно просто текст возвращать. В пост запросе сделать пост нужно было что-то осмысленное возвращать (json), думаю, что тут тоже.
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.
Кажется здесь лучше вообще ничего не возращать, код 200 о всем говорит
@@ -1,3 +1,4 @@ | |||
#include <api/v1/users/subscribe/subscribe.hpp> |
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.
Эту строчку убираем, повтор 17 строчки
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.
Нет проверки подписки на несуществующего пользователя (корректный uuid, которого нет в бд юзерс)
|
||
from utils.error_strings import * | ||
|
||
# No test for absent id |
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.
либо делай, либо убирай комментарий
не забывай про make format!!! И у тебя тесты не проходят из-за стиля питоновского кода |
bool Is_user_exists(const boost::uuids::uuid& user_id,const userver::storages::postgres::ClusterPtr& pg_cluster_) { | ||
auto user_exists = pg_cluster_->Execute(userver::storages::postgres::ClusterHostType::kMaster, | ||
"SELECT name FROM users WHERE id = $1", user_id); | ||
return !user_exists.IsEmpty(); | ||
} |
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.
Что это за стиль названия функции? Все функции до этого момента мы вроде в CamelCase называли
if(user_authorized_id == user_id) { | ||
throw errors::ValidationException("user","Can not subcribe to yourself"); | ||
} |
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.
Проблема не с валидацией, это логическая ошибка, тут другое должно быть
if(!Is_user_exists(user_authorized_id,pg_cluster_)) { | ||
throw errors::NotFoundException("user", "User with this id not found"); | ||
} | ||
if(!Is_user_exists(user_id,pg_cluster_)) { | ||
throw errors::NotFoundException("user", "User with this id not found"); | ||
} |
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.
pg_cluster_ лучше первым аргументом
"INSERT INTO follows(folowee_id,folower_id) VALUES($1,$2)", | ||
user_id,user_authorized_id); | ||
|
||
return "Subcribed"; |
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.
Кажется здесь лучше вообще ничего не возращать, код 200 о всем говорит
Fixed one bug from Timur
Added new handler - subscribe
Added tests to it