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

깃허브 로그인 #92

Merged
merged 34 commits into from
Mar 7, 2024
Merged

깃허브 로그인 #92

merged 34 commits into from
Mar 7, 2024

Conversation

uijin-j
Copy link
Contributor

@uijin-j uijin-j commented Mar 4, 2024

🎫 관련 이슈

Fixes #1

✅ 구현 내용

  • OAuth 셋팅
  • 깃허브 로그인 Controller 작성 (with 스웨거)
  • 회원 가입 기능 구현 (OAuth 최초 로그인 시 사용)
  • JWT 셋팅: Access Token 발급 역할을 하는 클래스 생성

💬 코멘트

  • 테스트 코드는 따로 작성하도록 하겠습니당ㅎㅎ.!

@uijin-j uijin-j added the 🚀 더 만들어봤슈 기능 구현 label Mar 4, 2024
@uijin-j uijin-j added this to the 3차 스프린트 milestone Mar 4, 2024
@uijin-j uijin-j self-assigned this Mar 4, 2024
Copy link

github-actions bot commented Mar 4, 2024

📝 Jacoco Test Coverage

Overall Project 54.69% -8.3%
Files changed 27.76%

File Coverage
User.java 100% 🍏
PasswordEncoderConfig.java 100% 🍏
SecurityConfig.java 100% 🍏
AuthService.java 95.83% 🍏
JWTManager.java 94.23% 🍏
ProviderType.java 77.78% -22.22%
UserInfoMapperFactory.java 72.73% -27.27%
AuthProviderRepositoryCustomImpl.java 21.95% -78.05%
GithubUserInfoMapper.java 13.21% -86.79%
OAuth2UserImpl.java 0%
OAuth2LoginSuccessHandler.java 0%
OAuth2UserServiceImpl.java 0%
SocialLoginResponse.java 0%
QAuthProvider.java 0% -5%
AuthProvider.java 0% -52.38%

Copy link
Member

@Sehee-Lee-01 Sehee-Lee-01 left a comment

Choose a reason for hiding this comment

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

어려운 OAuth2 적용해주시느라 고생많으셨습니다! 많이 배워가용!! :)

Copy link
Member

Choose a reason for hiding this comment

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

오늘 제 PR도 그랬는데 요 부분 배포 할 때 한 번 RDS 비우고 해야될 것 같네유!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

헉 그렇네용..! 아니면 Flyway 버전 업해서 사용해 보겠습니당!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

헉 그렇네용! 감사합니당🙇🏻‍♀️🙇🏻‍♀️

