From 58401a5ce188148fb22ce46524a1bdd27cf40234 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Mar 27 2017 15:52:22 +0000
Subject: Bring back `clean_updates`, have it delete updates from DB


Summary:
This is a modified revert of commit
2da425e354904273793e5321030882aac98d0c39.

Turns out this wasn't actually just cleaning up *deleted*
updates, though that's what it clearly thought it was doing.
It also marked updates for closed bugs - and other updates
which were no longer associated with a blocker or FE update,
for instance if the update was edited and no longer marked as
fixing the blocker bug - as 'deleted'. This is clearly wrong,
but did more or less the right thing sort of by accident; it
resulted in these updates not being shown in any of the views.

None of the 'update' code will actually update the status of
updates which no longer relate to a blocker or FE bug, so after
we removed `clean_updates`, there was a problem where updates
for closed bugs were still listed in the database with status
'testing', even though they're actually 'stable'. This probably
doesn't affect the main parts of the web UI much, since they
only actually show updates that fix bugs, but it does affect
the new 'requests' tab - it results in it including updates that
have already gone stable in both the 'compose request' and the
'stable push request' texts.

We could deal with this by tweaking the update code so we update
the status for all updates in the database, I guess, but I kinda
like this idea to trim the database by dropping updates that no
longer relate to blocker bugs from it entirely. If for whatever
reason an update pongs *back* to being relevant, it'll just get
re-added to the database at the next sync.

Test Plan:
Bit tricky, as you need to get an update into the
database that subsequently becomes 'irrelevant' to see the
behaviour. If you just build a database while we're in an active
cycle then wait a few days, till after some of the bugs get
closed, that should work. Without this patch, the update will
likely remain in the database with state 'testing', and show
up in the 'requests' tab.

Reviewers: tflink, mkrizek

Reviewed By: mkrizek

Differential Revision: https://phab.qa.fedoraproject.org/D1179

---

diff --git a/blockerbugs/util/update_sync.py b/blockerbugs/util/update_sync.py
index 2c15ca9..48573de 100644
--- a/blockerbugs/util/update_sync.py
+++ b/blockerbugs/util/update_sync.py
@@ -94,6 +94,24 @@ class UpdateSync(object):
         else:
             return False
 
+    def clean_updates(self, updates, relid):
+        """Remove updates for this release which are no longer related
+        to any blocker or freeze exception bug from the database.
+        """
+        query_updates = Update.query.filter(
+            Update.release_id == relid,
+        ).all()
+        db_updates = set(update.title for update in query_updates)
+        bodhi_updates = set(update['title'] for update in updates)
+        unneeded_updates = db_updates.difference(bodhi_updates)
+
+        for update in query_updates:
+            if update.title in unneeded_updates:
+                self.log.debug("Removing no longer relevant update %s" %
+                               update.title)
+                self.db.session.delete(update)
+                self.db.session.commit()
+
     def search_updates(self, bugids, release_num):
         queries_data = dict(
                 bugs=[str(bug_id) for bug_id in bugids],
@@ -147,6 +165,9 @@ class UpdateSync(object):
                     e=ex, r=release))
             return
 
+        # remove no longer relevant updates from the database
+        self.clean_updates(updates, release.id)
+
         for update in updates:
             self.log.debug('running sync for update %s' % update['title'])
             existing_update = Update.query.filter_by(
diff --git a/testing/testfunc_update_sync.py b/testing/testfunc_update_sync.py
index b732a9e..40ee6a2 100644
--- a/testing/testfunc_update_sync.py
+++ b/testing/testfunc_update_sync.py
@@ -194,3 +194,33 @@ class TestfuncUpdateSync(object):
         update_sync.sync_updates(self.test_release99)
         assert log_error.call_count == 1
         assert 'Internal Server Error' in log_error.call_args[0][0]
+
+    def test_sync_clean_updates(self):
+        bug1 = add_bug(3000, 'testbug1', self.test_milestone99alpha)
+
+        self.update_sync.sync_updates(self.test_release99)
+        updates = Update.query.all()
+        # by default we have two updates for bug #3000
+        assert len(updates) == 2
+
+        # now we do a sync with only *one* update for bug #3000
+        b = FakeBodhiInterface
+        b.updates[('f99', '3000')] = [update1_for_bug_3000]
+        update_sync = UpdateSync(db, b)
+        update_sync.sync_updates(self.test_release99)
+
+        # so now we should have just *one* update in the db, the other
+        # update should disappear. This is testing the (no longer
+        # likely) case where an update disappears entirely from Bodhi
+        updates = Update.query.all()
+        assert len(updates) == 1
+
+        # Now let's mark the bug as closed and sync again.
+        bug1.active = False
+        add_bug(4000, 'testbug2', self.test_milestone99alpha)
+        self.update_sync.sync_updates(self.test_release99)
+
+        # Now the other update should be removed from the db, as it
+        # no longer relates to an open blocker/FE bug
+        updates = Update.query.all()
+        assert len(updates) == 0