Fix bug with pending patch removal

If the patch moved between Patchwork projects during the time it's being
tested, it gets stuck in the pending patch table. Since patch IDs are
unique per instance (and don't depend on the project), change the
pending patch table to contain Patchwork instance's base URL in addition
to ID of (base URL, patch ID) pair. This solves the problem with patch
removal from pending table, while keeps the original information needed
for proper testing.

Also introduce a way to migrate the database after upgrades since it's
needed for this change.

Fixes #57

Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>

README.md

@@ -350,6 +350,21 @@ tested patch recorded, and further patches could be tested by executing the
 However, it can be used again to push the last tested patch back, and retest
 already-tested patches, or to push it forward to skip testing some patches.
 
+### Database upgrading
+
+In case database schema changes, new migration scripts will be provided in
+`db_migrations` directory. They aren't needed for new checkouts, but are
+required for sktm to work correctly when upgrading. New scripts since the last
+upgrade should be applied in the correct (numerical) order with commands:
+
+    sqlite3 <db_path> < <script_name>
+
+For example, if the database path is `.sktm.db` and migration `01-pending.sql`
+is being applied, the command will be
+
+    sqlite3 ~/.sktm.db < 01-pending.sql
+
+
 License
 -------
 sktm is distributed under GPLv2 license.

db_migrations/01-pending.sql

@@ -0,0 +1,14 @@
+CREATE TABLE pendingpatches_new(id INTEGER, pdate TEXT, patchsource_id INTEGER,
+                                timestamp INTEGER, baseurl TEXT,
+                                PRIMARY KEY (id, baseurl),
+                                FOREIGN KEY(patchsource_id)
+                                        REFERENCES patchsource(id));
+
+INSERT INTO pendingpatches_new(id, pdate, patchsource_id, timestamp, baseurl)
+        SELECT pendingpatches.id, pendingpatches.pdate, patchsource.id,
+               pendingpatches.timestamp, patchsource.baseurl
+        FROM pendingpatches, patchsource
+        WHERE pendingpatches.patchsource_id = patchsource.id;
+
+DROP TABLE pendingpatches;
+ALTER TABLE pendingpatches_new RENAME TO pendingpatches;

sktm/db.py

@@ -36,7 +36,7 @@ def createdb(self, db):
         c = tc.cursor()
 
         # FIXME The "patchsource_id" field should be a part of the primary key
-        #       for "pendingpatches" and "patch" tables.
+        #       for "patch" table.
         c.executescript("""
                 PRAGMA foreign_keys = on;
 
@@ -62,10 +62,12 @@ def createdb(self, db):
                 );
 
                 CREATE TABLE pendingpatches(
-                  id INTEGER PRIMARY KEY,
+                  id INTEGER,
                   pdate TEXT,
                   patchsource_id INTEGER,
                   timestamp INTEGER,
+                  baseurl TEXT,
+                  PRIMARY KEY (id, baseurl),
                   FOREIGN KEY(patchsource_id) REFERENCES patchsource(id)
                 );
 
@@ -408,8 +410,7 @@ def set_patchset_pending(self, baseurl, project_id, patchset):
         Add each specified patch to the list of "pending" patches, with
         specifed patch date, for specified Patchwork base URL and project ID,
         and marked with current timestamp. Replace any previously added
-        patches with the same ID (bug: should be "same ID, project ID and
-        base URL").
+        patches with the same ID, base URL and project ID.
 
         Args:
             baseurl:    Base URL of the Patchwork instance the project ID and
@@ -426,32 +427,30 @@ def set_patchset_pending(self, baseurl, project_id, patchset):
         logging.debug("setting patches as pending: %s", patchset)
         self.cur.executemany('INSERT OR REPLACE INTO '
                              'pendingpatches(id, pdate, patchsource_id, '
-                             'timestamp) '
-                             'VALUES(?, ?, ?, ?)',
-                             [(patch_id, patch_date, patchset_id, tstamp) for
+                             'timestamp, baseurl) '
+                             'VALUES(?, ?, ?, ?, ?)',
+                             [(patch_id, patch_date, patchset_id, tstamp,
+                               baseurl) for
                               (patch_id, patch_date) in patchset])
         self.conn.commit()
 
-    def unset_patchset_pending(self, baseurl, project_id, patchset):
+    def unset_patchset_pending(self, baseurl, patchset):
         """Remove a patch from the list of pending patches.
 
         Remove each specified patch from the list of "pending" patches, for
-        the specified Patchwork base URL and project ID.
+        the specified Patchwork base URL.
 
         Args:
-            baseurl:    Base URL of the Patchwork instance the project ID and
-                        patch IDs belong to.
-            project_id: ID of the Patchwork project the patch IDs belong to.
+            baseurl:    Base URL of the Patchwork instance the patch IDs
+                        belong to.
             patchset:   List of IDs of patches to be removed from the list.
 
         """
-        patchset_id = self.get_sourceid(baseurl, project_id)
-
         logging.debug("removing patches from pending list: %s", patchset)
 
         self.cur.executemany('DELETE FROM pendingpatches WHERE id = ? '
-                             'AND patchsource_id = ?',
-                             [(patch_id, patchset_id) for
+                             'AND baseurl = ?',
+                             [(patch_id, baseurl) for
                               patch_id in patchset])
         self.conn.commit()
 
@@ -536,7 +535,7 @@ def commit_patchtest(self, baserepo, commithash, patches, result,
              patch_date) in patches:
             # TODO: Can accumulate per-project list instead of doing it one by
             # one
-            self.unset_patchset_pending(baseurl, project_id, [patch_id])
+            self.unset_patchset_pending(baseurl, [patch_id])
 
         self.cur.execute('INSERT INTO '
                          'patchtest(patch_series_id, baseline_id, testrun_id) '

tests/test_db.py

@@ -510,15 +510,12 @@ def test_set_patchset_pending(self, mock_sql, mock_log):
         )
 
     @mock.patch('logging.debug')
-    @mock.patch('sktm.db.skt_db.get_sourceid')
     @mock.patch('sktm.db.sqlite3')
-    def test_unset_patchset_pending(self, mock_sql, mock_get_sourceid,
-                                    mock_log):
+    def test_unset_patchset_pending(self, mock_sql, mock_log):
         """Ensure patches are removed from the pendingpatch table."""
         testdb = skt_db(self.database_file)
-        mock_get_sourceid.return_value = 1
 
-        testdb.unset_patchset_pending('baseurl', '1', ['1'])
+        testdb.unset_patchset_pending('baseurl', ['1'])
 
         # Ensure a debug log was written
         mock_log.assert_called_once()
@@ -527,7 +524,7 @@ def test_unset_patchset_pending(self, mock_sql, mock_get_sourceid,
         execute_call_args = mock_sql.connect().cursor().executemany.\
             call_args[0]
         self.assertIn('DELETE FROM pendingpatches', execute_call_args[0])
-        self.assertEqual([('1', 1)], execute_call_args[1])
+        self.assertEqual([('1', 'baseurl')], execute_call_args[1])
 
         # Ensure the data was committed to the database
         mock_sql.connect().commit.assert_called()