Skip to content
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

Feature/post video [Done] #20

Merged
merged 26 commits into from
Jan 4, 2021
Merged

Feature/post video [Done] #20

merged 26 commits into from
Jan 4, 2021

Conversation

hywook4
Copy link
Contributor

@hywook4 hywook4 commented Dec 20, 2020

video post 올리기, 수정하기, 삭제하기 기능 추가 - resolves #15
+
Error 시 response error code 추가(안된 것들에 대해서) - resolves #16

Tag를 넣는 기능은 아직 tag 방식이 구체적이지 않으므로 우선 제외했음.
API 들에 대한 Documentation 추가 예정.

resolves #5
resolves #16
resolves #19
resolves #24

@hywook4
Copy link
Contributor Author

hywook4 commented Dec 22, 2020

resolves #5
resolves #16

@hywook4
Copy link
Contributor Author

hywook4 commented Dec 23, 2020

resolves #19
resolves #24

@hywook4 hywook4 changed the title Feature/post video [WIP] Feature/post video Dec 29, 2020
@hywook4 hywook4 requested a review from zych1751 January 4, 2021 10:23
@hywook4 hywook4 changed the title Feature/post video Feature/post video [Reviewing] Jan 4, 2021
src/api/index.ts Outdated
Comment on lines 16 to 17
// TODO : apply middleware
router.use('/v1/post', post);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

middleware도 구현이 다 되었는데, validateToken, getGoogleAccessToken을 바로 등록해서 TODO를 해결해도 괜찮지 않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

간단하게 테스트하기 위해 잠시 빼두고 넣는것을 잊었네요.
수정하도록 하겠습니다.

src/api/v1/post.ts Show resolved Hide resolved
Comment on lines 54 to 56
// TODO : videoPost의 주인이 아닌 다른 userId로 요청이 오는 경우 - put과 delete에 대해서
// AS-IS : SQL query 자체에서 만족하는 조건의 row가 없으면 수정되는 row 없음
// TO-BE : 요청된 videoPost가 요청한 user의 post인지를 확인?
Copy link
Collaborator

@zych1751 zych1751 Jan 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AS-IS 로도 괜찮은 것 같고, Sequelize.destory가 Promise<number>을 반환하는데, 이게 지워진 row 개수라면 0일때 지워진게 없다고 응답을 내려주면 좋을 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이건 지워진 row의 갯수가 필요한걸까요?
사용하지 않는다면 반환할 필요는 없을 것 같습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

잘 작동한다면 상관은 없는데, 만약 요청을 잘못 보냈을 때와 정상적일때를 구별할 수 있으면 좋겠다 싶어서 말한거였습니다.
유저 입장에선 지우는 버튼을 누르고 지워졌다라고 response까지 내려와서 지워진 줄 알았는데, 지워지지 않고 다시 타임라인에 보인다면 어색할 수도 있어서요.
근데 굳이 필요없겠다 싶으면 그냥 주석만 지워도 좋을 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아아 주석쪽이 혼란을 줄 수도 있겠군요. 이건 새로운 video post랑 로직이 거의 완전 같은데, 새로들어온 파라미터들로 정보를 싹 바꿔주는거라서 문제 없을 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

video post의 id와 owner id가 일치하는 row가 없는 경우 error를 리턴하도록 추가하였습니다.

@@ -0,0 +1,229 @@
import tagService from '../service/tagService';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validateTags 는 service에 있는 함수인데, validatePostVideoParameters은 error으로 따로 파둔 레포에 있습니다.
일관성을 위해서 통일을 하는게 좋지 않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

말씀주신대로 tag validation 코드를 postErrors로 옮기고, 다른 조건 변수들도 const화 시키면 좋을 것 같습니다.

src/service/tagService.ts Outdated Show resolved Hide resolved
@hywook4 hywook4 changed the title Feature/post video [Reviewing] Feature/post video [Done] Jan 4, 2021
@hywook4 hywook4 merged commit 265c886 into master Jan 4, 2021
@hywook4 hywook4 deleted the feature/post-video branch January 18, 2021 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants