Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

For #9599: Ability to edit top site URLs #9600

Merged
merged 3 commits into from
Feb 23, 2021
Merged

For #9599: Ability to edit top site URLs #9600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9aa6504
For #9599: Ability to update top site URLs
pepve Feb 3, 2021
c350bd9
For #9599: Add PinnedSitesStorageTest to increase top-sites test cove…
pepve Feb 10, 2021
f84dc4e
Merge branch 'master' into edit-top-site
mergify[bot] Feb 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions ...ature/top/sites/PinnedSitesStorageTest.kt → ...p/sites/OnDevicePinnedSitesStorageTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import java.util.concurrent.Executors
private const val MIGRATION_TEST_DB = "migration-test"

@Suppress("LargeClass")
class PinnedSitesStorageTest {
class OnDevicePinnedSitesStorageTest {
gabrielluong marked this conversation as resolved.
Show resolved Hide resolved
private lateinit var context: Context
private lateinit var storage: PinnedSiteStorage
private lateinit var executor: ExecutorService
Expand Down Expand Up @@ -153,7 +153,7 @@ class PinnedSitesStorageTest {
}

@Test
fun testRenamingPinnedSites() = runBlocking {
fun testUpdatingPinnedSites() = runBlocking {
storage.addPinnedSite("Mozilla", "https://www.mozilla.org")
var pinnedSites = storage.getPinnedSites()

Expand All @@ -162,20 +162,20 @@ class PinnedSitesStorageTest {
assertEquals("https://www.mozilla.org", pinnedSites[0].url)
assertEquals("Mozilla", pinnedSites[0].title)

storage.renamePinnedSite(pinnedSites[0], "")
storage.updatePinnedSite(pinnedSites[0], "", "")

pinnedSites = storage.getPinnedSites()
assertEquals(1, pinnedSites.size)
assertEquals(1, storage.getPinnedSitesCount())
assertEquals("https://www.mozilla.org", pinnedSites[0].url)
assertEquals("", pinnedSites[0].url)
assertEquals("", pinnedSites[0].title)

storage.renamePinnedSite(pinnedSites[0], "Mozilla Firefox")
storage.updatePinnedSite(pinnedSites[0], "Mozilla Firefox", "https://www.firefox.com")

pinnedSites = storage.getPinnedSites()
assertEquals(1, pinnedSites.size)
assertEquals(1, storage.getPinnedSitesCount())
assertEquals("https://www.mozilla.org", pinnedSites[0].url)
assertEquals("https://www.firefox.com", pinnedSites[0].url)
assertEquals("Mozilla Firefox", pinnedSites[0].title)
}

Expand Down
4 changes: 2 additions & 2 deletions ...re/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,10 @@ class DefaultTopSitesStorage(
}
}

override fun renameTopSite(topSite: TopSite, title: String) {
override fun updateTopSite(topSite: TopSite, title: String, url: String) {
scope.launch {
if (topSite.type != FRECENT) {
pinnedSitesStorage.renamePinnedSite(topSite, title)
pinnedSitesStorage.updatePinnedSite(topSite, title, url)
}

notifyObservers { onStorageUpdated() }
Expand Down
14 changes: 10 additions & 4 deletions ...feature/top-sites/src/main/java/mozilla/components/feature/top/sites/PinnedSiteStorage.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package mozilla.components.feature.top.sites

import android.content.Context
import androidx.annotation.VisibleForTesting
import kotlinx.coroutines.Dispatchers.IO
import kotlinx.coroutines.withContext
import mozilla.components.feature.top.sites.db.PinnedSiteEntity
Expand All @@ -16,6 +17,9 @@ import mozilla.components.feature.top.sites.db.toPinnedSite
*/
class PinnedSiteStorage(context: Context) {

@VisibleForTesting
internal var currentTimeMillis: () -> Long = { System.currentTimeMillis() }
Comment on lines +20 to +21
Copy link
Member

Choose a reason for hiding this comment

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

There's nothing wrong in particular about this, but from looking around our codebase, I would be more comfortable being consistent and calling System.currentTimeMillis() where it is.

Copy link
Contributor Author

@pepve pepve Feb 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I use it to inject a constant time from the tests (in addAllDefaultSites and addPinnedSite) so that I can use verify without much effort. What would be a good alternative?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can just check for anyLong() as an alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that but I think it doesn't work because the long is wrapped in an object. I think this works:
verify(obj).method(eq("foo"), anyLong())
But this doesn't:
verify(obj).method(SomeClass(eq("foo"), anyLong()))
And our case matches the latter. But I don't really have a lot of experience in this area so please correct me if I'm missing something. The real problem I think we're having is that there is no consistent way to mock time used throughout the project. Code that does straight calls to System.currentTimeMillis() is just not very unit-testable. Perhaps we should make an issue about this?

@VisibleForTesting
internal var database: Lazy<TopSiteDatabase> = lazy { TopSiteDatabase.get(context) }
private val pinnedSiteDao by lazy { database.value.pinnedSiteDao() }

Expand All @@ -35,7 +39,7 @@ class PinnedSiteStorage(context: Context) {
title = title,
url = url,
isDefault = isDefault,
createdAt = System.currentTimeMillis()
createdAt = currentTimeMillis()
)
}
pinnedSiteDao.insertAllPinnedSites(siteEntities)
Expand All @@ -55,7 +59,7 @@ class PinnedSiteStorage(context: Context) {
title = title,
url = url,
isDefault = isDefault,
createdAt = System.currentTimeMillis()
createdAt = currentTimeMillis()
)
entity.id = pinnedSiteDao.insertPinnedSite(entity)
}
Expand All @@ -77,14 +81,16 @@ class PinnedSiteStorage(context: Context) {
}

/**
* Renames the given pinned site.
* Updates the given pinned site.
*
* @param site The pinned site.
* @param title The new title for the top site.
* @param url The new url for the top site.
*/
suspend fun renamePinnedSite(site: TopSite, title: String) = withContext(IO) {
suspend fun updatePinnedSite(site: TopSite, title: String, url: String) = withContext(IO) {
val pinnedSite = site.toPinnedSite()
pinnedSite.title = title
pinnedSite.url = url
pinnedSiteDao.updatePinnedSite(pinnedSite)
}

Expand Down
5 changes: 3 additions & 2 deletions ...s/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,13 @@ interface TopSitesStorage : Observable<TopSitesStorage.Observer> {
fun removeTopSite(topSite: TopSite)

/**
* Renames the given [TopSite].
* Updates the given [TopSite].
*
* @param topSite The top site.
* @param title The new title for the top site.
* @param url The new url for the top site.
*/
fun renameTopSite(topSite: TopSite, title: String)
fun updateTopSite(topSite: TopSite, title: String, url: String)

/**
* Return a unified list of top sites based on the given number of sites desired.
Expand Down
15 changes: 8 additions & 7 deletions .../feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesUseCases.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,18 @@ class TopSitesUseCases(topSitesStorage: TopSitesStorage) {
}

/**
* Rename a top site use case.
* Update a top site use case.
*/
class RenameTopSiteUseCase internal constructor(private val storage: TopSitesStorage) {
class UpdateTopSiteUseCase internal constructor(private val storage: TopSitesStorage) {
/**
* Renames the given [TopSite].
* Updates the given [TopSite].
*
* @param topSite The top site.
* @param title The new title for the top site.
* @param url The new url for the top site.
*/
operator fun invoke(topSite: TopSite, title: String) {
storage.renameTopSite(topSite, title)
operator fun invoke(topSite: TopSite, title: String, url: String) {
storage.updateTopSite(topSite, title, url)
}
}

Expand All @@ -73,7 +74,7 @@ class TopSitesUseCases(topSitesStorage: TopSitesStorage) {
RemoveTopSiteUseCase(topSitesStorage)
}

val renameTopSites: RenameTopSiteUseCase by lazy {
RenameTopSiteUseCase(topSitesStorage)
val updateTopSites: UpdateTopSiteUseCase by lazy {
UpdateTopSiteUseCase(topSitesStorage)
}
}
14 changes: 7 additions & 7 deletions ...op-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ class DefaultTopSitesStorageTest {
}

@Test
fun `renameTopSite`() = runBlockingTest {
fun `updateTopSite`() = runBlockingTest {
val defaultTopSitesStorage = DefaultTopSitesStorage(
pinnedSitesStorage,
historyStorage,
Expand All @@ -124,9 +124,9 @@ class DefaultTopSitesStorageTest {
createdAt = 1,
type = TopSite.Type.DEFAULT
)
defaultTopSitesStorage.renameTopSite(defaultSite, "Mozilla Firefox")
defaultTopSitesStorage.updateTopSite(defaultSite, "Mozilla Firefox", "https://mozilla.com")

verify(pinnedSitesStorage).renamePinnedSite(defaultSite, "Mozilla Firefox")
verify(pinnedSitesStorage).updatePinnedSite(defaultSite, "Mozilla Firefox", "https://mozilla.com")

val pinnedSite = TopSite(
id = 2,
Expand All @@ -135,9 +135,9 @@ class DefaultTopSitesStorageTest {
createdAt = 2,
type = TopSite.Type.PINNED
)
defaultTopSitesStorage.renameTopSite(pinnedSite, "Wiki")
defaultTopSitesStorage.updateTopSite(pinnedSite, "Wiki", "https://en.wikipedia.org/wiki/Wiki")

verify(pinnedSitesStorage).renamePinnedSite(pinnedSite, "Wiki")
verify(pinnedSitesStorage).updatePinnedSite(pinnedSite, "Wiki", "https://en.wikipedia.org/wiki/Wiki")

val frecentSite = TopSite(
id = 1,
Expand All @@ -146,9 +146,9 @@ class DefaultTopSitesStorageTest {
createdAt = 1,
type = TopSite.Type.FRECENT
)
defaultTopSitesStorage.renameTopSite(frecentSite, "Moz")
defaultTopSitesStorage.updateTopSite(frecentSite, "Moz", "")

verify(pinnedSitesStorage, never()).renamePinnedSite(frecentSite, "Moz")
verify(pinnedSitesStorage, never()).updatePinnedSite(frecentSite, "Moz", "")
}

@Test
Expand Down
130 changes: 130 additions & 0 deletions ...re/top-sites/src/test/java/mozilla/components/feature/top/sites/PinnedSitesStorageTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
Copy link
Member

Choose a reason for hiding this comment

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

This is nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

package mozilla.components.feature.top.sites

import kotlinx.coroutines.runBlocking
import mozilla.components.feature.top.sites.db.PinnedSiteDao
import mozilla.components.feature.top.sites.db.PinnedSiteEntity
import mozilla.components.feature.top.sites.db.TopSiteDatabase
import mozilla.components.feature.top.sites.TopSite.Type.DEFAULT
import mozilla.components.feature.top.sites.TopSite.Type.PINNED
import mozilla.components.support.test.mock
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotNull
import org.junit.Test
import org.mockito.Mockito.verify
import org.mockito.Mockito.`when`

class PinnedSitesStorageTest {

@Test
fun addAllDefaultSites() = runBlocking {
val storage = PinnedSiteStorage(mock())
val dao = mockDao(storage)

storage.currentTimeMillis = { 42 }

storage.addAllPinnedSites(listOf(
Pair("Mozilla", "https://www.mozilla.org"),
Pair("Firefox", "https://www.firefox.com"),
Pair("Wikipedia", "https://www.wikipedia.com"),
Pair("Pocket", "https://www.getpocket.com")
), isDefault = true)

verify(dao).insertAllPinnedSites(listOf(
PinnedSiteEntity(title = "Mozilla", url = "https://www.mozilla.org", isDefault = true, createdAt = 42),
PinnedSiteEntity(title = "Firefox", url = "https://www.firefox.com", isDefault = true, createdAt = 42),
PinnedSiteEntity(title = "Wikipedia", url = "https://www.wikipedia.com", isDefault = true, createdAt = 42),
PinnedSiteEntity(title = "Pocket", url = "https://www.getpocket.com", isDefault = true, createdAt = 42)
))

Unit
}

@Test
fun addPinnedSite() = runBlocking {
val storage = PinnedSiteStorage(mock())
val dao = mockDao(storage)

storage.currentTimeMillis = { 3 }

storage.addPinnedSite("Mozilla", "https://www.mozilla.org")
storage.addPinnedSite("Firefox", "https://www.firefox.com", isDefault = true)

// PinnedSiteDao.insertPinnedSite is actually called with "id = null", but due to an
// extraneous assignment ("entity.id = ") in PinnedSiteStorage.addPinnedSite we can for now
// only verify the call with "id = 0". See issue #9708.
verify(dao).insertPinnedSite(PinnedSiteEntity(id = 0, title = "Mozilla", url = "https://www.mozilla.org", isDefault = false, createdAt = 3))
verify(dao).insertPinnedSite(PinnedSiteEntity(id = 0, title = "Firefox", url = "https://www.firefox.com", isDefault = true, createdAt = 3))

Unit
}

@Test
fun removePinnedSite() = runBlocking {
val storage = PinnedSiteStorage(mock())
val dao = mockDao(storage)

storage.removePinnedSite(TopSite(1, "Mozilla", "https://www.mozilla.org", 1, PINNED))
storage.removePinnedSite(TopSite(2, "Firefox", "https://www.firefox.com", 1, DEFAULT))

verify(dao).deletePinnedSite(PinnedSiteEntity(1, "Mozilla", "https://www.mozilla.org", false, 1))
verify(dao).deletePinnedSite(PinnedSiteEntity(2, "Firefox", "https://www.firefox.com", true, 1))
}

@Test
fun getPinnedSites() = runBlocking {
val storage = PinnedSiteStorage(mock())
val dao = mockDao(storage)

`when`(dao.getPinnedSites()).thenReturn(listOf(
PinnedSiteEntity(1, "Mozilla", "https://www.mozilla.org", false, 10),
PinnedSiteEntity(2, "Firefox", "https://www.firefox.com", true, 10)
))
`when`(dao.getPinnedSitesCount()).thenReturn(2)

val topSites = storage.getPinnedSites()
val topSitesCount = storage.getPinnedSitesCount()

assertNotNull(topSites)
assertEquals(2, topSites.size)
assertEquals(2, topSitesCount)

with(topSites[0]) {
assertEquals(1L, id)
assertEquals("Mozilla", title)
assertEquals("https://www.mozilla.org", url)
assertEquals(PINNED, type)
assertEquals(10L, createdAt)
}

with(topSites[1]) {
assertEquals(2L, id)
assertEquals("Firefox", title)
assertEquals("https://www.firefox.com", url)
assertEquals(DEFAULT, type)
assertEquals(10L, createdAt)
}
}

@Test
fun updatePinnedSite() = runBlocking {
val storage = PinnedSiteStorage(mock())
val dao = mockDao(storage)

val site = TopSite(1, "Mozilla", "https://www.mozilla.org", 1, PINNED)
storage.updatePinnedSite(site, "Mozilla (IT)", "https://www.mozilla.org/it")

verify(dao).updatePinnedSite(PinnedSiteEntity(1, "Mozilla (IT)", "https://www.mozilla.org/it", false, 1))
}

private fun mockDao(storage: PinnedSiteStorage): PinnedSiteDao {
val db = mock<TopSiteDatabase>()
storage.database = lazy { db }
val dao = mock<PinnedSiteDao>()
`when`(db.pinnedSiteDao()).thenReturn(dao)
return dao
}
}
7 changes: 4 additions & 3 deletions ...ture/top-sites/src/test/java/mozilla/components/feature/top/sites/TopSitesUseCasesTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,15 @@ class TopSitesUseCasesTest {
}

@Test
fun `RenameTopSiteUseCase`() = runBlocking {
fun `UpdateTopSiteUseCase`() = runBlocking {
val topSitesStorage: TopSitesStorage = mock()
val topSite: TopSite = mock()
val useCases = TopSitesUseCases(topSitesStorage)

val title = "New title"
useCases.renameTopSites(topSite, title)
val url = "https://www.example.com/new-url"
useCases.updateTopSites(topSite, title, url)

verify(topSitesStorage).renameTopSite(topSite, title)
verify(topSitesStorage).updateTopSite(topSite, title, url)
}
}
3 changes: 3 additions & 0 deletions docs/changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ permalink: /changelog/
* **service-glean**
* 🆙 Updated Glean to version 34.1.0 ([changelog](https://github.com/mozilla/glean/releases/tag/v34.1.0))

* **feature-top-sites**
* ⚠️ **This is a breaking change**: Replace `TopSitesUseCases.renameTopSites` with `TopSitesUseCases.updateTopSites` which allows for updating the title and the url of a top site. [#9599](https://github.com/mozilla-mobile/android-components/issues/9599)

# 72.0.0

* [Commits](https://github.com/mozilla-mobile/android-components/compare/v71.0.0...v72.0.0)
Expand Down