diff --git a/WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostRepository.kt b/WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostRepository.kt index 48d4b4ed98e3..0b6066f85e1d 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostRepository.kt +++ b/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 diff --git a/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListMainViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListMainViewModel.kt index a007e1746a81..13c92150c94d 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListMainViewModel.kt +++ b/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() { diff --git a/WordPress/src/main/java/org/wordpress/android/ui/posts/prepublishing/PrepublishingBottomSheetFragment.kt b/WordPress/src/main/java/org/wordpress/android/ui/posts/prepublishing/PrepublishingBottomSheetFragment.kt index 74415d70a1b0..7323af942cc6 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/posts/prepublishing/PrepublishingBottomSheetFragment.kt +++ b/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(SITE)) - viewModel.start(site, prepublishingScreenState) + viewModel.start(site, prepublishingScreenState, getEditorHook().editPostRepository.hasPost()) } private fun navigateToScreen(navigationTarget: PrepublishingNavigationTarget) { diff --git a/WordPress/src/main/java/org/wordpress/android/ui/posts/prepublishing/PrepublishingViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/posts/prepublishing/PrepublishingViewModel.kt index 725130edb0b2..8ef6138a9f61 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/posts/prepublishing/PrepublishingViewModel.kt +++ b/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 diff --git a/WordPress/src/main/java/org/wordpress/android/ui/posts/prepublishing/home/PrepublishingHomeViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/posts/prepublishing/home/PrepublishingHomeViewModel.kt index 30a19c97065c..87dbea59e99b 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/posts/prepublishing/home/PrepublishingHomeViewModel.kt +++ b/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) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/posts/PostListMainViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/posts/PostListMainViewModelTest.kt index b80137daee86..d3dd0494ea51 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/posts/PostListMainViewModelTest.kt +++ b/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() + 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 diff --git a/WordPress/src/test/java/org/wordpress/android/ui/posts/PrepublishingHomeViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/posts/PrepublishingHomeViewModelTest.kt index cc7de7211ba7..6542b486233f 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/posts/PrepublishingHomeViewModelTest.kt +++ b/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? { diff --git a/WordPress/src/test/java/org/wordpress/android/ui/posts/prepublishing/PrepublishingViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/posts/prepublishing/PrepublishingViewModelTest.kt index 352091399128..cac0d9ca1651 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/posts/prepublishing/PrepublishingViewModelTest.kt +++ b/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? = null + var navigationEvent: Event? = 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? = 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,7 +201,7 @@ class PrepublishingViewModelTest : BaseUnitTest() { event = it } - viewModel.start(mockSite, mock()) + viewModel.start(mockSite, mock(), hasPost = true) viewModel.onActionClicked(Action.NavigateToSharingSettings) assertThat(event?.peekContent()).isEqualTo(mockSite) @@ -185,7 +209,7 @@ class PrepublishingViewModelTest : BaseUnitTest() { @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)