카테고리 순서 변경

[HoeSeong123]

⚡️ 관련 이슈

close #966

📍주요 변경 사항

카테고리 순서를 변경할 수 있게 되었습니다.
이에 따라 카테고리 수정 로직이 크게 변경되었습니다.
모달 창에서 카테고리를 편집하는 경우 생성, 수정, 삭제가 한 번에 이루어지게 됩니다.

검증 로직

• 카테고리 CRUD에 대해 현재 진행중인 검증 로직은 다음과 같습니다.

카테고리 생성시 (템플릿 생성 화면에서 생성)

• 중복된 이름 검증 (DB에서 확인)
• 순서 검증 (마지막 순서로 들어왔는지)

카테고리 편집시 (모달창에서 생성, 수정, 삭제를 한 번에 하는 것)

• 중복된 이름 검증 (dto에서 확인)
• 순서 검증 (연속된 숫자인지)
• 수정 또는 삭제한 카테고리가 기본 카테고리인지 검증
• 수정 또는 삭제할 권한이 있는지 검증
• 삭제시 삭제할 카테고리가 템플릿을 가지고 있는지
• 동일한 카테고리에 대해 수정, 삭제를 동시에 하는 경우
• 정확하지 않은 개수의 카테고리에 대한 요청
  • ex) 2개의 카테고리가 있는데, 1개의 카테고리에 대해서만 수정 요청을 보낸 경우

혹시 더 추가로 검증할만한 내용이 있는지 확인해주시면 감사드리겠습니다.

🎸기타

• 파일 체인지가 26개인데 확인해야 하는 파일은 많지 않습니다.
  • MemberFixture가 기존에 2개 존재하고 있어 하나로 합쳐서 변경된 파일들이 많습니다.
• 중점적으로 확인해야 하는 파일은 다음과 같습니다.
  • CategoryController
  • CategoryService
  • CategoryServiceTest
  • CategoryControllerTest
• 실수로 한 번 날려버려서 처음부터 다시 작업했습니다. 혹시나 놓친 부분이 있는지 확인 부탁드립니다...

🍗 PR 첫 리뷰 마감 기한

12/26 23:59

[zeus6768]

코드 넘 깔끔해졌네요 ⚡⚡⚡

버그 발견해서 제안과 함께 RC 남겨요!!

[zeus6768]

⚡

로직이 잘못된 것 같아요. 기본 카테고리와 같은 ordinal을 갖는 카테고리가 생성 가능하네요.

ordinal + 1이 count와 일치해야 할 것 같아요~

해당 내용으로 테스트코드 추가하면 좋겠네용

[zeus6768]

코드가 너무 길어서 검증 로직을 CategoryValidationService로 분리하면 어떨까 제안해요.

@Service
@RequiredArgsConstructor
public class CategoryService {

    private final CategoryRepository categoryRepository;
    private final CategoryValidationService validationService;

    @Transactional
    public CreateCategoryResponse create(Member member, CreateCategoryRequest request) {
        validationService.validateDuplicatedCategory(request.name(), member);
        validationService.validateOrdinal(member, request);

        Category category = categoryRepository.save(createCategory(member, request));
        return CreateCategoryResponse.from(category);
    }

    @Transactional(readOnly = true)
    public FindAllCategoriesResponse findAllByMemberId(Long memberId) {
        return FindAllCategoriesResponse.from(categoryRepository.findAllByMemberIdOrderById(memberId));
    }

    @Transactional
    public void updateCategories(Member member, UpdateAllCategoriesRequest request) {
        validationService.validateDuplicateNameRequest(request);
        validationService.validateOrdinals(request);
        validationService.validateIds(request);

        createCategories(member, request);
        updateCategories(request.updateCategories(), member);
        deleteCategories(request.deleteCategoryIds(), member);

        validationService.validateCategoriesCount(member, request);
    }

    private void createCategories(Member member, UpdateAllCategoriesRequest request) {
        categoryRepository.saveAll(request.createCategories().stream()
                .map(createRequest -> new Category(createRequest.name(), member, createRequest.ordinal()))
                .toList());
    }

    private void updateCategories(List<UpdateCategoryRequest> updates, Member member) {
        updates.forEach(update -> {
            Category category = categoryRepository.fetchById(update.id());
            category.validateAuthorization(member);
            validationService.validateDefaultCategory(category);
            category.update(update.name(), update.ordinal());
        });
    }

    private void deleteCategories(List<Long> ids, Member member) {
        ids.forEach(id -> {
            Category category = categoryRepository.fetchById(id);
            category.validateAuthorization(member);
            validationService.validateDefaultCategory(category);
            validationService.validateHasTemplate(id);
            categoryRepository.deleteById(id);
        });
    }
}

package codezap.category.service;

import java.util.HashSet;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.springframework.stereotype.Service;

import codezap.category.domain.Category;
import codezap.category.dto.request.CreateCategoryRequest;
import codezap.category.dto.request.UpdateAllCategoriesRequest;
import codezap.category.dto.request.UpdateCategoryRequest;
import codezap.category.repository.CategoryRepository;
import codezap.global.exception.CodeZapException;
import codezap.global.exception.ErrorCode;
import codezap.member.domain.Member;
import codezap.template.repository.TemplateRepository;
import lombok.RequiredArgsConstructor;

@Service
@RequiredArgsConstructor
public class CategoryValidationService {

    private final CategoryRepository categoryRepository;
    private final TemplateRepository templateRepository;

    public void validateDuplicatedCategory(String categoryName, Member member) {
        if (categoryRepository.existsByNameAndMember(categoryName, member)) {
            throw new CodeZapException(ErrorCode.DUPLICATE_CATEGORY, "이름이 " + categoryName + "인 카테고리가 이미 존재합니다.");
        }
    }

    public void validateDefaultCategory(Category category) {
        if (category.isDefault()) {
            throw new CodeZapException(ErrorCode.DEFAULT_CATEGORY, "기본 카테고리는 수정 및 삭제할 수 없습니다.");
        }
    }

    public void validateHasTemplate(Long id) {
        if (templateRepository.existsByCategoryId(id)) {
            throw new CodeZapException(ErrorCode.CATEGORY_HAS_TEMPLATES, "템플릿이 존재하는 카테고리는 삭제할 수 없습니다.");
        }
    }

    public void validateDuplicateNameRequest(UpdateAllCategoriesRequest request) {
        List<String> allNames = Stream.concat(
                request.createCategories().stream().map(CreateCategoryRequest::name),
                request.updateCategories().stream().map(UpdateCategoryRequest::name)
        ).toList();

        if (hasDuplicates(allNames)) {
            throw new CodeZapException(ErrorCode.INVALID_REQUEST, "요청에 중복된 카테고리 이름이 존재합니다.");
        }
    }

    public void validateOrdinal(Member member, CreateCategoryRequest request) {
        long ordinal = request.ordinal();
        long count = categoryRepository.countByMember(member);
        if (ordinal != count) {
            throw new CodeZapException(ErrorCode.INVALID_REQUEST, "카테고리 순서가 잘못되었습니다.");
        }
    }

    public void validateOrdinals(UpdateAllCategoriesRequest request) {
        List<Long> allOrdinals = Stream.concat(
                request.createCategories().stream().map(CreateCategoryRequest::ordinal),
                request.updateCategories().stream().map(UpdateCategoryRequest::ordinal)
        ).sorted().toList();

        if (!isSequential(allOrdinals)) {
            throw new CodeZapException(ErrorCode.INVALID_REQUEST, "카테고리 순서가 연속적이지 않습니다.");
        }
    }

    public void validateIds(UpdateAllCategoriesRequest request) {
        List<Long> allIds = Stream.concat(
                request.updateCategories().stream().map(UpdateCategoryRequest::id),
                request.deleteCategoryIds().stream()
        ).toList();

        if (hasDuplicates(allIds)) {
            throw new CodeZapException(ErrorCode.INVALID_REQUEST, "요청에 중복된 id가 존재합니다.");
        }
    }

    public void validateCategoriesCount(Member member, UpdateAllCategoriesRequest request) {
        if (request.updateCategories().size() + request.createCategories().size()
                != categoryRepository.countByMember(member) - 1) {
            throw new CodeZapException(ErrorCode.INVALID_REQUEST, "카테고리의 개수가 일치하지 않습니다.");
        }
    }

    private boolean hasDuplicates(List<?> items) {
        return items.size() != new HashSet<>(items).size();
    }

    private boolean isSequential(List<Long> ordinals) {
        return ordinals.stream()
                .distinct()
                .sorted()
                .collect(Collectors.toList())
                .equals(ordinals);
    }
}

다른 분들 의견도 궁금하네용 동의한다면 따봉을, 다른 의견이 있다면 코멘트 부탁해요~!

[zangsu]

네이밍을 단순하게 CategoryValidator로 지어도 좋을 것 같긴 한데, XXXRepository 들의 의존성을 가지고 있기도 하네요.
CategoryValidationService도 좋을 것 같기도 하구요~~

[zeus6768]

⚡

(member_id, ordinal) 합성 필드에 unique 조건 걸어야 하지 않을까요?!

Suggested change

	ALTER TABLE category ADD COLUMN ordinal BIGINT NOT NULL;
	ALTER TABLE category ADD COLUMN ordinal BIGINT NOT NULL;
	ALTER TABLE category ADD CONSTRAINT member_ordinal_unique UNIQUE (member_id, ordinal);

