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

[Team-26][BE : Shine] Issue-Tracker 2주차 2회차 PR #200

Merged
merged 16 commits into from Jun 25, 2022
Merged

[Team-26][BE : Shine] Issue-Tracker 2주차 2회차 PR #200

merged 16 commits into from Jun 25, 2022

Conversation

zbqmgldjfh
Copy link
Member

@zbqmgldjfh zbqmgldjfh commented Jun 24, 2022

안녕하세요 Shine입니다!!

저의 리뷰를 담당해주시게 되어 감사하다는 말씀 전하고 싶습니다.

질문 3가지

우선 로직상의 질문이 아닌, 사용법 에 관한 질문을 드리게 되어 유감스럽게 생각합니다 ㅠ,ㅠ
너무 해결이 안되서 질문남겨봅니다 ㅠ,ㅠ

1. 테스트에서 사용할 환경변수 등록

이상하게 테스트에서 환경변수가 필요할 때 각 테스트 메서드마다 하나하나 똑같은 코드를 등록시켜줘야 작동하고 있습니다.
스크린샷 2022-06-24 오후 4 50 28

위 빨간 박스 안의 모든 메서드마다 Enviroment varibles를 똑같이 지정해주고 있게 되는 문제가 생겼습니다.
분명 Root 개념의 하나에 지정해주면 모든 테스트에서 동일하게 사용해야할 것 같은데, 각 테스트 단건마다 모두 환경변수를 지정해줘야 하고 있습니다.

All 이라는 것을 만들어 사용해보려 노력해봐도 적용되지 않았습니다.

혹시 리뷰어님은 테스트 코드 전체에 일괄적으로 환경변수를 어떻게 추가해주시나요?

2. Service 안에서 다른 Service 의존하기

IssueService를 만드는 도중, IssueRepository뿐만 아니라, labelService, milestoneService 에도 의존성이 생겨버리는데,
하나의 Service에서 다른 서비스에 의존해도 되는걸까요??

3. Controller에서의 커맨드, 쿼리 메서드 분리

우선 Issue 컨트롤러는 다음과 같습니다.
스크린샷 2022-06-24 오후 9 08 10

커맨드성 메서드들은 CUD 시에 사용하면 반환값이 void 이고,
쿼리성 메서드들은 R시에 사용하며 반환값이 DTO 입니다.

이렇게 분리하여 컨트롤러에서 구현하고 있는데, 혹시 커맨드 메서드들도 정상적으로 수행 됬음을 상태코드가 아닌, 뭔가를 반환해줘야 할까요?

진행사항

지난 번 리뷰에 남겨주신 부분 전부 수정하였습니다!!

  • nginx를 RHEL이나 RPM, apt로 까는 것과 직접 바이너리 다운로드/빌드 후 사용하는 것의 차이점도 한번 학습
  • Comment의 정적 create 메서드 고민해보기
  • isSameCommentId equals로 변경하기
  • CommentService.addComment 테스트 작성해서 확인하기
  • Issues의 comments에 고아객체 삭제 추가하기

- Issue에 Image가 종속되지 않아도 된다.
따라서 연관관계를 제거 하였다. issue의 본문에 url만 추가되면 된다.
- isOpen으로 유지할경우, requestDto을 변환할때 ObjectMapper에서 문제가 발생할 수 있음, 따라서 변
…iceIntegrationTest 구현

- Issue에 comment를 등록후, 부모인 Issue를 저장하면 comment도 저장 되어야 한다.
- 우선 전체 허용으로 해결, 아직 EC2에 FE는 배포하지 않음
@zbqmgldjfh zbqmgldjfh added the review-BE Improvements or additions to documentation label Jun 24, 2022
@zbqmgldjfh zbqmgldjfh requested a review from wheejuni June 24, 2022 07:59
@zbqmgldjfh zbqmgldjfh self-assigned this Jun 24, 2022
Copy link

@wheejuni wheejuni left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다. 💯
코드레벨에서 특별히 드릴 조언이 많진 않네요. 질문에 대한 답변은 별도 댓글로 드립니다

@@ -7,8 +7,9 @@ buildscript {
plugins {
id 'org.springframework.boot' version '2.7.0'
id 'io.spring.dependency-management' version '1.0.11.RELEASE'
id 'org.asciidoctor.convert' version '1.5.8'
//id 'org.asciidoctor.convert' version '1.5.8'

Choose a reason for hiding this comment

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

필요없는 건 제때 지워주시면 감사합니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

앗!! 일단 생행해보고 지운다는게 까먹었습니다 ㅎㅎ 삭제하였습니다!

@Slf4j
@Service
@RequiredArgsConstructor
public class FileService {

Choose a reason for hiding this comment

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

좋은 구현입니다만,
사실 백엔드를 거쳐서 S3에 업로드하는게 그리 좋은 방법이 아니긴 합니다.. FE에서 다 처리하고, 백엔드엔 파일의 위치만 알려주는 게 좋습니다.
근데 뭐 경험의 의미가 있는 거겠죠. 좋습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

아?? 원래 사진은 FE가 업로드를 하고 리소스의 위치만 저희한태 보내주는 것 이군요??
당연히 BE의 역할인줄 알았는데 하나 배워갑니다!!!

Comment on lines +24 to +26
private String imgName;
private String oriImgName;
private String imgUrl;

Choose a reason for hiding this comment

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

축약어를 사용하지 않기를 권해 드립니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

넵 fullName을 기술하도록 변경하겠습니다!!

@wheejuni
Copy link

  1. 테스트 코드에 환경변수가 왜 필요한지 정확히는 모르겠지만 환경 변수로 풀어야 할 부분이 있다면 test sources root에 application.yml 을 정의해서 사용할 것 같네요. 테스트 환경에 @SpringBootApplication 을 따로 두고요.

해당 애플리케이션에서 테스트 대상이 되는 컴포넌트가 ComponentScan 될 수 있도록 설정을 잘 조정해서 테스트 진행하시면 될듯합니다.

  1. 가급적 안하는게 좋습니다. 정말 필요한지 설계를 다시 검토해 보시는 것이 좋기는 합니다.

  2. 클라이언트 개발자와 잘 합의하셔서 개발하시면 됩니다. 어떻게 해도 무방합니다.

@zbqmgldjfh zbqmgldjfh merged commit ee66215 into codesquad-members-2022:team-26 Jun 25, 2022
@zbqmgldjfh
Copy link
Member Author

흠 아직 경험이 부족하여, 하나의 Service상에서 다른 Service를 포함했을때 어떤 단점이 생기는지 느끼지 못하는것 같습니다 ㅠ,ㅠ
혹 그럼 여러 다른 Repository들은 가져다 써도 되겠죠?
아직 경험이 없어 왜 사용하면 안되는지 잘 못느끼는 것 같습니다 ㅎㅎ

https://www.inflearn.com/questions/21703
위 글을 통해 또한 사람마다 관점이 다를 수 있는 부분이라 느끼게 되었습니다.... 역시 개발은 답이 없는 영역인것 같습니다 ㅎㅎ

혹시 Brian께서 느끼셨던 Service에서 다른 Service를 포함시켰을때의 단점을 조금만 알려주실 수 있을까요??

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-BE Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants