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

Fix/#232 프로필에 동일한 활동을 중복해서 등록할 수 있는 버그 #250

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ public void registerActivities(
final Member member,
final MemberActivityInitialRequest memberActivityInitialRequest
) {
if (member.isOnboarded()) {
throw new MemberException(MemberExceptionType.ALREADY_ONBOARDING);
}
final List<Long> activityIds = memberActivityInitialRequest.getActivityIds();
saveMemberActivities(member, activityIds);

Expand Down Expand Up @@ -62,11 +65,30 @@ public List<MemberActivityResponses> addActivity(
final MemberActivityAddRequest memberActivityAddRequest
) {
final List<Long> activityIds = memberActivityAddRequest.getActivityIds();
final List<MemberActivity> memberActivities = memberActivityRepository.findAllByMember(member);
if (isAlreadyExistActivity(memberActivities, activityIds) || hasDuplicateId(memberActivities,
hong-sile marked this conversation as resolved.
Show resolved Hide resolved
activityIds)) {
throw new MemberException(MemberExceptionType.ALREADY_EXIST_ACTIVITY);
}
saveMemberActivities(member, activityIds);

return MemberActivityResponses.from(memberActivityRepository.findAllByMember(member));
}

private boolean isAlreadyExistActivity(final List<MemberActivity> memberActivities,
final List<Long> activityIds) {
return memberActivities
.stream()
.anyMatch(memberActivity ->
activityIds.contains(memberActivity.getActivity().getId())
);
}

private boolean hasDuplicateId(final List<MemberActivity> memberActivities,
final List<Long> activityIds) {
return activityIds.stream().distinct().count() != memberActivities.size();
Copy link
Collaborator

Choose a reason for hiding this comment

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

activityIds가 중복으로 들어오는 경우도 있는건가요??
distinct 를 해주신 이유가 궁긍합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

현재 UI 상으로는 활동을 중복으로 선택하는 것이 불가능하기에 정상적인 경우 중복 데이터가 요청으로 들어올 일은 없을겁니다!
하지만 악의적으로 중복된 활동 id를 포함한 데이터를 담아 API 호출을 하는 경우, 예외를 반환하는 것이 옳은 대처라고 생각해서 유효성 검증을 추가해주었습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

넵 그러면 distinct 보다는 Set 어떠신가요? Long 사용하는거라 자료구조를 사용하면 더 좋아보여요!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

반영하였습니다!

}

public List<MemberActivityResponses> deleteActivity(
final Member member,
final MemberActivityDeleteRequest memberActivityDeleteRequest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,21 @@ public enum MemberExceptionType implements BaseExceptionType {
"해당 멤버는 존재하지 않습니다."
),

ALREADY_ONBOARDING(
HttpStatus.BAD_REQUEST,
"이미 온보딩을 완료한 사용자입니다."
),

INVALID_ACTIVITY_IDS(
HttpStatus.BAD_REQUEST,
"요청한 activity id들 중에 유효하지 않은 값이 존재합니다"
),

ALREADY_EXIST_ACTIVITY(
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기서 이미 존재한다고 하는 것 보다, 이미 등록된 활동입니다. 정도로 표현하는건 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

반영했습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

그거 그렇게 하는거 아닌데...

HttpStatus.BAD_REQUEST,
"요청한 activity id는 이미 존재합니다."
),

NULL_DESCRIPTION(
HttpStatus.BAD_REQUEST,
"한 줄 자기소개는 null이 될 수 없습니다."
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.emmsale.member.exception.MemberException;
import com.emmsale.member.exception.MemberExceptionType;
import java.util.List;
import org.assertj.core.api.ThrowableAssert.ThrowingCallable;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down Expand Up @@ -49,6 +50,31 @@ void registerActivities() throws Exception {
);
}

@Test
@DisplayName("특정 사용자에 대해 이미 등록되어 있는 Activity를 등록하려고 하면 예외를 반환한다.")
void registerActivities_fail() throws Exception {
//given
final List<Long> activityIds = List.of(1L, 2L, 3L, 4L);
final long savedMemberId = 1L;

final Member member = memberRepository.findById(savedMemberId).get();
final String updateName = "우르";

final MemberActivityInitialRequest request = new MemberActivityInitialRequest(updateName,
activityIds);

// when
memberActivityService.registerActivities(member, request);
final ThrowingCallable actual = () -> memberActivityService.registerActivities(member,
request);

// then
assertThatThrownBy(actual)
.isInstanceOf(MemberException.class)
.hasMessage(MemberExceptionType.ALREADY_ONBOARDING.errorMessage());

}

@Test
@DisplayName("Activity의 id를 통해서, 사용자의 Activity에 추가할 수 있다.")
void addActivity() throws Exception {
Expand Down Expand Up @@ -82,7 +108,6 @@ void addActivity() throws Exception {

//when
final List<MemberActivityResponses> actual = memberActivityService.addActivity(member, request);

//then
assertThat(expected)
.usingRecursiveComparison()
Expand All @@ -95,7 +120,7 @@ void addActivity() throws Exception {
void test_addActivity_invalid_activity_ids_Exception() throws Exception {
//given
final Member savedMember = memberRepository.findById(1L).get();
final List<Long> activityIds = List.of(1L, 2L, 7L);
final List<Long> activityIds = List.of(4L, 5L, 7L);
final MemberActivityAddRequest request = new MemberActivityAddRequest(activityIds);

//when & then
Expand All @@ -104,6 +129,34 @@ void test_addActivity_invalid_activity_ids_Exception() throws Exception {
.hasMessage(MemberExceptionType.INVALID_ACTIVITY_IDS.errorMessage());
}

@Test
@DisplayName("addActivity() : 이미 존재하는 activityId들이 있으면 ALREADY_EXIST_ACTIVITY Exception이 발생합니다.")
void test_addActivity_ALREADY_EXIST_ACTIVITY_Exception_duplicate_try() throws Exception {
//given
final Member savedMember = memberRepository.findById(1L).get();
final List<Long> activityIds = List.of(1L, 2L, 7L);
final MemberActivityAddRequest request = new MemberActivityAddRequest(activityIds);

// when, then
assertThatThrownBy(() -> memberActivityService.addActivity(savedMember, request))
.isInstanceOf(MemberException.class)
.hasMessage(MemberExceptionType.ALREADY_EXIST_ACTIVITY.errorMessage());
}

@Test
@DisplayName("addActivity() : 중복되는 Activity Id가 포함되어 있으면 ALREADY_EXIST_ACTIVITY Exception이 발생합니다.")
void test_addActivity_ALREADY_EXIST_ACTIVITY_Exception_duplicate_input() throws Exception {
//given
final Member savedMember = memberRepository.findById(1L).get();
final List<Long> activityIds = List.of(4L, 4L, 5L);
final MemberActivityAddRequest request = new MemberActivityAddRequest(activityIds);

// when, then
assertThatThrownBy(() -> memberActivityService.addActivity(savedMember, request))
.isInstanceOf(MemberException.class)
.hasMessage(MemberExceptionType.ALREADY_EXIST_ACTIVITY.errorMessage());
}

@Test
@DisplayName("deleteActivity() : member Id와 삭제할 activity Id를 통해서 사용자의 Activity를 삭제할 수 있다.")
void test_deleteActivity() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class MemberQueryServiceTest extends ServiceIntegrationTestHelper {
@DisplayName("사용자를 조회하고 조회 결과를 반환한다.")
void findOrCreateMemberTest() {
//given
final MemberQueryResponse expectResponse = new MemberQueryResponse(1L, true);
final MemberQueryResponse expectResponse = new MemberQueryResponse(1L, false);

final GithubProfileResponse githubProfileFromGithub = new GithubProfileResponse("1", "name",
"username", "https://imageUrl.com");
Expand Down Expand Up @@ -67,7 +67,7 @@ class FindProfile {
void findProfile_success() {
//given
final Long memberId = 1L;
final MemberProfileResponse expectResponse = new MemberProfileResponse(memberId, "member1",
final MemberProfileResponse expectResponse = new MemberProfileResponse(memberId, null,
"", "https://imageurl.com");

//when
Expand Down
2 changes: 1 addition & 1 deletion backend/emm-sale/src/test/resources/data-test.sql
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ insert into activity(id, type, name)
values (6, 'JOB', 'Backend');

insert into member(id, name, image_url, open_profile_url, github_id, created_at, updated_at)
values (1, 'member1', 'https://imageurl.com', 'https://openprofileurl.com', 1, CURRENT_TIMESTAMP(),
values (1, null, 'https://imageurl.com', 'https://openprofileurl.com', 1, CURRENT_TIMESTAMP(),
CURRENT_TIMESTAMP());

insert into member(id, name, image_url, open_profile_url, github_id, created_at, updated_at)
Expand Down
Loading