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

아이디 찾기 #180

Merged
merged 9 commits into from
Sep 20, 2023
Merged

아이디 찾기 #180

merged 9 commits into from
Sep 20, 2023

Conversation

5uhwann
Copy link
Member

@5uhwann 5uhwann commented Sep 20, 2023

Issue

Close #179

✅ 작업 내용

  • 유저 로그인 아이디 찾기 구현

🤔 고민 했던 부분

🔊 도움이 필요한 부분!!

  • 현재 아이디 찾기 api 명세 상 요청 시 전달할 데이터인 학번이 path variable을 통해 전달되고 있습니다. 전달되는 학번의 유저가 존재하는지 알 수 없으며, 해당 자원 조회 시 필요한 조건이니 query parameter를 쓰는게 더 좋을 거 같습니다. 의견 부탁드립니다!.
    (다만, 프론트 엔드 개발자와 공유된 내용이 아니기 때문에 기존 api 명세대로 기능 구현 완료했습니다!)

@5uhwann 5uhwann added ✨ feat 새로운 기능 개발 혹은 기존 기능 변경 ♻️ refactor 기본의 로직 변경 없이 코드 개선 labels Sep 20, 2023
@5uhwann 5uhwann added this to the 리팩토링 milestone Sep 20, 2023
@5uhwann 5uhwann self-assigned this Sep 20, 2023
@codecov
Copy link

codecov bot commented Sep 20, 2023 •
edited
Loading

Codecov Report

Merging #180 (2966944) into develop (2307b5c) will increase coverage by 0.13%.
The diff coverage is 95.23%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #180      +/-   ##
=============================================
+ Coverage      90.35%   90.49%   +0.13%     
- Complexity       779      795      +16     
=============================================
  Files            127      130       +3     
  Lines           2168     2189      +21     
  Branches          59       59              
=============================================
+ Hits            1959     1981      +22     
- Misses           179      180       +1     
+ Partials          30       28       -2
Files Changed Coverage Δ
.../myongjigraduatebe/core/config/SecurityConfig.java 100.00% <ø> (ø)
...r/application/port/in/find/UserAuthIdResponse.java 90.90% <90.90%> (ø)
...duatebe/core/exception/GlobalExceptionHandler.java 70.00% <100.00%> (+3.33%) ⬆️
...daptor/in/web/findauthid/FindAuthIdController.java 100.00% <100.00%> (ø)
...daptor/out/persistence/UserPersistenceAdaptor.java 52.94% <100.00%> (+2.94%) ⬆️
...pplication/service/find/FindUserAuthIdService.java 100.00% <100.00%> (ø)
...uate/myongjigraduatebe/user/domain/model/User.java 91.22% <100.00%> (+0.15%) ⬆️

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2307b5c...2966944. Read the comment docs.

@5uhwann 5uhwann changed the title Feature/아이디 찾기 아이디 찾기 Sep 20, 2023
stophwan
stophwan approved these changes Sep 20, 2023
View reviewed changes
Copy link
Member

@stophwan stophwan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

개인적으로 나는 url이 아닌 request body를 통한 POST 요청으로 보내는게 맞다고 생각.
API를 변경하는 것이 어떨까? 그리고 만약 이렇게 수정한다면 상태코드가 404가 아닌, 400으로 변경하는 것이 더 좋아보여.
findById는 IlliegalArgumentException findByStudentNumber은 NotSuchElementException인 것도 올바르지 않은 url이라 다른 상태 코드를 반환한다고 일어난 일이니까.

stophwan
stophwan reviewed Sep 20, 2023
View reviewed changes
...java/com/plzgraduate/myongjigraduatebe/user/application/port/in/find/UserAuthIdResponse.java Outdated
Comment on lines 20 to 24
public static UserAuthIdResponse from(String encryptedAutId, String studentNumber) {
return UserAuthIdResponse.builder()
.authId(encryptedAutId)
.studentNumber(studentNumber).build();
}
Copy link
Member

Choose a reason for hiding this comment

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

from -> of

Copy link
Member Author

Choose a reason for hiding this comment

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

넵

...a/com/plzgraduate/myongjigraduatebe/user/application/service/find/FindUserAuthIdService.java Outdated
Comment on lines 24 to 25
User user = findUserPort.findUserByStudentNumber(studentNumber)
.orElseThrow(() -> new NoSuchElementException("해당 학번의 사용자가 존재하지 않습니다"));
Copy link
Member

Choose a reason for hiding this comment

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

request body로 하고 400으로 수정해버리자. 기존 api 수정할 사항 있으면 그냥 하자.

Copy link
Member Author

Choose a reason for hiding this comment

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

음 저는 400번 보다는 404가 맞는거 같은게 학번으로 조회하는 요청 자체는 잘못된 요청이 아니고 찾으려는 자원이 존재하지 않는거니 404가 더 알맞은 에러코드인거 같습니다.

@5uhwann
Copy link
Member Author

5uhwann commented Sep 20, 2023

개인적으로 나는 url이 아닌 request body를 통한 POST 요청으로 보내는게 맞다고 생각. API를 변경하는 것이 어떨까? 그리고 만약 이렇게 수정한다면 상태코드가 404가 아닌, 400으로 변경하는 것이 더 좋아보여. findById는 IlliegalArgumentException findByStudentNumber은 NotSuchElementException인 것도 올바르지 않은 url이라 다른 상태 코드를 반환한다고 일어난 일이니까.

음 조회 메서드인데 POST 요청이 맞을까요?? 저는 GET 요청에 query string으로 유저 학번이 더 좋아보입니다..!

@sonarcloud
Copy link

sonarcloud bot commented Sep 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@5uhwann 5uhwann merged commit 2886a8c into develop Sep 20, 2023
6 checks passed
@5uhwann 5uhwann deleted the feature/아이디_찾기 branch September 20, 2023 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stophwan stophwan stophwan approved these changes

Assignees

@5uhwann 5uhwann

Labels
✨ feat 새로운 기능 개발 혹은 기존 기능 변경 ♻️ refactor 기본의 로직 변경 없이 코드 개선 size/L
Projects
None yet
Milestone
리팩토링
Development

Successfully merging this pull request may close these issues.

아이디 찾기
2 participants
@5uhwann @stophwan