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

전남대_11조_골라주마_6주차 #87

Merged
merged 145 commits into from
Oct 17, 2023
Merged

전남대_11조_골라주마_6주차 #87

merged 145 commits into from
Oct 17, 2023

Conversation

kssumin
Copy link
Contributor

@kssumin kssumin commented Oct 15, 2023

어떤 내용에 대한 PR인가요?

전남대_11조_골라주마_6주차 개발 진행하였습니다!

작업 내역

구현

  • 더미데이터 생성
  • 핫게시판
    리팩토링
  • 투표 조회 관련 리팩토링(active 속성 제거)
  • 테스트 리팩토링
  • redis db mysql로 이전
    배포
  • dockerfile 작업 중..
  • 크램폴린 작업 중...

6주차 작업 완료 PR

[REFACTOR] table명 및 더미데이터 수정 #84
[REFACTOR] comment refactoring 2 #74
[REFACTOR] redis 삭제 후 mysql로 변경 #73
[Fix] test 오류 해결 #72
[refactor] 투표 entity active 속성 제거에 따른 로직 변경 #71
[FEAT] mypage 구현 #70
[Refactor] 투표 리스트 조회 방식 변경 #66
[ FIX ] 투표 entity active 속성 제거 #65
[FEAT] 핫게시판 로직 #59
[10.11] 현수 작업 내용 #56
[REFACTOR] 투표 리스트 조회 공통 응답 DTO 리팩토링 #54
[Feat] decision persistence layer 계층 추가 #53
[FEAT] 투표 상세 기능 추가 #49
[FIX] conflict로 인한 security 파일 삭제 #48
[FIX] API 형식에 맞게 변경 #47

무엇을 위주로 보면 좋을까요?

  • 피드백 해주셨던 부분들을 리팩토링할 때 고치려고 노력하고 있지만.. 아직 구현이 덜 된 부분들이 있어 우선순위가 밀려 완전히 수정하지 못했습니다..! 죄송합니다..ㅜ 지금처럼 고쳐야하거나 더 나은 방법을 알려주셨던게 큰 도움이 됐던 것 같습니다. 앞으로도 잘 부탁드립니당

@kssumin kssumin changed the title 6주차_골라주마_ 전남대_11조_골라주마_6주차 Oct 15, 2023
Copy link

@lalwr lalwr left a comment

Choose a reason for hiding this comment

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

코드 잘 구현 해주셨네요~

개선 드린 부분에 대해 많이 개선해주신거 같습니다! 👍

궁금한 점은 언제든지 말씀 부탁드립니다.


saveToken(userId, refreshToken);

return tokenConverter.from(
Copy link

Choose a reason for hiding this comment

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

tokenConverter.from 에 특별한 기능은 없고 Response를 만드는것으로 보이는데
Response 에서 from을 선언해서 관리해도 되지 않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Response는 dto로 값을 전달하는 책임만 가지고 있다고 생각했습니다!!
그래서 해당 Response를 만드는 책임을 가지는 tokenConverter 객체를 따로 생성하여 해당 객체에 책임을 부여하였습니다!

Response가 전달 뿐만 아니라, Response 자체를 만드는 책임을 가져도 되는 이유가 궁금합니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

굳이 불필요한 작업인 것 같다.

@@ -19,11 +15,9 @@
@Transactional(readOnly = true)
public class LoginUserService {

private final TokenProvider tokenProvider;
Copy link

Choose a reason for hiding this comment

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

책임 분리 👍

// 1. 투표한 유저인지 확인 -decision이 나와야함
// decisionRepository.findByUserIdVoteId(voteId,userId).orElseThrow(new NoDecisionException("투표
// 후에 가능합니다.", HttpStatus.UNAUTHORIZED));
if (userId == null) throw new AuthorizationException("로그인 후 댓글을 작성할 수 있습니다.");
Copy link

Choose a reason for hiding this comment

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

Controller 에서 벨리데이션을 통해 검증하지 못하는 걸까요?

// return
SaveCommentResponse response = new SaveCommentResponse(commentEntity, true, 1);
String username = userRepository.findById(userId).get().getNickname();
Copy link

Choose a reason for hiding this comment

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

저장되지 않는 userId find 하는경우에는 어떻게 되나요?

@@ -88,14 +91,15 @@ public UpdateCommentResponse update(
String newContent = requestDto.getContent();
// setter는 명확하지 않기 때문에 댓글을 업데이트를 한다는걸 명시한 새로운 메서드 정의
commentEntity.updateContent(newContent);
String username = "asdf";
String username = userRepository.findById(userId).get().getNickname();
Copy link

Choose a reason for hiding this comment

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

저장되지 않는 userId find 하는경우에는 어떻게 되나요?

@@ -22,32 +23,34 @@ public class GetVoteListController {
@GetMapping("/votes")
public ApiResponse<ApiResponseBody.SuccessBody<GetVoteListResponse.MainAndFinishPage>>
getVoteList(
@Login Long userId,
@RequestParam(defaultValue = "99999999999999999") long idx,
Copy link

Choose a reason for hiding this comment

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

기본값이 99999999999999999 이어야 하는 부분에 대해 이유가 궁금합니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

db에 저장된 entity 중에 가장 최신순으로 조회하면 가장 id 값이 높은 순서대로 내림차순으로 정렬하여 보여주기때문에 no-offset 방식을 위하여 임의로 지정한 부분이었는데 현재는 offset 방식으로 바꿔 구현하여 해당 idx, totalCount에 대한 부분을 제거하였습니다

import lombok.ToString;

@NoArgsConstructor
@ToString
@Getter
public class CreateVoteRequest implements AbstractRequestDto {

@NotNull(message = "투표 제목은 필수입니다.")
Copy link

Choose a reason for hiding this comment

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

빈값도 검사해야 한다면 @NotEmpty, @NotBlank 도 사용해보시면 좋을꺼 같습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

@NotNull과 함께 사용하는 건가요?

String responseBody = resultActions.andReturn().getResponse().getContentAsString();

System.out.println("테스트 : " + responseBody);
resultActions
Copy link

Choose a reason for hiding this comment

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

검증 👍

// then
// resultActions.andExpect(MockMvcResultMatchers.jsonPath("$.success").value("true"));
// resultActions.andDo(MockMvcResultHandlers.print()).andDo(document);
resultActions
Copy link

Choose a reason for hiding this comment

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

값에 대한 검증도 있으면 좋을꺼 같습니다.

@Autowired private TokenProvider tokenProvider;
private String jwtToken;

@BeforeEach
Copy link

Choose a reason for hiding this comment

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

💯

@lalwr lalwr merged commit 63ecb9c into review Oct 17, 2023
@kssumin kssumin deleted the master branch October 21, 2023 09:11
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.

4 participants