✨ Implement Image Upload #137

ii2001 · 2024-08-01T06:57:58Z

Compressed pictures
Camera, Album Authorization
API communication with the server
use camera in App

close #111

Hogu59

구현하느라 고생 많았습니다. 권한을 포함해서 해야할 게 많았는데 그래도 빠르게 진행되어 다행입니다. 몇가지 코드에 대한 수정사항 보내드리니 확인 한번 해주시면 감사하겠습니다.

Hogu59 · 2024-08-01T07:15:25Z

android/app/build.gradle.kts

@@ -69,11 +69,14 @@ android {
 dependencies {
    implementation(libs.firebase.auth)
    implementation(libs.androidx.material3.android)
+    implementation(libs.volley)


volley 안쓰시면 삭제 하면 좋을 것 같습니다.

Hogu59 · 2024-08-01T07:15:41Z

android/app/build.gradle.kts

@@ -69,11 +69,14 @@ android {
 dependencies {
    implementation(libs.firebase.auth)
    implementation(libs.androidx.material3.android)
+    implementation(libs.volley)
+    implementation(libs.androidx.tools.core)


요건 어디에 쓰는 의존성일까요?

저 친구가 진짜 core라서
Lifecycle, viewmodel등 여러곳에 사용됩니다!

Hogu59 · 2024-08-01T07:15:56Z

android/app/build.gradle.kts

    val navVersion = "2.7.7"
    val pagingVersion = "3.3.0"
    val retrofitVersion = "2.11.0"
    val gsonVersion = "2.11.0"
    val coreKtx = "1.13.1"
+    val okhttpVersion = "4.12.0"


따로 변수 빼기 굿 👍

Hogu59 · 2024-08-01T07:17:11Z

android/app/build.gradle.kts

@@ -112,4 +126,10 @@ dependencies {

    // Indicator animation open source
    implementation("com.tbuonomo:dotsindicator:5.0")
+
+    // Image Cropper
+    implementation("commons-io:commons-io:2.4")


오프라인 코드리뷰에서 요거랑 conscrypt 사용하는 곳 설명 부탁드립니다.

FileUtils요거 사ㅏ용해서 이미지를 올려야해서 사용합니다.

Hogu59 · 2024-08-01T07:20:53Z

android/app/src/main/java/net/pengcook/android/data/remote/api/MakingRecipeService.kt

+    ): Response<Unit>
+}
+
+data class PresignedUrlResponse(


이건 여기에서만 쓰이는 data class 인가요?

data > model 쪽으로 분리하면 좋을 것 같습니다!

이거 아마도 이미지 업로드하는 모든 곳에서 아용할 것 같아서 나중에 나눠 놓아야 할 것 같은데 좀있다 코드리뷰때 어디로 옮길지 얘기해보시죠!

Hogu59 · 2024-08-01T07:34:00Z

android/app/src/main/java/net/pengcook/android/presentation/making/RecipeMakingFragment.kt

-    private val viewModel: RecipeMakingViewModel by viewModels()
+
+    private val viewModel: RecipeMakingViewModel by viewModels {
+        val retrofit =


추후 km 코드 활용해서 리팩토링 가능할 것 같은 코드입니다. km 코드 올라오면 참고 부탁드립니다.

Hogu59 · 2024-08-01T07:34:34Z

android/app/src/main/java/net/pengcook/android/presentation/making/RecipeMakingFragment.kt

+    private lateinit var photoUri: Uri
+    private var currentPhotoPath: String? = null
+
+    private val permissionArray =


permission이 array 형태여야하는 이유가 있나요? 카메라 하나밖에 없어서요

나중에 추가될것을 예상해서 이렇게 구현해놨습니다

추가될 가능성이 없다면 수정할 수 있겠습니다.

Hogu59 · 2024-08-01T07:35:23Z

android/app/src/main/java/net/pengcook/android/presentation/making/RecipeMakingFragment.kt

            }
        }
+
+        // Observer to handle the pre-signed URL response
+        viewModel.imageUri.observe(


observe들은 따로 함수분리해서 관리해주면 좋을 것 같습니다.

Hogu59 · 2024-08-01T07:37:14Z

android/app/src/main/java/net/pengcook/android/presentation/making/RecipeMakingFragment.kt

+        )
+    }
+
+    private fun onAddImageClicked() {


onAddImageClicked라고 하니 ClickListener 인터페이스를 만들어서 상속받아야할 것 같은 느낌인데, 네이밍 변경은 어떨까요? "addImage(ByCondition)" 라고 하는게 함수 네임에 대한 명확한 정보를 주는 것 같습니다.

이것도 KM리뷰에 남겼었던 내용인데
저는 작동하는 내용이 정확히 적혀있으면 좋을 것 같다고 생각하긴 합니다.. 한번 토론해보시죠

Hogu59 · 2024-08-01T07:38:02Z

android/app/src/main/java/net/pengcook/android/presentation/making/RecipeMakingFragment.kt

@@ -44,8 +217,8 @@ class RecipeMakingFragment : Fragment() {
    }

    private fun onNextClicked() {
-        val action = RecipeMakingFragmentDirections.actionRecipeMakingFragmentToStepMakingFragment()
-        findNavController().navigate(action)
+        // val action = RecipeMakingFragmentDirections.actionRecipeMakingFragmentToStepMakingFragment(File(currentPhotoPath!!).name)


제 부분이 엮여 있어서 제가 확인하고 삭제하겠습니다.

kmkim2689

하디 이미지 전송 구현 수고 많으셨습니다! 코드 잘 작성해주셨네요~

처음 겪어보는 기술들이 많았지만 잘 이겨내는 모습을 보고 깊은 감명을 받았습니다.
몇 가지 피드백 남겨드리니 참고 부탁드리겠습니다. 오늘도 화이팅

kmkim2689 · 2024-08-02T00:53:43Z

android/app/build.gradle.kts

+    testImplementation("org.mockito:mockito-core:3.11.2")
+    testImplementation("org.mockito.kotlin:mockito-kotlin:3.2.0")


mockito를 사용하는 코드가 보이지 않는데, 삭제 가능하실까요~?
mockito 대신 mockk를 활용하여 이미 mock 테스트를 진행하고 계신 만큼 mockito는 필요가 없을 것 같아서요!

kmkim2689 · 2024-08-02T00:55:00Z

android/app/src/main/AndroidManifest.xml

+            android:grantUriPermissions="true">
+            <meta-data
+                android:name="android.support.FILE_PROVIDER_PATHS"
+                android:resource="@xml/fileprovider" />


두 단어로 이뤄진 만큼 file_provider와 같은 이름으로 하는 것은 어떠신가요?

요거는 저렇게 붙여서 쓰더라고요 다들??
흠냐 어떻게할까요?

kmkim2689 · 2024-08-02T01:48:26Z

...ava/net/pengcook/android/data/datasource/makingrecipe/DefaultMakingRecipeRemoteDataSource.kt

+    override suspend fun fetchImageUri(keyName: String): String {
+        val response = makingRecipeService.fetchImageUri(keyName)
+        if (response.isSuccessful) {
+            return response.body()?.url ?: throw Exception("이미지 URI를 받을 수 없습니다.")
+        } else {
+            throw Exception("이미지 URI 요청 실패")
+        }
+    }


지금 단계에서는 구동되는 데 문제가 없지만, 개인적으로는 body가 null일 때 새로운 예외를 만들어 던지지는 않을 것 같습니다. body에 접근할 수 있다는 것은 네트워크 통신이 정상적으로 이뤄졌다는 것이라고 볼 수 있기 때문입니다. 개인적으로는 empty string 등 기본값을 주고 그것을 사용하는 쪽에서 empty한 경우에 대해 분기처리를 하는 것도 좋다고 생각했습니다.

좋은 생각입니다! 요건 이제 백엔드랑도 얘기해보고 고치겠습니다!
에러페이지 만들면 거기로 바로 이동하게 해도 괜찮을 것 같아요

kmkim2689 · 2024-08-02T01:49:49Z

...ava/net/pengcook/android/data/datasource/makingrecipe/DefaultMakingRecipeRemoteDataSource.kt

+    override suspend fun fetchImageUri(keyName: String): String {
+        val response = makingRecipeService.fetchImageUri(keyName)
+        if (response.isSuccessful) {
+            return response.body()?.url ?: throw Exception("이미지 URI를 받을 수 없습니다.")
+        } else {
+            throw Exception("이미지 URI 요청 실패")
+        }
+    }
+
+    override suspend fun uploadImageToS3(
+        presignedUrl: String,
+        file: File,
+    ) {
+        val requestFile = file.asRequestBody("image/jpeg".toMediaTypeOrNull())
+        val response = makingRecipeService.uploadImageToS3(presignedUrl, requestFile)
+        if (!response.isSuccessful) {
+            throw Exception("이미지 업로드 실패")
+        }
+    }
+}


만약 예외를 던지고 싶다면 최상위 표준 예외 Exception보다는 RuntimeException 혹은 커스텀 예외 등 좀 더 구체적인 예외를 던지면 좋을 것 같습니다! 여러 가지 이유가 있지만, 가장 큰 이유는 여러 가지 예외 상황을 모두 포괄하는 것인 Exception()을 던지게 된다면, 발생한 예외 종류에 따른 처리가 어려울 수 있기 때문입니다.

try { // ... } catch(e: Exception) { // 발생시킨 예외에 대해 모두 같은 코드 블록을 실행 }

try { // ... } catch (e: RuntimeException) { // 응답이 successful이 아닐 때 } catch (e: EmptyBodyException) { // 응답의 body가 비어있을 때 }

요거 조금만 나중에 자세히 설명해주세요!

kmkim2689 · 2024-08-02T01:50:36Z

android/app/src/main/java/net/pengcook/android/data/remote/api/MakingRecipeService.kt

+    ): Response<Unit>
+}
+
+data class PresignedUrlResponse(


data > model 쪽으로 분리하면 좋을 것 같습니다!

kmkim2689 · 2024-08-02T01:54:23Z

android/app/src/main/java/net/pengcook/android/presentation/making/RecipeMakingFragment.kt

+                if (currentPhotoPath != null) {
+                    viewModel.fetchImageUri(File(currentPhotoPath!!).name)


currentPhotoPath가 null이 아닌 조건에서 실행하는데 non-null 단언 연산자(!!)를 사용한 이유를 알 수 있을까요?

kmkim2689 · 2024-08-02T01:56:43Z

android/app/src/main/java/net/pengcook/android/presentation/making/RecipeMakingFragment.kt

+            Observer { uri ->
+                if (uri != null) {
+                    uploadImageToS3(uri)
+                }
+            },
+        )
+
+        // Observer to handle the upload success
+        viewModel.uploadSuccess.observe(
+            viewLifecycleOwner,
+            Observer { success ->
+                if (success == true) {
+                    Toast.makeText(requireContext(), "이미지 업로드 성공!", Toast.LENGTH_SHORT).show()
+                } else if (success == false) {
+                    Toast.makeText(requireContext(), "이미지 업로드 실패!", Toast.LENGTH_SHORT).show()
+                }
+            },
+        )
+
+        // Observer to handle upload error
+        viewModel.uploadError.observe(
+            viewLifecycleOwner,
+            Observer { errorMessage ->
+                if (errorMessage != null) {
+                    Toast.makeText(requireContext(), errorMessage, Toast.LENGTH_SHORT).show()
+                }
+            },
+        )


SAM의 특성 + 마지막 매개변수로 오는 함수는 바깥으로 뺄 수 있다는 특성을 활용하여 다음과 같이 개선해볼 수 있을 것 같습니다.

viewModel.uploadSuccess.observe(viewLifecycleOwner) { success -> if (success == true) { Toast.makeText(requireContext(), "이미지 업로드 성공!", Toast.LENGTH_SHORT).show() } else if (success == false) { Toast.makeText(requireContext(), "이미지 업로드 실패!", Toast.LENGTH_SHORT).show() } }

kmkim2689 · 2024-08-02T01:58:56Z

android/app/src/main/java/net/pengcook/android/presentation/making/RecipeMakingViewModel.kt

+    private val _uploadSuccess = MutableLiveData<Boolean>()
+    val uploadSuccess: LiveData<Boolean>
+        get() = _uploadSuccess
+
+    private val _uploadError = MutableLiveData<String>()
+    val uploadError: LiveData<String>
+        get() = _uploadError
+


uploadSuccess와 uploadError는 State라기보다는 Event라고 생각이 드는데, Event로 처리하는 것은 어떨까요?
MakingEvent 인터페이스 내부에 추가하여 처리하면 좋을 것 같습니다!

kmkim2689 · 2024-08-02T02:02:45Z

android/app/src/main/java/net/pengcook/android/presentation/making/RecipeMakingViewModel.kt

+    fun fetchImageUri(keyName: String) {
+        viewModelScope.launch {
+            try {
+                val uri = makingRecipeRepository.fetchImageUri(keyName)
+                _imageUri.postValue(uri)
+            } catch (e: Exception) {
+                e.printStackTrace()
+                _uploadError.postValue("Pre-signed URL 요청 실패: ${e.message}")
+            }
+        }
+    }
+
+    // Function to upload image to S3
+    fun uploadImageToS3(
+        presignedUrl: String,
+        file: File,
+    ) {
+        viewModelScope.launch {
+            try {
+                makingRecipeRepository.uploadImageToS3(presignedUrl, file)
+                _uploadSuccess.postValue(true)
+            } catch (e: Exception) {
+                e.printStackTrace()
+                _uploadSuccess.postValue(false)
+                _uploadError.postValue("이미지 업로드 실패: ${e.message}")
+            }
+        }
+    }
 }


setValue(MutableLiveData.value) 대신 postValue를 사용해야 할 특별한 이유가 있으셨나요?

개인적으로는 이미지가 단기간에 여러 차례 업로드되어 해당 Livedata의 값이 단기간 안에 변경된다거나 이벤트가 비동기적으로 처리되어야 할 일이 없을 것 같아서 위에서 작성하신 것 처럼 .value를 통해 값을 설정하는 것이 좋을 것 같다고 생각했습니다.

kmkim2689 · 2024-08-02T02:02:58Z

android/app/src/main/java/net/pengcook/android/presentation/making/RecipeMakingViewModel.kt

 sealed interface MakingEvent {
    data object NavigateToMakingStep : MakingEvent
+
+    data object AddImage : MakingEvent
 }


파일 분리 부탁드리겠습니다!

ii2001 added ✨ feature new feature AN Android labels Aug 1, 2024

ii2001 requested review from Hogu59 and kmkim2689 August 1, 2024 06:57

ii2001 self-assigned this Aug 1, 2024

Hogu59 requested changes Aug 1, 2024

View reviewed changes

kmkim2689 requested changes Aug 2, 2024

View reviewed changes

kmkim2689 self-requested a review August 2, 2024 10:36

kmkim2689 approved these changes Aug 2, 2024

View reviewed changes

Hogu59 self-requested a review August 3, 2024 02:51

Hogu59 approved these changes Aug 3, 2024

View reviewed changes

ii2001 added 15 commits August 5, 2024 17:58

🐛 resolve conflict

d19cf20

✨ add camera permission logic when clicking the Add image button

a188874

✨ impl sending Image to AWS S3

20a2f4e

✨ add Error Message

e779696

🐛 fix image upload in photos

88c6b71

✨ impl RecipeMakingViewModelTest

b08e763

♻️ ktFormating

8b3e9b6

♻️ refactor observe fun

a5e9bcc

♻️ refactor data class

bdacc8c

🚚 rename addImage, navigateToStepMaking

b9b186d

🚚 rename contentLauncher

bc93932

🚚 divide MakingEvent

e2aa2bc

🚚 move test file

8b7e6c9

♻️ change to strings.xml

af8eb29

♻️ ktFormat

c424f14

ii2001 force-pushed the an/feat/111 branch from 9a43300 to c424f14 Compare August 5, 2024 08:59

ii2001 merged commit 4abf678 into an/dev Aug 5, 2024
2 checks passed

ii2001 deleted the an/feat/111 branch August 5, 2024 09:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Implement Image Upload #137

✨ Implement Image Upload #137

ii2001 commented Aug 1, 2024 •

edited

Loading

Hogu59 left a comment

Hogu59 Aug 1, 2024

Hogu59 Aug 1, 2024

ii2001 Aug 2, 2024

Hogu59 Aug 1, 2024

Hogu59 Aug 1, 2024

ii2001 Aug 2, 2024

Hogu59 Aug 1, 2024

kmkim2689 Aug 2, 2024

ii2001 Aug 2, 2024

Hogu59 Aug 1, 2024

Hogu59 Aug 1, 2024

ii2001 Aug 2, 2024

ii2001 Aug 2, 2024

Hogu59 Aug 1, 2024

Hogu59 Aug 1, 2024

ii2001 Aug 2, 2024

Hogu59 Aug 1, 2024

kmkim2689 left a comment

kmkim2689 Aug 2, 2024

kmkim2689 Aug 2, 2024

ii2001 Aug 2, 2024

kmkim2689 Aug 2, 2024

ii2001 Aug 2, 2024

kmkim2689 Aug 2, 2024

ii2001 Aug 2, 2024

kmkim2689 Aug 2, 2024

kmkim2689 Aug 2, 2024

kmkim2689 Aug 2, 2024

kmkim2689 Aug 2, 2024

kmkim2689 Aug 2, 2024

kmkim2689 Aug 2, 2024

		testImplementation("org.mockito:mockito-core:3.11.2")
		testImplementation("org.mockito.kotlin:mockito-kotlin:3.2.0")

		if (currentPhotoPath != null) {
		viewModel.fetchImageUri(File(currentPhotoPath!!).name)

✨ Implement Image Upload #137

✨ Implement Image Upload #137

Conversation

ii2001 commented Aug 1, 2024 • edited Loading

Hogu59 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmkim2689 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ii2001 commented Aug 1, 2024 •

edited

Loading