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

Feature: 프로젝트 디테일 이미지 업로드 기능 구현 #151

Merged
merged 8 commits into from
Sep 18, 2024

Conversation

yunhacandy
Copy link
Member

@yunhacandy yunhacandy commented Sep 12, 2024

연관된 이슈

이슈링크(url): #92 (comment)

✅ 작업 내용

  • 디테일 이미지 업로드 메소드 추가
  • 디테일 이미지 순서 보장 로직 추가

칼럼 추가 SQL문

ALTER TABLE project_image
ADD COLUMN project_image_order INT NOT NULL;

- 디테일 이미지 업로드 메소드 추가
- 디테일 이미지 순서 보장 로직 추가
Copy link
Member

@Youthhing Youthhing left a comment

Choose a reason for hiding this comment

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

사진 순서를 별도의 컬럼으로 추가해 저장하는 방법으로 순서를 보장하신 것 같네요.

우선, 컬럼을 추가하는 경우엔 바뀐 컬럼에 대해 수정해야할 SQL문을 PR에 같이 남겨주세요.

response에는 사진 순서에 대한 정보가 전달되지 않는 것 같은데 해당 경우엔 결국 클라이언트 측에서 detail 사진의 순서를 알 수 없어보입니다.

또한, 로고와 썸네일은 한 장만 존재하는데 순서 컬럼을 만들고 null이 들어가는 것에 대해 어떻게 생각하시는지 의견 부탁드립니다

@yunhacandy
Copy link
Member Author

yunhacandy commented Sep 16, 2024

#93 (comment)

ddl 수정 후 언급했었는데, 여기에도 ddl 올립니당

@Youthhing
Copy link
Member

#93 (comment)

ddl 수정 후 언급했었는데, 여기에도 ddl 올립니당

이 경우엔 테이블을 새로 생성하는 것이 아닌 기존에 있는 컬럼명을 바꾸거나, 기존 테이블에 새로운 컬럼을 추가하는 SQL문이 필요합니다.
따라서, project_image_order가 추가되는 sql문을 남겨주세요

@yunhacandy
Copy link
Member Author

우선 pr 먼저 수정했습니다.
push 한번 하면 내용 확인 부탁드리겠습니다

- ProjectImageInfoResponse에 imageOrder에 대한 정보 추가
- 로고와 썸네일 이미지는 imageOrder가 필요하지 않으나 일관성을 위해 기본값 0으로 설정.
Copy link
Member

@Youthhing Youthhing left a comment

Choose a reason for hiding this comment

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

우선 썸네일의 사진 순서를 어떻게 처리할 것인가? 에 대한 명확한 고민이 필요해보입니다.

썸네일 사진의 순서를 보장하기 위한 방법으로 '순서 컬럼을 추가하는 방법'을 선택하신 것 같네요.
이 방법을 선택하신 경우, 순서가 필요없는 로고와 썸네일에 null을 넣을 것인지, 기본 값을 넣을 것인지, 기본 값을 0으로 지정해도 괜찮을지에 대한 고민이 필요해보입니다.

  • null을 넣는다면, 데이터 자체에 null값이 많아지는 것이 괜찮을까?
  • 기본 값을 0으로 지정한다면, '0이 아닌 다른 값을 허용하는 것처럼 인지될 것 같은데?'

이러한 고민이 들텐데 이에 대해서도 생각을 해보면 좋을 것 같네요.

고민 끝에 무엇 하나 답이 없다고 생각된다면 장단점을 비교하고, 그 중 하나를 선택한 이유에 대한 고민을 해보시거나 더 나은 접근은 없었을까?에 대해 생각해보면 좋을 것 같습니다.

- imageOrder의 기본값을 1로 설정
- 기본값 변경으로 인해 프로젝트 상세 정보 중 이미지 가져올때 오름차순 신경 안쓰고 projectId로 한번에 가져오게 수정
- DB에 null 값 최소화하기 위해 원시타입 사용
- 원시타입 사용으로 인해 null 체크 삭제
- 중복되는 로직 통합
@yunhacandy
Copy link
Member Author

리뷰해주신 내용을 바탕으로 대거 리팩토링 했습니다.

고민 끝에 사진이 존재한다는 의미를 보여주기 위해 로고와 썸네일도 순서를 1로 지정하였습니다.
이에 기본값을 그냥 1로 지정하였습니다.

나머지 부분에 대한 내용은 커밋 메시지에 적어놨습니다.

@yunhacandy
Copy link
Member Author

말하신 부분을 반영해서 올렸습니다.
빠트린 부분 있으면 알려주세요!
@Youthhing

@Youthhing Youthhing merged commit fe93868 into develop Sep 18, 2024
1 check passed
@Youthhing Youthhing deleted the feature/TCTSK10-3-upload-project-detail-info branch September 18, 2024 09:19
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.

3 participants