-
Notifications
You must be signed in to change notification settings - Fork 6
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
[feat] users API 구현 #52
[feat] users API 구현 #52
Conversation
Co-authored-by: yaongmeow <minnie_00@naver.com>
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.
수고하셨습니다~!!
BE/src/users/users.module.ts
Outdated
|
||
@Module({ | ||
imports: [TypeOrmModule.forFeature([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.
저는 imports 부분에 DatabaseModule을 넣었는데, 이 코드는 무슨 역할인가요?
|
||
@Module({ | ||
imports: [TypeOrmModule.forFeature([User])], | ||
imports: [DatabaseModule], |
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.
아하 바꾸셨군요 !!
{ | ||
provide: 'USER_REPOSITORY', | ||
useFactory: (dataSource: DataSource) => dataSource.getRepository(User), | ||
inject: ['DATA_SOURCE'], |
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.
저는 Posting할 때 postings.constants.ts
파일에 'USER_REPOSITORY'랑 'DATA_SOURCE' 같은 스트링을 상수로 따로 관리했어요!
그리고 저는 복수형 레포지터리(USES_REPOSITORY)라고 이름을 지었는데, 이 부분을 서로 얘기하고 통일해야 할 것 같네요!
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.
복수형은,, 음 어떻게 하는게 좋죠? 사실 아침에 말씀드린 것 처럼 저는 그냥 따라서 작성한 거라 ㅋㅋㅋㅋ 별 의미 없이 쓰긴 했습니다..ㅎㅎ..
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.
저는 왜 복수로 썼었냐면... nest generate 명령어로 생성한 파일들이 UsersController, UsersService처럼 복수형으로 썼길래 저도... 큰 의미 없이 통일감을 위해 UsersRepository로 쓰긴 했어요 ㅎㅎㅋㅋㅋㅋ..
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.
좋습니다!! 😎
updateUserDto: UpdateUserDto, | ||
file: Express.Multer.File | ||
) { | ||
if (file) { |
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를 처리하기 전에, 수정하려는 사용자의 id에 해당하는 데이터가 있는지 먼저 조회해보고나서 업로드를 처리하는 건 어떨까요? 조회했는데 회원이 없으면 그때 바로 BadRequest 날려주고요!
사진 업로드까지 완료했는데, 수정하려는 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.
아 회원의 존재 여부를 먼저 확인하자는 말씀이신걸까용??
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.
네네!
const user = findOne();
if (!user) {
throw new BadRequestException
}
파일 업로드
user update
순으로 한다면, 만약 id에 해당하는 회원이 없을 때 곧 바로 에러를 던져주니까 불필요한 파일 업로드 작업을 건너뛸 수도 있고, update 결과를 가지고 affect된 컬럼이 존재하는지 아닌지를 확인하지 않아도 되니까요!
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.
오~ 그렇네요!! 이것도 같이 반영하겠습니다 ㅎㅎ
async findById(id: string): Promise<User> { | ||
const user = await this.userRepository.findById(id); | ||
if (!user) { | ||
throw new HttpException('Not Found', 404); |
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.
HttpException()을 사용하신 이유가 있나요?
기본적인 http 에러 응답은 Nest에서 이미 제공을 해주고 있어요!
HttpException('Not Found', 404) => NotFoundException('id에 해당하는 회원이 없습니다')
HttpException('Bad Request', 400) => BadRequestException('따로 언급하실 에러 메시지')
이렇게 작성하셔도 됩니다!
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.
수정할 때 반영할게요!!
|
||
@Get('duplicate') | ||
@Get('duplicate/check') |
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.
아 정민님 이거 PR 내용에서도 봤는데요,
https://spiky-rat-16e.notion.site/30cae4064f014eb99b291aa6c9cbdbc7?pvs=4
제가 저희 노션에 이 부분 추가했으니까 한 번 참고해주시면 좋을 것 같아요 ㅎㅎ
결론부터 말씀드리면 users/duplicate
핸들러가 users/:id
핸들러보다 먼저 나오도록 코드 위치를 수정하면 users/duplicate/check
로 경로 변경 안 해도 돼요!
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.
오!! 수정해서 반영할게요 👍👍👍
🌎 PR 요약
🌱 작업한 브랜치
📚 작업한 내용
users/:id
: 사용자의 프로필 사진과 닉네임 정보를 가져온다.users/:id
: 사용자의 프로필 사진 또는 닉네임을 수정한다.users/duplicate/check
: 닉네임 변경 전에 중복 여부를 검사한다.📍 참고 사항
users/duplicate
였으나 GETusers/:id
와 충돌하는 문제가 있어 (duplicate를 id값의 일종으로 인식해버림) 임시로 GETusers/duplicate/check
로 수정해두었습니다! 혹시 더 괜찮은 URI가 있다면 리뷰로 남겨주세요 😄Storage Module
관련해서 아침에 말씀드린 건 다른 pr로 따로 뺄까 싶어서 이 pr에는 커밋 안했어요!관련 이슈