Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly placed pages tests #8303

Merged
merged 12 commits into from
Sep 18, 2018
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ sealed class PageItem(open val type: Type) {
override val type: Type
) : PageItem(type)

data class Divider(val title: String) : PageItem(DIVIDER)
data class Divider(val title: String = "") : PageItem(DIVIDER)

data class Empty(
@StringRes val textResource: Int = R.string.empty_list_default,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ class PageListViewModel @Inject constructor() : ViewModel() {
}
} else {
val pagesWithBottomGap = newPages.toMutableList()
pagesWithBottomGap.addAll(listOf(Divider(""), Divider("")))
pagesWithBottomGap.addAll(listOf(Divider(), Divider()))
_pages.postValue(pagesWithBottomGap)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import android.arch.lifecycle.LiveData
import android.arch.lifecycle.MutableLiveData
import android.arch.lifecycle.ViewModel
import android.support.annotation.StringRes
import kotlinx.coroutines.experimental.CommonPool
import kotlinx.coroutines.experimental.CoroutineDispatcher
import kotlinx.coroutines.experimental.Job
import kotlinx.coroutines.experimental.delay
Expand All @@ -19,6 +18,7 @@ import org.wordpress.android.fluxc.model.page.PageModel
import org.wordpress.android.fluxc.model.page.PageStatus
import org.wordpress.android.fluxc.store.PageStore
import org.wordpress.android.fluxc.store.PostStore.OnPostUploaded
import org.wordpress.android.modules.COMMON_POOL_CONTEXT
import org.wordpress.android.modules.UI_CONTEXT
import org.wordpress.android.ui.pages.PageItem.Action
import org.wordpress.android.ui.pages.PageItem.Action.DELETE_PERMANENTLY
Expand Down Expand Up @@ -47,12 +47,17 @@ import javax.inject.Inject
import javax.inject.Named
import kotlin.coroutines.experimental.Continuation

private const val ACTION_DELAY = 100
private const val SEARCH_DELAY = 200
private const val SEARCH_COLLAPSE_DELAY = 500

class PagesViewModel
@Inject constructor(
private val pageStore: PageStore,
private val dispatcher: Dispatcher,
private val actionPerfomer: ActionPerformer,
@Named(UI_CONTEXT) private val uiContext: CoroutineDispatcher
@Named(UI_CONTEXT) private val uiContext: CoroutineDispatcher,
@Named(COMMON_POOL_CONTEXT) private val commonPoolContext: CoroutineDispatcher
) : ViewModel() {
private val _isSearchExpanded = MutableLiveData<Boolean>()
val isSearchExpanded: LiveData<Boolean> = _isSearchExpanded
Expand Down Expand Up @@ -84,14 +89,10 @@ class PagesViewModel
private val _setPageParent = SingleLiveEvent<PageModel?>()
val setPageParent: LiveData<PageModel?> = _setPageParent

private var _pageMap: MutableMap<Long, PageModel> = mutableMapOf()
private var pageMap: MutableMap<Long, PageModel>
get() {
return _pageMap
}
private var pageMap: Map<Long, PageModel> = mapOf()
set(value) {
_pageMap = value
_pages.postValue(pageMap.values.toList())
field = value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

_pages.postValue(field.values.toList())

if (isSearchExpanded.value == true) {
onSearch(lastSearchQuery)
Expand Down Expand Up @@ -137,8 +138,7 @@ class PagesViewModel
actionPerfomer.onCleanup()
}

private fun reloadPagesAsync() = launch(CommonPool) {
pageMap = pageStore.getPagesFromDb(site).associateBy { it.remoteId }.toMutableMap()
private fun reloadPagesAsync() = launch(commonPoolContext) {
refreshPages()

val loadState = if (pageMap.isEmpty()) FETCHING else REFRESHING
Expand All @@ -160,11 +160,11 @@ class PagesViewModel
}

private suspend fun refreshPages() {
pageMap = pageStore.getPagesFromDb(site).associateBy { it.remoteId }.toMutableMap()
pageMap = pageStore.getPagesFromDb(site).associateBy { it.remoteId }
}

fun onPageEditFinished(pageId: Long) {
launch {
launch(uiContext) {
refreshPages() // show local changes immediately
waitForPageUpdate(pageId)
reloadPages()
Expand All @@ -179,7 +179,7 @@ class PagesViewModel
}

fun onPageParentSet(pageId: Long, parentId: Long) {
launch {
launch(uiContext) {
pageMap[pageId]?.let { page ->
setParent(page, parentId)
}
Expand All @@ -198,11 +198,11 @@ class PagesViewModel
_isNewPageButtonVisible.postOnUi(isNotEmpty && hasNoExceptions)
}

fun onSearch(searchQuery: String) {
fun onSearch(searchQuery: String, delay: Int = SEARCH_DELAY) {
searchJob?.cancel()
if (searchQuery.isNotEmpty()) {
searchJob = launch {
delay(200)
searchJob = launch(uiContext) {
delay(delay)
searchJob = null
if (isActive) {
_lastSearchQuery = searchQuery
Expand All @@ -218,7 +218,7 @@ class PagesViewModel
private suspend fun groupedSearch(
site: SiteModel,
searchQuery: String
): SortedMap<PageListType, List<PageModel>> = withContext(CommonPool) {
): SortedMap<PageListType, List<PageModel>> = withContext(commonPoolContext) {
val list = pageStore.search(site, searchQuery).groupBy { PageListType.fromPageStatus(it.status) }
return@withContext list.toSortedMap(
Comparator { previous, next ->
Expand Down Expand Up @@ -250,8 +250,8 @@ class PagesViewModel
_isSearchExpanded.value = false
clearSearch()

launch {
delay(500)
launch(uiContext) {
delay(SEARCH_COLLAPSE_DELAY)
checkIfNewPageButtonShouldBeVisible()
}
return true
Expand All @@ -275,7 +275,7 @@ class PagesViewModel
}

fun onDeleteConfirmed(remoteId: Long) {
launch {
launch(commonPoolContext) {
pageMap[remoteId]?.let { deletePage(it) }
}
}
Expand All @@ -289,7 +289,7 @@ class PagesViewModel
}

fun onPullToRefresh() {
launch {
launch(uiContext) {
reloadPages(FETCHING)
}
}
Expand All @@ -298,70 +298,75 @@ class PagesViewModel
val oldParent = page.parent?.remoteId ?: 0

val action = PageAction(UPLOAD) {
launch(CommonPool) {
launch(commonPoolContext) {
if (page.parent?.remoteId != parentId) {
page.parent = _pageMap[parentId]
pageMap = _pageMap
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By removing this line, the logic in the setter is not called and we lost the instant updates. We could just extract that code and call it from here explicitly.

Copy link
Contributor Author

@planarvoid planarvoid Sep 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right! I missed that. I'll call the setter explicitly 👍
edit: Actually I noticed that the PageModel object is being modified directly and I don't think that's a good practice so I changed it to use the copy editing instead of setters and I'm doing the same for the pageMap

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!

val updatedPage = updateParent(page, parentId)

pageStore.uploadPageToServer(page)
pageStore.uploadPageToServer(updatedPage)
}
}
}
action.undo = {
launch(CommonPool) {
launch(commonPoolContext) {
pageMap[page.remoteId]?.let { changed ->
changed.parent = _pageMap[oldParent]
pageMap = _pageMap
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, lost instant updates.

val updatedPage = updateParent(changed, oldParent)

pageStore.uploadPageToServer(changed)
pageStore.uploadPageToServer(updatedPage)
}
}
}
action.onSuccess = {
launch(CommonPool) {
launch(commonPoolContext) {
reloadPages()

delay(100)
delay(ACTION_DELAY)
_showSnackbarMessage.postValue(
SnackbarMessageHolder(string.page_parent_changed, string.undo, action.undo))
}
}
action.onError = {
launch(CommonPool) {
launch(commonPoolContext) {
refreshPages()

_showSnackbarMessage.postValue(SnackbarMessageHolder(string.page_parent_change_error))
}
}

launch {
launch(uiContext) {
_arePageActionsEnabled = false
actionPerfomer.performAction(action)
_arePageActionsEnabled = true
}
}

private fun updateParent(page: PageModel, parentId: Long): PageModel {
val updatedPage = page.copy(parent = pageMap[parentId])
val updatedMap = pageMap.toMutableMap()
updatedMap[page.remoteId] = updatedPage
pageMap = updatedMap
return updatedPage
}

private fun deletePage(page: PageModel) {
val action = PageAction(REMOVE) {
launch(CommonPool) {
_pageMap.remove(page.remoteId)
pageMap = _pageMap
launch(commonPoolContext) {
pageMap = pageMap.filter { it.key != page.remoteId }

checkIfNewPageButtonShouldBeVisible()

pageStore.deletePageFromServer(page)
}
}
action.onSuccess = {
launch(CommonPool) {
delay(100)
launch(commonPoolContext) {
delay(ACTION_DELAY)
reloadPages()

_showSnackbarMessage.postValue(SnackbarMessageHolder(string.page_permanently_deleted))
}
}
action.onError = {
launch(CommonPool) {
launch(commonPoolContext) {
refreshPages()

_showSnackbarMessage.postValue(SnackbarMessageHolder(string.page_delete_error))
Expand All @@ -377,48 +382,56 @@ class PagesViewModel
pageMap[remoteId]?.let { page ->
val oldStatus = page.status
val action = PageAction(UPLOAD) {
page.status = status
launch(CommonPool) {
pageStore.updatePageInDb(page)
val updatedPage = updatePageStatus(page, status)
launch(commonPoolContext) {
pageStore.updatePageInDb(updatedPage)
refreshPages()

pageStore.uploadPageToServer(page)
pageStore.uploadPageToServer(updatedPage)
}
}
action.undo = {
page.status = oldStatus
launch(CommonPool) {
pageStore.updatePageInDb(page)
val updatedPage = updatePageStatus(page, oldStatus)
launch(commonPoolContext) {
pageStore.updatePageInDb(updatedPage)
refreshPages()

pageStore.uploadPageToServer(page)
pageStore.uploadPageToServer(updatedPage)
}
}
action.onSuccess = {
launch(CommonPool) {
delay(100)
launch(commonPoolContext) {
delay(ACTION_DELAY)
reloadPages()

val message = prepareStatusChangeSnackbar(status, action.undo)
_showSnackbarMessage.postValue(message)
}
}
action.onError = {
launch(CommonPool) {
launch(commonPoolContext) {
action.undo()

_showSnackbarMessage.postValue(SnackbarMessageHolder(string.page_status_change_error))
}
}

launch {
launch(uiContext) {
_arePageActionsEnabled = false
actionPerfomer.performAction(action)
_arePageActionsEnabled = true
}
}
}

private fun updatePageStatus(page: PageModel, oldStatus: PageStatus): PageModel {
val updatedPage = page.copy(status = oldStatus)
val updatedMap = pageMap.toMutableMap()
updatedMap[page.remoteId] = updatedPage
pageMap = updatedMap
return updatedPage
}

private fun prepareStatusChangeSnackbar(newStatus: PageStatus, undo: (() -> Unit)? = null): SnackbarMessageHolder {
val message = when (newStatus) {
PageStatus.DRAFT -> string.page_moved_to_draft
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import android.arch.lifecycle.LiveData
import android.arch.lifecycle.MutableLiveData
import android.arch.lifecycle.Observer
import android.arch.lifecycle.ViewModel
import kotlinx.coroutines.experimental.CoroutineDispatcher
import kotlinx.coroutines.experimental.launch
import org.wordpress.android.R.string
import org.wordpress.android.fluxc.model.page.PageModel
import org.wordpress.android.fluxc.model.page.PageStatus
import org.wordpress.android.modules.UI_CONTEXT
import org.wordpress.android.ui.pages.PageItem
import org.wordpress.android.ui.pages.PageItem.Action
import org.wordpress.android.ui.pages.PageItem.Divider
Expand All @@ -21,9 +23,13 @@ import org.wordpress.android.viewmodel.ResourceProvider
import org.wordpress.android.viewmodel.pages.PageListViewModel.PageListType
import java.util.SortedMap
import javax.inject.Inject
import javax.inject.Named

class SearchListViewModel
@Inject constructor(private val resourceProvider: ResourceProvider) : ViewModel() {
@Inject constructor(
private val resourceProvider: ResourceProvider,
@Named(UI_CONTEXT) private val uiContext: CoroutineDispatcher
) : ViewModel() {
private val _searchResult: MutableLiveData<List<PageItem>> = MutableLiveData()
val searchResult: LiveData<List<PageItem>> = _searchResult

Expand All @@ -50,7 +56,7 @@ class SearchListViewModel

pagesViewModel.checkIfNewPageButtonShouldBeVisible()
} else {
_searchResult.postValue(listOf(Empty(string.pages_search_suggestion, true)))
_searchResult.value = listOf(Empty(string.pages_search_suggestion, true))
}
}

Expand All @@ -62,7 +68,7 @@ class SearchListViewModel
pagesViewModel.onItemTapped(pageItem)
}

private fun loadFoundPages(pages: SortedMap<PageListType, List<PageModel>>) = launch {
private fun loadFoundPages(pages: SortedMap<PageListType, List<PageModel>>) = launch(uiContext) {
if (pages.isNotEmpty()) {
val pageItems = pages
.map { (listType, results) ->
Expand All @@ -73,9 +79,9 @@ class SearchListViewModel
acc.addAll(list)
return@fold acc
}
_searchResult.postValue(pageItems)
_searchResult.value = pageItems
} else {
_searchResult.postValue(listOf(Empty(string.pages_empty_search_result, true)))
_searchResult.value = listOf(Empty(string.pages_empty_search_result, true))
}
}

Expand Down
10 changes: 10 additions & 0 deletions WordPress/src/test/java/org/wordpress/android/CoroutinesUtils.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package org.wordpress.android

import kotlinx.coroutines.experimental.CoroutineScope
import kotlinx.coroutines.experimental.runBlocking
import kotlin.coroutines.experimental.CoroutineContext
import kotlin.coroutines.experimental.EmptyCoroutineContext

fun <T> test(context: CoroutineContext = EmptyCoroutineContext, block: suspend CoroutineScope.() -> T) {
runBlocking(context, block)
}
Loading