-
Notifications
You must be signed in to change notification settings - Fork 0
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
Go chi 버전 업데이트 및 돌봄급구 관련 API를 추가합니다. #22
Conversation
이거 배포 전에 마이그레이션 스크립트 만들고 PR 파서 그것부터 배포 나가야할 것 같아요~ https://github.com/pet-sitter/pets-next-door-api/blob/f8f6466/cmd/import_breeds/main.go 이거 참고해주세요. |
게시글의 타임존이 있을텐데 타임존은 어느 걸로 참고할건가요? |
return | ||
} | ||
|
||
res, err := h.sosPostService.UploadSosPost(uid, &uploadSosPostRequest) |
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.
nitpicking이긴 하지만 upload보단 create, write 같은게 어떨까요?
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.
저도 지금 생각해보니 upload 표현은 파일 업로드에 좀 더 가깝다고 생각이 드네요 write으로 변경하겠습니다.
// findSosPostsByAuthorID godoc | ||
// @Summary 작성자 ID로 돌봄급구 게시글을 조회합니다. | ||
// @Description | ||
// @Tags posts | ||
// @Accept json | ||
// @Produce json | ||
// @Param page query int false "페이지 번호" default(1) | ||
// @Param size query int false "페이지 사이즈" default(20) | ||
// @Security FirebaseAuth | ||
// @Success 200 {object} commonviews.PaginatedView[sos_post.FindSosPostResponse] | ||
// @Router /posts/sos/author [get] | ||
func (h *SosPostHandler) FindSosPostsByAuthorID(w http.ResponseWriter, r *http.Request) { |
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.
이런 검색은 기존 목록 조회 API에 쿼리로 필터링을 하면 좋을 것 같아요
e.g., /posts/sos?author_id={author_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.
사용자의 ID(fid_uid) 값을 파라미터에 담기에 보안적으로 문제가 있을 듯하여 현재 로그인한 사용자의 ID(fid_uid)를 가지고 조회하도록 구현하였습니다. 그래서 /posts/sos?author_id={author_id}
author_id에 Boolean 타입으로 필터링을 하거나 기존 그대로 하는 건 어떻게 생각하시나요?
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.
네, boolean(written_by_me
?)으로 해도 괜찮긴 한데 그러면 다른 사람이 작성한 글을 필터링 할 수가 없어서, 그냥 int pk(author_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.
현재 구현된 API에서는 DB author_id
칼럼에 fid_uid
가 들어가고 있는데 User 테이블의 PK로 조회하도록 변경하면 될까요?
internal/domain/sos_post/service.go
Outdated
mediaView = append(mediaView, view) | ||
} | ||
|
||
conditions, err := service.sosPostStore.FindConditionByID(sosPost.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.
본인이 작성한 글이 아니라면 확인이 필요할 것 같아요! 403 내려주면 될 것 같습니다.
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.
오 이 부분까진 고려못했는데 반영하도록 하겠습니다 :)
혹시 다른 API에서 사용하고 있는 타임존이 있을까요?? |
한국 서비스라 한국을 쓰긴 하지만 다양한 케이스를 열어두면 좋을 것 같아요. 만약 이렇게 하신다면 request body schema가 바뀌고 DB는 유지될 것 같네요 e.g., |
리베이스해서 충돌을 해결해주면 좋을 것 같아요! |
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.
코멘트 남긴 것 확인 부탁드려요! 거의 다 온 것 같습니다 👍
// @Success 200 {object} commonviews.PaginatedView[sos_post.FindSosPostResponse] | ||
// @Router /posts/sos [get] | ||
func (h *SosPostHandler) FindSosPosts(w http.ResponseWriter, r *http.Request) { | ||
authorIDQuery := r.URL.Query().Get("authorID") |
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.
변수명은 authorID가 맞지만 쿼리는 컨벤션에 따라 author_id
가 좋겠네요. Swagger도 수정되어야 할 것 같아요
DateStartAt string `field:"date_start_at"` | ||
DateEndAt string `field:"date_end_at"` | ||
TimeStartAt time.Time `field:"time_start_at"` | ||
TimeEndAt time.Time `field:"time_end_at"` |
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.
model은 DateXAt이 string이고 TimeXAt이 time.Time인데 Request는 반대인데 의도한거겠죠?
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.
DB에서 TimeXAt 칼럼을 string 타입으로 가져온다고 해도 22:00
이 아닌 0000-01-01T20:00:00Z
로 반환됩니다. 그래서 22:00
형태로 변환하기 위해 model에서는 TimeXAt을 time.Time 받고 service 단(datetimeToTime
)에서 형 변환을 진행했습니다.
추가로 Request의 경우, DateXAt는 날짜, 시간을 비롯한 타임존까지 받아야 하기 때문에 time.Time 타입으로 정의하였습니다. 반대로 TimeXAt는 시간만 받으면 되기에 string 타입으로 정의하였습니다.
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.
코드 레벨에서 형변환 안하고 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.
쿼리 레벨에서 할 생각은 안해봤었네요 한번 찾아보겠습니다. 장소 추가할때 함께 리팩토링 진행하면 되겠네요 👍🏻
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.
근데 리베이스를 했어야 하는데 머지를 한 것 같네요...ㅋㅋㅋ
} | ||
|
||
func datetimeToTime(datetime time.Time) string { | ||
return datetime.Format("15:04") |
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.
근데 15:04는 어떤 뜻인가요?
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.
아 상수값에 없어서 "15:04"
으로 한거군요.
리베이스로 충돌난 건 해결했었는데 추가로 작업한 내용을 커밋하다가 fetch + merge가 되었나봐요 ... 담부턴 주의하도록 하겠습니다 |
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.
고생하셨어요~ 리베이스 머지는 안될 것 같고 머지 후 배포 가보시죠!
} | ||
|
||
func datetimeToTime(datetime time.Time) string { | ||
return datetime.Format("15:04") |
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.
아 상수값에 없어서 "15:04"
으로 한거군요.
감사합니다 ~ 코드리뷰하시느라 수고 많으셨어요 🙇🏻♂️ |
작업 내용
Go chi 버전 업데이트
기존 버전
v1.5.4
->v5.0.10
따로 마이그레이션없이 버전만 업데이트 해도 문제 없음을 확인하였습니다. [참고자료] (go-chi/chi@v1.5.4...v5.0.0)
돌봄급구 API
테스트 사전 작업
sos_conditions
돌봄급구 조건을 미리 등록해두어야 합니다./post/sos
[POST]request
전체 게시글 조회 API
/post/sos?page={page}&size={size}&sort_by={sort}
[GET]sort_by
는 newest, deadline 조건이 있으며 각각 최신순, 마감일순을 의미합니다.작성자 ID로 게시글 조회 API
/post/sos/author
[GET]게시글 ID로 게시글 조회 API
/post/sos/{id}
[GET]게시글 수정 API
/post/sos
[PUT]request
스웨거까지 작업완료했습니다.