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

[PC-106] SMS 인증 시스템 구현 #14

Open
wants to merge 10 commits into
base: develope
Choose a base branch
from

Conversation

devchlee12
Copy link
Member

@devchlee12 devchlee12 commented Dec 30, 2024

🔗 관련 이슈

PC-106

✨ 작업 내용

  • 인증 코드 생성기 구현
  • CoolSMS 라이브러리 추가
  • SMS 전송 기능 구현
  • SMS 인증 기능 구현
  • 단위 테스트 작성
  • 통합 테스트로 작동 확인

✅ 체크리스트

  • 코드가 정상적으로 컴파일되나요?
  • 테스트 코드를 통과했나요?
  • merge할 브랜치의 위치를 확인했나요?
  • Label을 지정했나요?

🎃 새롭게 알게된 사항

📋 참고 사항

*
* @return 6자리 난수
*/
public int generate() {
Copy link
Member

Choose a reason for hiding this comment

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

현재 코드상으로 생성되는 난수의 범위는 100000부터 999999까지로 확인됩니다. 하지만, 문자 인증을 할 때는 숫자 문자열을 입력하는 것이기 때문에 "008712" 같은 0으로 시작하는 경우도 포함하여 숫자 문자열을 전달하는 것이 조금 더 바람직해보입니다.

/**
* 테스트를 위한 초기화 메서드
*/
public void initForTest(String apiKey, String apiSecret, String fromNumber,
Copy link
Member

Choose a reason for hiding this comment

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

테스트 코드를 위해서 운영 코드에 별도의 메서드를 추가하는 것은 코드의 일관상이 떨어지는 것 같습니다.

@Service
@RequiredArgsConstructor
public class SmsSenderService {
@Value("${coolsms.apikey}")
Copy link
Member

Choose a reason for hiding this comment

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

RequiredArgsConstructor는 private final인 필드에 대해서 생성자를 생성하는 것으로 알고있는데, final이 붙은 필드가 없어서 어노테이션이 의미가 없어진 것 같습니다 .혹시 다른 의도가 있으셨나요 ?

*
* @param phoneNumber 인증 번호를 받을 핸드폰 번호
*/
public void sendAuthCodeTo(String phoneNumber) {
Copy link
Member

Choose a reason for hiding this comment

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

네트워크 에러로 인증번호를 보내지 못하면 어떻게 되나요 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants