Merge lp:~jcsackett/charmworld/multiple-appflowers into lp:charmworld

[j.c.sackett]

Removes old-revision proof error charms from the review queue.

The linked bug reports a situation wherein a charm (appflower) is shown to have
proof errors but the actual charm page shows no errors. This is because we have
introduced versioned charms. The charm page uses the tip revision of any charm,
but CharmSource isn't actually really revision aware. This results in
find_proof_error_charms returning all charms with proof errors, even those where
a subsequent version has fixed the proof error.

One solution that was investigated was using the elastic search index to search
for proof charms, as it's only aware of the tip version of charms. However, the
only way to query for the existence of data, without trying to match on it, is
the "exists" filter, which cannot be used as proof is a json object, which
"exists" can't use. Equally, testing for the existence of proof.e fails, because
that is a list object--again a type that exists fails on.

We can however attempt to find the charm_id of a charm to test if it's in the
index; if it is, we can reliably say that is the tip.

charmworld/models.py
--------------------
* _is_latest is added to CharmSource, which uses the index to test for the charm
  id.
* find_proof_error_charms is updated to use _is_latest to filter out the old
  charms.
* As a drive by, the undocumented "sub" feature is removed, making the code
  simpler. This feature isn't used.

charmworld/views/tools.py
-------------------------
* The "sub" feature, accessible only by url mangling, is removed. Addiitonally,
  this entire view will be removed soon.

charmworld/tests/test_models.py
-------------------------------
A test for the new find_proof_error_charms behavior is added.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

[Juju Gui Bot (juju-gui-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
http://162.213.35.27:8080/job/charmworld-autoland-lxc/70/
Executed test runs:

review: Needs Fixing (continuous-integration)

[Juju Gui Bot (juju-gui-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
http://162.213.35.27:8080/job/charmworld-autoland-lxc/71/
Executed test runs:

review: Needs Fixing (continuous-integration)

[lp:~jcsackett/charmworld/multiple-appflowers]

454. By j.c.sackett on 2013-11-08

Shut up, lint.

[Juju Gui Bot (juju-gui-bot)]

review: Approve (continuous-integration)

=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-11-06 02:32:43 +0000
+++ charmworld/models.py	2013-11-08 14:33:22 +0000
@@ -1262,16 +1262,23 @@
             {"store_data.errors": {"$exists": True}})
         return (Charm(data) for data in self.collection.find(query))
 
-    def find_proof_error_charms(self, sub_only):
+    def find_proof_error_charms(self):
         query = {"proof.e": {"$exists": True}, "promulgated": True}
-        if sub_only:
-            query['subordinate'] = True
         query = self.exclude_obsolete(query)
         sort = [("series", pymongo.DESCENDING), ("name", pymongo.ASCENDING)]
-        return (Charm(data) for data in self.collection.find(query, sort=sort))
+        return (Charm(data) for data in self.collection.find(query, sort=sort)
+                if self._is_latest(data))
+
+    def _is_latest(self, charm_data):
+        """Determine whether or not the data is for the latest store_revision
+        of this charm.
+        """
+        # Only the latest version of a charm is in elasticsearch.
+        latest = self.index_client.get(charm_data['_id'])
+        return latest is not None
 
     def _get_all(self, charm_id):
-        """"Return all stored values for a given charm."""
+        """Return all stored values for a given charm."""
         yield self.collection.find_one({'_id': charm_id})
         yield self.index_client.get(charm_id)
 
 
=== modified file 'charmworld/tests/test_models.py'
--- charmworld/tests/test_models.py	2013-11-06 02:32:43 +0000
+++ charmworld/tests/test_models.py	2013-11-08 14:33:22 +0000
@@ -1482,30 +1482,49 @@
             self.assertEqual(foo, bar)
 
     def test_find_proof_error_charms(self):
-        self.use_index_client()
+        client = self.use_index_client()
         proof_with_errors = {'e': ['error 1']}
         proof_with_warnings = {'w': ['warning 1']}
-        # The view retrieves promulgated charms with proof errors.
         ignore, charm_1 = factory.makeCharm(
             self.db, owner='joe', promulgated=True, proof=proof_with_errors)
-        # The view ignores promulgated charms with proof warning.
-        factory.makeCharm(self.db, promulgated=True, proof=proof_with_warnings)
-        # The view ignored unpromulgated charms with proof errors.
-        factory.makeCharm(self.db, promulgated=False, proof=proof_with_errors)
+        client.index_charm(charm_1)
+        ignore, charm_2 = factory.makeCharm(
+            self.db, promulgated=True, proof=proof_with_warnings)
+        client.index_charm(charm_2)
+        ignore, charm_3 = factory.makeCharm(
+            self.db, promulgated=False, proof=proof_with_errors)
+        client.index_charm(charm_3)
         charm_source = CharmSource.from_request(self)
-        errors = list(charm_source.find_proof_error_charms(False))
+        errors = list(charm_source.find_proof_error_charms())
         self.assertEqual(1, len(errors))
         self.assertEqual(charm_1['name'], errors[0].name)
 
     def test_obsolete_charms_ignored_by_find_proof_error_charms(self):
-        self.use_index_client()
+        client = self.use_index_client()
         proof_with_errors = {'e': ['error 1']}
         # The view retrieves promulgated charms with proof errors.
         ignore, charm_1 = factory.makeCharm(
             self.db, owner='joe', promulgated=True, proof=proof_with_errors,
             series='oneiric')
-        charm_source = CharmSource.from_request(self)
-        errors = list(charm_source.find_proof_error_charms(False))
+        client.index_charm(charm_1)
+        charm_source = CharmSource.from_request(self)
+        errors = list(charm_source.find_proof_error_charms())
+        self.assertEqual([], errors)
+
+    def test_find_proof_error_charms_ignores_old_revisions(self):
+        client = self.use_index_client()
+        proof_with_errors = {'e': ['error 1']}
+        # The view retrieves promulgated charms with proof errors.
+        ignore, charm_1 = factory.makeCharm(
+            self.db, name='foo', owner='joe', promulgated=True,
+            proof=proof_with_errors, store_revision=1, series='precise')
+        client.index_charm(charm_1)
+        ignore, charm_2 = factory.makeCharm(
+            self.db, name='foo', owner='joe', promulgated=True,
+            series='precise', store_revision=2)
+        client.index_charm(charm_2)
+        charm_source = CharmSource.from_request(self)
+        errors = list(charm_source.find_proof_error_charms())
         self.assertEqual([], errors)
 
     def test_store_errors_in_queue(self):
 
=== modified file 'charmworld/views/tests/test_tools.py'
--- charmworld/views/tests/test_tools.py	2013-10-10 15:42:33 +0000
+++ charmworld/views/tests/test_tools.py	2013-11-08 14:33:22 +0000
@@ -17,7 +17,6 @@
     format_items_for_queue,
     get_missing_qa_items,
     get_sparklines,
-    proof_errors,
 )
 from charmworld.testing import factory
 from charmworld.testing import ViewTestBase
@@ -181,29 +180,3 @@
                                             series='oneiric')
         response = charm_store_missing(self.getRequest())
         self.assertEqual([], response['queue'])
-
-
-class ProofErrorsTestCase(ViewTestBase):
-
-    def test_promulgated_charms_with_errors(self):
-        proof_with_errors = {'e': ['error 1']}
-        proof_with_warnings = {'w': ['warning 1']}
-        # The view retrieves promulgated charms with proof errors.
-        ignore, charm_1 = factory.makeCharm(
-            self.db, owner='joe', promulgated=True, proof=proof_with_errors)
-        # The view ignores promulgated charms with proof warning.
-        factory.makeCharm(self.db, promulgated=True, proof=proof_with_warnings)
-        # The view ignored unpromulgated charms with proof errors.
-        factory.makeCharm(self.db, promulgated=False, proof=proof_with_errors)
-        response = proof_errors(self.getRequest())
-        charm = Charm(charm_1)
-        self.assertEqual([charm], response['charms'])
-
-    def test_obsolete_charms_ignored(self):
-        proof_with_errors = {'e': ['error 1']}
-        # The view retrieves promulgated charms with proof errors.
-        ignore, charm_1 = factory.makeCharm(
-            self.db, owner='joe', promulgated=True, proof=proof_with_errors,
-            series='oneiric')
-        response = proof_errors(self.getRequest())
-        self.assertEqual([], response['charms'])
 
=== modified file 'charmworld/views/tools.py'
--- charmworld/views/tools.py	2013-10-10 16:22:17 +0000
+++ charmworld/views/tools.py	2013-11-08 14:33:22 +0000
@@ -65,7 +65,7 @@
 
 def build_review_queue(request):
     source = CharmSource.from_request(request)
-    proof_items = source.find_proof_error_charms(False)
+    proof_items = source.find_proof_error_charms()
     store_items = source.find_mia_charms()
     qa_items = get_missing_qa_items(source, request.db)
 
@@ -163,12 +163,10 @@
     http_cache=REPORT_CACHE,
     renderer="charmworld:templates/charm-proof-errors.pt")
 def proof_errors(request):
-    sub_only = bool(request.GET.get('sub'))
     source = CharmSource.from_request(request)
-    charms = list(source.find_proof_error_charms(sub_only))
+    charms = list(source.find_proof_error_charms())
 
     return {
         'charms': charms,
         'format_proof': format_proof,
-        'sub_only': sub_only,
     }