Fix crash when the Prepublishing sheet is restored without a post

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)