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.

Sign up for GitHub

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

Jump to bottom

Feat: nickname to intraid #125

Merged
merged 20 commits into from
Mar 11, 2022
Merged

Feat: nickname to intraid #125

merged 20 commits into from
Mar 11, 2022

Conversation

rockpell
Copy link
Member

@rockpell rockpell commented Feb 23, 2022 •
edited
Loading

바뀐점

  • 유저 닉네임이 자동으로 인트라 아이디가 들어가도록 수정
  • ft-auth entitiy를 intra-auth로 변경
  • 닉네임, intra-auth entity 마이그레이션 코드 추가
  • intra 인증 메일 전송 test code 추가

바꾼이유

  • nickname을 인트라 아이디로 사용 하도록 하는 요구사항이 많아서 반영
  • ft-auth는 이름이 비직관적이어서 intra-auth로 수정
    • ft-auth를 삭제하고 user에 합치려는 시도도 하였으나 intra 정보를 추가로 더 저장할 계획도 있기 때문에 이름만 변경하였음

rockpell and others added 6 commits February 9, 2022 23:56
@rockpell
Refactor: user entitiy에 githubUid, githubUsername, ftIntraId 추가
3349830 
- user 엔티티에 깃헙 관련 아이디 컬럼 이름 수정
- intra id 를 저장하는 컬럼 추가
@blingblin-g
!BREAKING CHANGE: remove ft-auth
fc5c155 
- ft-auth entity 삭제
- ft-auth를 intra-auth로 바꿈
- 이에 따른 api url 변경
@rockpell
Merge branch 'develop' into feature/nickname-to-intraid
784461d
@rockpell
Feat: 삭제됐던 ft_auth를 intra_auth로 부활
9143615 
- user의 intraId를 intraAuth로 이동
- intra 인증 하면 nickname이 인트라 id로 바뀌도록 수정
@rockpell
Merge branch 'develop' into feature/nickname-to-intraid
e907d60 
# Conflicts:
#	src/intra-auth/intra-auth.controller.ts
#	test/jest-e2e.json
@rockpell
Test: user 엔티티에 githubUserName이 추가됨에 따라 dummy 수정
d7d5225 
- user 엔티티에 githubUserName이 추가되어 user를 생성하는 dummy에도 반영하였음
@mimseong
Copy link
Contributor

오우 락펠 별건 아니지만 PR 제목 수정해야해요

mimseong
mimseong approved these changes Feb 27, 2022
View reviewed changes
Copy link
Contributor

@mimseong mimseong left a comment

Choose a reason for hiding this comment

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

수고하셨습니다👍

src/database/migrations/1645622620898-intra-auth.ts Show resolved Hide resolved
src/intra-auth/intra-auth.controller.ts Outdated Show resolved Hide resolved
@rockpell
Copy link
Member Author

오우 락펠 별건 아니지만 PR 제목 수정해야해요

맞아요 이거 아직 작업이 완료가 안되서 나중에 다시 리뷰 요청드리겠습니다

@rockpell
Fix: intra-auth 컨트롤러, 서비스 변수 이름 변경
4d49276 
- ftAuth에서 intraAuth로 전환하였는데 변수 이름은 반영이 안되어 있어서 수정
@rockpell rockpell changed the title Feature/nickname to intraid Feat: nickname to intraid Feb 27, 2022
@rockpell
Feat: IntraAuth에 Cache 모듈 적용
accd475 
- 기존에는 모듈이 아닌 cacheManager를 inject해서 사용하였는데 좀 더 명확히 분리하고 테스트를 편하게 하기 위해 별도의 모듈로 분리하였음
@rockpell
Feat: intra-auth mail 보내는 함수 예외처리
46ebca5 
- node mailer에서 발생하는 에러를 nest가 제대로 처리하지 못해서 nest가 정의한 에러로 변환
@rockpell
Test: intra-auth 테스트, 메일 전송 관련
81120b9 
- 메일 전송하는 api만 테스트코드 추가
@rockpell
Refactor: jwt auth guard 에러 메시지 상수로 분리
3a18791
@rockpell
Test: image test 500에러 글로벌 필터 적용
7022e02
@rockpell rockpell changed the title Feat: nickname to intraid Draft - Feat: nickname to intraid Feb 27, 2022
@rockpell rockpell changed the title Draft - Feat: nickname to intraid Feat: nickname to intraid Feb 27, 2022
mimseong
mimseong reviewed Mar 2, 2022
View reviewed changes
src/cache/cache.service.ts Outdated Show resolved Hide resolved
@mimseong
Copy link
Contributor

mimseong commented Mar 2, 2022

다 확인했습니다! 수고하셨습니다

Skyrich2000
Skyrich2000 reviewed Mar 2, 2022
View reviewed changes
Copy link
Member

@Skyrich2000 Skyrich2000 left a comment

Choose a reason for hiding this comment

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

정말 어려운 PR인데 잘 해결하신거같습니다. 사소한 부분만 수정하면 머지해도 될거같아요 ㅎㅎ

src/cache/cache.service.ts Outdated Show resolved Hide resolved
src/database/migrations/1645622620898-intra-auth.ts Show resolved Hide resolved
src/intra-auth/intra-auth.controller.ts Outdated Show resolved Hide resolved
src/intra-auth/constant.ts Outdated Show resolved Hide resolved
src/intra-auth/intra-auth.service.ts Outdated Show resolved Hide resolved
src/intra-auth/intra-auth.service.ts Outdated Show resolved Hide resolved
src/user/dto/update-user.dto.ts Outdated Show resolved Hide resolved
@rockpell
Fix: UpdateIntraAuthDto -> UpdateToCadetDto로 변경
c6d34c1 
- intra auth 테이블을 수정할거 같은 이름이라서 좀 더 용도에 맞게 변경
@rockpell
Refactor: intra-auth service 중복 코드 개선, 이름변경 dto 적용, 리턴타입, 에러메시지 상수화
b214f06 
- 이미 가입된 카뎃인지 검사하는 코드가 중복되서 private 함수로 분리
- 리턴 타입 추가
- 에러메시지 상수 적용
@rockpell
Feat: cache service에 범용적으로 쓸수 있는 함수 추가
39e12c1 
- 범용적인 함수를 추가했을때 기존에 CacheManager를 inject해서 쓰는것보다는 좀 더 명시적이고 mocking하기 쉬운것 같아 쓰는게 좋아보이긴 함
- 아직 기존에 사용하던 함수를 대체할지는 모르겠어서 함수 추가만 하였음
- option 기본값 같은거는 해주는게 편하기 때문에 기본 설정이 가능하다면 범용 함수 사용해도 좋을것으로 보임
Skyrich2000
Skyrich2000 approved these changes Mar 5, 2022
View reviewed changes
Copy link
Member

@Skyrich2000 Skyrich2000 left a comment

Choose a reason for hiding this comment

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

완벽합니다.

src/user/user.service.ts Outdated Show resolved Hide resolved
@Skyrich2000
Copy link
Member

Skyrich2000 commented Mar 5, 2022 •
edited
Loading

지금은 가입시 기본 닉네임을 그냥 인트라로 하였는데, 이전에 회의에서 나온 내용대로라면, 인트라 아이디가 중복되는 경우를 생각해서 조금 변형을 줘야합니다. 근데 그때 정한내용이 정확하게 기억나지 않네요..

그리고 이렇게 된다면, 닉네임에 다시 유니크 인덱스를 걸어야할거같네요

@rockpell
Copy link
Member Author

rockpell commented Mar 6, 2022

지금은 가입시 기본 닉네임을 그냥 인트라로 하였는데, 이전에 회의에서 나온 내용대로라면, 인트라 아이디가 중복되는 경우를 생각해서 조금 변형을 줘야합니다. 근데 그때 정한내용이 정확하게 기억나지 않네요..

그리고 이렇게 된다면, 닉네임에 다시 유니크 인덱스를 걸어야할거같네요

이거는 오늘 논의해보죠

@rockpell
Fix, Test: intra-auth 메일 전송 api all 에러 필터 적용
13fd467 
- 구글 계정에서 생기는 에러를 구분할 방법이 없어서 방법을 찾을때까지는 all error filter 적용
@Skyrich2000
Copy link
Member

지금은 가입시 기본 닉네임을 그냥 인트라로 하였는데, 이전에 회의에서 나온 내용대로라면, 인트라 아이디가 중복되는 경우를 생각해서 조금 변형을 줘야합니다. 근데 그때 정한내용이 정확하게 기억나지 않네요..
그리고 이렇게 된다면, 닉네임에 다시 유니크 인덱스를 걸어야할거같네요

이거는 오늘 논의해보죠

닉네임 중복 허용하기로 했습니다 그대로 가도 될거같습니다 😆

@rockpell
Merge branch 'develop' into feature/nickname-to-intraid
baa5f51
@rockpell
Merge branch 'develop' into feature/nickname-to-intraid
26e8d84
@rockpell
Test: user test, dummy형식 안맞아서 에러나는거 수정
fdd2247 
- github username이 추가되었는데 dummy를 호출할때 github username이 없는 상태로 호출해서 생기는 에러 수정
@blingblin-g blingblin-g self-assigned this Mar 6, 2022
@rockpell rockpell assigned rockpell and unassigned blingblin-g Mar 6, 2022
@blingblin-g blingblin-g added this to the 🔥 5 ~ 7주차 🔥 milestone Mar 6, 2022
@rockpell rockpell added the enhancement New feature or request label Mar 6, 2022
@Skyrich2000 Skyrich2000 mentioned this pull request Mar 9, 2022
Closed
@mimseong mimseong self-requested a review March 11, 2022 12:16
mimseong
mimseong approved these changes Mar 11, 2022
View reviewed changes
Copy link
Contributor

@mimseong mimseong left a comment

Choose a reason for hiding this comment

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

머지해도 괜찮을듯요!

@rockpell rockpell merged commit 60ebf6c into develop Mar 11, 2022
@rockpell rockpell deleted the feature/nickname-to-intraid branch March 11, 2022 12:18
@Skyrich2000 Skyrich2000 linked an issue Mar 15, 2022 that may be closed by this pull request
[Feature] 닉네임을 인트라 id로 변경 #87
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Skyrich2000 Skyrich2000 Skyrich2000 approved these changes

@mimseong mimseong mimseong approved these changes

Assignees

@rockpell rockpell

Labels
enhancement New feature or request
Projects
None yet
Milestone
🔥 5 ~ 7주차 🔥
Development

Successfully merging this pull request may close these issues.

[Feature] 닉네임을 인트라 id로 변경
4 participants
@rockpell @mimseong @Skyrich2000 @blingblin-g