-
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
DEAD-15: implement handlers to get and change user's data #6
base: DEAD-10
Are you sure you want to change the base?
Conversation
v1.HandleFunc("/users/{userID:[0-9]+}", hV1.GetUserInfo).Methods(http.MethodGet) | ||
v1.HandleFunc("/users/{userID:[0-9]+}", hV1.UpdateUserInfo).Methods(http.MethodPut) | ||
|
||
v1.HandleFunc("/users/{userID:[0-9]+}/changepassword", hV1.UpdatePassword).Methods(http.MethodPut) |
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.
Давай слова отделять четко:
v1.HandleFunc("/users/{userID:[0-9]+}/changepassword", hV1.UpdatePassword).Methods(http.MethodPut) | |
v1.HandleFunc("/users/{userID:[0-9]+}/changePassword", hV1.UpdatePassword).Methods(http.MethodPut) |
v1.HandleFunc("/users/{authorID:[0-9]+}/articles", hV1.UserArticles).Methods(http.MethodGet) | ||
|
||
v1.HandleFunc("/users/{userID:[0-9]+}", hV1.GetUserInfo).Methods(http.MethodGet) | ||
v1.HandleFunc("/users/{userID:[0-9]+}", hV1.UpdateUserInfo).Methods(http.MethodPut) | ||
|
||
v1.HandleFunc("/users/{userID:[0-9]+}/changepassword", hV1.UpdatePassword).Methods(http.MethodPut) |
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.
Раз уж нам все равно никуда не деться и надо в будущем userID кастить от строки к инту, давай тогда не будем задавать регуляркой валидацию в роутах. В текущем варианте если случайно в айдишнике 1 символ цифры подменить какой нибудь буквой, нам будет возвращаться 404 not found, так как роутер ручки такой не найдет вообще. Наверное правильнее ему отдавать 400 и сообщать что userID должен быть числом
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.
и давай кстати везде сделаем userID для единообразия, а то в одной ручке authorID
userIDInt, err := strconv.Atoi(userIDStr) | ||
|
||
if err != nil { | ||
utils.ProcessBadRequestError(h.log, w, err) | ||
return | ||
} | ||
|
||
userID := domain.UserID(userIDInt) | ||
|
||
articles, err := h.UC.Article.GetUserArticles(r.Context(), userID) | ||
|
||
if err != nil { | ||
utils.ProcessInternalServerError(h.log, w, err) | ||
return | ||
} |
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.
давай ошибки обрабатывать без пустых строк между возвращенной err и началом обработки, это все таки 1 логический блок
|
||
userID := utils.GetCtxUserID(r.Context()) | ||
if userID == 0 { | ||
utils.SendError(h.log, w, resterr.NewUnauthorizedError("unauthorized, please login")) |
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.
текст ошибки можно в константу вынести, ниже он точно такой же отправляется
|
||
userIDInput := domain.UserID(userIDInt) | ||
if userID != userIDInput { | ||
utils.SendError(h.log, w, resterr.NewForbiddenError("no rights to change this user")) |
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.
и этот
|
||
type UserInputRegister struct { | ||
Email string `json:"email" validate:"required,email"` | ||
Password string `json:"password" validate:"required,regexp=/^[a-zA-Z0-9?!_\\-*$]{6,255}$/"` |
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.
у вас на фронте кстати совпадает регулярка для пароля?
SubscribersNum int `json:"num-subscribers" validate:"required,gte=0"` | ||
SubscriptionsNum int `json:"num-subscriptions" validate:"required,gte=0"` |
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.
Кстати, судя по всему именно к этому рк это и делать надо будет. Я уточню этот момент
@@ -22,7 +22,7 @@ func NewRepository(adapter *adapters.AdapterPG) *Repository { | |||
} | |||
|
|||
func (r *Repository) ListArticles(ctx context.Context) ([]*domain.Article, error) { | |||
q := `SELECT (id, title, media_url, body) FROM feed.article` | |||
q := `SELECT (id, title, media_url, body, author_id) FROM article` |
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.
Надо кстати будет поставить на кого то таск на доработку получения списка статей, потому что там бы надо не просто юзер айди получать, а его имя фамилию для отрисовки ленты
|
||
rows, err := r.PG.Query(ctx, q, authorID) | ||
if err != nil { | ||
return nil, interr.NewNotFoundError("article Repository.GetUserArticles pg.Query") |
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.
а почему нот фаунд еррор? тут же обработать бы надо ошибку в зависимости от того, что база вернула
|
||
err := rows.Scan(&a) | ||
if err != nil { | ||
return nil, interr.NewNotFoundError("article Repository.GetUserArticles rows.Scan") |
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 err := rows.Err(); err != nil { | ||
return nil, interr.NewNotFoundError("article Repository.GetUserArticles rows.Err") |
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.
ну и тоже, аналогично тому, что я Ване писал, если не вернулось строк на запрос списка обычно на такое кидают пустой список + 200 статус, а не ошибку, потому что это нормальное поведение системы, просто у какого то юзера нет статей, соответственно фронт просто ничего не отрисует.
ALTER TABLE account ADD COLUMN num_subscribers INTEGER NOT NULL DEFAULT 0, | ||
ADD COLUMN num_subscriptions INTEGER NOT NULL DEFAULT 0, |
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.
в будущем надо будет либо убрать эти поля и считать подписчиков через связи таблиц, либо четко следить за консистентностью этого поля, если его так оставить в этой таблице, денормализовав таблицу
@@ -0,0 +1,10 @@ | |||
-- +goose Up | |||
-- +goose StatementBegin | |||
ALTER TABLE article ADD COLUMN author_id INTEGER NOT NULL DEFAULT 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.
а почему мы по дефолту все статьи присваиваем автору с айди 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.
вообще на самом деле из этого вопроса вытекает то, что как раз я и так хотел написать, что задавать дефолтные значения в базе не очень хорошая практика. По сути это костыль, с помощью которого мы перекладываем с себя на базу ответственность за то, что это поле будет not_null. Заполнение всех полей хорошо бы контролировать на уровне приложения
FirstName string `json:"first-name" validate:"required, max=50"` | ||
LastName string `json:"last-name" validate:"required, max=50"` |
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.
Хорошо бы как то более строго валидировать имя фамилию, чтобы нельзя было в них что попало записывать
err = h.UC.User.UpdatePassword(r.Context(), updateData, userID) | ||
|
||
if err != nil { | ||
utils.ProcessInternalServerError(h.log, w, err) | ||
return | ||
} |
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.
а почему мы тут всегда кидаем интернал? У нас же снизу может прийти нот фаунд например
info, err := h.UC.User.GetUserInfo(r.Context(), userID) | ||
|
||
if err != nil { | ||
utils.ProcessInternalServerError(h.log, w, err) | ||
return | ||
} |
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.
Аналогично тут тоже не всегда должно быть 500. проверь пожалуйста все такие моменты по всем своим ручкам
b4bda2f
to
870e1ea
Compare
No description provided.