@@ -121,7 +121,7 @@ class 회원가입_테스트 {

// then
assertThatExceptionOfType(IllegalArgumentException.class).isThrownBy(signup)
.withMessage("유효하지 않은 이메일 형식입니다.");
Copy link
Member

Choose a reason for hiding this comment

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

요 부분 에러코드 메시지가 정의된 걸로 아는데 적용해도 좋을 것 가탕요!

public static final int MAX_EMAIL_LENGTH = 50;
public static final int MAX_INTRODUCTION_LENGTH = 100;
public static final int MAX_JOB_LENGTH = 30;
public static final int MAX_CAREER_LENGTH = 30;
Copy link
Member

Choose a reason for hiding this comment

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

요 부분 제 PR 머지되면서 src/main/java/sixgaezzang/sidepeek/users/util/UserConstant.java에 정의된 것들이 있습니다! 참고해주시면 감사하겠습니당! 🧚‍♀️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵! 감사합니당ㅎㅎ

Copy link
Member

Choose a reason for hiding this comment

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

👍 빈 설정 좋습니다!!!

Copy link
Member

Choose a reason for hiding this comment

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

인터페이스 적용 배워갑니당!!👍

Copy link
Contributor

@yenzip yenzip left a comment

Choose a reason for hiding this comment

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

러닝커브가 높았을 것 같은데... 정말 고생 많으셨습니다..! ❤️‍🔥

GOOGLE;

public static ProviderType from(String provider) {
String upperCaseProvider = provider.toUpperCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

소문자로 들어올 경우도 고려하시다니..! 꼼꼼하시네요! 👀 ✨

import lombok.extern.slf4j.Slf4j;
import sixgaezzang.sidepeek.users.domain.User;

@Slf4j
Copy link
Contributor

Choose a reason for hiding this comment

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

로깅을 사용하는 곳이 없으니 지워주셔도 될 것 같습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

자꾸 개발하다 로깅하고 삭제하는걸 까먹네용.. 감사합니당!! 🦅👀✨

Copy link
Contributor Author

Choose a reason for hiding this comment

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

42d4f21 반영 완료 했습니당😉

Comment on lines 54 to 55
this.isRegistrationComplete =
isNull(isRegistrationComplete) ? false : isRegistrationComplete;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.isRegistrationComplete =
isNull(isRegistrationComplete) ? false : isRegistrationComplete;
this.isRegistrationComplete = isRegistrationComplete;

스키마에서 isRegistrationComplete이 NOT NULL로 설정되어 있기 때문에, NULL이 들어올 수 없는 거 아닌가요?
Default가 false로 설정되어 있기 때문에, 아래와 같이 기본적으로 false로 설정될 수 있도록 하는 건 어떠신가요?

public static final Boolean DEFAULT_IS_REGISTRATION_COMPLETE = false;

@Column(name = "is_registration_complete", nullable = false, columnDefinition = "TINYINT")
private boolean isRegistrationComplete = DEFAULT_IS_REGISTRATION_COMPLETE;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

헉 좋은데요? 제가 왜 이 생각을 못 했을까용..👍🏻👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f262013 반영 완료 했습니당😉

UserSummary user
) {

public static SocialLoginResponse of(LoginResponse loginResponse, AuthProvider authProvider) {
Copy link
Contributor

Choose a reason for hiding this comment

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

여러개의 매개 변수라면 메서드 이름이 from이 아니라 of가 맞겠네요..! 잊고 있었는데... 감사합니다 👍
저도 수정해야겠네요..!

Copy link
Contributor

Choose a reason for hiding this comment

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

메서드 분리를 너무 잘하셔서 코드가 깔끔하네요..! 👍

Copy link

github-actions bot commented Mar 6, 2024

📝 Jacoco Test Coverage

Overall Project 65.26% -6.83%
Files changed 31.54%

File Coverage
Password.java 100% 🍏
User.java 100% 🍏
UserService.java 100% 🍏
PasswordEncoderConfig.java 100% 🍏
SecurityConfig.java 100% 🍏
AuthService.java 95.83% 🍏
JWTManager.java 94.23% 🍏
ProviderType.java 77.78% -22.22%
UserInfoMapperFactory.java 72.73% -27.27%
AuthProviderRepositoryCustomImpl.java 21.95% -78.05%
AuthProvider.java 16.67% -41.67%
GithubUserInfoMapper.java 6.12% -93.88%
OAuth2UserImpl.java 0%
OAuth2LoginSuccessHandler.java 0%
OAuth2UserServiceImpl.java 0%
SocialLoginResponse.java 0%
QAuthProvider.java 0% -5%

@uijin-j uijin-j added the 🥕 더 수정해봤슈 리뷰 후 피드백 반영 label Mar 6, 2024
Copy link

github-actions bot commented Mar 7, 2024

📝 Jacoco Test Coverage

Overall Project 63.45% -5.93%
Files changed 31.54%

File Coverage
Password.java 100% 🍏
User.java 100% 🍏
UserService.java 100% 🍏
PasswordEncoderConfig.java 100% 🍏
SecurityConfig.java 100% 🍏
AuthService.java 95.83% 🍏
JWTManager.java 94.23% 🍏
ProviderType.java 77.78% -22.22%
UserInfoMapperFactory.java 72.73% -27.27%
AuthProviderRepositoryCustomImpl.java 21.95% -78.05%
AuthProvider.java 16.67% -41.67%
GithubUserInfoMapper.java 6.12% -93.88%
OAuth2UserImpl.java 0%
OAuth2LoginSuccessHandler.java 0%
OAuth2UserServiceImpl.java 0%
SocialLoginResponse.java 0%
QAuthProvider.java 0% -5%

@uijin-j uijin-j merged commit a45a797 into dev Mar 7, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 더 만들어봤슈 기능 구현 🥕 더 수정해봤슈 리뷰 후 피드백 반영
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants