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

전남대_16조_애니모리_9주차 #244

Merged
merged 96 commits into from
Nov 6, 2023
Merged

전남대_16조_애니모리_9주차 #244

merged 96 commits into from
Nov 6, 2023

Conversation

xGreenNarae
Copy link
Contributor

jminkkk and others added 30 commits October 23, 2023 21:37
swagger api에 해당하는 커스텀 예외만 표시 및 예외문서화 상세화
나이 응답 시 1년 미만일 경우 개월수만 전송
긴급 도움 페이지 조회 시 보호만료기간이 Null값인 경우 제외
입양상태, 중성화 여부 응답에 한글로 전달
숏폼 누락된 입양 상태 한글로 전달
좋아요 기능 구현을 위한 DB 구조 변경
xGreenNarae and others added 25 commits November 2, 2023 00:16
SpringBoot Actuator 추가
ShortForm Video 조회 시, 전체 검색 조건(PetType, Area) 대응
PetVideo 조회 조건 필터링의 동적 쿼리 가독성 개선
SecurityFilterChain을 거치는 POST 요청을 처리하기 위한 CORS 설정 추가.
보호소 정보 수정 페이지를 위한 우편번호 관리 추가
Shelter 수정 시 zonecode(우편번호) 입력 추가
Province Enum 입력 값에 대한 유연한 처리(제주, 제주도, 제주특별자치도 등)
Week 9 Release
Copy link

github-actions bot commented Nov 6, 2023

Test Coverage Report

Overall Project 66.13% -7.25% 😞
Files changed 67.67% 😍

File Coverage
ShelterService.java 100% 😍
ProvinceConverter.java 100% 😍
TokenProvider.java 100% 😍
LikeCountToStringConverter.java 100% 😍
CategoryShortFormPage.java 100% 😍
HomeShortFormPage.java 100% 😍
ShortFormService.java 100% 😍
AdoptionStatus.java 100% 😍
PetType.java 100% 😍
NeutralizationStatus.java 100% 😍
EnumConverterConfiguration.java 100% 😍
PetVideoJpqlRepository.java 94.12% -5.88% 😍
PetAgeToBirthDateConverter.java 90.57% -9.43% 😍
Shelter.java 89.38% 😍
ShelterUpdateDto.java 89.33% 😍
ShelterSignUpDto.java 88.14% 😍
ShortFormDto.java 88% 😍
PetRegisterRequestDto.java 87.68% 😍
ShelterAddressSignUpDto.java 87.65% 😍
NewPetDto.java 87.5% 😍
SecurityConfig.java 87.21% 😍
PetDto.java 87.18% 😍
ShelterAddress.java 87.05% 😍
Province.java 81.37% -18.63% 😍
Pet.java 71.52% -10.13% 😍
ShelterAddressUpdateDto.java 71.05% 😍
ShelterController.java 70.37% 😍
PetVideo.java 67.11% -32.89% 😍
PetController.java 65.22% 😍
PetReadService.java 64.71% -17.65% 😍
JwtAuthenticationFilter.java 61.21% 😍
ShortFormController.java 52.38% -47.62% 😞
PetWriteServiceTransactionManager.java 47.06% -4.9% 😞
PetWriteService.java 46% -6% 😞
UserDetailsServiceImpl.java 31.58% 😍
GlobalExceptionHandler.java 24.41% -46.95% 😞
PetVideoLikeService.java 11.11% -88.89% 😞
PetVideoLike.java 5.77% -94.23% 😞
NotLikedPetVideo.java 0% 😞
AlreadyLikedPetVideo.java 0% 😞
ShortFormExceptionMessage.java 0% 😞
ShortFormNotFound.java 0% 😞
PetRegisterInfoDto.java 0% -19.74% 😞

@latteisacat latteisacat merged commit 7a3b934 into review Nov 6, 2023
3 checks passed
@chimaek chimaek self-requested a review November 6, 2023 07:33
Copy link
Contributor

@chimaek chimaek left a comment

Choose a reason for hiding this comment

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

코드리뷰 완료 전체적으로 깔끔하네요 👍 수고하셨습니다.

Comment on lines +40 to +50
final CorsConfiguration configuration = new CorsConfiguration();
configuration.addAllowedHeader("*");
configuration.addAllowedMethod("*");
configuration.addAllowedOriginPattern("*");
configuration.setAllowCredentials(true);
configuration.addExposedHeader("Authorization");

final UrlBasedCorsConfigurationSource source = new UrlBasedCorsConfigurationSource();
source.registerCorsConfiguration("/**", configuration);
return source;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

아시겠지만, 개발편의를 위해 모두 Accept 하신것으로 보이나 실무에서는 값을 지정해줍니다 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

편의 목적이 아닌, 보안을 위해서라면 Cors를 아예 적용하지 않는게 좋을 것 같은데,
값을 지정하면서까지 허용하는 이유가 있을까요?

import com.daggle.animory.domain.pet.entity.AdoptionStatus;
import com.daggle.animory.domain.pet.entity.Pet;
import com.daggle.animory.domain.pet.entity.PetType;
import com.daggle.animory.domain.pet.entity.Sex;
import com.daggle.animory.domain.pet.util.PetAgeToBirthDateConverter;
import lombok.Builder;

import java.time.LocalDate;

@Builder
public record PetRegisterInfoDto(
Copy link
Contributor

Choose a reason for hiding this comment

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

dto의 목적이 request인지, response인지 명확이 네이밍을 작성하시는 것을 권장드리면서 파라미터에 대한 유효성 검증로직도 있다면 더 나은 코드가 될 것같네요 👍

}

public void deleteLikeCount() {
this.likeCount--;
Copy link
Contributor

Choose a reason for hiding this comment

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

ikeCount가 0 미만으로 감소하지 않도록 검사하는 로직이 추가되어야 합니다. 현재 상태에서는 likeCount가 음수가 될 가능성이 있습니다.

제주;

@JsonCreator(mode = JsonCreator.Mode.DELEGATING)
// Mode.DELEGATING에 대해서 자세히 이해 못함: https://github.com/FasterXML/jackson-module-kotlin/issues/336
Copy link
Contributor

Choose a reason for hiding this comment

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

이 모드는 제공된 JSON 값을 Enum의 생성자나 팩토리 메서드에 직접 전달하여 객체를 생성하도록 합니다. 즉, JSON의 값을 Enum 타입으로 변환하는 데 사용됩니다.

private Integer id;

@NotNull
private String ipAddress;
Copy link
Contributor

Choose a reason for hiding this comment

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

IP 주소만으로 사용자를 식별하는 것은 여러 단점이 있을 수 있습니다(동적 IP, 같은 네트워크 상의 사용자 구분 불가 등). 좀 더 정교한 사용자 식별 방법을 고려해 보는 것이 좋을 수 있습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 부분은 반드시 사용자를 구분할 필요가 없는 기능이라고 판단했었습니다.
더 빠르고 간단하게 구현할 수 있으면서도, 정교한 사용자 식별 방법이란 어떤 것이 있을까요 ?

Comment on lines +24 to +25
final PetVideo petVideo = petVideoJpaRepository.findById(petVideoId)
.orElseThrow(ShortFormNotFound::new);
Copy link
Contributor

Choose a reason for hiding this comment

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

아래의 메서드에도 중복된 코드가 있다면 별도의 메서드로 분리하는 것을 고려해보시길 바랍니다 :)

import static org.assertj.core.api.Assertions.assertThat;

@Slf4j
@Sql("classpath:sql/PetVideoRepositoryOrderByTest.sql")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants