-
Notifications
You must be signed in to change notification settings - Fork 2
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/#665 사용자 프로필 이미지 업데이트 #679
The head ref may contain hidden characters: "Feature/#665-\uC0AC\uC6A9\uC790_\uD504\uB85C\uD544_\uC774\uBBF8\uC9C0_\uC5C5\uB370\uC774\uD2B8"
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다~! 몇 가지 코멘트 남겨두었으니 확인 부탁드려요~!
import org.springframework.beans.factory.annotation.Value; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.web.multipart.MultipartFile; | ||
|
||
@Service | ||
@RequiredArgsConstructor | ||
//얘 @Component로 바꾸는 거 어떤가요? @Service보단 @Component가 더 적절한 것 같아요. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 듣고 보니 Component가 더 적절할 것 같네요!
찬성합니다~~
|
||
if (member.isNotGithubProfile()) { | ||
final String imageName = s3Client.convertImageName(member.getImageUrl()); | ||
s3Client.deleteImages(List.of(imageName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 로직과 밑의 이미지를 업로드하는 로직들을 ImageService의 메서드로 구현해도 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분도 조금 고민중이긴 합니다.
ImageService의 범위를 어떻게 정할 지에 따라 다를 것 같아요.
현재 해당 로직에서, 별도의 Image테이블에 대한 접근은 없고, 단순히 S3에 이미지 추가 삭제만 하고 있어서,
ImageService가 아닌 S3를 직접적으로 의존하게 했어요.
ImageService는 뭔가 Image 테이블에 연관된 작업을 할 것 같아서요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 동의합니다
근데 만약에 ImageService를 사용하지 않는다면 member의 이미지는 Image Table에서 관리하지 않고 자체 URL을 칼럼으로 저장하고 있는 건가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네, 맞아요. 그렇게 구현되어 있습니다.
근데 지금 고민 중인게 있습니다.
Member의 이미지도 ImageTable에서 관리해야하나?에 대해서 고민중입니다.
Member의 경우 다른 Feeds나 Events들과 다르게 이미지와 1대N 관계가 아닌 1대1 관계에요.
그래서 컬럼에서, 그냥 가지고 있었죠.
그리고, 이미 가입할 때 github프로필 이미지url을 컬럼으로 갖고 있어요.
1대1이어도, Image Table을 쓰는 방향도 생각해봤어요.
근데 그러면, 이미지를 담당하는 방법이 두 개가 되고,(Member의 ImageUrl 컬럼, ImageTable의 row)
방법이 두개로 변하다보니 어떤 곳이 진짜 이미지인지 나타내는 flag 형식의 값을 추가적으로 저장해야 해요.
이런 이유 때문에, Image Table을 별도로 관리하진 않았습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
괜찮네여 이대로 진행하시죠
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀을 듣고 보니 구현하신 내용이 납득이 되네요!
지금 방식으로 가도 괜찮을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존 이미지 삭제까지 깔끔하게 잘 구현해주셨네요.
수고하셨습니다~!
import org.springframework.beans.factory.annotation.Value; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.web.multipart.MultipartFile; | ||
|
||
@Service | ||
@RequiredArgsConstructor | ||
//얘 @Component로 바꾸는 거 어떤가요? @Service보단 @Component가 더 적절한 것 같아요. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
찬성합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다.
궁금한 점들이 좀 있어서 RC 남깁니다요
import org.springframework.beans.factory.annotation.Value; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.web.multipart.MultipartFile; | ||
|
||
@Service | ||
@RequiredArgsConstructor | ||
//얘 @Component로 바꾸는 거 어떤가요? @Service보단 @Component가 더 적절한 것 같아요. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인졍
private final AmazonS3 amazonS3; | ||
|
||
|
||
public S3Client(@Value("${cloud.aws.s3.bucket}") final String bucket, final AmazonS3 amazonS3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -106,6 +110,10 @@ public boolean isOnboarded() { | |||
return name != null; | |||
} | |||
|
|||
public boolean isNotGithubProfile() { | |||
return !imageUrl.contains(GITHUB_PROFILE_DOMAIN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
startsWith 은 어때여?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그게 조금 더 정확하겠네요. 변경해보겠습니다.
|
||
if (member.isNotGithubProfile()) { | ||
final String imageName = s3Client.convertImageName(member.getImageUrl()); | ||
s3Client.deleteImages(List.of(imageName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 동의합니다
근데 만약에 ImageService를 사용하지 않는다면 member의 이미지는 Image Table에서 관리하지 않고 자체 URL을 칼럼으로 저장하고 있는 건가요?
#️⃣연관된 이슈
📝작업 내용
예상 소요 시간 및 실제 소요 시간
💬리뷰 요구사항(선택)