Move purgeMediaToPostAssociationIfNotInPostAnymore to UseCase #10798

malinajirka · 2019-11-14T14:47:12Z

Merge instruction:

Review and merge Replace EditorMediaPostData with PostImmutableModel #10795 first
Change the target branch to develop or master-edit-post-activity-refactor

Moves purgeMedia.. into its own useCase and introduces unit tests.

To test:

Create a post
Turn on airplane mode
Add an image
Kill the app
Open the post - you can attach debugger or verify in Stetho that the MediaModel gets deleted
PR submission checklist:

I have considered adding unit tests where possible.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

...ain/java/org/wordpress/android/ui/posts/editor/media/CleanUpMediaToPostAssociationUseCase.kt

...java/org/wordpress/android/ui/posts/editor/media/CleanUpMediaToPostAssociationUseCaseTest.kt

peril-wordpress-mobile · 2019-11-14T15:03:13Z

You can test the changes on this Pull Request by downloading the APK here.

planarvoid · 2019-11-15T12:35:40Z

...ress/src/main/java/org/wordpress/android/ui/posts/editor/AztecEditorFragmentStaticWrapper.kt

+ *
+ */
+@Reusable
+class AztecEditorFragmentStaticWrapper @Inject constructor(private val appContext: Context) {


I've created something similar in my PR, we need to think about it when we merge the PRs

The PR is already merged. Do you think you can now merge the 2 classes? :-)

@planarvoid Hmm, I checked develop yesterday and I couldn't find AztecEditorWrapper there. It seems we merged the PR into the master_edit_post_activity_refactoring branch but this PR targets develop.

planarvoid · 2019-11-15T12:44:41Z

...java/org/wordpress/android/ui/posts/editor/media/CleanUpMediaToPostAssociationUseCaseTest.kt

+import org.wordpress.android.ui.prefs.AppPrefsWrapper
+
+@InternalCoroutinesApi
+@RunWith(Parameterized::class)


malinajirka · 2019-11-15T13:07:24Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java

