Fix crash when the Prepublishing sheet is restored without a post (#23148)

* Fix crash when the Prepublishing sheet is restored without a post

The sheet is a DialogFragment, so the FragmentManager restores it after a
config change or process death regardless of whether the host activity has
loaded a post, and PrepublishingHomeFragment then built its UI against an
empty EditPostRepository. Dismiss the sheet instead, and stop
EditPostRepository.status from laundering a null post through the
unannotated PostStatus.fromPost.

Fixes JETPACK-ANDROID-1EVE

* Move the no-post guard up to PrepublishingViewModel

The guard added in the previous commit only covered the HOME screen, but a
restored sheet navigates to whatever screen was saved in KEY_SCREEN_STATE. On a
restore into TAGS, CATEGORIES or PUBLISH the sheet stayed open over an empty
EditPostRepository and still crashed on the first interaction, via
requireNotNull(post) in updateAsync.

Guarding in PrepublishingViewModel.start() instead covers every screen the sheet
can restore into, and reuses the existing dismissBottomSheet channel, so the
dismissSheet LiveData and the parentFragment cast are no longer needed. The
PrepublishingHomeViewModel early return stays as a last line of defense, since
the dismissal is asynchronous and the FragmentManager can restore that fragment
before it commits.

Also reverts the EditPostRepository.status leniency: with the guard in place its
only reader is unreachable with a null post, so returning UNKNOWN just hid the
missing post from the other callers.

* Rebind the EditPostRepository when the posts list is recreated

PostsListActivity has no configChanges, so it is recreated on rotation with a
freshly injected EditPostRepository, while PostListMainViewModel survives. Its
start() returned early on isStarted before assigning the repository and before
loading the post into it, so after any rotation the ViewModel kept writing to the
previous activity's dead repository while the restored Prepublishing sheet read
the new empty one. That is the state the sheet guard was catching: publishing
stayed broken for the rest of the activity's life, silently once the sheet began
dismissing itself instead of crashing.

Rebind on every start instead, and keep currentBottomSheetPostId in sync so the
id onSaveInstanceState reads survives a second restore.

Also makes PrepublishingViewModel.start()'s hasPost parameter required, so the
guard can't be bypassed by omission, and corrects the comment: the guard
dismisses on every restore path but does not stop the saved child fragment from
being restored and started first, since the dismissal is async.

Author: nbradbury

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostRepository.kt

@@ -73,7 +73,7 @@ class EditPostRepository
     val password: String
         get() = post!!.password
     val status: PostStatus
-        get() = fromPost(getPost())
+        get() = fromPost(post!!)
     val isPage: Boolean
         get() = post!!.isPage
     val isLocalDraft: Boolean

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListMainViewModel.kt

@@ -244,9 +244,14 @@ class PostListMainViewModel @Inject constructor(
         currentBottomSheetPostId: LocalId,
         editPostRepository: EditPostRepository
     ) {
+        // The activity is recreated on rotation with a freshly injected EditPostRepository while this
+        // ViewModel survives, so rebind on every start rather than only the first. Otherwise the
+        // restored Prepublishing sheet reads an empty repository and this ViewModel goes on writing
+        // to the previous activity's, which breaks publishing for the rest of the activity's life.
+        bindEditPostRepository(editPostRepository, site, currentBottomSheetPostId)
+
         if (isStarted) return
         this.site = site
-        this.editPostRepository = editPostRepository
 
         val authorFilterSelection: AuthorFilterSelection = if (isFilteringByAuthorSupported) {
             prefs.postListAuthorSelection
@@ -289,23 +294,34 @@ class PostListMainViewModel @Inject constructor(
         )
         _previewState.value = _previewState.value ?: initPreviewState
 
-        currentBottomSheetPostId.let { postId ->
-            if (postId.value != 0) {
-                editPostRepository.loadPostByLocalPostId(postId.value)
-            }
-        }
-
         lifecycleOwner.lifecycleRegistry.currentState = Lifecycle.State.STARTED
 
         uploadStarter.queueUploadFromSite(site)
 
-        editPostRepository.run {
-            postChanged.observe(lifecycleOwner, Observer {
-                savePostToDbUseCase.savePostToDb(editPostRepository, site)
-            })
+        isStarted = true
+    }
+
+    private fun bindEditPostRepository(
+        repository: EditPostRepository,
+        site: SiteModel,
+        bottomSheetPostId: LocalId
+    ) {
+        if (this::editPostRepository.isInitialized) {
+            if (this.editPostRepository === repository) return
+            // drop the previous activity's repository, or its observer would pin it for our lifetime
+            this.editPostRepository.postChanged.removeObservers(lifecycleOwner)
+        }
+        this.editPostRepository = repository
+
+        if (bottomSheetPostId.value != 0) {
+            // keep the field in sync so onSaveInstanceState can round-trip it again
+            currentBottomSheetPostId = bottomSheetPostId
+            repository.loadPostByLocalPostId(bottomSheetPostId.value)
         }
 
-        isStarted = true
+        repository.postChanged.observe(lifecycleOwner, Observer {
+            savePostToDbUseCase.savePostToDb(repository, site)
+        })
     }
 
     override fun onCleared() {

WordPress/src/main/java/org/wordpress/android/ui/posts/prepublishing/PrepublishingBottomSheetFragment.kt

@@ -191,7 +191,7 @@ class PrepublishingBottomSheetFragment : WPBottomSheetDialogFragment(),
             KEY_SCREEN_STATE
         )
         val site = requireNotNull(arguments?.getSerializableCompat<SiteModel>(SITE))
-        viewModel.start(site, prepublishingScreenState)
+        viewModel.start(site, prepublishingScreenState, getEditorHook().editPostRepository.hasPost())
     }
 
     private fun navigateToScreen(navigationTarget: PrepublishingNavigationTarget) {

WordPress/src/main/java/org/wordpress/android/ui/posts/prepublishing/PrepublishingViewModel.kt

@@ -62,10 +62,23 @@ class PrepublishingViewModel @Inject constructor(private val dispatcher: Dispatc
 
     fun start(
         site: SiteModel,
-        currentScreenFromSavedState: PrepublishingScreen?
+        currentScreenFromSavedState: PrepublishingScreen?,
+        hasPost: Boolean
     ) {
         this.site = site
 
+        // The sheet is a DialogFragment, so the FragmentManager restores it after a config change or
+        // process death even when the host hasn't loaded a post. Every restore passes through here,
+        // whatever screen was saved in KEY_SCREEN_STATE, so this dismisses the sheet in all of them.
+        // Note it does not stop the saved child fragment from being restored and started first: the
+        // dismissal is async. The non-HOME screens tolerate an empty repository at start time, and
+        // HOME has its own early return. A new screen that reads the post at start needs one too.
+        if (!hasPost) {
+            AppLog.e(T.POSTS, "Prepublishing sheet started without a post; dismissing.")
+            _dismissBottomSheet.postValue(Event(Unit))
+            return
+        }
+
         // Set screen: use saved state if available (config change), otherwise reset to HOME (dismissal + reopen)
         val targetScreen = currentScreenFromSavedState ?: HOME
         this.currentScreen = targetScreen

WordPress/src/main/java/org/wordpress/android/ui/posts/prepublishing/home/PrepublishingHomeViewModel.kt

@@ -23,6 +23,7 @@ import org.wordpress.android.ui.posts.prepublishing.home.usecases.GetButtonUiSta
 import org.wordpress.android.ui.posts.trackPrepublishingNudges
 import org.wordpress.android.ui.utils.UiString.UiStringRes
 import org.wordpress.android.ui.utils.UiString.UiStringText
+import org.wordpress.android.util.AppLog
 import org.wordpress.android.util.StringUtils
 import org.wordpress.android.util.analytics.AnalyticsTrackerWrapper
 import org.wordpress.android.util.merge
@@ -68,6 +69,13 @@ class PrepublishingHomeViewModel @Inject constructor(
     fun start(editPostRepository: EditPostRepository, site: SiteModel) {
         this.editPostRepository = editPostRepository
         if (isStarted) return
+        // PrepublishingViewModel already dismisses the sheet when the host has no post, but that
+        // dismissal is asynchronous and the FragmentManager can restore this fragment and create its
+        // view before it commits. Skip the UI rather than reading a post that isn't there.
+        if (!editPostRepository.hasPost()) {
+            AppLog.e(AppLog.T.POSTS, "Prepublishing home started without a post; skipping setup.")
+            return
+        }
         isStarted = true
 
         setupHomeUiState(editPostRepository, site)

WordPress/src/test/java/org/wordpress/android/ui/posts/PostListMainViewModelTest.kt

@@ -159,6 +159,33 @@ class PostListMainViewModelTest : BaseUnitTest() {
         verify(editPostRepository, times(0)).loadPostByLocalPostId(any())
     }
 
+    @Test
+    fun `when the activity is recreated, then the new EditPostRepository is rebound and reloaded`() {
+        // arrange - the activity is recreated on rotation with a freshly injected repository
+        val bottomSheetPostId = LocalId(2)
+        viewModel.start(site, PostListRemotePreviewState.NONE, bottomSheetPostId, editPostRepository)
+        val recreatedRepository = mock<EditPostRepository>()
+        whenever(recreatedRepository.postChanged).thenReturn(MutableLiveData(Event(PostModel())))
+
+        // act - start() runs again on the surviving ViewModel
+        viewModel.start(site, PostListRemotePreviewState.NONE, bottomSheetPostId, recreatedRepository)
+
+        // assert - the second repository must be loaded too, or the restored sheet reads an empty one
+        verify(recreatedRepository, times(1)).loadPostByLocalPostId(bottomSheetPostId.value)
+    }
+
+    @Test
+    fun `given a restored bottom sheet post id, when started, then the id is kept for the next save`() {
+        // arrange
+        val bottomSheetPostId = LocalId(2)
+
+        // act
+        viewModel.start(site, PostListRemotePreviewState.NONE, bottomSheetPostId, editPostRepository)
+
+        // assert - onSaveInstanceState reads this field, so it has to survive a restore round-trip
+        assertThat(viewModel.currentBottomSheetPostId).isEqualTo(bottomSheetPostId)
+    }
+
     @Test
     fun `if post in EditPostRepository is modified then the savePostToDbUseCase should update the post`() {
         // arrange

WordPress/src/test/java/org/wordpress/android/ui/posts/PrepublishingHomeViewModelTest.kt

@@ -71,6 +71,7 @@ class PrepublishingHomeViewModelTest : BaseUnitTest() {
         ).doAnswer {
             PublishButtonUiState(it.arguments[2] as (PublishPost) -> Unit)
         }
+        whenever(editPostRepository.hasPost()).thenReturn(true)
         whenever(editPostRepository.getEditablePost()).thenReturn(PostModel())
         whenever(postSettingsUtils.getPublishDateLabel(any())).thenReturn((""))
         whenever(site.name).thenReturn("")
@@ -86,7 +87,7 @@ class PrepublishingHomeViewModelTest : BaseUnitTest() {
         val expectedActionsAmount = 3
 
         // act
-        viewModel.start(mock(), site)
+        viewModel.start(editPostRepository, site)
 
         // assert
         assertThat(viewModel.uiState.value?.filterIsInstance(HomeUiState::class.java)?.size).isEqualTo(
@@ -163,7 +164,7 @@ class PrepublishingHomeViewModelTest : BaseUnitTest() {
         val expectedActionsAmount = 1
 
         // act
-        viewModel.start(mock(), site)
+        viewModel.start(editPostRepository, site)
 
         // assert
         assertThat(viewModel.uiState.value?.filterIsInstance(HeaderUiState::class.java)?.size).isEqualTo(
@@ -177,7 +178,7 @@ class PrepublishingHomeViewModelTest : BaseUnitTest() {
         val expectedActionsAmount = 1
 
         // act
-        viewModel.start(mock(), site)
+        viewModel.start(editPostRepository, site)
 
         // assert
         assertThat(viewModel.uiState.value?.filterIsInstance(ButtonUiState::class.java)?.size).isEqualTo(
@@ -191,7 +192,7 @@ class PrepublishingHomeViewModelTest : BaseUnitTest() {
         val expectedActionType = PrepublishingScreenNavigation.Publish
 
         // act
-        viewModel.start(mock(), site)
+        viewModel.start(editPostRepository, site)
         val publishAction = getHomeUiState(expectedActionType)
         publishAction?.onNavigationActionClicked?.invoke(expectedActionType)
 
@@ -205,7 +206,7 @@ class PrepublishingHomeViewModelTest : BaseUnitTest() {
         val expectedActionType = PrepublishingScreenNavigation.Tags
 
         // act
-        viewModel.start(mock(), site)
+        viewModel.start(editPostRepository, site)
         val tagsAction = getHomeUiState(expectedActionType)
         tagsAction?.onNavigationActionClicked?.invoke(expectedActionType)
 
@@ -397,6 +398,32 @@ class PrepublishingHomeViewModelTest : BaseUnitTest() {
         assertThat(uiSocialState).isEqualTo(SocialUiState.Hidden)
     }
 
+    @Test
+    fun `given a repository with no post, when the viewModel is started, then no ui state is built`() {
+        // arrange
+        whenever(editPostRepository.hasPost()).thenReturn(false)
+
+        // act
+        viewModel.start(editPostRepository, site)
+
+        // assert
+        assertThat(viewModel.uiState.value).isNull()
+    }
+
+    @Test
+    fun `given a start with no post, when started again once the post loads, then the ui state is built`() {
+        // arrange
+        whenever(editPostRepository.hasPost()).thenReturn(false)
+        viewModel.start(editPostRepository, site)
+
+        // act - the guard must not latch isStarted, or the sheet would stay empty forever
+        whenever(editPostRepository.hasPost()).thenReturn(true)
+        viewModel.start(editPostRepository, site)
+
+        // assert
+        assertThat(viewModel.uiState.value).isNotNull
+    }
+
     private fun getHeaderUiState() = viewModel.uiState.value?.filterIsInstance(HeaderUiState::class.java)?.first()
 
     private fun getButtonUiState(): ButtonUiState? {

WordPress/src/test/java/org/wordpress/android/ui/posts/prepublishing/PrepublishingViewModelTest.kt

@@ -35,7 +35,7 @@ class PrepublishingViewModelTest : BaseUnitTest() {
             event = it
         }
 
-        viewModel.start(mock(), null)
+        viewModel.start(mock(), null, hasPost = true)
 
         assertThat(event?.peekContent()?.targetScreen).isEqualTo(expectedScreen)
     }
@@ -49,11 +49,35 @@ class PrepublishingViewModelTest : BaseUnitTest() {
             event = it
         }
 
-        viewModel.start(mock(), expectedScreen)
+        viewModel.start(mock(), expectedScreen, hasPost = true)
 
         assertThat(event?.peekContent()?.targetScreen).isEqualTo(expectedScreen)
     }
 
+    @Test
+    fun `when viewModel start is called without a post, dismiss the sheet instead of navigating`() {
+        var dismissEvent: Event<Unit>? = null
+        var navigationEvent: Event<PrepublishingNavigationTarget>? = null
+        viewModel.dismissBottomSheet.observeForever { dismissEvent = it }
+        viewModel.navigationTarget.observeForever { navigationEvent = it }
+
+        // TAGS rather than HOME because a restored sheet can land on any screen
+        viewModel.start(mock(), TAGS, hasPost = false)
+
+        assertThat(dismissEvent).isNotNull
+        assertThat(navigationEvent).isNull()
+    }
+
+    @Test
+    fun `when viewModel start is called with a post, don't dismiss the sheet`() {
+        var dismissEvent: Event<Unit>? = null
+        viewModel.dismissBottomSheet.observeForever { dismissEvent = it }
+
+        viewModel.start(mock(), TAGS, hasPost = true)
+
+        assertThat(dismissEvent).isNull()
+    }
+
     @Test
     fun `when onBackClicked is pressed and currentScreen isn't HOME, navigate to HOME`() {
         val expectedScreen = HOME
@@ -63,7 +87,7 @@ class PrepublishingViewModelTest : BaseUnitTest() {
             event = it
         }
 
-        viewModel.start(mock(), TAGS)
+        viewModel.start(mock(), TAGS, hasPost = true)
         viewModel.onBackClicked()
 
         assertThat(event?.peekContent()?.targetScreen).isEqualTo(expectedScreen)
@@ -76,7 +100,7 @@ class PrepublishingViewModelTest : BaseUnitTest() {
             event = it
         }
 
-        viewModel.start(mock(), HOME)
+        viewModel.start(mock(), HOME, hasPost = true)
         viewModel.onBackClicked()
 
         assertThat(event).isNotNull
@@ -103,7 +127,7 @@ class PrepublishingViewModelTest : BaseUnitTest() {
             event = it
         }
 
-        viewModel.start(mock(), mock())
+        viewModel.start(mock(), mock(), hasPost = true)
         viewModel.onActionClicked(PrepublishingScreenNavigation.Tags)
 
         assertThat(event?.peekContent()?.targetScreen).isEqualTo(expectedScreen)
@@ -118,7 +142,7 @@ class PrepublishingViewModelTest : BaseUnitTest() {
             event = it
         }
 
-        viewModel.start(mock(), mock())
+        viewModel.start(mock(), mock(), hasPost = true)
         viewModel.onActionClicked(PrepublishingScreenNavigation.Publish)
 
         assertThat(event?.peekContent()?.targetScreen).isEqualTo(expectedScreen)
@@ -133,7 +157,7 @@ class PrepublishingViewModelTest : BaseUnitTest() {
             event = it
         }
 
-        viewModel.start(mock(), mock())
+        viewModel.start(mock(), mock(), hasPost = true)
         viewModel.onActionClicked(PrepublishingScreenNavigation.Categories)
 
         assertThat(event?.peekContent()?.targetScreen).isEqualTo(expectedScreen)
@@ -148,7 +172,7 @@ class PrepublishingViewModelTest : BaseUnitTest() {
             event = it
         }
 
-        viewModel.start(mock(), mock())
+        viewModel.start(mock(), mock(), hasPost = true)
         viewModel.onActionClicked(PrepublishingScreenNavigation.AddCategory)
 
         assertThat(event?.peekContent()?.targetScreen).isEqualTo(expectedScreen)
@@ -163,7 +187,7 @@ class PrepublishingViewModelTest : BaseUnitTest() {
             event = it
         }
 
-        viewModel.start(mock(), mock())
+        viewModel.start(mock(), mock(), hasPost = true)
         viewModel.onActionClicked(PrepublishingScreenNavigation.Social)
 
         assertThat(event?.peekContent()?.targetScreen).isEqualTo(expectedScreen)
@@ -177,15 +201,15 @@ class PrepublishingViewModelTest : BaseUnitTest() {
             event = it
         }
 
-        viewModel.start(mockSite, mock())
+        viewModel.start(mockSite, mock(), hasPost = true)
         viewModel.onActionClicked(Action.NavigateToSharingSettings)
 
         assertThat(event?.peekContent()).isEqualTo(mockSite)
     }
 
     @Test
     fun `when onSubmitButtonClicked is triggered then bottom sheet should close and listener is triggered`() {
-        viewModel.start(mock(), mock())
+        viewModel.start(mock(), mock(), hasPost = true)
 
         viewModel.onSubmitButtonClicked(true)