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/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/PrepublishingViewModel.kt

@@ -63,14 +63,16 @@ class PrepublishingViewModel @Inject constructor(private val dispatcher: Dispatc
     fun start(
         site: SiteModel,
         currentScreenFromSavedState: PrepublishingScreen?,
-        hasPost: Boolean = true
+        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. Guarding here rather than in an
-        // individual screen's ViewModel covers every screen the sheet can restore into, since the
-        // saved screen below may be TAGS, CATEGORIES or PUBLISH rather than HOME.
+        // 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))

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/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,7 +49,7 @@ class PrepublishingViewModelTest : BaseUnitTest() {
             event = it
         }
 
-        viewModel.start(mock(), expectedScreen)
+        viewModel.start(mock(), expectedScreen, hasPost = true)
 
         assertThat(event?.peekContent()?.targetScreen).isEqualTo(expectedScreen)
     }
@@ -87,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)
@@ -100,7 +100,7 @@ class PrepublishingViewModelTest : BaseUnitTest() {
             event = it
         }
 
-        viewModel.start(mock(), HOME)
+        viewModel.start(mock(), HOME, hasPost = true)
         viewModel.onBackClicked()
 
         assertThat(event).isNotNull
@@ -127,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)
@@ -142,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)
@@ -157,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)
@@ -172,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)
@@ -187,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)
@@ -201,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)