charmworld

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

  1. multiple-appflowers
  2. Merge into trunk
Proposed by j.c.sackett
Status: Merged
Approved by: j.c.sackett
Approved revision: 454
Merged at revision: 451
Proposed branch: lp:~jcsackett/charmworld/multiple-appflowers
Merge into: lp:charmworld
Diff against target: 165 lines (+43/-46)
4 files modified
charmworld/models.py (+12/-5)
charmworld/tests/test_models.py (+29/-10)
charmworld/views/tests/test_tools.py (+0/-27)
charmworld/views/tools.py (+2/-4)
To merge this branch: bzr merge lp:~jcsackett/charmworld/multiple-appflowers
Reviewer Review Type Date Requested Status
Juju Gui Bot continuous-integration Approve
Curtis Hovey (community) code Approve
Review via email: mp+194241@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

Thank you.

review: Approve (code)
Revision history for this message
Juju Gui Bot (juju-gui-bot) wrote :

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)
Revision history for this message
Juju Gui Bot (juju-gui-bot) wrote :

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 updated
454. By j.c.sackett

Shut up, lint.

Revision history for this message
Juju Gui Bot (juju-gui-bot) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charmworld/models.py'
2--- charmworld/models.py 2013-11-06 02:32:43 +0000
3+++ charmworld/models.py 2013-11-08 14:33:22 +0000
4@@ -1262,16 +1262,23 @@
5 {"store_data.errors": {"$exists": True}})
6 return (Charm(data) for data in self.collection.find(query))
7
8- def find_proof_error_charms(self, sub_only):
9+ def find_proof_error_charms(self):
10 query = {"proof.e": {"$exists": True}, "promulgated": True}
11- if sub_only:
12- query['subordinate'] = True
13 query = self.exclude_obsolete(query)
14 sort = [("series", pymongo.DESCENDING), ("name", pymongo.ASCENDING)]
15- return (Charm(data) for data in self.collection.find(query, sort=sort))
16+ return (Charm(data) for data in self.collection.find(query, sort=sort)
17+ if self._is_latest(data))
18+
19+ def _is_latest(self, charm_data):
20+ """Determine whether or not the data is for the latest store_revision
21+ of this charm.
22+ """
23+ # Only the latest version of a charm is in elasticsearch.
24+ latest = self.index_client.get(charm_data['_id'])
25+ return latest is not None
26
27 def _get_all(self, charm_id):
28- """"Return all stored values for a given charm."""
29+ """Return all stored values for a given charm."""
30 yield self.collection.find_one({'_id': charm_id})
31 yield self.index_client.get(charm_id)
32
33
34=== modified file 'charmworld/tests/test_models.py'
35--- charmworld/tests/test_models.py 2013-11-06 02:32:43 +0000
36+++ charmworld/tests/test_models.py 2013-11-08 14:33:22 +0000
37@@ -1482,30 +1482,49 @@
38 self.assertEqual(foo, bar)
39
40 def test_find_proof_error_charms(self):
41- self.use_index_client()
42+ client = self.use_index_client()
43 proof_with_errors = {'e': ['error 1']}
44 proof_with_warnings = {'w': ['warning 1']}
45- # The view retrieves promulgated charms with proof errors.
46 ignore, charm_1 = factory.makeCharm(
47 self.db, owner='joe', promulgated=True, proof=proof_with_errors)
48- # The view ignores promulgated charms with proof warning.
49- factory.makeCharm(self.db, promulgated=True, proof=proof_with_warnings)
50- # The view ignored unpromulgated charms with proof errors.
51- factory.makeCharm(self.db, promulgated=False, proof=proof_with_errors)
52+ client.index_charm(charm_1)
53+ ignore, charm_2 = factory.makeCharm(
54+ self.db, promulgated=True, proof=proof_with_warnings)
55+ client.index_charm(charm_2)
56+ ignore, charm_3 = factory.makeCharm(
57+ self.db, promulgated=False, proof=proof_with_errors)
58+ client.index_charm(charm_3)
59 charm_source = CharmSource.from_request(self)
60- errors = list(charm_source.find_proof_error_charms(False))
61+ errors = list(charm_source.find_proof_error_charms())
62 self.assertEqual(1, len(errors))
63 self.assertEqual(charm_1['name'], errors[0].name)
64
65 def test_obsolete_charms_ignored_by_find_proof_error_charms(self):
66- self.use_index_client()
67+ client = self.use_index_client()
68 proof_with_errors = {'e': ['error 1']}
69 # The view retrieves promulgated charms with proof errors.
70 ignore, charm_1 = factory.makeCharm(
71 self.db, owner='joe', promulgated=True, proof=proof_with_errors,
72 series='oneiric')
73- charm_source = CharmSource.from_request(self)
74- errors = list(charm_source.find_proof_error_charms(False))
75+ client.index_charm(charm_1)
76+ charm_source = CharmSource.from_request(self)
77+ errors = list(charm_source.find_proof_error_charms())
78+ self.assertEqual([], errors)
79+
80+ def test_find_proof_error_charms_ignores_old_revisions(self):
81+ client = self.use_index_client()
82+ proof_with_errors = {'e': ['error 1']}
83+ # The view retrieves promulgated charms with proof errors.
84+ ignore, charm_1 = factory.makeCharm(
85+ self.db, name='foo', owner='joe', promulgated=True,
86+ proof=proof_with_errors, store_revision=1, series='precise')
87+ client.index_charm(charm_1)
88+ ignore, charm_2 = factory.makeCharm(
89+ self.db, name='foo', owner='joe', promulgated=True,
90+ series='precise', store_revision=2)
91+ client.index_charm(charm_2)
92+ charm_source = CharmSource.from_request(self)
93+ errors = list(charm_source.find_proof_error_charms())
94 self.assertEqual([], errors)
95
96 def test_store_errors_in_queue(self):
97
98=== modified file 'charmworld/views/tests/test_tools.py'
99--- charmworld/views/tests/test_tools.py 2013-10-10 15:42:33 +0000
100+++ charmworld/views/tests/test_tools.py 2013-11-08 14:33:22 +0000
101@@ -17,7 +17,6 @@
102 format_items_for_queue,
103 get_missing_qa_items,
104 get_sparklines,
105- proof_errors,
106 )
107 from charmworld.testing import factory
108 from charmworld.testing import ViewTestBase
109@@ -181,29 +180,3 @@
110 series='oneiric')
111 response = charm_store_missing(self.getRequest())
112 self.assertEqual([], response['queue'])
113-
114-
115-class ProofErrorsTestCase(ViewTestBase):
116-
117- def test_promulgated_charms_with_errors(self):
118- proof_with_errors = {'e': ['error 1']}
119- proof_with_warnings = {'w': ['warning 1']}
120- # The view retrieves promulgated charms with proof errors.
121- ignore, charm_1 = factory.makeCharm(
122- self.db, owner='joe', promulgated=True, proof=proof_with_errors)
123- # The view ignores promulgated charms with proof warning.
124- factory.makeCharm(self.db, promulgated=True, proof=proof_with_warnings)
125- # The view ignored unpromulgated charms with proof errors.
126- factory.makeCharm(self.db, promulgated=False, proof=proof_with_errors)
127- response = proof_errors(self.getRequest())
128- charm = Charm(charm_1)
129- self.assertEqual([charm], response['charms'])
130-
131- def test_obsolete_charms_ignored(self):
132- proof_with_errors = {'e': ['error 1']}
133- # The view retrieves promulgated charms with proof errors.
134- ignore, charm_1 = factory.makeCharm(
135- self.db, owner='joe', promulgated=True, proof=proof_with_errors,
136- series='oneiric')
137- response = proof_errors(self.getRequest())
138- self.assertEqual([], response['charms'])
139
140=== modified file 'charmworld/views/tools.py'
141--- charmworld/views/tools.py 2013-10-10 16:22:17 +0000
142+++ charmworld/views/tools.py 2013-11-08 14:33:22 +0000
143@@ -65,7 +65,7 @@
144
145 def build_review_queue(request):
146 source = CharmSource.from_request(request)
147- proof_items = source.find_proof_error_charms(False)
148+ proof_items = source.find_proof_error_charms()
149 store_items = source.find_mia_charms()
150 qa_items = get_missing_qa_items(source, request.db)
151
152@@ -163,12 +163,10 @@
153 http_cache=REPORT_CACHE,
154 renderer="charmworld:templates/charm-proof-errors.pt")
155 def proof_errors(request):
156- sub_only = bool(request.GET.get('sub'))
157 source = CharmSource.from_request(request)
158- charms = list(source.find_proof_error_charms(sub_only))
159+ charms = list(source.find_proof_error_charms())
160
161 return {
162 'charms': charms,
163 'format_proof': format_proof,
164- 'sub_only': sub_only,
165 }

Subscribers

People subscribed via source and target branches

to all changes: