Commit b6245d84 authored by MozLando's avatar MozLando
Browse files

Merge #6715



6715: Issue #6331: Add isDefault bool to TopSiteEntity and APIs for flagging default top sites r=ekager,Amejia481 a=gabrielluong

Fixes #6331. 

@Amejia481 Just to give you some context about the changes in this PR:

In Fenix, we have added 3 default top sites (Pocket, Wikipedia, YouTube) along with telemetry. 
It is not enough to just have a hardcoded list of these 3 default top sites since we can't differentiate from the user added top sites and top sites migrated from fennec.

We want to add this isDefault boolean to each TopSiteEntity so that in telemetry we can easily just query if the top site is a default top site added by the application. On the front end, we can also sort all the isDefault top sites to the beginning when we are showing the list of top sites.

In this PR, we did the following:

- Add isDefault bool to TopSiteEntity
- Migrate the DB to account for this new property
- Migrate the 3 default top sites to have its isDefault bool set to true
Co-authored-by: default avatarGabriel Luong <gabriel.luong@gmail.com>
Co-authored-by: default avatarArturo Mejia <arturomejiamarmol@gmail.com>
parents 45dba0c3 6b483d8b
{
"formatVersion": 1,
"database": {
"version": 2,
"identityHash": "a57788de8a7351e0bdaf5a2044489dcf",
"entities": [
{
"tableName": "top_sites",
"createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` INTEGER PRIMARY KEY AUTOINCREMENT, `title` TEXT NOT NULL, `url` TEXT NOT NULL, `isDefault` INTEGER NOT NULL, `created_at` INTEGER NOT NULL)",
"fields": [
{
"fieldPath": "id",
"columnName": "id",
"affinity": "INTEGER",
"notNull": false
},
{
"fieldPath": "title",
"columnName": "title",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "url",
"columnName": "url",
"affinity": "TEXT",
"notNull": true
},
{
"fieldPath": "isDefault",
"columnName": "isDefault",
"affinity": "INTEGER",
"notNull": true
},
{
"fieldPath": "createdAt",
"columnName": "created_at",
"affinity": "INTEGER",
"notNull": true
}
],
"primaryKey": {
"columnNames": [
"id"
],
"autoGenerate": true
},
"indices": [],
"foreignKeys": []
}
],
"views": [],
"setupQueries": [
"CREATE TABLE IF NOT EXISTS room_master_table (id INTEGER PRIMARY KEY,identity_hash TEXT)",
"INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, 'a57788de8a7351e0bdaf5a2044489dcf')"
]
}
}
\ No newline at end of file
......@@ -8,18 +8,26 @@ import android.content.Context
import androidx.arch.core.executor.testing.InstantTaskExecutorRule
import androidx.paging.PagedList
import androidx.room.Room
import androidx.room.testing.MigrationTestHelper
import androidx.sqlite.db.framework.FrameworkSQLiteOpenHelperFactory
import androidx.test.core.app.ApplicationProvider
import androidx.test.platform.app.InstrumentationRegistry
import mozilla.components.feature.top.sites.db.Migrations
import mozilla.components.feature.top.sites.db.TopSiteDatabase
import mozilla.components.support.android.test.awaitValue
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import java.util.concurrent.ExecutorService
import java.util.concurrent.Executors
private const val MIGRATION_TEST_DB = "migration-test"
@Suppress("LargeClass")
class TopSiteStorageTest {
private lateinit var context: Context
......@@ -29,6 +37,13 @@ class TopSiteStorageTest {
@get:Rule
var instantTaskExecutorRule = InstantTaskExecutorRule()
@get:Rule
val helper: MigrationTestHelper = MigrationTestHelper(
InstrumentationRegistry.getInstrumentation(),
TopSiteDatabase::class.java.canonicalName,
FrameworkSQLiteOpenHelperFactory()
)
@Before
fun setUp() {
executor = Executors.newSingleThreadExecutor()
......@@ -48,7 +63,7 @@ class TopSiteStorageTest {
@Test
fun testAddingTopSite() {
storage.addTopSite("Mozilla", "https://www.mozilla.org")
storage.addTopSite("Firefox", "https://www.firefox.com")
storage.addTopSite("Firefox", "https://www.firefox.com", isDefault = true)
val topSites = getAllTopSites()
......@@ -56,8 +71,10 @@ class TopSiteStorageTest {
assertEquals("Mozilla", topSites[0].title)
assertEquals("https://www.mozilla.org", topSites[0].url)
assertFalse(topSites[0].isDefault)
assertEquals("Firefox", topSites[1].title)
assertEquals("https://www.firefox.com", topSites[1].url)
assertTrue(topSites[1].isDefault)
}
@Test
......@@ -82,7 +99,7 @@ class TopSiteStorageTest {
@Test
fun testGettingTopSites() {
storage.addTopSite("Mozilla", "https://www.mozilla.org")
storage.addTopSite("Firefox", "https://www.firefox.com")
storage.addTopSite("Firefox", "https://www.firefox.com", isDefault = true)
val topSites = storage.getTopSites().awaitValue()
......@@ -92,11 +109,74 @@ class TopSiteStorageTest {
with(topSites[0]) {
assertEquals("Mozilla", title)
assertEquals("https://www.mozilla.org", url)
assertFalse(isDefault)
}
with(topSites[1]) {
assertEquals("Firefox", title)
assertEquals("https://www.firefox.com", url)
assertTrue(isDefault)
}
}
@Test
fun migrate1to2() {
val dbVersion1 = helper.createDatabase(MIGRATION_TEST_DB, 1).apply {
execSQL(
"INSERT INTO " +
"top_sites " +
"(title, url, created_at) " +
"VALUES " +
"('Mozilla','mozilla.org',1)," +
"('Top Articles','https://getpocket.com/fenix-top-articles',2)," +
"('Wikipedia','https://www.wikipedia.org/',3)," +
"('YouTube','https://www.youtube.com/',4)"
)
}
dbVersion1.query("SELECT * FROM top_sites").use { cursor ->
assertEquals(4, cursor.columnCount)
}
val dbVersion2 = helper.runMigrationsAndValidate(
MIGRATION_TEST_DB, 2, true, Migrations.migration_1_2
).apply {
execSQL(
"INSERT INTO " +
"top_sites " +
"(title, url, isDefault, created_at) " +
"VALUES " +
"('Firefox','firefox.com',1,5)," +
"('Monitor','https://monitor.firefox.com/',0,5)"
)
}
dbVersion2.query("SELECT * FROM top_sites").use { cursor ->
assertEquals(5, cursor.columnCount)
// Check isDefault for Mozilla
cursor.moveToFirst()
assertEquals(0, cursor.getInt(cursor.getColumnIndexOrThrow("isDefault")))
// Check isDefault for Top Articles
cursor.moveToNext()
assertEquals(1, cursor.getInt(cursor.getColumnIndexOrThrow("isDefault")))
// Check isDefault for Wikipedia
cursor.moveToNext()
assertEquals(1, cursor.getInt(cursor.getColumnIndexOrThrow("isDefault")))
// Check isDefault for YouTube
cursor.moveToNext()
assertEquals(1, cursor.getInt(cursor.getColumnIndexOrThrow("isDefault")))
// Check isDefault for Firefox
cursor.moveToNext()
assertEquals(1, cursor.getInt(cursor.getColumnIndexOrThrow("isDefault")))
// Check isDefault for Monitor
cursor.moveToNext()
assertEquals(0, cursor.getInt(cursor.getColumnIndexOrThrow("isDefault")))
}
}
......
......@@ -46,6 +46,7 @@ class TopSiteDaoTest {
val topSite = TopSiteEntity(
title = "Mozilla",
url = "https://www.mozilla.org",
isDefault = false,
createdAt = 200
).also {
it.id = topSiteDao.insertTopSite(it)
......@@ -67,6 +68,7 @@ class TopSiteDaoTest {
val topSite1 = TopSiteEntity(
title = "Mozilla",
url = "https://www.mozilla.org",
isDefault = false,
createdAt = 200
).also {
it.id = topSiteDao.insertTopSite(it)
......@@ -75,6 +77,7 @@ class TopSiteDaoTest {
val topSite2 = TopSiteEntity(
title = "Firefox",
url = "https://www.firefox.com",
isDefault = false,
createdAt = 100
).also {
it.id = topSiteDao.insertTopSite(it)
......
......@@ -22,4 +22,9 @@ interface TopSite {
* The URL of the top site.
*/
val url: String
/**
* Whether or not the top site is a default top site (added as a default by the application).
*/
val isDefault: Boolean
}
......@@ -22,11 +22,17 @@ class TopSiteStorage(
/**
* Adds a new [TopSite].
*
* @param title The title string.
* @param url The URL string.
* @param isDefault Whether or not the top site added should be a default top site. This is
* used to identify top sites that are added by the application.
*/
fun addTopSite(title: String, url: String) {
fun addTopSite(title: String, url: String, isDefault: Boolean = false) {
TopSiteEntity(
title = title,
url = url,
isDefault = isDefault,
createdAt = System.currentTimeMillis()
).also { entity ->
entity.id = database.value.topSiteDao().insertTopSite(entity)
......
......@@ -19,6 +19,9 @@ internal class TopSiteAdapter(
override val url: String
get() = entity.url
override val isDefault: Boolean
get() = entity.isDefault
override fun equals(other: Any?): Boolean {
if (other !is TopSiteAdapter) {
return false
......
......@@ -8,16 +8,19 @@ import android.content.Context
import androidx.room.Database
import androidx.room.Room
import androidx.room.RoomDatabase
import androidx.room.migration.Migration
import androidx.sqlite.db.SupportSQLiteDatabase
/**
* Internal database for storing top sites.
*/
@Database(entities = [TopSiteEntity::class], version = 1)
@Database(entities = [TopSiteEntity::class], version = 2)
internal abstract class TopSiteDatabase : RoomDatabase() {
abstract fun topSiteDao(): TopSiteDao
companion object {
@Volatile private var instance: TopSiteDatabase? = null
@Volatile
private var instance: TopSiteDatabase? = null
@Synchronized
fun get(context: Context): TopSiteDatabase {
......@@ -27,9 +30,33 @@ internal abstract class TopSiteDatabase : RoomDatabase() {
context,
TopSiteDatabase::class.java,
"top_sites"
).addMigrations(
Migrations.migration_1_2
).build().also {
instance = it
}
}
}
}
internal object Migrations {
val migration_1_2 = object : Migration(1, 2) {
override fun migrate(database: SupportSQLiteDatabase) {
// Add the new isDefault column and set isDefault to 0 (false) for every entry.
database.execSQL(
"ALTER TABLE top_sites ADD COLUMN isDefault INTEGER NOT NULL DEFAULT 0"
)
// Prior to version 2, pocket top sites, wikipedia and youtube were added as default
// sites in Fenix. Look for these entries and set isDefault to 1 (true).
database.execSQL(
"UPDATE top_sites " +
"SET isDefault = 1 " +
"WHERE url IN " +
"('https://getpocket.com/fenix-top-articles', " +
"'https://www.wikipedia.org/', " +
"'https://www.youtube.com/')"
)
}
}
}
......@@ -23,6 +23,9 @@ internal data class TopSiteEntity(
@ColumnInfo(name = "url")
var url: String,
@ColumnInfo(name = "is_default")
var isDefault: Boolean = false,
@ColumnInfo(name = "created_at")
var createdAt: Long = System.currentTimeMillis()
)
......@@ -271,7 +271,7 @@ class FennecMigratorTest {
assertTrue(historyStore.getVisited().isEmpty())
assertTrue(bookmarksStore.searchBookmarks("mozilla").isEmpty())
verify(topSiteStorage, never()).addTopSite(any(), any())
verify(topSiteStorage, never()).addTopSite(any(), any(), anyBoolean())
// Can run once.
with(migrator.migrateAsync(mock()).await()) {
......@@ -299,7 +299,7 @@ class FennecMigratorTest {
assertEquals(5, historyStore.getVisited().size)
assertEquals(2, bookmarksStore.searchBookmarks("mozilla").size)
verify(topSiteStorage, times(2)).addTopSite(any(), any())
verify(topSiteStorage, times(2)).addTopSite(any(), any(), anyBoolean())
verify(topSiteStorage).addTopSite(
"Featured extensions for Android – Add-ons for Firefox Android (en-US)",
"https://addons.mozilla.org/en-US/android/collections/4757633/mob/?page=1&collection_sort=-popularity"
......
......@@ -12,6 +12,8 @@ permalink: /changelog/
* [Gecko](https://github.com/mozilla-mobile/android-components/blob/master/buildSrc/src/main/java/Gecko.kt)
* [Configuration](https://github.com/mozilla-mobile/android-components/blob/master/buildSrc/src/main/java/Config.kt)
* **feature-top-sites**
* Added `isDefault` to the top site entity, which allows application to specify a default top site that is added by the application. This is called through `TopSiteStorage.addTopSite`.
# 39.0.0
......@@ -55,6 +57,12 @@ permalink: /changelog/
* **feature-media**
* Muted media will not start the media service anymore, causing no media notification to be shown and no audio focus getting requested.
* **feature-fullscreen**
* ⚠️ **This is a breaking change**: Added `viewportFitChanged` to support Android display cutouts.
* **feature-qr**
* Moved `AutoFitTextureView` from `support-base` to `feature-qr`.
* **feature-session**
* ⚠️ **This is a breaking change**: Added `viewportFitChanged` to `FullScreenFeature` for supporting Android display cutouts.
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment