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.

Sign up for GitHub

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

[Nullability Annotations to Java Classes] Use Updated and Null Proof MediaModel Class (breaking) #10078

Merged
merged 11 commits into from
Nov 9, 2023
Merged

[Nullability Annotations to Java Classes] Use Updated and Null Proof MediaModel Class (breaking) #10078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
faf148b
Build: Update fluxc version to pr hash (#2886)
ParaskP7 Nov 1, 2023
a06e525
Analysis: Resolve media model constructor deprecation warnings
ParaskP7 Nov 1, 2023
508ffcd
Analysis: Un-guard usages of non-null media model url
ParaskP7 Nov 1, 2023
73a45ba
Analysis: Guard usages of nullable media model file path
ParaskP7 Nov 1, 2023
afa7aa7
Analysis: Make media model parameter nullable on media upload exception
ParaskP7 Nov 1, 2023
33dbabb
Merge branch 'trunk' of github.com:woocommerce/woocommerce-android in…
ParaskP7 Nov 7, 2023
430a47d
Build: Update fluxc version to pr hash (#2886)
ParaskP7 Nov 7, 2023
f102b0f
Merge branch 'trunk' of github.com:woocommerce/woocommerce-android in…
ParaskP7 Nov 8, 2023
2a44866
Build: Update fluxc version to trunk hash (#2886)
ParaskP7 Nov 8, 2023
27c3110
Merge branch 'trunk' of github.com:woocommerce/woocommerce-android in…
ParaskP7 Nov 9, 2023
66ee06e
Build: Update fluxc version to trunk hash (#2886)
ParaskP7 Nov 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions WooCommerce/src/main/kotlin/com/woocommerce/android/media/FileUploadUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,7 @@ object FileUploadUtils {
localSiteId: Int,
mimeType: String
): MediaModel? {
val media = mediaStore.instantiateMediaModel()
var filename = FluxCMediaUtils.getFileName(fetchedUri.path)

var filename = FluxCMediaUtils.getFileName(fetchedUri.path) ?: ""
val fileExtension: String = filename
.substringAfterLast(delimiter = ".", missingDelimiterValue = "")
.ifBlank {
Expand All @@ -84,15 +82,23 @@ object FileUploadUtils {
return null
}

media.fileName = filename
media.title = filename
media.filePath = path
media.localSiteId = localSiteId
media.fileExtension = fileExtension
media.mimeType = mimeType
media.uploadDate = DateTimeUtils.iso8601UTCFromTimestamp(System.currentTimeMillis() / 1000)

return media
val media = MediaModel(
localSiteId,
DateTimeUtils.iso8601UTCFromTimestamp(System.currentTimeMillis() / 1000),
filename,
path,
fileExtension,
mimeType,
filename,
null
)
val instantiatedMedia = mediaStore.instantiateMediaModel(media)
return if (instantiatedMedia != null) {
instantiatedMedia
} else {
WooLog.w(T.MEDIA, "We couldn't instantiate the media")
null
}
}

@Suppress("TooGenericExceptionCaught")
Expand Down
19 changes: 10 additions & 9 deletions WooCommerce/src/main/kotlin/com/woocommerce/android/media/MediaFilesRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,9 @@ class MediaFilesRepository @Inject constructor(
}.onEach {
if (it is UploadSuccess) {
// Remove local file if it's in cache directory
if (localMediaModel.filePath.contains(context.cacheDir.absolutePath)) {
File(localMediaModel.filePath).delete()
val filePath = localMediaModel.filePath
if (filePath != null && filePath.contains(context.cacheDir.absolutePath)) {
File(filePath).delete()
}
}
}
Expand All @@ -99,7 +100,6 @@ class MediaFilesRepository @Inject constructor(
emit(
UploadFailure(
error = MediaUploadException(
media = MediaModel(),
errorMessage = "Media couldn't be found",
errorType = MediaStore.MediaErrorType.NULL_MEDIA_ARG
)
Expand Down Expand Up @@ -144,21 +144,22 @@ class MediaFilesRepository @Inject constructor(
}

event.completed -> {
val channelResult = if (event.media?.url != null) {
WooLog.i(T.MEDIA, "MediaFilesRepository > uploaded media ${event.media?.id}")
val media = event.media
val channelResult = if (media != null && media.url.isNotBlank()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Warning (⚠️): I am not fully confident about this change here as I am not sure which check is better, the version I chose here:

if (media != null && media.url.isNotBlank()) { UploadSuccess(media) }

Or another version of it, which just checks for a nullable media, before marking this as upload success, but without checking for a blank URL:

if (media != null) { UploadSuccess(media) }

FYI: Mind the fact media.url can no longer be nullable with the updated media model.

Let me know your thoughts on this as I fear this has the potential to change the behavior somehow, on cases that we think a media is still valid even with a blank URL, thank YOU for helping! 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't sure either so I checked for the usage of MediaModel.url and it seems like we don't use/check for the empty value anywhere, so the the 1st option should be fine. Besides, uploading an empty URL doesn't make sense anyway :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, thanks for checking, verifying and clarifying this @0nko , much appreciated! 🙇 🙏

FYI: I'll then stick with the version we have here then. 👍

WooLog.i(T.MEDIA, "MediaFilesRepository > uploaded media ${media.id}")
producerScope.trySendBlocking(
UploadSuccess(event.media)
UploadSuccess(media)
)
} else {
WooLog.w(
T.MEDIA,
"MediaFilesRepository > error uploading media ${event.media?.id}, null url"
"MediaFilesRepository > error uploading media [null media or blank url ${media?.id}]"
)

producerScope.trySendBlocking(
UploadFailure(
error = MediaUploadException(
event.media,
media,
MediaStore.MediaErrorType.GENERIC_ERROR,
resourceProvider.getString(R.string.product_image_service_error_uploading)
)
Expand Down Expand Up @@ -189,7 +190,7 @@ class MediaFilesRepository @Inject constructor(
}

class MediaUploadException(
val media: MediaModel,
val media: MediaModel? = null,
val errorType: MediaStore.MediaErrorType,
val errorMessage: String
) : Exception()
Expand Down
3 changes: 1 addition & 2 deletions WooCommerce/src/main/kotlin/com/woocommerce/android/ui/media/MediaFileUploadHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,6 @@ class MediaFileUploadHandler @Inject constructor(
private fun Event.MediaUploadEvent.toStatus(): ProductImageUploadData {
val uploadStatus = when (this) {
is Event.MediaUploadEvent.FetchFailed -> UploadStatus.Failed(
media = MediaModel(),
mediaErrorMessage = resourceProvider.getString(R.string.product_image_service_error_media_null),
mediaErrorType = MediaStore.MediaErrorType.NULL_MEDIA_ARG
)
Expand Down Expand Up @@ -293,7 +292,7 @@ class MediaFileUploadHandler @Inject constructor(

@Parcelize
data class Failed(
val media: MediaModel,
val media: MediaModel? = null,
val mediaErrorType: MediaStore.MediaErrorType,
val mediaErrorMessage: String
) : UploadStatus()
Expand Down
9 changes: 4 additions & 5 deletions ...ommerce/src/main/kotlin/com/woocommerce/android/ui/media/MediaUploadErrorListViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import androidx.lifecycle.SavedStateHandle
import com.woocommerce.android.R
import com.woocommerce.android.ui.media.MediaFileUploadHandler.ProductImageUploadData
import com.woocommerce.android.ui.media.MediaFileUploadHandler.UploadStatus
import com.woocommerce.android.ui.media.MediaFileUploadHandler.UploadStatus.Failed
import com.woocommerce.android.viewmodel.LiveDataDelegate
import com.woocommerce.android.viewmodel.ResourceProvider
import com.woocommerce.android.viewmodel.ScopedViewModel
Expand All @@ -32,7 +31,7 @@ class MediaUploadErrorListViewModel @Inject constructor(
val errorList = navArgs.errorList
if (errorList != null) {
val currentErrors = errorList.map<ProductImageUploadData, ErrorUiModel> {
ErrorUiModel(it.uploadStatus as Failed)
ErrorUiModel(it.uploadStatus as UploadStatus.Failed)
}
viewState = viewState.copy(
uploadErrorList = currentErrors,
Expand All @@ -47,7 +46,7 @@ class MediaUploadErrorListViewModel @Inject constructor(
.filter { it.isNotEmpty() }
.onEach { errors ->
val currentErrors =
viewState.uploadErrorList + errors.map { ErrorUiModel(it.uploadStatus as Failed) }
viewState.uploadErrorList + errors.map { ErrorUiModel(it.uploadStatus as UploadStatus.Failed) }
viewState = viewState.copy(
uploadErrorList = currentErrors,
toolBarTitle = resourceProvider.getString(
Expand Down Expand Up @@ -75,9 +74,9 @@ class MediaUploadErrorListViewModel @Inject constructor(
val filePath: String
) : Parcelable {
constructor(state: UploadStatus.Failed) : this(
fileName = state.media.fileName.orEmpty(),
fileName = state.media?.fileName.orEmpty(),
errorMessage = state.mediaErrorMessage,
filePath = state.media.filePath.orEmpty()
filePath = state.media?.filePath.orEmpty()
)
}
}
19 changes: 9 additions & 10 deletions WooCommerce/src/test/kotlin/com/woocommerce/android/media/ProductImagesUploadWorkerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ class ProductImagesUploadWorkerTest : BaseUnitTest() {
companion object {
private const val REMOTE_PRODUCT_ID = 1L
private const val TEST_URI = "test"
private val FETCHED_MEDIA = MediaModel()
private val UPLOADED_MEDIA = MediaModel().apply {
private val FETCHED_MEDIA = MediaModel(0, 0)
private val UPLOADED_MEDIA = MediaModel(0, 0).apply {
fileName = ""
filePath = ""
url = ""
Expand Down Expand Up @@ -121,7 +121,7 @@ class ProductImagesUploadWorkerTest : BaseUnitTest() {
val job = launch {
worker.events.toList(eventsList)
}
worker.enqueueWork(Work.UploadMedia(REMOTE_PRODUCT_ID, TEST_URI, MediaModel()))
worker.enqueueWork(Work.UploadMedia(REMOTE_PRODUCT_ID, TEST_URI, MediaModel(0, 0)))

advanceUntilIdle()
verify(mediaFilesRepository).uploadMedia(any(), any())
Expand All @@ -132,9 +132,9 @@ class ProductImagesUploadWorkerTest : BaseUnitTest() {
@Test
fun `when media upload progress changes, then update notification`() = testBlocking {
whenever(mediaFilesRepository.uploadMedia(any(), any()))
.thenReturn(flowOf(UploadProgress(0.5f), UploadSuccess(MediaModel())))
.thenReturn(flowOf(UploadProgress(0.5f), UploadSuccess(MediaModel(0, 0))))

worker.enqueueWork(Work.UploadMedia(REMOTE_PRODUCT_ID, TEST_URI, MediaModel()))
worker.enqueueWork(Work.UploadMedia(REMOTE_PRODUCT_ID, TEST_URI, MediaModel(0, 0)))
advanceUntilIdle()

verify(notificationHandler).setProgress(0.5f)
Expand All @@ -143,17 +143,16 @@ class ProductImagesUploadWorkerTest : BaseUnitTest() {
@Test
fun `when media upload fails for an image, then send an event`() = testBlocking {
val error = MediaUploadException(
MediaModel(),
GENERIC_ERROR,
""
errorType = GENERIC_ERROR,
errorMessage = ""
)
whenever(mediaFilesRepository.uploadMedia(any(), any())).thenReturn(flowOf(UploadResult.UploadFailure(error)))

val eventsList = mutableListOf<Event>()
val job = launch {
worker.events.toList(eventsList)
}
worker.enqueueWork(Work.UploadMedia(REMOTE_PRODUCT_ID, TEST_URI, MediaModel()))
worker.enqueueWork(Work.UploadMedia(REMOTE_PRODUCT_ID, TEST_URI, MediaModel(0, 0)))

advanceUntilIdle()
assertThat(eventsList).contains(UploadFailed(REMOTE_PRODUCT_ID, TEST_URI, error))
Expand All @@ -166,7 +165,7 @@ class ProductImagesUploadWorkerTest : BaseUnitTest() {
val job = launch {
worker.events.toList(eventsList)
}
worker.enqueueWork(Work.UploadMedia(REMOTE_PRODUCT_ID, TEST_URI, MediaModel()))
worker.enqueueWork(Work.UploadMedia(REMOTE_PRODUCT_ID, TEST_URI, MediaModel(0, 0)))

advanceUntilIdle()
assertThat(eventsList).contains(ProductUploadsCompleted(REMOTE_PRODUCT_ID))
Expand Down
15 changes: 7 additions & 8 deletions WooCommerce/src/test/kotlin/com/woocommerce/android/ui/media/MediaFileUploadHandlerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class MediaFileUploadHandlerTest : BaseUnitTest() {
fun `when media is fetched, then start uploading it`() = testBlocking {
mediaFileUploadHandler.enqueueUpload(REMOTE_PRODUCT_ID, listOf(TEST_URI))

val fetchedMedia = MediaModel()
val fetchedMedia = MediaModel(0, 0)
eventsFlow.tryEmit(
Event.MediaUploadEvent.FetchSucceeded(
REMOTE_PRODUCT_ID,
Expand Down Expand Up @@ -113,7 +113,6 @@ class MediaFileUploadHandlerTest : BaseUnitTest() {
remoteProductId = REMOTE_PRODUCT_ID,
localUri = TEST_URI,
uploadStatus = UploadStatus.Failed(
media = MediaModel(),
mediaErrorMessage = resourceProvider.getString(string.product_image_service_error_media_null),
mediaErrorType = NULL_MEDIA_ARG
)
Expand All @@ -130,7 +129,7 @@ class MediaFileUploadHandlerTest : BaseUnitTest() {
assertThat(successfulUpload.uploadState).isEqualTo(UPLOADED.toString())
}

val mediaModel = MediaModel().apply {
val mediaModel = MediaModel(0, 0).apply {
postId = REMOTE_PRODUCT_ID
setUploadState(UPLOADED)
}
Expand All @@ -147,7 +146,7 @@ class MediaFileUploadHandlerTest : BaseUnitTest() {
fun `given there is no external observer, when uploads finish, then start product update`() = testBlocking {
mediaFileUploadHandler.enqueueUpload(REMOTE_PRODUCT_ID, listOf(TEST_URI))

val mediaModel = MediaModel().apply {
val mediaModel = MediaModel(0, 0).apply {
postId = REMOTE_PRODUCT_ID
fileName = "test"
url = "url"
Expand All @@ -171,7 +170,7 @@ class MediaFileUploadHandlerTest : BaseUnitTest() {
val testUri2 = "file:///test2"
mediaFileUploadHandler.enqueueUpload(REMOTE_PRODUCT_ID, listOf(TEST_URI, testUri2))

val mediaModel = MediaModel().apply {
val mediaModel = MediaModel(0, 0).apply {
postId = REMOTE_PRODUCT_ID
fileName = "test"
url = "url"
Expand Down Expand Up @@ -204,7 +203,7 @@ class MediaFileUploadHandlerTest : BaseUnitTest() {

val job = launch { mediaFileUploadHandler.observeSuccessfulUploads(REMOTE_PRODUCT_ID).collect() }

val mediaModel = MediaModel().apply {
val mediaModel = MediaModel(0, 0).apply {
postId = REMOTE_PRODUCT_ID
setUploadState(FAILED)
}
Expand All @@ -230,7 +229,7 @@ class MediaFileUploadHandlerTest : BaseUnitTest() {
fun `given there is no external observer, when an upload fails, then show notification`() = testBlocking {
mediaFileUploadHandler.enqueueUpload(REMOTE_PRODUCT_ID, listOf(TEST_URI))

val mediaModel = MediaModel().apply {
val mediaModel = MediaModel(0, 0).apply {
postId = REMOTE_PRODUCT_ID
setUploadState(FAILED)
}
Expand Down Expand Up @@ -262,7 +261,7 @@ class MediaFileUploadHandlerTest : BaseUnitTest() {
@Test
fun `when assigning uploads to created product, then update the id for the successful ones`() = testBlocking {
mediaFileUploadHandler.enqueueUpload(ProductDetailViewModel.DEFAULT_ADD_NEW_PRODUCT_ID, listOf(TEST_URI))
val mediaModel = MediaModel().apply {
val mediaModel = MediaModel(0, 0).apply {
fileName = "test"
url = "url"
uploadDate = DateTimeUtils.iso8601FromDate(Date())
Expand Down
6 changes: 2 additions & 4 deletions ...ommerce/src/test/kotlin/com/woocommerce/android/ui/products/ProductDetailViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ import org.mockito.kotlin.spy
import org.mockito.kotlin.times
import org.mockito.kotlin.verify
import org.mockito.kotlin.whenever
import org.wordpress.android.fluxc.model.MediaModel
import org.wordpress.android.fluxc.model.SiteModel
import org.wordpress.android.fluxc.store.MediaStore.MediaErrorType
import org.wordpress.android.fluxc.store.WooCommerceStore
Expand Down Expand Up @@ -746,9 +745,8 @@ class ProductDetailViewModelTest : BaseUnitTest() {
PRODUCT_REMOTE_ID,
"uri",
UploadStatus.Failed(
MediaModel(),
MediaErrorType.GENERIC_ERROR,
"error"
mediaErrorType = MediaErrorType.GENERIC_ERROR,
mediaErrorMessage = "error"
)
)
)
Expand Down
6 changes: 2 additions & 4 deletions ...est/kotlin/com/woocommerce/android/ui/products/variations/VariationDetailViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import org.mockito.kotlin.doAnswer
import org.mockito.kotlin.doReturn
import org.mockito.kotlin.mock
import org.mockito.kotlin.whenever
import org.wordpress.android.fluxc.model.MediaModel
import org.wordpress.android.fluxc.store.MediaStore.MediaErrorType.GENERIC_ERROR
import org.wordpress.android.fluxc.store.WCProductStore.OnVariationChanged
import java.math.BigDecimal
Expand Down Expand Up @@ -153,9 +152,8 @@ class VariationDetailViewModelTest : BaseUnitTest() {
TEST_VARIATION.remoteVariationId,
"uri",
UploadStatus.Failed(
MediaModel(),
GENERIC_ERROR,
"error"
mediaErrorType = GENERIC_ERROR,
mediaErrorMessage = "error"
)
)
)
Expand Down
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ tasks.register("installGitHooks", Copy) {
}

ext {
fluxCVersion = '2890-5ca88c9aa82b96dfc9c9cb83a71eb19045437225'
fluxCVersion = 'trunk-7ea7465680431fa4df5f0433d4c8b8395055b997'
glideVersion = '4.13.2'
coilVersion = '2.1.0'
constraintLayoutVersion = '1.2.0'
Expand Down