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

전남대 BE_조설빈 5주차 과제 (1단계) #261

Open
wants to merge 13 commits into
base: seolbin96
Choose a base branch
from

Conversation

seolbin96
Copy link

No description provided.

Copy link

@seungy0 seungy0 left a comment

Choose a reason for hiding this comment

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

리뷰가 늦어 죄송합니다.
고생하셨습니다👍👍

Copy link

Choose a reason for hiding this comment

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

좋습니다. 도메인 객체가 컨트롤러에 노출되는 것만 지향하시면 더 좋겠습니다.

Copy link

Choose a reason for hiding this comment

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

과제 요구사항 밖일 수 있으나, 생각해보면 카테고리를 누구나 수정, 삭제할 수 있는 건 조금 이상한 것 같습니다.

@GetMapping("/")
public RedirectView callback(@RequestParam(name = "code") String code, RedirectAttributes redirectAttributes) throws Exception {
String token = kakaoService.login(code);
redirectAttributes.addFlashAttribute("token", token);
Copy link

Choose a reason for hiding this comment

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

이렇게 되면 로그인이 잘 유지되는건지 궁금합니다.

Comment on lines +48 to +53
ResponseEntity<String> entity = client.post()
.uri(URI.create(url))
.contentType(MediaType.APPLICATION_FORM_URLENCODED)
.header(HttpHeaders.AUTHORIZATION, "Bearer " + accessToken)
.retrieve()
.toEntity(String.class);
Copy link

Choose a reason for hiding this comment

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

이러한 외부 의존성은 분리해주세요~

Copy link

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants