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

[BE] 로그 중복 작성 제거, 비즈니스 로직 내 로그 추가(#608) #610

Merged
merged 6 commits into from
Oct 16, 2024
Merged

[BE] 로그 중복 작성 제거, 비즈니스 로직 내 로그 추가(#608) #610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
20f2324
feat: additivity false 로 변경
youngsu5582 Oct 15, 2024
96eda7b
refactor: 참가,취소 로직 분리
youngsu5582 Oct 15, 2024
81d6168
chore: 로그 레벨 변경
youngsu5582 Oct 15, 2024
70aab4c
feat: 로그 추가
youngsu5582 Oct 15, 2024
c9f508d
Merge branch 'develop' into feat/#608
youngsu5582 Oct 16, 2024
7d00764
refactor: 피드백 반영
youngsu5582 Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion backend/src/main/java/corea/auth/resolver/LoginMemberArgumentResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ public AuthInfo resolveArgument(MethodParameter parameter, ModelAndViewContainer
if (accessToken.equals(ANONYMOUS)) {
throw new CoreaException(ExceptionType.AUTHORIZATION_ERROR);
}
log.info("로그인 시도[토큰={}]", accessToken);

long memberId = tokenService.findMemberIdByToken(accessToken);
Member member = memberRepository.findById(memberId)
Expand Down
13 changes: 11 additions & 2 deletions backend/src/main/java/corea/auth/service/LoginService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@
import corea.member.domain.Member;
import corea.member.repository.MemberRepository;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import static corea.exception.ExceptionType.INVALID_TOKEN;
import static corea.exception.ExceptionType.TOKEN_EXPIRED;

@Slf4j
@Service
@RequiredArgsConstructor
@Transactional(readOnly = true)
Expand All @@ -35,7 +37,13 @@ public TokenInfo login(GithubUserInfo userInfo) {
}

private Member register(GithubUserInfo userInfo) {
return memberRepository.save(new Member(userInfo.login(), userInfo.avatarUrl(), userInfo.name(), userInfo.email(), true, userInfo.id()));
Member member = memberRepository.save(new Member(userInfo.login(), userInfo.avatarUrl(), userInfo.name(), userInfo.email(), true, userInfo.id()));
logCreateMembers(member);
return member;
}

private void logCreateMembers(Member member) {
log.info("멤버를 생성했습니다. 멤버 id={}, 멤버 이름={},깃허브 id={}, 닉네임={}", member.getId(), member.getName(), member.getGithubUserId(), member.getUsername());
}

private String extendAuthorization(Member member) {
Expand All @@ -56,7 +64,8 @@ public String refresh(String refreshToken) {
.orElseThrow(() -> new CoreaException(INVALID_TOKEN));
return tokenService.createAccessToken(info.getMember());
} catch (CoreaException e) {
if (e.getExceptionType().equals(TOKEN_EXPIRED)) {
if (e.getExceptionType()
.equals(TOKEN_EXPIRED)) {
logoutService.logoutByExpiredRefreshToken(refreshToken);
}
throw e;
Expand Down
4 changes: 2 additions & 2 deletions backend/src/main/java/corea/feedback/service/DevelopFeedbackService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public class DevelopFeedbackService {
@Transactional
public DevelopFeedbackResponse create(long roomId, long deliverId, DevelopFeedbackRequest request) {
validateAlreadyExist(roomId, deliverId, request.receiverId());
log.debug("개발 피드백 작성[작성자({}), 요청값({})", deliverId, request);
log.info("개발 피드백 작성[작성자({}), 요청값({})", deliverId, request);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


MatchResult matchResult = matchResultRepository.findByRoomIdAndReviewerIdAndRevieweeId(roomId, deliverId, request.receiverId())
.orElseThrow(() -> new CoreaException(ExceptionType.NOT_MATCHED_MEMBER));
Expand All @@ -50,7 +50,7 @@ private DevelopFeedback saveDevelopFeedback(long roomId, DevelopFeedbackRequest

@Transactional
public DevelopFeedbackResponse update(long feedbackId, long deliverId, DevelopFeedbackRequest request) {
log.debug("개발 피드백 업데이트[작성자({}), 피드백 ID({}), 요청값({})", deliverId, feedbackId, request);
log.info("개발 피드백 업데이트[작성자({}), 피드백 ID({}), 요청값({})", deliverId, feedbackId, request);

DevelopFeedback feedback = developFeedbackRepository.findById(feedbackId)
.orElseThrow(() -> new CoreaException(ExceptionType.FEEDBACK_NOT_FOUND));
Expand Down
4 changes: 2 additions & 2 deletions backend/src/main/java/corea/feedback/service/SocialFeedbackService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public class SocialFeedbackService {
@Transactional
public SocialFeedbackResponse create(long roomId, long deliverId, SocialFeedbackRequest request) {
validateAlreadyExist(roomId, deliverId, request.receiverId());
log.debug("소설 피드백 작성[작성자({}), 요청값({})", deliverId, request);
log.info("소설 피드백 작성[작성자({}), 요청값({})", deliverId, request);

MatchResult matchResult = matchResultRepository.findByRoomIdAndReviewerIdAndRevieweeId(roomId, request.receiverId(), deliverId)
.orElseThrow(() -> new CoreaException(ExceptionType.NOT_MATCHED_MEMBER));
Expand All @@ -50,7 +50,7 @@ private SocialFeedback saveSocialFeedback(long roomId, SocialFeedbackRequest req

@Transactional
public SocialFeedbackResponse update(long feedbackId, long deliverId, SocialFeedbackRequest request) {
log.debug("소설 피드백 업데이트[작성자({}), 피드백 ID({}), 요청값({})", deliverId, feedbackId, request);
log.info("소설 피드백 업데이트[작성자({}), 피드백 ID({}), 요청값({})", deliverId, feedbackId, request);

SocialFeedback feedback = socialFeedbackRepository.findById(feedbackId)
.orElseThrow(() -> new CoreaException(ExceptionType.FEEDBACK_NOT_FOUND));
Expand Down
3 changes: 3 additions & 0 deletions backend/src/main/java/corea/matching/domain/ParticipationFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
import corea.exception.CoreaException;
import corea.exception.ExceptionType;
import corea.participation.domain.Participation;
import lombok.extern.slf4j.Slf4j;

import java.util.ArrayList;
import java.util.List;

@Slf4j
public class ParticipationFilter {

private final List<Participation> participations;
Expand Down Expand Up @@ -51,6 +53,7 @@ private List<Participation> findPRSubmittedParticipation(PullRequestInfo pullReq

private void invalidateIfNotSubmitPR(PullRequestInfo pullRequestInfo, Participation participation) {
if (!hasSubmittedPR(pullRequestInfo, participation)) {
log.warn("매칭에 실패 했습니다. 방 id={},사용자 id={},사용자 깃허브 닉네임={}",participation.getRoomsId(),participation.getMembersId(),participation.getMember().getUsername());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.warn("매칭에 실패 했습니다. 방 id={},사용자 id={},사용자 깃허브 닉네임={}",participation.getRoomsId(),participation.getMembersId(),participation.getMember().getUsername());
log.warn("매칭에 실패 했습니다. 방 id={},사용자 id={},사용자 깃허브 닉네임={}", participation.getRoomsId(), participation.getMembersId(), participation.getMember().getUsername());

participation.invalidate();
}
}
Expand Down
8 changes: 8 additions & 0 deletions backend/src/main/java/corea/member/domain/MemberRole.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import corea.exception.CoreaException;
import corea.exception.ExceptionType;
import corea.participation.domain.ParticipationStatus;

public enum MemberRole {

Expand All @@ -22,4 +23,11 @@ public static MemberRole from(String role) {
public boolean isReviewer() {
return this == REVIEWER;
}

public ParticipationStatus getParticipationStatus() {
return switch (this) {
case REVIEWER,REVIEWEE,BOTH -> ParticipationStatus.PARTICIPATED;
case NONE -> ParticipationStatus.NOT_PARTICIPATED;
};
}
Comment on lines +27 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

break 없이 잘 동작하는가 싶었는데, 람다라 괜찮군요. 구두로 물어봤으니 해결했습니당~

}
13 changes: 4 additions & 9 deletions backend/src/main/java/corea/participation/domain/Participation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,12 @@ public class Participation extends BaseTimeEntity {

private int matchingSize;

public Participation(Room room, Member member, MemberRole role, int matchingSize) {
this(null, room, member, role, ParticipationStatus.PARTICIPATED, matchingSize);
debug(room.getId(), member.getId());
public Participation(Room room, Member member, MemberRole memberRole, ParticipationStatus status, int matchingSize) {
this(null, room, member, memberRole, status, matchingSize);
}

public Participation(Room room, Member member) {
this(null, room, member, MemberRole.REVIEWER, ParticipationStatus.MANAGER, room.getMatchingSize());
debug(room.getId(), member.getId());
public Participation(Room room, Member member, MemberRole role, int matchingSize) {
this(null, room, member, role, ParticipationStatus.PARTICIPATED, matchingSize);
}

public boolean isNotMatchingMemberId(long memberId) {
Expand Down Expand Up @@ -94,7 +92,4 @@ public String getMemberGithubId() {
return member.getGithubUserId();
}

private static void debug(long roomId, long memberId) {
log.debug("참가자 생성[방 ID={}, 멤버 ID={}", roomId, memberId);
}
}
15 changes: 4 additions & 11 deletions backend/src/main/java/corea/participation/service/ParticipationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
public class ParticipationService {

private final ParticipationRepository participationRepository;
private final ParticipationWriter participationWriter;
private final RoomRepository roomRepository;
private final MemberRepository memberRepository;

Expand All @@ -34,23 +35,15 @@ private Participation saveParticipation(ParticipationRequest request) {
Member member = memberRepository.findById(request.memberId())
.orElseThrow(() -> new CoreaException(ExceptionType.MEMBER_NOT_FOUND));
MemberRole memberRole = MemberRole.from(request.role());
Room room = getRoom(request.roomId());

Participation participation = new Participation(getRoom(request.roomId()), member, memberRole, request.matchingSize());
participation.participate();
return participationRepository.save(participation);
return participationWriter.create(room, member, memberRole, request.matchingSize());
}

@Transactional
public void cancel(long roomId, long memberId) {
validateMemberExist(memberId);
deleteParticipation(roomId, memberId);
}

private void deleteParticipation(long roomId, long memberId) {
Participation participation = participationRepository.findByRoomIdAndMemberId(roomId, memberId)
.orElseThrow(() -> new CoreaException(ExceptionType.NOT_ALREADY_APPLY));
participation.cancel();
participationRepository.delete(participation);
participationWriter.delete(roomId, memberId);
}

private void validateIdExist(long roomId, long memberId) {
Expand Down
57 changes: 57 additions & 0 deletions backend/src/main/java/corea/participation/service/ParticipationWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package corea.participation.service;

import corea.exception.CoreaException;
import corea.exception.ExceptionType;
import corea.member.domain.Member;
import corea.member.domain.MemberRole;
import corea.participation.domain.Participation;
import corea.participation.domain.ParticipationStatus;
import corea.participation.repository.ParticipationRepository;
import corea.room.domain.Room;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Component;
import org.springframework.transaction.annotation.Transactional;

@Slf4j
@Component
@RequiredArgsConstructor
@Transactional
public class ParticipationWriter {

private final ParticipationRepository participationRepository;

public Participation create(Room room, Member member, MemberRole memberRole, ParticipationStatus participationStatus) {

return create(room, member, memberRole, participationStatus, room.getMatchingSize());
}

public Participation create(Room room, Member member, MemberRole memberRole, int matchingSize) {
return create(room, member, memberRole, memberRole.getParticipationStatus(), matchingSize);
}

private Participation create(Room room, Member member, MemberRole memberRole, ParticipationStatus participationStatus, int matchingSize) {
Participation participation = participationRepository.save(new Participation(room, member, memberRole, participationStatus, matchingSize));
participation.participate();
logCreateParticipation(participation);
return participation;
}

// TODO 객체 두개를 넣어서 삭제하는 방향으로 변경 해야합니다.
// 현재, 로직상 cancel 호출 시, 의도하지 않는 room 조회문 발생
public void delete(long roomId, long memberId) {
Participation participation = participationRepository.findByRoomIdAndMemberId(roomId, memberId)
.orElseThrow(() -> new CoreaException(ExceptionType.NOT_ALREADY_APPLY));
participation.cancel();
logDeleteParticipation(participation);
participationRepository.delete(participation);
}

private void logCreateParticipation(Participation participation) {
log.info("방에 참가했습니다. id={}, 방 id={}, 참가한 사용자 id={}, 역할={}, 원하는 매칭 인원={}", participation.getId(), participation.getRoomsId(), participation.getMembersId(), participation.getMemberRole(), participation.getMatchingSize());
}

private void logDeleteParticipation(Participation participation) {
log.info("참여를 취소했습니다. 방 id={}, 참가한 사용자 id={}", participation.getRoomsId(), participation.getMembersId());
}
}
8 changes: 6 additions & 2 deletions backend/src/main/java/corea/room/service/RoomService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import corea.participation.domain.Participation;
import corea.participation.domain.ParticipationStatus;
import corea.participation.repository.ParticipationRepository;
import corea.participation.service.ParticipationWriter;
import corea.room.domain.Room;
import corea.room.dto.*;
import corea.room.repository.RoomRepository;
Expand Down Expand Up @@ -39,7 +40,7 @@ public class RoomService {
private final ParticipationRepository participationRepository;
private final FailedMatchingRepository failedMatchingRepository;
private final RoomAutomaticService roomAutomaticService;

private final ParticipationWriter participationWriter;

@Transactional
public RoomResponse create(long memberId, RoomCreateRequest request) {
Expand All @@ -48,7 +49,9 @@ public RoomResponse create(long memberId, RoomCreateRequest request) {
Member manager = memberRepository.findById(memberId)
.orElseThrow(() -> new CoreaException(ExceptionType.MEMBER_NOT_FOUND));
Room room = roomRepository.save(request.toEntity(manager));
Participation participation = new Participation(room, manager);
log.info("방을 생성했습니다. 방 생성자 id={}, 요청한 사용자 id={}", room.getManagerId(), memberId);

Participation participation = participationWriter.create(room, manager, MemberRole.REVIEWER, ParticipationStatus.MANAGER);

participationRepository.save(participation);
roomAutomaticService.createAutomatic(room);
Expand Down Expand Up @@ -107,6 +110,7 @@ public void delete(long roomId, long memberId) {
Room room = getRoom(roomId);
validateDeletionAuthority(room, memberId);

log.info("방을 삭제했습니다. 방 id={}, 사용자 iD={}", roomId, memberId);
Copy link
Contributor

Choose a reason for hiding this comment

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

변경된 부분이 아니여서 이곳에 커멘트 남길게요. RoomService에서 ParticipationWriter를 잘 사용하고 있는가에 대한 커멘트입니다.

개인적으로 리팩터링으로 수고하고 있다는거 잘 알지만 👍, 약간 우려된 부분이 ParticipationWriter에서 나온 것 같아 글 남겨봐요!
조이썬이 생각할 때 ParticipationWriter가 잘 사용되고 있다 느껴지나요?

현재 Room과 Participation이 저희 서비스상 많은 연관관계를 가지지만 방을 삭제할 때, 막상 RoomService에서는 ParticipationWriter의 delete 메서드를 활용하기 어려운 구조라 생각해요.
그래서 조이썬도 그냥 두셨다 판단됩니다.

제가 느낄 수 있는 한가지 ParticipationWriter의 장점으로는 방을 참여할 때, participation.participate();를 같이 호출해주어 개발자로 하여금 휴먼 에러를 방지한다?? 정도가 있겠네요.

근데 이 장점도 저희의 처음 코드였던 Room을 생성할 때 초기 참여자 수를 1로 하고, 저장한다면 똑같이 해결되고 오히려 지금 코드보다 깔끔해진다 느껴집니다.

전에 조이썬한테 제가 먼저 이러한 구조로 바꿔보는거 좋을 것 같다고 동영상을 추천해줬던 것 같아요.
저도 다른 사람들을 통해 먼저 들었고, 개선할만한 부분이라 생각해서 추천했었습니다.

근데 나중에 다시 이 방식을 도입한 사람들에게 의견을 들었을 때, 큰 장점을 못 느꼈다는 말도 들었고,
저도 약간 ParticipationWriter에서 그러한 느낌을 받을 수 있네요.

솔직히 말하자면 ParitipationWriter는 필요해서 나온 느낌이 아니라 그냥 써보고 싶어서 만든 느낌이랄까요??

다른 영역에서 자주 사용되는 Room이나 Member 같은 경우는 다음과 같이 사용하면 의미가 있다 느껴지네요~
이 부분은 조이썬도 한번 고민해보면 좋겠습니다! 👍

Copy link
Contributor

@youngsu5582 youngsu5582 Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

제가 느낄 수 있는 한가지 ParticipationWriter의 장점으로는 방을 참여할 때, participation.participate();를 같이 호출해주어 개발자로 하여금 휴먼 에러를 방지한다?? 정도가 있겠네요.

사실, 해당 부분이 Writer 가 분리되는 이유라고 생각해요.
나중에, participation.participate() 를 room 의 size 를 올리지 말고, 엔티티 개수로 파악을 하자고 로직이 변경이 되어도
그때는 온전히 particiaptionWriter 에서만 로직이 수정되게 됩니다.

저희의 처음 코드였던 Room을 생성할 때 초기 참여자 수를 1로 하고, 저장한다면 똑같이 해결되고 오히려 지금 코드보다 깔끔해진다 느껴집니다.

해당 내용도, 엔티티 라면 비즈니스 변화에 의존되지 않게 코드를 사용해야 한다고 생각해요 (+ 생성자도요)
1이라고 했으나, 나중에 방장은 참여자에서 빼자라고 했을때
이 room 도메인을 1로 지정 했던거도 기억하는 거 자체가 비즈니스 로직이 도메인 까지 침범이 된거라고 생각이 들어요.

저는 이런 관점에서 Reader / Writer 의 필요성을 명확하게 느낀거 같아요.
추가로, 이렇게 객체를 넣으면서 코드를 짜며 느낀점으론 좀 더 코드를 생각할 수 있었어요 ( 물론, 제 기준 )
Reader 와 Writer 가 최대한 재사용 될 수 있게, "해당 객체와 Reader 의 의존성이 맞는가?" 등등

추가로, 지금 당장은 그냥 로직을 넣은게 다지만, ( 사실 이거 하나로도 만족스럽긴 해요 전 - 에러 일괄적 관리 + 로그 관리 )
비즈니스 로직이나 검증이 늘어나도
Reader / Writer 내 기능이 늘어나지 Service 의 길이가 길어지지 않는게 매력적으로 느껴졌어요.

솔직히 말하자면 ParitipationWriter는 필요해서 나온 느낌이 아니라 그냥 써보고 싶어서 만든 느낌이랄까요??
다른 영역에서 자주 사용되는 Room이나 Member 같은 경우는 다음과 같이 사용하면 의미가 있다 느껴지네요~

해당 부분 역시도 DB 의 원자성과 비슷하다고 생각해요.
모든 곳에 도입하거나, 도입 하지 않거나가 궁극적으로 되야 할 거 같아요.

이들은 물론 다 제 의견이니 더 생각 달아줘도 좋습니당

Copy link
Contributor

Choose a reason for hiding this comment

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

에러 일괄적 관리 + 로그 관리
비즈니스 로직이나 검증이 늘어나도
Reader / Writer 내 기능이 늘어나지 Service 의 길이가 길어지지 않는게 매력적으로 느껴졌어요.

ParticipationWriter에 대한 조이썬 의견 궁금했는데, 좋습니다~!
로그 관리가 된다는 장점도 있었네요.

팀 안에서도 로그는 엔티티 내부가 아닌 Writer, Service로 한정하도록 컨벤션 정해봐도 좋을듯 하네요!

모든 곳에 도입하거나, 도입 하지 않거나가 궁극적으로 되야 할 거 같아요.

저도 이 부분 동의합니다!
저도 관리할 부분이 많은 곳은 Writer, Reader 쓰는게 좋다 느껴지긴 하니, 다 쓰는 쪽으로 해보는 게 좋겠네요!

저도 앞으로 리팩토링 할때는 최대한 Service가 Repo를 의존하지 않는 방향으로 같이 리팩터링 해보겠습니다~

Copy link
Contributor

@ashsty ashsty Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

엔티티/생성자는 비즈니스 변화에 의존하지 않아야 한다고 생각해요. (+ 생성자도요) 나중에 방장의 자동 참여 기능이 빠진다고 했을때 이 room 도메인의 참여자 수를 1로 지정했던 사실을 기억해야 한다는 것 자체가 비즈니스 로직이 도메인� 영역을 침범한 경우라고 생각해요.

가볍게 추가만 하자면 저도 이 말에 동의하는 것 같습니다~

이번에 Participation 리팩토링 과정에서 기본 설정값(MemberRole, ParticipationStatus)을 가지는 생성자를 만들었었는데요. 작성자인 저는 제법 명확한 의미를 담고 있다고 생각했던 부분도 다른 사람의 눈으로 봤을 때는 "저게 뭐야?" 싶은 것 같더라고요. 😆

기본 참여자를 1로 설정한다는 건 물론 지금 단계에서는 깔끔한 코드처럼 보이고 실제로 깔끔한 코드를 작성할 수 있게 해주지만,
시간이 지나 이 코드를 누군가 리팩터링해야 한다는 입장에서 보았을 때는 다소 의문일 수도 있겠어요. (정책과 결부되어 있기 때문)

그런 관점에서 Writer를 분리하고 create를 오버로딩한 지금의 리팩터링 방향이 상당히 좋은 것 같다고 느꼈습니다. 👍

전체적으로 이런 방향의 리팩터링이 이루어지고 전체 코드의 구조가 일관적이 된다고 하면 더할 나위 없겠네요.
같은 맥락에서 로깅이 들어가는 클래스를 Writer, Reader로 한정하는 데 동의하고요.

그런데 조금만 덧붙이자면... 이런 리팩터링이 이루어졌을 때 이전에 테스트하던 메서드가 주석 처리 되는 경우가 종종 있는 것 같아요.

그 때는 가능하다면 이 테스트가 주석 처리된 이유도 함께 적어주셨으면 좋겠습니다. 이번에 베타 테스트로 QA하면서도 느꼈지만 테스트가 잘 작성되었다면 사전에 확인할 수 있었을 것 같은 오류가 꽤 많더라고요. 그런 관점에서 보았을 때 리팩터링 이후 이전 테스트 로직이 비활성화되는 건 조금 의문스럽게 느껴지는 것 같아요. (그렇다면 이 로직이 정확히 이전과 똑같이 동작하는지는 뭘 보고 확신할 수 있지?)

리팩터링은 기본적으로 '이전의 로직과 동일한 동작을 하지만 로직만 교체되는 일'인 만큼

"이전에 테스트하던 걸 왜 이제는 테스트하지 않아도 되는 지"에 대해 코드를 보는 사람도 한 눈에 알 수 있으면 바뀐 이후에도 오류가 나지 않을 수 있을 거라는 확신이 더 강해질 것 같기 때문입니다~~! ☺️

이번에 생성자 관련해서 남긴 리뷰도 같은 결이었던 것 같아요!

리팩터링은 정말 너무너무 귀찮고 힘든 일인 걸 알고 있기 때문에 다들 고생하고 계신 거 너무 잘 알고 있습니다~~ 만!
이런 점을 혹시나! 한 번만! 더 부탁드려도 될지! 에 대한 이야기였어요~!! 감사합니다!

요약: 리팩터링할 때 테스트도 신경 쓰자 (작성자도 리뷰어도 다같이) (뭐라하는 거 아님!!)

Copy link
Contributor

Choose a reason for hiding this comment

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

말 안했던 건 제 잘못인거 같네용.
다음부터는 테스트 주석 할 때 사유 미리 남겨놓을게용

주석한 사유는 그전 PR 에 남겼으므로 생략하겠습니다.

roomRepository.delete(room);
participationRepository.deleteAllByRoomId(roomId);
roomAutomaticService.deleteAutomatic(room);
Expand Down
4 changes: 2 additions & 2 deletions backend/src/main/resources/logback-spring.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
<springProfile name="dev">
<include resource="logs/loki-appender.xml"/>

<logger name="corea" level="DEBUG">
<logger name="corea" level="DEBUG" additivity="false">
<appender-ref ref="LOKI"/>
</logger>

Expand All @@ -42,7 +42,7 @@
<include resource="logs/file-warn-appender.xml"/>
<include resource="logs/file-error-appender.xml"/>

<logger name="corea" level="INFO">
<logger name="corea" level="INFO" additivity="false">
<appender-ref ref="LOKI"/>
<appender-ref ref="FILE-INFO"/>
<appender-ref ref="FILE-WARN"/>
Expand Down
12 changes: 7 additions & 5 deletions backend/src/test/java/corea/matching/domain/ParticipationFilterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import corea.member.domain.Member;
import corea.member.domain.MemberRole;
import corea.participation.domain.Participation;
import corea.participation.domain.ParticipationStatus;
import corea.room.domain.Room;
import org.assertj.core.api.InstanceOfAssertFactories;
import org.junit.jupiter.api.DisplayName;
Expand All @@ -33,10 +34,11 @@ class ParticipationFilterTest {
@Test
@DisplayName("방 참여자의 수가 매칭 사이즈보다 작거나 같다면 예외가 발생한다.")
void invalid() {

Room movinRoom = RoomFixture.ROOM_DOMAIN(1L, movin);
List<Participation> participations = List.of(
new Participation(RoomFixture.ROOM_DOMAIN(1L, pororo), pororo),
new Participation(RoomFixture.ROOM_DOMAIN(1L, movin), movin)
);
new Participation(movinRoom, pororo,MemberRole.REVIEWER, ParticipationStatus.MANAGER, movinRoom.getMatchingSize()),
new Participation(movinRoom, movin,MemberRole.REVIEWER, ParticipationStatus.MANAGER, movinRoom.getMatchingSize()));

assertThatThrownBy(() -> new ParticipationFilter(participations, 2))
.asInstanceOf(InstanceOfAssertFactories.type(CoreaException.class))
Expand All @@ -51,7 +53,7 @@ void filterPRSubmittedParticipation() {
List<Participation> participations = List.of(
new Participation(room, pororo, MemberRole.BOTH, 2),
new Participation(room, movin, MemberRole.BOTH, 2),
new Participation(room, joyson),
new Participation(room, joyson,MemberRole.REVIEWER, ParticipationStatus.MANAGER, 2),
new Participation(room, choco, MemberRole.BOTH, 2)
);

Expand All @@ -70,7 +72,7 @@ void filterPRSubmittedParticipation() {
void validatePRSubmittedParticipationSize() {
Room room = RoomFixture.ROOM_DOMAIN(1L, pororo);
List<Participation> participations = List.of(
new Participation(room, pororo),
new Participation(room, pororo,MemberRole.REVIEWER, ParticipationStatus.MANAGER, room.getMatchingSize()),
new Participation(room, movin, MemberRole.BOTH, 2),
new Participation(room, choco, MemberRole.BOTH, 2)
);
Expand Down
4 changes: 3 additions & 1 deletion backend/src/test/java/corea/participation/service/ParticipationServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
import corea.fixture.MemberFixture;
import corea.fixture.RoomFixture;
import corea.member.domain.Member;
import corea.member.domain.MemberRole;
import corea.member.repository.MemberRepository;
import corea.participation.domain.Participation;
import corea.participation.domain.ParticipationStatus;
import corea.participation.dto.ParticipationRequest;
import corea.participation.repository.ParticipationRepository;
import corea.room.domain.Room;
Expand Down Expand Up @@ -92,7 +94,7 @@ void cancel_participate() {
Member manager = memberRepository.save(MemberFixture.MEMBER_ROOM_MANAGER_JOYSON());
Member member = memberRepository.save(MEMBER_YOUNGSU());
Room room = roomRepository.save(RoomFixture.ROOM_DOMAIN(manager));
participationRepository.save(new Participation(room, member));
participationRepository.save(new Participation(room, member, MemberRole.REVIEWER, ParticipationStatus.MANAGER, room.getMatchingSize()));

participationService.cancel(room.getId(), member.getId());

Expand Down
4 changes: 3 additions & 1 deletion backend/src/test/java/corea/room/repository/RoomRepositoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
import corea.fixture.MemberFixture;
import corea.fixture.RoomFixture;
import corea.member.domain.Member;
import corea.member.domain.MemberRole;
import corea.member.repository.MemberRepository;
import corea.participation.domain.Participation;
import corea.participation.domain.ParticipationStatus;
import corea.participation.repository.ParticipationRepository;
import corea.room.domain.Room;
import corea.room.domain.RoomClassification;
Expand Down Expand Up @@ -55,7 +57,7 @@ void findAllByMemberAndStatus_participated() {
Room pororoRoom = roomRepository.save(RoomFixture.ROOM_DOMAIN(pororo, LocalDateTime.now().plusDays(2)));
roomRepository.save(RoomFixture.ROOM_DOMAIN(joyson, LocalDateTime.now().plusDays(3)));

participationRepository.save(new Participation(pororoRoom, pororo));
participationRepository.save(new Participation(pororoRoom, pororo, MemberRole.REVIEWER, ParticipationStatus.MANAGER,pororoRoom.getMatchingSize()));
Page<Room> roomPage = roomRepository.findAllByStatusOrderByRecruitmentDeadline(RoomStatus.OPEN, PageRequest.of(0, 8));

List<String> managerNames = getManagerNames(roomPage.getContent());
Expand Down
3 changes: 2 additions & 1 deletion backend/src/test/java/corea/room/service/RoomInquiryServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import corea.member.domain.MemberRole;
import corea.member.repository.MemberRepository;
import corea.participation.domain.Participation;
import corea.participation.domain.ParticipationStatus;
import corea.participation.repository.ParticipationRepository;
import corea.room.domain.Room;
import corea.room.domain.RoomStatus;
Expand Down Expand Up @@ -50,7 +51,7 @@ void findRoomsWithRoomStatus_login_member(RoomStatus roomStatus) {

roomRepository.save(RoomFixture.ROOM_DOMAIN(pororo, LocalDateTime.now().plusDays(2), roomStatus));
Room ashRoom = roomRepository.save(RoomFixture.ROOM_DOMAIN(ash, LocalDateTime.now().plusDays(3), roomStatus));
participationRepository.save(new Participation(ashRoom, ash));
participationRepository.save(new Participation(ashRoom, ash,MemberRole.REVIEWER, ParticipationStatus.MANAGER, ashRoom.getMatchingSize()));

RoomResponses response = roomInquiryService.findRoomsWithRoomStatus(pororo.getId(), 0, "all", roomStatus);
List<String> managerNames = getManagerNames(response);
Expand Down
Loading
Loading