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

Feature/#49 내 명함에서 활동을 추가 삭제하는 기능 #52

Merged
Show file tree
Hide file tree
Changes from all 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
32 changes: 32 additions & 0 deletions backend/emm-sale/src/docs/asciidoc/index.adoc
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
= EMM-SALE API Docs
:doctype: book
:icons: font
:source-highlighter: highlightjs
:toc: left
:toclevels: 2
:sectlinks:

== Product
Copy link
Collaborator

Choose a reason for hiding this comment

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

Product는 이전 기획과 관련된 명세이니 지워도 괜찮을 것 같습니다!

Copy link
Collaborator Author

@hong-sile hong-sile Jul 23, 2023

Choose a reason for hiding this comment

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

RestDocs를 위한 예시 코드였어요. 삭제하도록 할게요!

Expand Down Expand Up @@ -87,3 +89,33 @@ include::{snippets}/wish-with-no-authentication-products/request-headers.adoc[]

.HTTP response
include::{snippets}/wish-with-no-authentication-products/http-response.adoc[]

== Career
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT : '커리어'라는 이름을 봤을 때 정확히 어떤 기능이 포함될지 예상하기 어려운 것 같아요(직관성이 떨어지는 것 같습니다)
API 명세는 읽기 위한 문서이니 저희 팀의 고유 명사를 확실히 정해서 조금 더 이해하기 쉽게 서술하면 좋을 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

고유명사를 다같이 논의를 해봐야할 것 같네요. 월요일에 논의해봐요!

=== `POST`: 커리어 등록 API

.HTTP request 설명
include::{snippets}/add-career/request-fields.adoc[]

.HTTP request
include::{snippets}/add-career/http-request.adoc[]

.HTTP response
include::{snippets}/add-career/http-response.adoc[]

.HTTP response 설명
include::{snippets}/add-career/response-fields.adoc[]


=== `DELETE`: 커리어 삭제 API

.HTTP request 설명
include::{snippets}/delete-career/request-fields.adoc[]

.HTTP request
include::{snippets}/delete-career/http-request.adoc[]

.HTTP response
include::{snippets}/delete-career/http-response.adoc[]

.HTTP response 설명
include::{snippets}/delete-career/response-fields.adoc[]
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
package com.emmsale.member.api;

import com.emmsale.member.application.MemberCareerService;
import com.emmsale.member.application.dto.MemberCareerDeleteRequest;
import com.emmsale.member.application.dto.MemberCareerInitialRequest;
import com.emmsale.member.application.dto.MemberCareerAddRequest;
import com.emmsale.member.application.dto.MemberCareerResponse;
import com.emmsale.member.domain.Member;
import java.util.List;
import lombok.RequiredArgsConstructor;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RestController;
Expand All @@ -23,4 +29,21 @@ public ResponseEntity<Void> register(
memberCareerService.registerCareer(member, memberCareerInitialRequest);
return ResponseEntity.noContent().build();
}

@PostMapping("/members/careers")
public ResponseEntity<List<MemberCareerResponse>> addCareer(
final Member member,
@RequestBody final MemberCareerAddRequest memberCareerAddRequest
) {
return ResponseEntity.status(HttpStatus.CREATED)
.body(memberCareerService.addCareer(member, memberCareerAddRequest));
}

@DeleteMapping("/members/careers")
public ResponseEntity<List<MemberCareerResponse>> deleteCareer(
final Member member,
@RequestBody final MemberCareerDeleteRequest memberCareerDeleteRequest
) {
return ResponseEntity.ok(memberCareerService.deleteCareer(member, memberCareerDeleteRequest));
}
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
package com.emmsale.member.application;

import static java.util.stream.Collectors.toList;

