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

fix: 이미지 순서 수정 시 반영 안되는 문제 #650 #651

Merged
merged 4 commits into from
Feb 25, 2025

Conversation

Ho-Tea
Copy link
Contributor

@Ho-Tea Ho-Tea commented Feb 23, 2025

⭐️ Issue Number

🚩 Summary

  • 스타카토 수정 시 기존 이미지에 대한 순서 변경이 적용 안되는 이슈를 해결하였습니다.
  • 기존 로직보다 쿼리수는 더 나가겠지만, 전체 이미지가 5개로 제한되는 상황이므로 크리티컬하지 않다고 판단하였습니다.

🛠️ Technical Concerns

  • 이미지의 순서를 부여하는 칼럼을 새롭게 구성하는 방식 또한 고려해보았는데, 새롭게 칼럼을 추가해서 관리하는 것보다 최대 5개의 이미지로 제한되므로 전체 삭제 후 전체 갱신으로 구성하였습니다.

🙂 To Reviewer

📋 To Do

@Ho-Tea Ho-Tea added backend We are backend>< fix 버그 (버그 수정) labels Feb 23, 2025
@Ho-Tea Ho-Tea added this to the sprint-10 milestone Feb 23, 2025
@Ho-Tea Ho-Tea self-assigned this Feb 23, 2025
Copy link

github-actions bot commented Feb 23, 2025

Test Results

 36 files   36 suites   6s ⏱️
262 tests 262 ✅ 0 💤 0 ❌
279 runs  279 ✅ 0 💤 0 ❌

Results for commit 48ad868.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Feb 23, 2025

🌻Test Coverage Report

Overall Project 79.35% -0.05% 🍏
Files changed 85% 🍏

File Coverage
MomentService.java 100% 🍏
MemoryService.java 97.45% -1.09%
MomentImages.java 97.26% 🍏
Moment.java 66.67% 🍏

Copy link
Contributor

@BurningFalls BurningFalls 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 +105 to +107
public List<MomentImage> existingImages() {
return momentImages.getImages();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

public String getThumbnailUrl() {
    return momentImages.getImages().get(0).getImageUrl();
}

이 클래스에 있는 다른 메서드(위의 getThumbnailUrl)처럼, 앞에 get을 붙여서 getExistingImages로 하는 것도 좋을 것 같아요.

필드명이 momentImages라서 lombok을 통해 생성된 getter와의 충돌은 없는 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

코드의 일관성 측면에서 get을 쓴부분이 있다면 getXXX로 맞추는 것이 합리적이라고 생각합니다.
그렇다면 혹시 getThumbnailUrlthumbnailUrl로 변경하는 것은 어떨까요!

Copy link
Contributor

Choose a reason for hiding this comment

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

lombok을 사용해서 다른 부분에서도 get을 사용하지 않고 필드명으로 가져오다보니, 그렇게 통일성을 맞추는 것도 좋다고 생각합니다!

@Ho-Tea Ho-Tea requested a review from BurningFalls February 24, 2025 09:49
Copy link
Contributor

@linirini linirini left a comment

Choose a reason for hiding this comment

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

LGTM!
이미 존재하는 이미지까지 모두 삭제하고 다시 업로드한다는 점은 비효율적이지만, 우선 가장 간단한 해결 방법인 것 같아요! (S3에 너무 많은 이미지가 생길수도 있을 것 같네요!)
추후에 개선 방안 같이 고민해봅시다:)


private boolean contains(MomentImage momentImage) {
return this.images.stream()
.anyMatch(image -> image.isSame(momentImage));
Copy link
Contributor

Choose a reason for hiding this comment

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

😢😢

Copy link
Contributor

@BurningFalls BurningFalls left a comment

Choose a reason for hiding this comment

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

@MethodSource에서 사용되는 메서드 위치 코멘트만 한번 확인해주세요. 수고하셨습니다, 호티!

assertThat(images).containsExactlyElementsOf(updatedImageNames);
}

static Stream<Arguments> provideUpdateData() {
Copy link
Contributor

Choose a reason for hiding this comment

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

보통 @MethodSource로 사용하는 메서드들은 맨 위에 모아놓더라구요.
찾아보니까 MethodSource는 클래스 레벨에서 미리 접근 가능한 상태를 만들기 위해 static이 붙여졌고, 일반적으로 static 메서드는 클래스의 맨 위에 모아서 관리하기 때문에 그렇게 된 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영했씁니다!

@Ho-Tea Ho-Tea merged commit 48a4391 into develop Feb 25, 2025
2 checks passed
@linirini linirini deleted the feature/#650-Image-order-modification-not-reflected branch February 27, 2025 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend We are backend>< fix 버그 (버그 수정)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants