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

[Team-15][BE] 포키&나단의 airbnb 프로젝트 PR2 #74

Merged
merged 7 commits into from
May 29, 2022

Conversation

nathan29849
Copy link
Member

@nathan29849 nathan29849 commented May 27, 2022

안녕하세요 Henry! 포키와 나단입니다!😎

저희가 이번 PR까지 진행한 사항은 다음과 같습니다.

  • 도메인 모델 및 schema 수정
  • Entity 작성

지난 피드백 반영 사항

  • Review와 User를 연결하였습니다.
  • 저번에 말씀주셨던 Category는 좀 더 의미를 명확히 하기위하여 Event로 이름을 변경하였습니다.

해당 사항들에 대한 자세한 정보는 위키에 링크와 사진을 첨부해 두었습니다.
(wiki 바로가기)

@nathan29849 nathan29849 added the review-BE Improvements or additions to documentation label May 27, 2022
@nathan29849 nathan29849 requested a review from wooody92 May 27, 2022 04:46
Copy link

@wooody92 wooody92 left a comment

Choose a reason for hiding this comment

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

안녕하세요~ 이번주도 수고 많으셨습니다. 👏
코멘트 남겼으니 확인 부탁드리고 계속 화이팅입니다~~!


@MappedSuperclass
public class BaseEntity {

Choose a reason for hiding this comment

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

오 공통필드 빼는 것 좋은 접근입니다. 👏👏

public class BaseEntity {

@NotNull
private boolean delete_yn;

Choose a reason for hiding this comment

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

오타로 보이네요.

Suggested change
private boolean delete_yn;
private boolean isDeleted;
private boolean deleteYn;

Copy link
Member Author

Choose a reason for hiding this comment

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

사실은 오타가 아니었지만, 오타로 하는 것으로.. ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

수정하겠습니다!

Comment on lines 17 to 21
@NotNull
private LocalDateTime createdDate;

@NotNull
private LocalDateTime lastModifiedDate;

Choose a reason for hiding this comment

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

Date보다는 시간을 나타내도록 표현하면 좋을 것 같아요. createdDateTime, createdAt, ...

Copy link
Member Author

Choose a reason for hiding this comment

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

createdAt으로 표현을 바꾸겠습니다!

@@ -0,0 +1,22 @@
package team15.airbnb.domain;

import com.sun.istack.NotNull;

Choose a reason for hiding this comment

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

적용하려고하시는게 이 패키지로부터 오는게 맞을까요~?

Copy link
Member Author

Choose a reason for hiding this comment

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

javax.validation.constraints.NotNull를 쓰려고 했는데, 잘못 import 해 온 것 같습니다! 해당 부분 수정하겠습니다

@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

@NotNull

Choose a reason for hiding this comment

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

모두 이걸로 사용되고 있네요. 안먹히지않나요?

Choose a reason for hiding this comment

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

맞습니다..ㅎ 해당 어노테이션을 제대로 모르고 사용했습니다. 수정하겠습니다!

private Long id;

@Column(length = 50)
private String accommodationName;

Choose a reason for hiding this comment

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

name 정도로 가도 괜찮지않을까요? ㅎㅎ

Suggested change
private String accommodationName;
private String name;

Copy link
Member Author

Choose a reason for hiding this comment

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

테이블 칼럼 명은 accommodation_name으로, Entity의 필드명은 name으로 하는 것으로 다른 데이터들도 통일하려 합니다.

이유 : ex. accommodation.getId() 일 때가 accommodation.getAccommodationId() 보다 가독성이나 의미 면에서 더 명확해보이기 때문입니다.

private double latitude;

@NotNull
private double longtitude;

Choose a reason for hiding this comment

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

오타 발견 😁

Suggested change
private double longtitude;
private double longitude;

Copy link
Member Author

Choose a reason for hiding this comment

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

이건 진짜 오타입니다.. ㅎㅎ 앞으로 주의하겠습니다!

Comment on lines +80 to +82
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "policy_id")
private DiscountPolicy discountPolicy;

Choose a reason for hiding this comment

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

할인 정책같은건가요? DiscountPolicy 이것도 enum 이라던가 다른방식으로 처리하지 않고 엔티티로 가져가는게 어떤 이점이 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

엔티티로 가져가게 되면, 하나의 Accommodation에 여러 개의 DiscountPolicy를 적용하기 쉽다는 이점이 있다고 생각하여 엔티티로 처리하였습니다.

Choose a reason for hiding this comment

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

하나의 Accommodation이 하나의 DiscountPolicy만 적용되는 구조인가요?

Copy link
Member Author

@nathan29849 nathan29849 May 29, 2022

Choose a reason for hiding this comment

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

원래의 의도는 확장성을 고려하여, 하나의 Accommodation이 여러개의 DiscountPolicy를 갖도록 하는 것이었는데, 설계 미스로 인하여 빠지게 되었습니다.
위를 고려하였지만, 일단은 하나의 DiscountPolicy만을 갖도록 진행하려고 합니다.
이 부분도 다음 PR 때 수정하여 올리도록 하겠습니다.

private double regionLatitude;

@NotNull
private double regionLongtitude;

Choose a reason for hiding this comment

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

여기도 오타발견 ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

수정하겠습니다!


@NotNull
@Column(length = 20)
private String country;

Choose a reason for hiding this comment

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

이것도 뭔가 enum으로 정의하기 좋은 필드네요. 만약 하지않는다면 로직에 따라 다르겠지만 입력값대로 저장한다면.. korea, south_korea, republic_of_korea 등 사실 같은 국가인데 다르게 처리되기도 하니까요.

Choose a reason for hiding this comment

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

말씀해주신 경우는 고려하지 못했던것 같습니다.
Country와 City Enum 클래스를 만들어서 사용하도록 변경했습니다!

@wooody92 wooody92 merged commit 710a218 into codesquad-members-2022:team-15 May 29, 2022
@nathan29849
Copy link
Member Author

Henry 주말임에도 불구하고 PR 리뷰에 신경 써주셔서 정말 감사드립니다!
2번째 PR에서 세심한 부분을 많이 못챙겼는데, 다음 PR에서는 조금 더 면밀하게 코드를 짜보도록 노력하겠습니다!
감사합니다!!

sabgilhun pushed a commit that referenced this pull request Jun 6, 2022
[Android] [히데]  인원수 입력 화면 ViewModel 설계로 변경 완료
wooody92 pushed a commit that referenced this pull request Jun 9, 2022
…dattions_by_keywords_api

[Backend] [43] 검색 조건에 따른 숙소 목록 조회 기능 구현
junzero741 pushed a commit that referenced this pull request Jun 9, 2022
* Feat : 달력 클릭 이벤트 구현중

* Feat : 클릭시 서치바 날짜 변경, 선택된 날짜 표시 구현

* Fix: pr 리뷰 수정
- 파일명 변경
- 변수명 수정
- 텍스트 수정

* Refact : 2주차 금요일 리뷰 적용

* Merge branch 'dev-FE' of https://github.com/street62/airbnb into dev-FE

* Merge remote-tracking branch 'origin/53-fe-캘린더-구현' into dev-FE

* Feat : 지난 날짜 클릭 비활성화 구현

* Merge remote-tracking branch 'origin/53-fe-캘린더-구현' into dev-FE

* Feat : 버튼 클릭시 인원 변경

PersonnelContext reducer, usePersonnelDispatch 구현
PersonnelModal 클릭시 인원 변경되는 기능 구현

* [FE] 모금 슬라이더 구현 (#74)

* Design: 슬라이더 UI 구현

* Rename: 파일명 변경

* Feat: thumb를 움직이면 value가 업데이트 되도록 추가

* Fix: sliderValue와 관련된 state를 props로 받도록 수정

* Feat: 각 thumb들이 서로를 넘어가지 않는 기능 추가

* Style: 중복선언된 react import 제거

* Feat: 슬라이더 이동시 범위 밖의 영역을 회색으로 보이도록 하는 기능 구현

* Feat: 금액 차트에 devicePixelRatio 적용

* Feat: 범위에 따라 border-bottom의 색이 변경되는 기능 추가

* Feat: 슬라이더가 조정되면 요금 context의 state도 바뀌도록 구현

* Style: usePrice와 관련된 훅을 파일로 분리

* Feat: price reset 버튼 기능 구현

* Fix: 모달을 켠 상태에서 리셋버튼 클릭시 모달이 리렌더링 되도록 수정

* Feat : 모달 버튼 클릭시 서치바 텍스트 변경하는 기능 구현

* Feat: Period의 리셋 버튼 기능 구현

* Merge remote-tracking branch 'origin/73-fe-인원-모달-창-기능-구현' into dev-FE

* Feat: 어린이, 유아 인원 추가에 대한 기능 구현

* Style: 상대경로 수정

* Refact : 그래프 해상도 로직 변경

* Fix: a태그를 Link 컴포넌트로 변경

* Feat : 미니서치바 기간 문구 기간state로 변경

* Fix: InputButton의 period setText 로직을 서치버튼으로 이동

* Feat: 인원 리셋버튼 기능 추가

* Feat: result 페이지의 큰 서치바의 외부영역을 클릭하면 작은서치바로 돌아오는 기능 추가

* Feat : 날짜 입력 컨트롤,  텍스트 컨트롤 기능 구현

* Refact : header index.js htmlelement->node로 교체

* Feat: kakao map api 불러오기 성공

* Feat : 라이브러리 추가

* Feat: 로그인 모달 완성

* [FE] 숙소 예약 모달 기능 구현 (street62/airbnb/#85)

* Design: 일부 스타일 변경, Map의 z-index 임시로 조정

* Feat: 숙소 예약 모달 UI 구현

* Fix: 검색버튼을 클릭하면 모든 모달이 닫히도록 수정

* Fix: 숙소의 이미지나 이름을 클릭하면 예약 모달이 뜨도록 수정

* Style: 함수명 변경

* Style: 변수 이름 변경

* Fix: 서치바 모달과 예약 모달의 state 분리

* Fix: 날짜가 선택안됐을때 예약 모달에서 날짜 입력 텍스트를 보여주도록 수정

* [Fe] context의 hook 분리, 헤더 영역 밖 클릭 개선 (https://github.com/street62/airbnb/#87)

* Fix: 게스트가 0명이면 인원 선택 텍스트가 보이도록 수정 + 함수명 수정

* Style: 함수명 수정

* Style: PeriodContext에 있는 hook을 파일로 분리
- import 순서 변경

* Style: PersonnelContext의 hook을 파일로 분리

* Refactor: 헤더 외부 영역 클릭하는 로직 개선

* Design: z-index 수정

* Fix: 헤더 밖 영역을 클릭하면 큰 서치바의 모달이 닫히도록 구현

* Design: 예약 모달 백그라운드 너비 수정

Co-authored-by: SeungHyun <fm10033@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-BE Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants