Merge lp:~stolowski/unity-scope-click/fix-1352451 into lp:unity-scope-click/devel

[Paweł Stołowski]

Remove depts_v. Store department mappings for top-level departments in depts table. Bump schema to version 2 and provide schema_update.sh script executed with the upstart job. Update default departments db.

The problem this MR solves has a few causes - in the order of importance:
* the root "All" department (with empty id) didn't really exist in the database (in particular in 'depts' table, which keeps the hierarchy of departments). Instead, we had a depts_v view which returned virtual root for all top level departments. However, the SQL that populated that view was problematic, as it had to do a 'join' against deptnames *without* a locale restriction, and therefore it was "picking" entries for the department names of the fallback empty locale ("").
* the department names for a "fallback" empty locale would never get updated, since the shell always gives us correct locale name, such as en_US, and we update db records for those. The entries for "" locale contained departments such as '3d'.

What this MR does:
* it bumps db schema to version 2
* it provides the new default departments db.
* it removes the problematic depts_v. Instead, the depts table now contains real mappings also for top-level departments (e.g. "accessories" is a child of ""). This eliminates the problem of joining with deptnames: the depts table contains complete hierarchy.
* it provides a script for updating schema. This script gets rid of the department names for "" locale, removes depts_v and inserts missing mappings for root departments into detps.
* the update_schema.sh script doesn't remove obsolete departments, such as 3d, but it fixes the root cause that kept that department around even though it was gone in the store. Visiting Ubuntu store scope will re-sync departments db and remove any obsolete departments.

Note: make sure you reboot after updating click scope from this branch. The update-schema.sh script is required to be executed which normally happens during start of unity8 session.

Testing:
Ideally, this branch should be tested by updating an existing image with new click scope (and rebooting), and by testing a "first-boot" experience (use --wipe, or remove /home/phablet/.cache/click-departments.db while click scope is not running + reboot).

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:392
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/247/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/222/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/221/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/221/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-team-unity-scope-click-devel-ci/247/rebuild

review: Needs Fixing (continuous-integration)

[lp:~stolowski/unity-scope-click/fix-1352451]

393. By Paweł Stołowski on 2014-08-06

Merged devel.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:393
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/251/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/226
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/225
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/225/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/225

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-team-unity-scope-click-devel-ci/251/rebuild

review: Approve (continuous-integration)

[lp:~stolowski/unity-scope-click/fix-1352451]

394. By Paweł Stołowski on 2014-08-06

Don't fail if name for a single subdepartment is missing, just ignore it.

395. By Paweł Stołowski on 2014-08-06

Insert names for top-level departments in the update schema script.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:395
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/253/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/228
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/227
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/227/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/227

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-team-unity-scope-click-devel-ci/253/rebuild

review: Approve (continuous-integration)

[Alejandro J. Cura (alecu)]

Code looks good. Will test on the device shortly.

[Alejandro J. Cura (alecu)]

Seems to be working fine on the device.

review: Approve

=== modified file 'data/CMakeLists.txt'
--- data/CMakeLists.txt	2014-07-14 16:53:54 +0000
+++ data/CMakeLists.txt	2014-08-06 17:40:43 +0000
@@ -45,6 +45,11 @@
 )
 
 install(
+  PROGRAMS update_schema.sh
+  DESTINATION "${APPS_DATA_DIR}"
+)
+
+install(
   FILES "${CMAKE_CURRENT_BINARY_DIR}/${STORE_INI_TARGET}"
   DESTINATION "${STORE_LIB_DIR}"
 )

=== modified file 'data/click-scope-departments-db.conf.in'
--- data/click-scope-departments-db.conf.in	2014-07-14 16:21:26 +0000
+++ data/click-scope-departments-db.conf.in	2014-08-06 17:40:43 +0000
@@ -5,5 +5,10 @@
 
 script
     DB_FILE=@APPS_DATA_DIR@/departments.db
-    [ -f $HOME/.cache/click-departments.db ] || cp $DB_FILE $HOME/.cache/click-departments.db
+    if [ -f $HOME/.cache/click-departments.db ]
+    then
+        @APPS_DATA_DIR@/update_schema.sh $HOME/.cache/click-departments.db
+    else
+        cp $DB_FILE $HOME/.cache/click-departments.db
+    fi
 end script

=== modified file 'data/departments.db'
Binary files data/departments.db	2014-07-15 14:15:37 +0000 and data/departments.db	2014-08-06 17:40:43 +0000 differ
=== added file 'data/update_schema.sh'
--- data/update_schema.sh	1970-01-01 00:00:00 +0000
+++ data/update_schema.sh	2014-08-06 17:40:43 +0000
@@ -0,0 +1,55 @@
+#!/bin/sh
+DBFILE=$1
+
+if [ -z "$DBFILE" ]
+then
+    echo "Usage: $0 <departments db file>"
+    exit 1
+fi
+
+if [ ! -w "$DBFILE" ]
+then
+    echo "Cannot open $DBFILE for writing"
+    exit 2
+fi
+
+SCHEMA_VERSION=$(sqlite3 "$DBFILE" "SELECT value FROM meta WHERE name='version'")
+
+##############################################
+# Update departments db from version 1 to 2
+##############################################
+if [ "x$SCHEMA_VERSION" = "x1" ]
+then
+    # updating deptnames may fail if en_US is already there, in such case just delete entries for empty locale
+    sqlite3 "$DBFILE" "UPDATE deptnames SET locale='en_US' WHERE locale=''" || sqlite3 "$DBFILE" "DELETE FROM deptnames WHERE locale=''"
+    sqlite3 "$DBFILE" << _UPDATE_TO_VER2
+
+    INSERT INTO deptnames (deptid,locale,name) VALUES ('universal-access','en_US','Universal Access');
+    INSERT INTO deptnames (deptid,locale,name) VALUES ('office','en_US','Office');
+    INSERT INTO deptnames (deptid,locale,name) VALUES ('graphics','en_US','Graphics');
+    INSERT INTO deptnames (deptid,locale,name) VALUES ('science-engineering','en_US','Science & Engineering');
+    INSERT INTO deptnames (deptid,locale,name) VALUES ('internet','en_US','Internet');
+    INSERT INTO deptnames (deptid,locale,name) VALUES ('accessories','en_US','Accessories');
+    INSERT INTO deptnames (deptid,locale,name) VALUES ('developer-tools','en_US','Developer Tools');
+    INSERT INTO deptnames (deptid,locale,name) VALUES ('games','en_US','Games');
+    INSERT INTO deptnames (deptid,locale,name) VALUES ('books-magazines','en_US','Books & Magazines');
+    INSERT INTO deptnames (deptid,locale,name) VALUES ('education','en_US','Education');
+    INSERT INTO deptnames (deptid,locale,name) VALUES ('sound-video','en_US','Sound & Video');
+
+    BEGIN TRANSACTION;
+    DROP VIEW depts_v;
+    INSERT INTO depts (deptid,parentid) VALUES ('universal-access','');
+    INSERT INTO depts (deptid,parentid) VALUES ('office','');
+    INSERT INTO depts (deptid,parentid) VALUES ('graphics','');
+    INSERT INTO depts (deptid,parentid) VALUES ('science-engineering','');
+    INSERT INTO depts (deptid,parentid) VALUES ('internet','');
+    INSERT INTO depts (deptid,parentid) VALUES ('accessories','');
+    INSERT INTO depts (deptid,parentid) VALUES ('developer-tools','');
+    INSERT INTO depts (deptid,parentid) VALUES ('games','');
+    INSERT INTO depts (deptid,parentid) VALUES ('books-magazines','');
+    INSERT INTO depts (deptid,parentid) VALUES ('education','');
+    INSERT INTO depts (deptid,parentid) VALUES ('sound-video','');
+    UPDATE meta SET value='2' WHERE name='version';
+    END TRANSACTION;
+_UPDATE_TO_VER2
+fi

=== modified file 'debian/unity-scope-click-departmentsdb.install'
--- debian/unity-scope-click-departmentsdb.install	2014-07-14 16:53:54 +0000
+++ debian/unity-scope-click-departmentsdb.install	2014-08-06 17:40:43 +0000
@@ -1,2 +1,3 @@
 usr/share/unity/scopes/clickapps/departments.db
+usr/share/unity/scopes/clickapps/update_schema.sh
 usr/share/upstart/sessions/*.conf

=== modified file 'libclickscope/click/departments-db.cpp'
--- libclickscope/click/departments-db.cpp	2014-07-22 20:07:28 +0000
+++ libclickscope/click/departments-db.cpp	2014-08-06 17:40:43 +0000
@@ -94,10 +94,10 @@
     insert_dept_id_query_->prepare("INSERT OR REPLACE INTO depts (deptid, parentid) VALUES (:deptid, :parentid)");
     insert_dept_name_query_->prepare("INSERT OR REPLACE INTO deptnames (deptid, locale, name) VALUES (:deptid, :locale, :name)");
     select_pkgs_by_dept_->prepare("SELECT pkgid FROM pkgmap WHERE deptid=:deptid");
-    select_pkgs_by_dept_recursive_->prepare("WITH RECURSIVE recdepts(deptid) AS (SELECT deptid FROM depts_v WHERE deptid=:deptid UNION SELECT depts_v.deptid FROM recdepts,depts_v WHERE recdepts.deptid=depts_v.parentid) SELECT pkgid FROM pkgmap NATURAL JOIN recdepts");
+    select_pkgs_by_dept_recursive_->prepare("WITH RECURSIVE recdepts(deptid) AS (SELECT deptid FROM depts WHERE deptid=:deptid UNION SELECT depts.deptid FROM recdepts,depts WHERE recdepts.deptid=depts.parentid) SELECT pkgid FROM pkgmap NATURAL JOIN recdepts");
     select_pkg_by_pkgid_->prepare("SELECT pkgid FROM pkgmap WHERE pkgid=:pkgid");
-    select_children_depts_->prepare("SELECT deptid,(SELECT COUNT(1) from DEPTS_V AS inner WHERE inner.parentid=outer.deptid) FROM DEPTS_V AS outer WHERE parentid=:parentid");
-    select_parent_dept_->prepare("SELECT parentid FROM depts_v WHERE deptid=:deptid");
+    select_children_depts_->prepare("SELECT deptid,(SELECT COUNT(1) from depts AS inner WHERE inner.parentid=outer.deptid) FROM depts AS outer WHERE parentid=:parentid");
+    select_parent_dept_->prepare("SELECT parentid FROM depts WHERE deptid=:deptid");
     select_dept_name_->prepare("SELECT name FROM deptnames WHERE deptid=:deptid AND locale=:locale");
 }
 
@@ -107,6 +107,11 @@
 
 void DepartmentsDb::init_db()
 {
+    //
+    // CAUTION:
+    // DON'T FORGET TO BUMP SCHEMA VERSION BELOW AND HANDLE SCHEMA UPGRADE IN data/update_schema.sh
+    // WHENEVER YOU CHANGE ANY OF THE TABLES BELOW!
+
     QSqlQuery query;
 
     // FIXME: for some reason enabling foreign keys gives errors about number of arguments of prepared queries when doing query.exec(); do not enable
@@ -141,14 +146,11 @@
     {
         report_db_error(query.lastError(), "Failed to create meta table");
     }
-    query.exec("INSERT INTO meta (name, value) VALUES ('version', 1)");
 
-    // view of the depts table that automatically adds fake "" department for root departments
-    if (!query.exec("CREATE VIEW IF NOT EXISTS depts_v AS SELECT deptid, parentid FROM depts UNION SELECT deptid,'' AS parentid FROM deptnames WHERE NOT EXISTS "
-        "(SELECT * FROM depts WHERE depts.deptid=deptnames.deptid)"))
-    {
-        report_db_error(query.lastError(), "Failed to create depts_v view");
-    }
+    //
+    // note: this will fail due to unique constraint, but that's fine; it's expected to succeed only when new database is created; in other
+    // cases the version needs to be bumped in the update_schema.sh script.
+    query.exec("INSERT INTO meta (name, value) VALUES ('version', 2)");
 
     if (!db_.commit())
     {
@@ -304,11 +306,6 @@
         throw std::logic_error("Invalid empty department id");
     }
 
-    if (parent_department_id.empty())
-    {
-        throw std::logic_error("Invalid empty parent department id");
-    }
-
     insert_dept_id_query_->bindValue(":deptid", QVariant(QString::fromStdString(department_id)));
     insert_dept_id_query_->bindValue(":parentid", QVariant(QString::fromStdString(parent_department_id)));
     if (!insert_dept_id_query_->exec())
@@ -408,6 +405,12 @@
     delete_deptnames_query_->finish();
     delete_depts_query_->finish();
 
+    // store mapping of top level departments to root ""
+    for (auto const& dept: depts)
+    {
+        store_department_mapping(dept->id(), "");
+    }
+
     store_departments_(depts, locale);
 
     if (!db_.commit())

=== modified file 'libclickscope/tests/test_departments-db.cpp'
--- libclickscope/tests/test_departments-db.cpp	2014-07-23 14:53:10 +0000
+++ libclickscope/tests/test_departments-db.cpp	2014-08-06 17:40:43 +0000
@@ -68,6 +68,10 @@
     void SetUp() override
     {
         db.reset(new DepartmentsDbCheck(db_path, true));
+
+        db->store_department_mapping("tools", "");
+        db->store_department_mapping("games", "");
+
         db->store_department_name("tools", "", "Tools");
         db->store_department_name("office", "", "Office");
 
@@ -230,7 +234,6 @@
     EXPECT_THROW(db->store_department_name("", "", "Foo"), std::logic_error);
     EXPECT_THROW(db->store_department_name("foo", "", ""), std::logic_error);
     EXPECT_THROW(db->store_department_mapping("", "foo"), std::logic_error);
-    EXPECT_THROW(db->store_department_mapping("foo", ""), std::logic_error);
     EXPECT_THROW(db->store_package_mapping("", "foo"), std::logic_error);
     EXPECT_THROW(db->store_package_mapping("foo", ""), std::logic_error);
 }

=== modified file 'libclickscope/tests/test_interface.cpp'
--- libclickscope/tests/test_interface.cpp	2014-07-21 12:41:31 +0000
+++ libclickscope/tests/test_interface.cpp	2014-08-06 17:40:43 +0000
@@ -222,6 +222,8 @@
 
     depts_db->store_department_name("utilities", "", "Utilities");
     depts_db->store_department_name("accessories", "", "Accessories");
+    depts_db->store_department_mapping("utilities", "");
+    depts_db->store_department_mapping("accessories", "");
 
     auto results = iface.find_installed_apps("", "utilies", depts_db);
     EXPECT_EQ(0, results.size());

=== modified file 'scope/clickapps/apps-query.cpp'
--- scope/clickapps/apps-query.cpp	2014-08-01 05:12:28 +0000
+++ scope/clickapps/apps-query.cpp	2014-08-06 17:40:43 +0000
@@ -294,7 +294,7 @@
 void click::apps::Query::push_local_departments(scopes::SearchReplyProxy const& replyProxy)
 {
     auto const current_dep_id = query().department_id();
-    const std::list<std::string> locales = { search_metadata().locale(), "en_US", "" };
+    const std::list<std::string> locales = { search_metadata().locale(), "en_US" };
 
     unity::scopes::Department::SPtr root;
 
@@ -309,10 +309,18 @@
         // attach subdepartments to it
         for (auto const& subdep: impl->depts_db->get_children_departments(current_dep_id))
         {
-            name = impl->depts_db->get_department_name(subdep.id, locales);
-            unity::scopes::Department::SPtr dep = unity::scopes::Department::create(subdep.id, query(), name);
-            dep->set_has_subdepartments(subdep.has_children);
-            current->add_subdepartment(dep);
+            // if single supdepartment fails, then ignore it and continue with others
+            try
+            {
+                name = impl->depts_db->get_department_name(subdep.id, locales);
+                unity::scopes::Department::SPtr dep = unity::scopes::Department::create(subdep.id, query(), name);
+                dep->set_has_subdepartments(subdep.has_children);
+                current->add_subdepartment(dep);
+            }
+            catch (const std::exception &e)
+            {
+                qWarning() << "Failed to create subdeparment:" << QString::fromStdString(e.what());
+            }
         }
 
         // if current is not the top, then gets its parent

=== modified file 'scope/tests/test_apps_query.cpp'
--- scope/tests/test_apps_query.cpp	2014-08-01 05:09:06 +0000
+++ scope/tests/test_apps_query.cpp	2014-08-06 17:40:43 +0000
@@ -195,7 +195,7 @@
     const std::vector<click::Application> installed_apps = {{"app1", "App1", 0.0f, "icon", "url", "descr", "scrshot", ""}};
     const scopes::SearchMetadata metadata{"en_EN", "phone"};
     const scopes::CategoryRenderer renderer{"{}"};
-    const std::list<std::string> expected_locales {"en_EN", "en_US", ""};
+    const std::list<std::string> expected_locales {"en_EN", "en_US"};
 
 };
 

=== modified file 'scope/tests/test_query.cpp'
--- scope/tests/test_query.cpp	2014-07-31 19:37:05 +0000
+++ scope/tests/test_query.cpp	2014-08-06 17:40:43 +0000
@@ -301,7 +301,9 @@
     auto depts_db = std::make_shared<MockDepartmentsDb>(":memory:", true);
 
     EXPECT_CALL(*depts_db, store_department_name(_, _, _)).Times(3);
-    EXPECT_CALL(*depts_db, store_department_mapping(_, _)).Times(2);
+    EXPECT_CALL(*depts_db, store_department_mapping("1", ""));
+    EXPECT_CALL(*depts_db, store_department_mapping("1-1", "1"));
+    EXPECT_CALL(*depts_db, store_department_mapping("1-2", "1"));
 
     MockIndex mock_index(click::Packages(), click::DepartmentList(), init_departments);
     scopes::SearchMetadata metadata("en_EN", "phone");