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

2-2 로그인 리팩토링 #50

Open
wants to merge 9 commits into
base: dyrlqhffo
Choose a base branch
from

Conversation

dyrlqhffo
Copy link

안녕하세요 리뷰어님.

많이 늦었습니다...😢
최대한 깔끔하게 짜보고 싶었는데 쉽지 않네요...
일단 다 만들긴 한 거 같은데 맞게 만들었는지는 잘모르겠습니다..

Copy link

@NewWisdom NewWisdom left a comment

Choose a reason for hiding this comment

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

안녕하세요 용식님 :)
미션 잘 진행해주셨네요 👍
몇 가지 코멘트 남겼으니 확인 부탁드릴게요~!

Comment on lines +54 to +56
if(user.getRole() != Role.ADMIN){
throw new AuthorizationException(ErrorCode.UNAUTHORIZED_ADMIN, "관리자 권한이 필요합니다.");
}

Choose a reason for hiding this comment

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

이번 단계에서 어느정도 admin 기능에 대한 권한 제어를 해주셨네요 👍
다음 단계 요구사항으로 interceptor 를 통해 검증하도록 바꾸면서 interceptor 의 역할에 대해 더 깊게 체감할 수 있겠군요!

Comment on lines +49 to +53
// if(user.getRole() == Role.ADMIN) {
// User user = authRepository.findById(request.getUserId())
// .orElseThrow(() -> new UserNotFoundException(ErrorCode.USER_NOT_FOUND, "해당 회원이 존재하지 않습니다."));
// }

Choose a reason for hiding this comment

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

사용하지 않는 코드는 제거해주세요~

@@ -2,11 +2,14 @@

import roomescape.domain.User;

import java.util.List;
import java.util.Optional;

public interface AuthRepository {

Choose a reason for hiding this comment

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

해당 repository의 네이밍에 Auth 가 들어가기 보다 데이터 네임에 맞게 UserRepository 와 같이 되는 것은 어떤가요~?
repository 의 역할은 해당 데이터의 접근이 auth와 관련된다는 것은 모르고 단순히 user 데이터에 액세스하는 역할로 볼 수 있을 것 같아서요!

Choose a reason for hiding this comment

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

추가로 회원과 관련된 테이블을 user 로 네이밍했는데, 보통 user는 데이터베이스의 예약어로 사용될 수 있어 사용을 권장하지 않는 것 같아요.
h2 에서도 user 를 예약어로 사용하고 있네요 :)
member 와 같은 다른 네이밍을 적용해보는 것은 어떨까요~?

Copy link
Author

Choose a reason for hiding this comment

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

넵 member로 바꾸도록 하겠슴돠

import java.util.Optional;


public class LoginArgumentResolver implements HandlerMethodArgumentResolver {

Choose a reason for hiding this comment

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

ArgumentResolver 잘 활용해주셨네요 👍

import roomescape.dto.auth.AuthCheckResponse;
import roomescape.dto.auth.AuthLoginRequest;
import roomescape.dto.auth.AuthUserResponse;
import roomescape.dto.auth.UserResponse;
import roomescape.exception.ErrorCode;
import roomescape.exception.custom.AuthorizationException;
import roomescape.exception.custom.CookieNotFoundException;
import roomescape.service.AuthService;
import roomescape.util.CookieUtil;

import java.util.Arrays;

Choose a reason for hiding this comment

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

불필요한 import 문과 공백들은 제거 부탁드려요!

Choose a reason for hiding this comment

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

현재 signUp 페이지가 정상 작동 하지 않는데 확인 부탁드려요!

Copy link
Author

Choose a reason for hiding this comment

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

넵,, 깜빡했습니다... :(

@@ -1,20 +1,22 @@
package roomescape.dto.reservation.create;

import jakarta.validation.constraints.*;
import roomescape.domain.User;

public class ReservationCreateRequest {

Choose a reason for hiding this comment

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

미션 요구사항에 맞게 API 스펙이 변경되지 않은 것 같은데 확인 부탁드려요!

POST /reservations HTTP/1.1
authorization: Bearer eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxIiwiaWF0IjoxNjYzMjk4NTkwLCJleHAiOjE2NjMzMDIxOTAsInJvbGUiOiJBRE1JTiJ9.-OO1QxEpcKhmC34HpmuBhlnwhKdZ39U8q91QkTdH9i0
content-type: application/json; charset=UTF-8
host: localhost:8080

{
    "scheduleId": 1
}

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

Successfully merging this pull request may close these issues.

2 participants