Move purgeMediaToPostAssociationIfNotInPostAnymore to UseCase #10798
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| package org.wordpress.android.ui.posts.editor | ||
|
|
||
| import android.content.Context | ||
| import dagger.Reusable | ||
| import org.wordpress.android.editor.AztecEditorFragment | ||
| import javax.inject.Inject | ||
|
|
||
| /** | ||
| * Injectable wrapper around AztecEditorFragment. | ||
| * | ||
| * AppPrefs interface contains some static methods, which make the client code difficult to test/mock. Main purpose of | ||
| * this wrapper is to make testing of these static methods easier. | ||
| * | ||
| */ | ||
| @Reusable | ||
| class AztecEditorFragmentStaticWrapper @Inject constructor(private val appContext: Context) { | ||
|
There was a problem hiding this comment. I've created something similar in my PR, we need to think about it when we merge the PRs There was a problem hiding this comment. The PR is already merged. Do you think you can now merge the 2 classes? :-) There was a problem hiding this comment. @planarvoid Hmm, I checked develop yesterday and I couldn't find |
||
| fun isMediaInPostBody(postContent: String, localMediaId: String) = | ||
| AztecEditorFragment.isMediaInPostBody(appContext, postContent, localMediaId) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| package org.wordpress.android.ui.posts.editor.media | ||
|
|
||
| import dagger.Reusable | ||
| import kotlinx.coroutines.CoroutineDispatcher | ||
| import kotlinx.coroutines.withContext | ||
| import org.wordpress.android.fluxc.Dispatcher | ||
| import org.wordpress.android.fluxc.generated.UploadActionBuilder | ||
| import org.wordpress.android.fluxc.model.PostImmutableModel | ||
| import org.wordpress.android.fluxc.store.UploadStore | ||
| import org.wordpress.android.fluxc.store.UploadStore.ClearMediaPayload | ||
| import org.wordpress.android.modules.BG_THREAD | ||
| import org.wordpress.android.ui.posts.PostUtilsWrapper | ||
| import org.wordpress.android.ui.posts.editor.AztecEditorFragmentStaticWrapper | ||
| import javax.inject.Inject | ||
| import javax.inject.Named | ||
|
|
||
| @Reusable | ||
| class CleanUpMediaToPostAssociationUseCase @Inject constructor( | ||
| private val dispatcher: Dispatcher, | ||
| private val uploadStore: UploadStore, | ||
| private val aztecEditorWrapper: AztecEditorFragmentStaticWrapper, | ||
| private val postUtilsWrapper: PostUtilsWrapper, | ||
| @Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher | ||
| ) { | ||
| suspend fun purgeMediaToPostAssociationsIfNotInPostAnymore(post: PostImmutableModel) { | ||
| withContext(bgDispatcher) { | ||
| val mediaAssociatedWithPost = uploadStore.getFailedMediaForPost(post) + | ||
| uploadStore.getCompletedMediaForPost(post) + | ||
| uploadStore.getUploadingMediaForPost(post) | ||
|
|
||
| mediaAssociatedWithPost | ||
| .filter { media -> | ||
| // Find media which is not in the post anymore | ||
| val containsGutenbergBlocks = postUtilsWrapper.contentContainsGutenbergBlocks(post.content) | ||
| if (containsGutenbergBlocks) { | ||
| !postUtilsWrapper.isMediaInGutenbergPostBody(post.content, media.id.toString()) | ||
| } else { | ||
| !aztecEditorWrapper.isMediaInPostBody(post.content, media.id.toString()) | ||
| } | ||
| } | ||
| .filter { media -> | ||
| // Featured images are not in post content, don't delete them | ||
| !media.markedLocallyAsFeatured | ||
| } | ||
| .toSet() | ||
| .let { mediaToDeleteAssociationFor -> | ||
| if (mediaToDeleteAssociationFor.isNotEmpty()) { | ||
| val clearMediaPayload = ClearMediaPayload( | ||
| post, | ||
| mediaToDeleteAssociationFor | ||
| ) | ||
| dispatcher.dispatch( | ||
| UploadActionBuilder.newClearMediaForPostAction( | ||
| clearMediaPayload | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,171 @@ | ||
| package org.wordpress.android.ui.posts.editor.media | ||
|
|
||
| import com.nhaarman.mockitokotlin2.anyOrNull | ||
| import com.nhaarman.mockitokotlin2.argumentCaptor | ||
| import com.nhaarman.mockitokotlin2.doAnswer | ||
| import com.nhaarman.mockitokotlin2.doReturn | ||
| import com.nhaarman.mockitokotlin2.mock | ||
| import com.nhaarman.mockitokotlin2.never | ||
| import com.nhaarman.mockitokotlin2.verify | ||
| import kotlinx.coroutines.InternalCoroutinesApi | ||
| import org.assertj.core.api.Assertions.assertThat | ||
| import org.junit.Test | ||
| import org.junit.runner.RunWith | ||
| import org.junit.runners.Parameterized | ||
| import org.mockito.ArgumentMatchers.any | ||
| import org.wordpress.android.BaseUnitTest | ||
| import org.wordpress.android.TEST_DISPATCHER | ||
| import org.wordpress.android.fluxc.Dispatcher | ||
| import org.wordpress.android.fluxc.annotations.action.Action | ||
| import org.wordpress.android.fluxc.model.MediaModel | ||
| import org.wordpress.android.fluxc.store.UploadStore | ||
| import org.wordpress.android.fluxc.store.UploadStore.ClearMediaPayload | ||
| import org.wordpress.android.test | ||
| import org.wordpress.android.ui.posts.PostUtilsWrapper | ||
| import org.wordpress.android.ui.posts.editor.AztecEditorFragmentStaticWrapper | ||
| import org.wordpress.android.ui.posts.editor.media.CleanUpMediaToPostAssociationUseCaseTest.Fixtures.createAztecEditorWrapper | ||
| import org.wordpress.android.ui.posts.editor.media.CleanUpMediaToPostAssociationUseCaseTest.Fixtures.createMediaList | ||
| import org.wordpress.android.ui.posts.editor.media.CleanUpMediaToPostAssociationUseCaseTest.Fixtures.createPostUtilsWrapper | ||
| import org.wordpress.android.ui.posts.editor.media.CleanUpMediaToPostAssociationUseCaseTest.Fixtures.createUploadStore | ||
|
|
||
| @InternalCoroutinesApi | ||
| @RunWith(Parameterized::class) | ||
|
|
||
| class CleanUpMediaToPostAssociationUseCaseTest(private val containsGutenbergBlocks: Boolean) : BaseUnitTest() { | ||
| @Test | ||
| fun `media which are NOT in post are cleared`() = test { | ||
| // Arrange | ||
| val mediaList = createMediaList() | ||
| val uploadStore = createUploadStore( | ||
| failedMedia = setOf(mediaList[0]), | ||
| uploadedMedia = setOf(mediaList[1]), | ||
| uploadingMedia = setOf(mediaList[2]) | ||
| ) | ||
| val mediaInPost = mediaList.slice(3..9).toSet() | ||
| val dispatcher = mock<Dispatcher>() | ||
| // Act | ||
| createUseCase( | ||
| dispatcher, | ||
| uploadStore, | ||
| mediaInPost | ||
| ).purgeMediaToPostAssociationsIfNotInPostAnymore(mock()) | ||
|
|
||
| // Assert | ||
| val captor = argumentCaptor<Action<ClearMediaPayload>>() | ||
| verify(dispatcher).dispatch(captor.capture()) | ||
| assertThat(captor.firstValue.payload.media) | ||
| .isEqualTo(setOf(mediaList[0], mediaList[1], mediaList[2])) | ||
| } | ||
|
|
||
| @Test | ||
| fun `media which are in post are NOT cleared`() = test { | ||
| // Arrange | ||
| val mediaList = createMediaList() | ||
|
|
||
| val dispatcher = mock<Dispatcher>() | ||
| val uploadStore = createUploadStore( | ||
| failedMedia = setOf(mediaList[0]), | ||
| uploadedMedia = setOf(mediaList[1]), | ||
| uploadingMedia = setOf(mediaList[2]) | ||
| ) | ||
| // Act | ||
| createUseCase( | ||
| dispatcher, | ||
| uploadStore, | ||
| mediaInPost = mediaList.toSet() | ||
| ).purgeMediaToPostAssociationsIfNotInPostAnymore(mock()) | ||
|
|
||
| // Assert | ||
| verify(dispatcher, never()).dispatch(any<Action<ClearMediaPayload>>()) | ||
| } | ||
|
|
||
| @Test | ||
| fun `featured images are NOT cleared even though they are NOT in post`() = test { | ||
| // Arrange | ||
| val mediaList = createMediaList() | ||
| val dispatcher = mock<Dispatcher>() | ||
| val uploadStore = createUploadStore( | ||
| failedMedia = setOf(mediaList[0]), | ||
| uploadedMedia = setOf(mediaList[1]), | ||
| uploadingMedia = setOf(mediaList[2]) | ||
| ) | ||
| mediaList[0].markedLocallyAsFeatured = true | ||
| val mediaInPost = mediaList.slice(3..9).toSet() | ||
| // Act | ||
| createUseCase( | ||
| dispatcher, | ||
| uploadStore, | ||
| mediaInPost | ||
| ).purgeMediaToPostAssociationsIfNotInPostAnymore(mock()) | ||
|
|
||
| // Assert | ||
| val captor = argumentCaptor<Action<ClearMediaPayload>>() | ||
| verify(dispatcher).dispatch(captor.capture()) | ||
| assertThat(captor.firstValue.payload.media) | ||
| .isEqualTo(setOf(mediaList[1], mediaList[2])) | ||
| } | ||
|
|
||
| private fun createUseCase( | ||
| dispatcher: Dispatcher = mock(), | ||
| uploadStore: UploadStore = mock(), | ||
| mediaInPost: Set<MediaModel> | ||
| ) = CleanUpMediaToPostAssociationUseCase( | ||
| dispatcher, | ||
| uploadStore, | ||
| createAztecEditorWrapper(mediaInPost), | ||
| createPostUtilsWrapper(mediaInPost, containsGutenbergBlocks), | ||
| TEST_DISPATCHER | ||
| ) | ||
|
|
||
| companion object { | ||
| @JvmStatic | ||
| @Parameterized.Parameters | ||
| fun parameters() = listOf( | ||
| arrayOf(true), // Test with posts containing gutenberg blocks | ||
| arrayOf(false) // Test with posts not containing gutenberg blocks | ||
| ) | ||
| } | ||
|
|
||
| private object Fixtures { | ||
| fun createUploadStore( | ||
| failedMedia: Set<MediaModel> = setOf(), | ||
| uploadedMedia: Set<MediaModel> = setOf(), | ||
| uploadingMedia: Set<MediaModel> = setOf() | ||
| ) = mock<UploadStore> { | ||
| on { getFailedMediaForPost(any()) }.thenReturn(failedMedia) | ||
| on { getCompletedMediaForPost(any()) }.thenReturn(uploadedMedia) | ||
| on { getUploadingMediaForPost(any()) }.thenReturn(uploadingMedia) | ||
| } | ||
|
|
||
| fun createPostUtilsWrapper( | ||
| mediaInPost: Set<MediaModel>, | ||
| containsGutenbergBlocks: Boolean | ||
| ) = | ||
| mock<PostUtilsWrapper> { | ||
| on { isMediaInGutenbergPostBody(anyOrNull(), anyOrNull()) } | ||
| .doAnswer { invocation -> | ||
| mediaInPost.map { it.id } | ||
| .contains((invocation.arguments[1] as String).toInt()) | ||
| } | ||
| on { contentContainsGutenbergBlocks(anyOrNull()) }.doReturn( | ||
| containsGutenbergBlocks | ||
| ) | ||
| } | ||
|
|
||
| fun createAztecEditorWrapper(mediaInPost: Set<MediaModel>) = | ||
| mock<AztecEditorFragmentStaticWrapper> { | ||
| on { isMediaInPostBody(anyOrNull(), anyOrNull()) } | ||
| .doAnswer { invocation -> | ||
| mediaInPost.map { it.id } | ||
| .contains((invocation.arguments[1] as String).toInt()) | ||
| } | ||
| } | ||
|
|
||
| fun createMediaList(): List<MediaModel> { | ||
| val list = mutableListOf<MediaModel>() | ||
| for (i in 1..10) { | ||
| list.add(MediaModel().apply { id = i }) | ||
| } | ||
| return list | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@hypest I'm a bit confused by this code and I was wondering if you could bring some light into it.
Both
AppPrefs.isAztecEditorEnabled();andAppPrefs.isGutenbergEditorEnabled();always returntrue. Which means we always perform the search for media usingAztecEditorFragment.isMediaInPostBody(and we never usePostUtils.isMediaInGutenbergPostBody. Do you think it's desired behaviour and it's just a preparation for the time when we remove Aztec from the app or it's a bug?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.
It might be a combination of "prepare for removing Aztec" and a bug.
It makes sense to have the different conditionals (in preparation of removing Aztec) but the Gutenberg check should probably come first, I think. I'm not super sure though so, let's also ping @mzorz who wrote this to confirm.
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.
Looks like a "prepare for removing Aztec" glitch, which then introduced other bugs.
Not exactly :D looks like it was actually you @hypest 😄, but I certainly appreciate being considered as someone who participated in building this 🙇 (jokes aside I was definitely involved in these conversations about opt-in and such so, thanks for the ping)
I had a look back in code's history, to make sense about existing code and past decisions (git blame is my friend). A bit of searching shows where this comes from:
isGutenbergEditorEnabled()andisAztecEditorEnabled()are hardcoded to returntrue630f454
That explains why / what was done, but of course history doesn't say much about future plans, so deferring back to @hypest here looks like the right way to go.
What to do
IMO, regarding that piece of code itself (code in
purgeMediaToPostAssociationsIfNotInPostAnymore()), we should make sure bothAztecEditorFragment.isMediaInPostBody()runs when in a non-Gutenberg Post, and also make sure thatPostUtils.isMediaInGutenbergPostBody()gets run for Gutenberg-block-containing Posts, which apparently is not the case anymore since that commit up there.In principle, this sounds like a good approach (inverting the
ifchecks to first check for gutenberg, andelse ifcheck Aztec). Needless to say, all of these cases should be hunted down and scrutinized thoroughly though, to make sure code does what we want it t do.We should also care checking for other places where these checks are made, for example here
I'd also suggest doing a search for both keywords as these throw various places where conditions shown won't be met and are worth taking a deep look into (and fix them), could be in a separate PR but most probably still within the context of this migration to UseCase (not sure what the plans are but I'd tend to think this is important)
Search for
isAztecEditorEnabledSearch for
isGutenbergEditorEnabledAlso note
Another thing worth noting in the context of evaluating this method and its purpose: the actual purge code there (method
purgeMediaToPostAssociationsIfNotInPostAnymore()) was introduced in fe89892, belonging to this PR #7081, it was needed in case we had lostMediaUploadModelinstances in FluxC that are no longer valid (for example when an un unexpected close happened during an upload for an image in a Post), leaving things in an inconsistent state (you can check the usages forClearMediaPayloadhere)Hope that helps clarify things a bit!
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.
❤️ ❤️ ❤️ Thanks for such a detailed elaboration @mzorz !!
Do you think we should check if the post itself contains a gutenberg block using (
WordPress-Android/WordPress/src/main/java/org/wordpress/android/ui/posts/PostUtils.java
Line 391 in a29f28c
if(contentContainsGutenbergBlocks ) PostUtils.isMediaInGutenbergPostBody() else AztecEditorFragment.isMediaInPostBody()and not use isAztecEditorEnabled/isGutenbergEditorEnabled at all?I'll do my best checking those as well, but I'm not sure I have enough context to make the right decision.
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.
yw! ❤️
That sounds good to me! 👍
Understood @malinajirka, your willingness is appreciated, maybe this belongs more in the realm of Gutenberg team? I can help checking your code with @hypest (or someone @hypest may like to point to) if you need 👍
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.
I've created a separate issue for the audit - #10881. I'll look into it this or next week.