[zeus6768]

@AllArgsConstructor 생성자 체이닝 제안해요~

Suggested change

	public Category(String name, Member member, long ordinal) {
	this.name = name;
	this.member = member;
	this.isDefault = false;
	this.ordinal = ordinal;
	}
	public Category(String name, Member member, long ordinal) {
	this(null, member, name, false, ordinal);
	}

[kyum-q]

초롱 수고했어요~ 최대한 꼼꼼하게 리뷰했습니다~ 확인 부탁드려요~!

[kyum-q]

null 뿐만 아니라 최소 값 check도 하면 좋을 것 같아요

@Min(value = 1, message = "1 이상이어야 합니다.")

[kyum-q]

여기도 null 뿐만 아니라 최소 값 check도 하면 좋을 것 같아요

@Min(value = 1, message = "1 이상이어야 합니다.")

[kyum-q]

설명을 조금 수정하면 좋을 것 같아요 !
설명만 보면 업데이트 기능만 존재하는 것 같으나, 실제로는 생성, 삭제도 진행되고 있으니까요 !

카테고리를 생성, 수정, 삭제할 수 있습니다. 혹은 사용자의 카테고리들을 생성, 수정, 삭제할 수 있습니다. 어떤가요?

[kyum-q]

@ErrorCase(description = "모든 필드 중 null인 값이 있는 경우", exampleMessage = "카테고리 이름이 null 입니다."),
@ErrorCase(description = "카테고리 이름이 15자를 초과한 경우", exampleMessage = "카테고리 이름은 최대 15자까지 입력 가능합니다."),
@ErrorCase(description = "삭제하려는 카테고리에 템플릿이 존재하는 경우", exampleMessage = "템플릿이 존재하는 카테고리는 삭제할 수 없습니다."),

해당 에러 케이스도 추가해주세요~!

그리고 마지막 에러 케이스에도 ,를 두면 추후 또 다른 에러 케이스가 추가됬을 때 변경 사항이 안잡혀서 좋을 것 같아요 ~!

[kyum-q]

(⚡어필의 차원에서 zap을 사용합니다 ㅎㅎ )

해당 dto에서 fail fast를 위해 소스 코드 request처럼 SourceCodesOrdinalValidator를 이용해서 dto에서도 순서 검증하는 건 어떨까요?

현재 해당 로직이 템플릿 수정 로직과 매우 유사하다고 생각합니다. 그래서 현재 SourceCodesOrdinalValidator의 이름을 OrdinalValidator로 변경하여 소스코드에서만 사용하지 않고 이렇게 순서 검증이 필요한 곳에서 재사용하면 좋을 것 같아요 ~!

이렇게 한다면 ValidatedSourceCodesOrdinalRequest의
@SourceCodesOrdinal(message = "소스 코드 순서가 잘못되었습니다.", groups = SourceCodeOrdinalGroup.class)
이 메시지를 순서가 잘못되었습니다. 로 어디서든 사용될 수 있게 변경한다면 더 재사용하는데 좋을 것 같아요~

이에 대해 어떻게 생각하시나요?

만약 변경에 동의하시다면 지금 당장 클래스 이름은 변경 안하면 좋을 것 같아요 ,,! 해당 클래스를 #991 에서 사용하고 있어서 추후 둘 다 머지되고 이름 변경하면 좋을 것 같아요~

[zangsu]

이미 동일한 기능을 하는 Validator가 존재하니 좋은 의견이네요~~
소스코드와 카테고리의 동작이 언제까지 동일할지 모르니 기존의 클래스를 완전 재사용하는 것이 조금은 조심스럽기도 하지만, 나중에 동작이 달라져야 할 때 상속을 통한 분리를 고려해 봐도 좋을 것 같고요~~

[kyum-q]

swagger에는 "이름이 %s 인 카테고리가 이미 존재합니다."로 에러메시지로 되어있네요~ 둘 중 하나를 변경해 통일성 있게 해주세요~

[kyum-q]

애플리케이션 예외 메시지와 동일하게

Suggested change

	@ErrorCase(description = "카테고리의 순서가 잘못된 경우", exampleMessage = "템플릿 순서가 중복됩니다.")
	@ErrorCase(description = "카테고리의 순서가 잘못된 경우", exampleMessage = "카테고리 순서가 잘못되었습니다.")

로 변경해주세요~!

[kyum-q]

해당 에러 메시지를 카테고리 순서가 잘못되었습니다.로 해서 순서에 대한 예외처리 메시지를 동일하게 해주면 좋을 것 같아요.
그렇지 않다면 해당 예외 메시지도 swagger에 추가해주세요

[kyum-q]

해당 예외도 swagger에 추가해주세요~

[kyum-q]

해당 예외도 swagger에 추가해주세요~