import com.emmsale.career.domain.CareerRepository;
import com.emmsale.member.application.dto.MemberCareerAddRequest;
import com.emmsale.member.application.dto.MemberCareerDeleteRequest;
import com.emmsale.member.application.dto.MemberCareerInitialRequest;
import com.emmsale.member.application.dto.MemberCareerResponse;
import com.emmsale.member.domain.Member;
import com.emmsale.member.domain.MemberCareer;
import com.emmsale.member.domain.MemberCareerRepository;
import com.emmsale.member.exception.MemberException;
import com.emmsale.member.exception.MemberExceptionType;
import java.util.List;
import java.util.stream.Collectors;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
Expand All @@ -19,17 +25,61 @@ public class MemberCareerService {
private final MemberCareerRepository memberCareerRepository;
private final CareerRepository careerRepository;

public void registerCareer(final Member member, final MemberCareerInitialRequest memberCareerInitialRequest) {
public void registerCareer(
final Member member,
final MemberCareerInitialRequest memberCareerInitialRequest
) {
final List<Long> careerIds = memberCareerInitialRequest.getCareerIds();
saveMemberCareers(member, careerIds);

member.updateName(memberCareerInitialRequest.getName());
}

private void saveMemberCareers(final Member member, final List<Long> careerIds) {
final List<MemberCareer> memberCareers = careerRepository.findAllById(careerIds)
.stream()
.map(it -> new MemberCareer(it, member))
.collect(Collectors.toList());
.collect(toList());

member.updateName(memberCareerInitialRequest.getName());
validateAllCareerIdsExist(careerIds, memberCareers);

memberCareerRepository.saveAll(memberCareers);
}

private void validateAllCareerIdsExist(
final List<Long> careerIds,
final List<MemberCareer> memberCareers
) {
if (memberCareers.size() != careerIds.size()) {
throw new MemberException(MemberExceptionType.INVALID_CAREER_IDS);
}
}

public List<MemberCareerResponse> addCareer(
Copy link
Collaborator

Choose a reason for hiding this comment

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

이건 개인적인 코드 취향입니다!!

public 메서드들 사이에 private 메서드가 껴있는게 저는 조금 어색하게 느껴집니다.

특히, saveMemberCareers 메서드의 경우 registerCareer에서도 사용되지만 addCareer에서도 사용되고 있는데 registerCareer 밑에, addCarrer 위에 위치해서 더욱 어색하게 느껴지는 것 같아요.

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 private 기준으로 분류할 필요는 없다고 생각하거든요.

위에서 아래로 읽었을 때 바로 인지할 수 있으면 된다고 생각해요.

코드 배치정도까지는 각자 편한 스타일로 작성하는건 어떨까요?

final Member member,
final MemberCareerAddRequest memberCareerAddRequest
) {
final List<Long> careerIds = memberCareerAddRequest.getCareerIds();
saveMemberCareers(member, careerIds);

return MemberCareerResponse.from(memberCareerRepository.findAllByMember(member));
}

public List<MemberCareerResponse> deleteCareer(
final Member member,
final MemberCareerDeleteRequest memberCareerDeleteRequest
) {
final List<Long> deleteCareerIds = memberCareerDeleteRequest.getCareerIds();

final List<Long> savedMemberCareerIds =
memberCareerRepository.findAllByMemberAndCareerIds(member, deleteCareerIds)
.stream()
.map(MemberCareer::getId)
.collect(toList());

memberCareerRepository.deleteAllByIdInBatch(savedMemberCareerIds);

return MemberCareerResponse.from(memberCareerRepository.findAllByMember(member));
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.emmsale.member.application.dto;

import lombok.Getter;
import lombok.RequiredArgsConstructor;

@Getter
@RequiredArgsConstructor
public class MemberActivityResponse {

private final Long id;
private final String name;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.emmsale.member.application.dto;

import java.util.List;
import lombok.Getter;
import lombok.RequiredArgsConstructor;

@Getter
@RequiredArgsConstructor
public class MemberCareerAddRequest {

private final List<Long> careerIds;

private MemberCareerAddRequest() {
this(null);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.emmsale.member.application.dto;

import java.util.List;
import lombok.Getter;
import lombok.RequiredArgsConstructor;

@Getter
@RequiredArgsConstructor
public class MemberCareerDeleteRequest {

private final List<Long> careerIds;

private MemberCareerDeleteRequest() {
this(null);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package com.emmsale.member.application.dto;

import static java.util.Comparator.comparing;
import static java.util.stream.Collectors.groupingBy;
import static java.util.stream.Collectors.toList;

import com.emmsale.career.domain.ActivityType;
import com.emmsale.career.domain.Career;
import com.emmsale.member.domain.MemberCareer;
import java.util.ArrayList;
import java.util.EnumMap;
import java.util.List;
import java.util.Map.Entry;
import lombok.Getter;
import lombok.RequiredArgsConstructor;

@Getter
@RequiredArgsConstructor
public class MemberCareerResponse {

private final String activityName;
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기 activityName과 아래 MemberActivityResponsename 필드만 봐선 도메인 지식이 없으면 헷갈릴 것 같아요. 나중에 재정의 할 필요가 있어 보입니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

말씀해주신 것처럼 activity career라는 용어는 인지하기 어려울 것 같아요. 심지어 이번에 직무도 빠졌으니까 조금 더 애매한 용어가 되버린 것 같네요. 고유명사에 대한 논의를 한 번 해보고 크게 리팩터링 해봐야할 것 같네요.

private final List<MemberActivityResponse> memberActivityResponses;

public static List<MemberCareerResponse> from(final List<MemberCareer> memberCareers) {
final EnumMap<ActivityType, List<Career>> groupByActivityType =
groupingByActivityTypeAndSortedByCareerName(memberCareers);
Copy link
Collaborator

Choose a reason for hiding this comment

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

코드가 아름답네요 b


final List<MemberCareerResponse> responses = new ArrayList<>();

for (final Entry<ActivityType, List<Career>> entry : groupByActivityType.entrySet()) {
final List<MemberActivityResponse> activityResponse =
mapToMemberActivityResponses(entry);

responses.add(new MemberCareerResponse(entry.getKey().getValue(), activityResponse));
}

return responses;
}

private static List<MemberActivityResponse> mapToMemberActivityResponses(
final Entry<ActivityType, List<Career>> entry
) {
return entry.getValue()
.stream()
.map(it -> new MemberActivityResponse(it.getId(), it.getName()))
.collect(toList());
}

private static EnumMap<ActivityType, List<Career>> groupingByActivityTypeAndSortedByCareerName(
Copy link
Collaborator

Choose a reason for hiding this comment

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

from() 메서드에서 mapToMemberActivityResponses()보다 groupingByActivityTypeAndSortedByCareerName()가 먼저 사용되었으니 두 메서드의 순서를 바꿔주는 편이 가독성 측면에서 더 좋을 것 같아요!

로직은 흠잡을 데 없는 것 같습니다👍👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

앗 이 피드백을 미쳐 못보고 머지해버렸네요. 추가로 하나 PR 파서 빠르게 수정하고 머지하도록 하겠습니다.

final List<MemberCareer> memberCareers
) {
return memberCareers
.stream()
.map(MemberCareer::getCareer)
.sorted(comparing(career -> career.getName().toLowerCase()))
.collect(
groupingBy(Career::getActivityType, () -> new EnumMap<>(ActivityType.class), toList())
);
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,20 @@
package com.emmsale.member.domain;

import java.util.List;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.Query;
import org.springframework.data.repository.query.Param;

public interface MemberCareerRepository extends JpaRepository<MemberCareer, Long> {

@Query("select mc from MemberCareer mc where mc.member = :member")
List<MemberCareer> findAllByMember(@Param("member") final Member member);

@Query("select mc from MemberCareer mc "
+ "where mc.member = :member "
+ "and mc.career.id in :deleteCareerIds")
List<MemberCareer> findAllByMemberAndCareerIds(
@Param("member") final Member member,
@Param("deleteCareerIds") final List<Long> deleteCareerIds
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ public enum MemberExceptionType implements BaseExceptionType {
NOT_FOUND_MEMBER(
HttpStatus.NOT_FOUND,
"해당 멤버는 존재하지 않습니다."
),

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

private final HttpStatus httpStatus;
Expand Down
16 changes: 16 additions & 0 deletions backend/emm-sale/src/main/resources/http/member.http
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,19 @@ Content-Type: application/json
"name": "우르",
"careerIds": [1, 2]
}

### 사용자 커리어 추가
POST http://localhost:8080/members/careers
Content-Type: application/json

{
"careerIds": [4, 5, 6]
}

### 사용자 커리어 제거
DELETE http://localhost:8080/members/careers
Content-Type: application/json

{
"careerIds": [1, 2]
}
Loading
Loading