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

Api: ✨ 사용자 정의 카테고리 이동 API #125

Merged
merged 21 commits into from
Jul 31, 2024

Conversation

asn6878
Copy link
Member

@asn6878 asn6878 commented Jul 10, 2024

작업 이유

Image 1 Image 2

커스텀 카테고리를 삭제하기전에 해당 카테고리에 존재하는 지출내역들을 다른 카테고리로 이동시키는 API의 필요.

작업 사항

API Spec

  • url : PATCH /v2/spending-categories/{fromCategoryId}/migration?from-?type=&to-id=&to-type=
  • spending_category_id : 현재 선택된 카테고리 아이디
  • from-type : spending_category_id의 카테고리 유형
  • to-id : 옮길 카테고리 아이디
  • to-type : 옮길 카테고리 유형
  • pre-condition : 사용자는 로그인 한 상태이고, 삭제하고자 하는 사용자 정의 카테고리와 이전하고자 하는 사용자 정의 카테고리의 작성자여야 한다.

결과 응답

{
  "code" : "2000",
  "data" : { }
}

핵심 로직

간략하게 핵심 로직만 설명하겠습니다.

~~ 기본 카테고리를 삭제해 소비내역을 이동시키는 경우는 없기에, 크게 두가지 경우가 존재합니다. ~~
기본 카테고리를 삭제하는 행위는 불가능하나, 추후 기본 카테고리 -> ?? 로 소비내역을 이동하는 기능도 생길 것을 고려해 본 API는 총 4가지 행위를 포함합니다.

  • 기본 카테고리 -> 커스텀 카테고리로 이전하는 경우
  • 기본 카테고리 -> 기본 카테고리로 이전하는 경우
  • 커스텀 카테고리 -> 커스텀 카테고리로 이전하는 경우
  • 커스텀 카테고리 -> 기본 카테고리로 이전하는 경우

리뷰어가 중점적으로 확인해야 하는 부분

하나의 요청이 총 4가지의 데이터 액세스 요청으로 분기되어 서비스 코드가 꽤나 지저분 해졌습니다,
읽기 복잡하실 것 같아 설명해드리면 코드의 if-else문 순서와 위에서 언급한 4가지 행위의 순서가 동일합니다.

    @Transactional
    public void migrateSpendings(Long fromId, SpendingCategoryType fromType, Long toId, SpendingCategoryType toType) {
        if (fromType.equals(SpendingCategoryType.DEFAULT)) {
            SpendingCategory fromCategory = SpendingCategory.fromCode(fromId.toString());
            if (toType.equals(SpendingCategoryType.CUSTOM)) {
                spendingService.migrateCategoryByCategoryId(fromCategory, toId);
            } else {
                SpendingCategory spendingCategory = SpendingCategory.fromCode(toId.toString());
                spendingService.migrateCategoryByCategory(fromCategory, spendingCategory);
            }
        } else {
            if (toType.equals(SpendingCategoryType.CUSTOM)) {
                spendingService.migrateCustomCategoryByCategoryId(fromId, toId);
            } else {
                SpendingCategory spendingCategory = SpendingCategory.fromCode(toId.toString());
                spendingService.migrateCustomCategoryByCategory(fromId, spendingCategory);
            }
        }
    }
  • url path 관련
    migrations 라는 용어 자체가 명사형태이긴 하지만 행위를 나타내는 듯 하여 path에 포함시켜야 할지 고민이었습니다. 어떻게 생각하시나요??

  • RequestBody, PathVariable 관련
    해당 동작을 수행하기 위해서 필요한 정보들은 아래와 같습니다.
    삭제하고자 하는 카테고리의 ID (fromCategoryId),
    이전하고자 하는 카테고리의 ID (toCategoryId),
    카테고리의 종류 (SpendingCategoryType)

여기서 fromCategory는 현재 수정하고자 하는 자원을 뜻하므로, PathVariable로 전달받고, 나머지 정보들은 ReqeustBody로 전달받고 있습니다. 다만 이 방식이 IOS팀이 추후 작업 시 혼동이 올 우려가 있어보여 모두다 RequestBody에 넣을까 고민중입니다.


발견한 이슈

X

@asn6878 asn6878 added the enhancement New feature or request label Jul 10, 2024
@asn6878 asn6878 self-assigned this Jul 10, 2024
@psychology50
Copy link
Member

일단 고민하고 계신 url 부분에 대해서만 의견 공유해볼게요!
성윤님이 전에 말씀하신 것처럼 지출 리스트를 "옮기는 작업"과 "삭제"하는 작업은 분리하기로 하셨으니, 요청 메서드는 DELETE가 아니라 fk를 수정하는 PATCH가 되어야 할 것 같습니다!

그리고 spending에 포커싱을 할 지, spending-category에 포커싱을 할 지에 따라 url이 좀 바뀔 거 같은데,
여기선 "현재 카테고리의 내역을 다른 카테고리로 옮긴다"는 행위를 의미하니 spending-category에 포커싱 해봐도 괜찮을 거 같아용.

저라면 다음과 같은 url을 써봤을 거 같긴 한데,
PATCH /v2/spending-categories/{spending_category_id}/migration?type=&to=&to-type=

  • spending_category_id : 현재 선택된 카테고리 아이디
  • type : spending_category_id의 카테고리 유형
  • to : 옮길 카테고리 아이디
  • to-type : 옮길 카테고리 유형

네이밍이 좀 구리긴 한데..여튼 어떻게 생각하시나요??
해당 url을 어떻게 할 지는 좀 더 대화가 필요할 거 같긴 하네요 ㅎㅎ

@asn6878
Copy link
Member Author

asn6878 commented Jul 14, 2024

요청 메서드는 DELETE가 아니라 fk를 수정하는 PATCH가 되어야 할 것 같습니다!

엥, 코드에는 PATCH를 썼는데 PR에는 DELETE라고 써놨네요...ㅋㅋㅋ 복붙하다 생긴 이슈같습니다.

spending-category에 포커싱 해봐도 괜찮을 거 같아용.

저도 이 부분은 "소비내역을 옮긴다" VS "카테고리에 존재하는 소비내역을 옮긴다" 둘중에 어떤 행위로 해석해야할지 고민하다가 Spending으로 중심을 잡았는데, 지금 생각해보니 조금 어색하네요. 변경하겠습니다ㅎ

저라면 다음과 같은 url을 써봤을 거 같긴 한데,
PATCH /v2/spending-categories/{spending_category_id}/migration?type=&to=&to-type=

저는 GET 요청을 제외하고는 가능하면 쿼리파라미터 사용을 지양해야한다는 생각에 ResponseBody로 작성하였습니다. 어떻게 생각하시나요??
또한 카테고리의 삭제는 커스텀 카테고리만 가능하기에, 현재 선택된 카테고리의 유형은 입력받지 않았는데, 혹시 제가 놓친부분이 있는지 알려주시면 감사하겠습니다ㅎ

@psychology50
Copy link
Member

저는 GET 요청을 제외하고는 가능하면 쿼리파라미터 사용을 지양해야한다는 생각에 ResponseBody로 작성하였습니다.

생각해보니 Query param은 주로 GET, DELETE 요청에서만 사용하긴 하네요. 잊고 있었습니다. ㅎㅎ
다만, 민감한 정보가 아니라면 굳이 사용해서 해가 될만한 요인은 없어보여요.
반드시 원칙 상으로 GET, DELETE 요청에만 Query Param을 써야한다는 건 없는 듯 하여...혹시나 그런 프로토콜이 정해져있다는 공식 문서가 있다면 저도 알려주세요! (진짜 모름)

카테고리의 삭제는 커스텀 카테고리만 가능하기에, 현재 선택된 카테고리의 유형은 입력받지 않았는데, 혹시 제가 놓친부분이 있는지 알려주시면 감사하겠습니다ㅎ

카테고리 삭제는 커스텀 카테고리만 가능한 게 맞습니다.
하지만 지금 구현하고 계신 건 카테고리를 이동하는 게 아닌가요??

커스텀 카테고리에 등록된 소비 내역 리스트가 기본 카테고리로 옮겨질 수 있어야 합니다!

@asn6878
Copy link
Member Author

asn6878 commented Jul 14, 2024

카테고리 삭제는 커스텀 카테고리만 가능한 게 맞습니다.

하지만 지금 구현하고 계신 건 카테고리를 이동하는 게 아닌가요??

커스텀 카테고리에 등록된 소비 내역 리스트가 기본 카테고리로 옮겨질 수 있어야 합니다!

엇, 지금 살짝 오해가 있으신 것 같은데, 제가 말한건 from 카테고리가 기본 카테고리인 경우를 배제 하였다는 뜻이었어요.
소비내역을 옮기는 행위 자체가 카테고리를 삭제할때 시작된다고 생각해서 아래의 소비내역 이동 케이스들만 고려했습니다!

  • 커스텀 카테고리 -> 기본 카테고리
  • 커스텀 카테고리 -> 커스텀 카테고리

혹시 제가 핀트를 못잡은걸까요...?? 진짜 순수하게 헷갈려서요ㅋㅋㅋㅋ😱

@psychology50
Copy link
Member

엇, 지금 살짝 오해가 있으신 것 같은데, 제가 말한건 from 카테고리가 기본 카테고리인 경우를 배제 하였다는 뜻이었어요.

앗, 맞네요 ㅎ.
근데 기본 카테고리에서 사용자 정의 카테고리로 옮기는 기능도 필요하긴 할 텐데,,,

지금 당장 해당 기능이 없으니 버려도 되긴 하지만, 혹시 그닥 어렵지 않다고 판단된다면 구현 부탁드리고
문제가 너무 복잡해진다고 판단하신다면, 일단 from 타입은 배제시키셔도 무방합니다.

나중에 필요할 때 수정해도 되니까요!

@asn6878 asn6878 marked this pull request as ready for review July 28, 2024 11:17
Copy link
Member

@psychology50 psychology50 left a comment

Choose a reason for hiding this comment

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

테케 보니 될 거 같긴 한데, 이야기해보고 싶은 게 있어서 일단 커멘트로 남깁니다~~

Copy link
Member

@psychology50 psychology50 left a comment

Choose a reason for hiding this comment

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

일단 승인해두겠습니다.
커멘트 마지막 답변 한 번만 더 부탁드릴게여

@asn6878 asn6878 merged commit 5566c29 into dev Jul 31, 2024
1 check passed
@asn6878 asn6878 deleted the feat/PW-410-migrate-category branch July 31, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants