Fix/project step order logic fix #303

[Eseas]

바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!

✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)

• 기능 추가
• 리뷰 반영 수정
• 리팩토링
• 버그 수정
• 컨벤션 수정
• 브랜치 최신화

🏆 구현 목표

ex)
bucket post 및 bucket list view 초기 구현

📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)

ex)

1. bucket post 기본 기능 구현
2. bucket list view 기본 기능 구현

🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)

ex)

1. npm test 실행
2. Postman으로 API 호출 테스트

🛠️ 쓰이는 모듈

ex)

• corsheaders
• jwt

💬 기타 질문 및 특이 사항

ex) 백프 배포 nginx 검토

[github-actions]

Code review by ChatGPT

[github-actions]

코드 리뷰 결과는 다음과 같습니다:

1. 경로 변경: @PutMapping의 경로에서 {projectId}를 추가하여 프로젝트 ID에 따라 단계를 수정할 수 있게 되었습니다. 이는 API 사용성을 개선하지만, 해당 변경이 기존 호출 방식과 호환되는지 확인해야 합니다. 만약 기존 클라이언트가 /steps/{stepId} 경로를 사용하고 있었다면, 이 변경으로 인해 클라이언트 코드에 영향을 미칠 수 있습니다.

2. 매개변수 검증: projectId와 stepId에 대한 유효성 검사를 추가하는 것이 좋습니다. 예를 들어, 이들 값이 null이 아니거나 유효한 범위 내에 있는지를 확인하여 예외를 발생시키는 로직을 추가할 수 있습니다.

3. 오류 처리: API가 실패할 경우에 대한 명확한 오류 처리 로직이 필요합니다. 예외 발생 시 적절한 응답 상태 코드 및 메시지를 반환하는 것이 좋습니다.

4. 주석 추가: 함수에 대한 주석이 이미 잘 작성되어 있으나, projectId와 stepId가 각각 어떤 역할을 하는지에 대한 설명을 추가하면 가독성이 향상될 것입니다.

5. 하드코딩 방지: 경로와 관련된 문자열을 하드코딩하는 대신 상수로 선언하는 것이 좋습니다. 이는 코드의 유지 보수성을 높이고, 추후 경로 변경 시 수정을 용이하게 합니다.

6. API 문서화: API 문서화를 통해 변경된 경로와 매개변수에 대한 정보를 외부 개발자와 공유하는 것도 중요합니다.

이 외에도 테스트 케이스를 충분히 작성하여 변경된 코드가 모든 시나리오를 잘 처리하는지 확인하는 것이 좋습니다.

[github-actions]

코드 패치를 검토해보겠습니다.

변경 사항

1. @PutMapping("/steps/{stepId}")에서 @PutMapping("/{projectId}/steps/{stepId}")로 변경되었습니다.
   • 이 변경은 프로젝트 ID를 URI에 추가하여 특정 프로젝트의 단계 수정 API를 보다 명확하게 합니다.

개선 사항 및 버그 위험

1. 경로 변수 유효성 검사:

   • @PathVariable로 정의된 projectId와 stepId의 유효성을 검증하는 로직이 필요합니다. 이 두 값이 유효한지 확인하지 않으면 잘못된 값으로 DB에 접근할 위험이 있습니다.

2. 오류 처리:

   • 만약 주어진 projectId와 stepId가 DB에서 찾을 수 없는 경우에 대한 오류 처리가 필요한데, 이를 명확하게 처리하지 않으면 클라이언트에게 모호한 오류 응답을 줄 수 있습니다.
   • 예를 들어, 단계 수정 시 해당 프로젝트가 존재하지 않거나, 해당 단계가 해당 프로젝트에 속하지 않는 경우를 처리해야 합니다.

3. API 문서화:

   • @Operation 주석 내의 description은 현재 유용한 정보를 제공하지만, 요청 및 응답 형식에 대한 더 구체적인 설명을 추가하면 API 사용자가 이해하기 쉬워집니다.

4. 테스트 케이스:

   • 변경된 경로에 대한 충분한 테스트 케이스를 작성하여 정상적인 경우와 예외적인 경우(예: 존재하지 않는 projectId, stepId)를 모두 다루는 것이 좋습니다.

5. HTTP 상태 코드:

   • 수정 성공 시 어떤 HTTP 상태 코드를 반환할 것인지 명확히 정의하는 것이 중요합니다 (예: 200 OK, 404 Not Found 등).

이러한 점을 고려하여 개선하면 더 안정적이고 사용하기 쉬운 API가 될 것입니다. 추가적인 질문이 있으시면 언제든지 문의해 주세요!

[github-actions]

코드 패치에 대한 간단한 리뷰를 제공하겠습니다.

긍정적인 점

1. 로깅 추가: @Slf4j 어노테이션을 추가하여 로그를 설정한 점은 좋은 개선입니다. 이는 코드의 디버깅 및 유지보수에 도움이 될 것입니다.
2. 안정성 강화: putProjectStep 메서드에서 requestDto.getStepOrder() 값을 로그로 기록함으로써, 입력값 추적이 가능해졌습니다.

버그 리스크

1. 순서 수정 로직의 안전성: orderList.removeIf(order -> order.equals(projectStep.getStepOrder())); 이 부분에서 projectStep.getStepOrder()가 null이거나 유효하지 않은 값인 경우 예외가 발생할 수 있습니다. 입력값 검증이 필요합니다.
2. getStepOrder 메서드에서: target < 0을 체크하도록 수정했지만, 이 검증이 비즈니스 로직에 적합한지 확인해야 합니다. 조건이 비즈니스 규칙에 맞는지 여부를 검토해야 합니다.
3. 리턴된 순서의 유효성 검사: return find / 2; 부분에서 find의 값에 따라 0이 될 위험이 있습니다. 이로 인해 이후 코드가 예기치 않게 동작할 수 있습니다.

개선 사항

1. 입력 값 검증: putProjectStep와 getStepOrder에서 입력값을 더 철저하게 검증하여 예외를 방지해야 합니다.
2. 로그 레벨: 로그의 레벨을 info에서 debug로 변경할 수 있습니다. 이는 더 많은 정보를 기록해야 할 때 유용할 수 있습니다.
3. 메소드 문서화: 각 메서드에 주석을 달아 의도를 명확히 하고 코드의 가독성을 높이는 것이 좋습니다.

이 외에 필요 시 코드 리팩토링을 검토하고, 단위 테스트를 통해 각 메서드의 동작을 확인하는 것도 좋은 방법입니다.

[github-actions]

코드 리뷰를 진행하겠습니다.

세부 변경 사항

1. 상수 대문자화: DEVELOPER, CUSTOMER, APPROVER, PARTICIPANT 상수의 값을 대문자로 변경했습니다. 이는 코드의 가독성을 높이고, 의미의 일관성을 유지하는 좋은 결정입니다.

2. 대문자 비교 수정: authorization.get().getAuthorization()와의 비교에서 toUpperCase()를 제거했습니다. 이제 직접적으로 대문자 상수와 비교하고 있어, 성능이 향상될 수 있습니다.

개선 사항 및 버그 리스크

1. 상수 사용 일관성: 코드 전반에 걸쳐 상수를 사용할 때 일관성을 유지해야 합니다. 예를 들어, 현재 CUSTOMER는 대문자로 변경되었는데, 상수 값에 대한 모든 부분(예: customer.toLowerCase())에서도 대문자로 비교하는 것이 좋습니다. 이를 통해 불필요한 대소문자 변환을 줄일 수 있습니다.

2. == 연산자 사용: company == (companyCode.equals(CUSTOMER) ? project.getCustomer() : project.getDeveloper()) 구문에서 == 연산자를 사용해 객체를 비교하고 있습니다. 이는 의도한 동작을 하지 않을 수 있습니다. 문자열을 비교할 때는 equals() 메서드를 사용하는 것이 더 안전합니다. 예를 들어, company.equals(...)와 같이 해야 합니다.

3. Null 체크: project.getCustomer()와 project.getDeveloper()가 null일 수 있는 가능성도 고려해야 합니다. NullPointerException을 방지하기 위해 적절한 null 체크 또는 Optional을 사용하는 것이 좋습니다.

4. 로깅 개선: 현재 로깅은 오류 메시지를 출력하지만, 어떤 권한이 필요한지 등에 대한 정보가 부족합니다. 추가적인 정보(예: 필요한 권한 등을 포함한 메시지)를 로깅하여 디버깅 시 유용할 수 있습니다.

5. 패키지 구조 검토: ProjectAuthorizationCheckServiceImpl이름이 다소 길고 복잡할 수 있습니다. 서비스의 책임이 명확히 드러나지 않으면, 이름을 좀 더 간단하고 명확하게 바꾸는 것을 고려해 볼 수 있습니다.

종합적으로, 코드 변경 내용은 방향성이 좋으며, 몇 가지 일관성을 높이고 안정성을 강화하는 방향으로 개선할 필요가 있습니다.