Pending 이슈 해결 #180
Merged
Pending 이슈 해결 #180
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
yesjjin99
commented
Jun 11, 2022
•
yesjjin99
commented
- 작업하면서 확인되는 Cannot set headers after they are sent to the client 에러나 비로그인 상태일 때 응답이 아예 안오는 케이스들 수정했습니다
- 알림 생성 시 알림을 보낼 USER_ID를 받아올 때 동작 안되는 부분들이 발견되어 모두 수정했습니다
- 스터디 디테일, 수정, 삭제, 마감하기에 403 에러 처리하는 부분을 추가해놓은 상태입니다. 이 부분도 프론트 대응이 필요하지 않을까 싶어서 머지 되는대로 이슈 올리겠습니다
Jeyoung-Park
approved these changes
Jun 12, 2022
src/routes/study/study.controller.ts
Outdated
| title: '모집 종료', | ||
| about: `모집이 종료되었어요. 스터디를 응원합니다!`, | ||
| about: `모집이 종료되었어요. ${member.USER_ID}님의 스터디를 응원합니다!`, |
There was a problem hiding this comment.
이거 userid값이면 uuid값 아닌가요?
There was a problem hiding this comment.
앗 USER_NAME 이 맞습니다ㅋㅋㅋㅋㅋ 수정했습니다 🙏
| if (req.body.dueDate) { | ||
| const due = new Date(req.body.dueDate); | ||
| const now = new Date(); | ||
|
|
||
| if (due.toISOString().split('T')[0] < now.toISOString().split('T')[0]) { | ||
| if (due.toISOString().split('T')[0] < now.toISOString().split('T')[0]) |
There was a problem hiding this comment.
date-fns나 dayjs 같은 날짜 관련 라이브러리 사용하는 것도 좋아 보입니다!
중요도 높은 건 아니라 다음 번에 고려해주셔도 될 듯 합니다 :)
src/routes/study/study.controller.ts
Outdated
|
|
||
| await studyService.updateStudy( | ||
| { | ||
| title: req.body.title ? req.body.title : study.title, |
There was a problem hiding this comment.
??나 || 연산자로도 대체될 수 있을 것 같아요!
rudy3091
reviewed
Jun 12, 2022
Comment on lines
+181
to
+186
| return await getRepository(Study) | ||
| .createQueryBuilder() | ||
| .update() | ||
| .set(studyDTO) | ||
| .where('id = :studyId', { studyId }) | ||
| .execute(); |
There was a problem hiding this comment.
이부분은 이전에 프론트에서 수정할 파라미터만 전달하면 반영될 수 있도록 요청해서 구현된걸로 아는데 프론트에서도 이 변경이 반영되기 전까지 이 버전이 반영되면 안되지 않을까요..?
There was a problem hiding this comment.
컨트롤러에서 전달된 파라미터들은 해당 값으로, 전달되지 않은 값들은 기존 스터디의 값으로 대체해서 업데이트해주기 때문에 테스트해본 결과 몇 가지 파라미터만 수정하도록 요청한 경우에도 이상 없이 동작하는 것을 확인했습니다!
Comment on lines
+27
to
+32
| export const findAllStudyUserByStudyId = async (studyId: string) => { | ||
| return await getRepository(StudyUser) | ||
| .createQueryBuilder() | ||
| .select() | ||
| .where('STUDY_ID = :id', { id: studyId }) | ||
| .execute(); | ||
| .createQueryBuilder('studyUser') | ||
| .select('studyUser.USER_ID') | ||
| .where('studyUser.STUDY_ID = :studyId', { studyId }) | ||
| .getMany(); |
There was a problem hiding this comment.
혹시 이렇게 이름이랑 로직을 수정하면 영향받는 곳은 없나요??
기존에 사용되던 곳이 있으면 그 부분도 수정해주고 정상동작하는지 확인이 먼저 필요할 것 같습니다
There was a problem hiding this comment.
기존에 위 로직이 사용되던 부분들은 모두 수정해놓은 상태입니다!
Comment on lines
+439
to
+457
| const study = await getRepository(Study) | ||
| .createQueryBuilder('study') | ||
| .addSelect('study.dueDate') | ||
| .leftJoinAndSelect('study.hostId', 'UserProfile') | ||
| .where('study.id = :id', { id: studyid }) | ||
| .getOne(); | ||
|
|
||
| expect(study?.title).toEqual('STUDY TITLE'); | ||
| expect(study?.studyAbout).toEqual('STUDY ABOUT'); | ||
| expect(study?.weekday).toEqual( | ||
| expect.arrayContaining([WeekDayEnum.WED, WeekDayEnum.THU]) | ||
| ); | ||
| expect(study?.frequency).toEqual(FrequencyEnum.MORE); | ||
| expect(study?.location).toEqual( | ||
| expect.arrayContaining([LocationEnum.LIBRARY, LocationEnum.NO_CONTACT]) | ||
| ); | ||
|
|
||
| expect(study?.capacity).toEqual(8); | ||
| expect(study?.categoryCode).toEqual(101); |
yesjjin99
force-pushed
the
hotfix/study-detail
branch
from
57f9df4 to
6b4d66c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.