-        if (!allMedia.isEmpty()) {
-            HashSet<MediaModel> mediaToDeleteAssociationFor = new HashSet<>();
-            for (MediaModel media : allMedia) {
-                if (useAztec) {


@hypest I'm a bit confused by this code and I was wondering if you could bring some light into it.
Both AppPrefs.isAztecEditorEnabled(); and AppPrefs.isGutenbergEditorEnabled(); always return true. Which means we always perform the search for media using AztecEditorFragment.isMediaInPostBody( and we never use PostUtils.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?

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.

Looks like a "prepare for removing Aztec" glitch, which then introduced other bugs.

let's also ping @mzorz who wrote this to confirm.

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:

This is where both isGutenbergEditorEnabled() and isAztecEditorEnabled() are hardcoded to return true
630f454

That commit belongs to this PR Enable Gutenberg for all and hide legacy editors options #9208

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 both AztecEditorFragment.isMediaInPostBody() runs when in a non-Gutenberg Post, and also make sure that PostUtils.isMediaInGutenbergPostBody() gets run for Gutenberg-block-containing Posts, which apparently is not the case anymore since that commit up there.

It makes sense to have the different conditionals (in preparation of removing Aztec) but the Gutenberg check should probably come first, I think.

In principle, this sounds like a good approach (inverting the if checks to first check for gutenberg, and else if check 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 isAztecEditorEnabled

Search for isGutenbergEditorEnabled

Also 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 lost MediaUploadModel instances 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 for ClearMediaPayload here)

Hope that helps clarify things a bit!

❤️ ❤️ ❤️ Thanks for such a detailed elaboration @mzorz !!

we should make sure both AztecEditorFragment.isMediaInPostBody() runs when in a non-Gutenberg Post, and also make sure that PostUtils.isMediaInGutenbergPostBody() gets run for Gutenberg-block-containing Posts

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

public static boolean contentContainsGutenbergBlocks(String postContent) {

) => so basically replace the if with if(contentContainsGutenbergBlocks ) PostUtils.isMediaInGutenbergPostBody() else AztecEditorFragment.isMediaInPostBody() and not use isAztecEditorEnabled/isGutenbergEditorEnabled at all?

We should also care checking for other places where these checks are made, for example here

I'll do my best checking those as well, but I'm not sure I have enough context to make the right decision.

Thanks for such a detailed elaboration

yw! ❤️

so basically replace the if with if(contentContainsGutenbergBlocks ) PostUtils.isMediaInGutenbergPostBody() else AztecEditorFragment.isMediaInPostBody() and not use isAztecEditorEnabled/isGutenbergEditorEnabled at all?

That sounds good to me! 👍

We should also care checking for other places where these checks are made, for example here

I'll do my best checking those as well, but I'm not sure I have enough context to make the right decision.

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 👍

I've created a separate issue for the audit - #10881. I'll look into it this or next week.

jkmassel · 2019-11-18T22:49:32Z

Howdy folks! I'm freezing 13.7 today, so this is being bumped to 13.8. If you'd like it to go out as part of 13.7, please target release/13.7 for this PR, and DM me once it's merged – I'll be happy to cut a new beta release!

malinajirka · 2019-12-02T11:43:21Z

Thanks @planarvoid! I think it's ready for another round.

jkmassel · 2019-12-03T06:32:43Z

We're freezing 13.8 today, so I'm moving this forward to 13.9. If you'd like it released as part of 13.8, please adjust the target from develop to release/13.8, and let me know once this PR has been merged – I'll be happy to ship a new release candidate!

planarvoid

Looks good, the purge runs (I tested it with the debugger) 👍

Move purgeMediaToPostAssociationIfNotInPostAnymore to UseCase

860a5a7

malinajirka added Tech Debt Post-Logic-Improvements labels Nov 14, 2019

malinajirka added this to the 13.7 milestone Nov 14, 2019

malinajirka requested a review from planarvoid November 14, 2019 14:47

wpmobilebot reviewed Nov 14, 2019

View reviewed changes

...ain/java/org/wordpress/android/ui/posts/editor/media/CleanUpMediaToPostAssociationUseCase.kt Outdated Show resolved Hide resolved

wpmobilebot reviewed Nov 14, 2019

View reviewed changes

...java/org/wordpress/android/ui/posts/editor/media/CleanUpMediaToPostAssociationUseCaseTest.kt Outdated Show resolved Hide resolved

wpmobilebot reviewed Nov 14, 2019

View reviewed changes

...java/org/wordpress/android/ui/posts/editor/media/CleanUpMediaToPostAssociationUseCaseTest.kt Outdated Show resolved Hide resolved

wpmobilebot reviewed Nov 14, 2019

View reviewed changes

...java/org/wordpress/android/ui/posts/editor/media/CleanUpMediaToPostAssociationUseCaseTest.kt Outdated Show resolved Hide resolved

Fix lint issues

6dbe3d4

planarvoid reviewed Nov 15, 2019

View reviewed changes

malinajirka commented Nov 15, 2019

View reviewed changes

jkmassel modified the milestones: 13.7, 13.8 Nov 18, 2019

malinajirka changed the base branch from simplify-editor-media to develop November 19, 2019 10:22

malinajirka added 3 commits December 2, 2019 11:50

Merge branch 'develop' into move-purge-media-method

812ab44

Use PostUtils.containsGutenbergBlocks in CleanUpMediaToPostAssociation

a2bd9ea

Fix lint

1d7b7bc

malinajirka mentioned this pull request Dec 2, 2019

Check usages of isAztecEditorEnabled and isGutenbergEditorEnabled #10881

Open

jkmassel modified the milestones: 13.8, 13.9 Dec 3, 2019

planarvoid approved these changes Dec 3, 2019

View reviewed changes

planarvoid merged commit 7ed34a5 into develop Dec 3, 2019

planarvoid deleted the move-purge-media-method branch December 3, 2019 10:32

malinajirka mentioned this pull request Jan 7, 2020

Post Logic Improvements: Out-of-Scope refactoring tasks #10692

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move purgeMediaToPostAssociationIfNotInPostAnymore to UseCase #10798

Move purgeMediaToPostAssociationIfNotInPostAnymore to UseCase #10798

malinajirka commented Nov 14, 2019

peril-wordpress-mobile bot commented Nov 14, 2019 •

edited

Loading

planarvoid Nov 15, 2019 •

edited

Loading

planarvoid Dec 3, 2019

malinajirka Dec 3, 2019

planarvoid Nov 15, 2019

malinajirka Nov 15, 2019

hypest Nov 19, 2019

mzorz Nov 19, 2019

malinajirka Nov 20, 2019

mzorz Nov 20, 2019

malinajirka Dec 2, 2019

jkmassel commented Nov 18, 2019

malinajirka commented Dec 2, 2019

jkmassel commented Dec 3, 2019

planarvoid left a comment

Move purgeMediaToPostAssociationIfNotInPostAnymore to UseCase #10798

Move purgeMediaToPostAssociationIfNotInPostAnymore to UseCase #10798

Conversation

malinajirka commented Nov 14, 2019

peril-wordpress-mobile bot commented Nov 14, 2019 • edited Loading

planarvoid Nov 15, 2019 • edited Loading

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

jkmassel commented Nov 18, 2019

malinajirka commented Dec 2, 2019

jkmassel commented Dec 3, 2019

planarvoid left a comment

Choose a reason for hiding this comment

peril-wordpress-mobile bot commented Nov 14, 2019 •

edited

Loading

planarvoid Nov 15, 2019 •

edited

Loading