Merge lp:~james-w/python-oops/record-published-explicitly into lp:python-oops

[James Westby]

Change to explicitly track published state.

If the state is tracked by the presence of the id then a report can't come
with a pre-generated id.

This prevents applications that know an oops will be generated, but don't
have access to the generation, from reporting the oops id to anyone.

Hi,

In discussions around the fact that Django can't easily tell anyone
what the id is for the oops they just encountered Rob said that the
best way to handle it was probably to have django allocate the oops id.

That works great, and the publishers will just use it. The only drawback
is that it means that publish_only_new will never trigger its publisher
any more, because of the double meaning of the id in the report.

Applications that do this could avoid publish_only_new, at the cost of
duplicating all oopses and then de-duplicating them later.

Instead I propose to track the published state more explicitly in the
oops, which this branch does.

Thanks,

James

[j.c.sackett (jcsackett)]

Looks good.

Thanks, James.

review: Approve

[Robert Collins (lifeless)]

So the challenge here is that a second publisher will publish a report *containing* the key 'published'. I think we want the metadata, such as it is, separate.

So, we need to change the API rather than stashing it in the report.

review: Needs Fixing

Unmerged revisions

30. By James Westby on 2012-06-07

Check for the published key in publish_new_only.

29. By James Westby on 2012-06-07

Set a published key in the report to indicate when it has been published.

=== modified file 'oops/config.py'
--- oops/config.py	2011-11-13 21:24:42 +0000
+++ oops/config.py	2012-06-07 21:03:22 +0000
@@ -159,13 +159,13 @@
         returned to the caller, allowing them to handle the case when multiple
         publishers allocated different ids. The id from the last publisher is
         left in the report's id key. This is done for three reasons:
-        
+
         * As a convenience for the common case when only one publisher is
           present.
-          
+
         * This allows the next publisher in the chain to reuse that id if it
           wishes.
-          
+
         * Showing users that a report was made only needs one id and this means
           other code doesn't need to arbitrarily pick one - it is done by
           publish().
@@ -189,6 +189,7 @@
             id = publisher(report)
             if id:
                 report['id'] = id
+                report['published'] = True
                 result.append(id)
             else:
                 result.append(None)

=== modified file 'oops/publishers.py'
--- oops/publishers.py	2011-11-13 21:24:42 +0000
+++ oops/publishers.py	2012-06-07 21:03:22 +0000
@@ -33,7 +33,7 @@
       >>> config.publishers.append(publish_new_only(datedir_repo.publish))
     """
     def result(report):
-        if report.get('id'):
+        if report.get('published'):
             return None
         return publisher(report)
     return result

=== modified file 'oops/tests/test_config.py'
--- oops/tests/test_config.py	2011-11-13 21:24:42 +0000
+++ oops/tests/test_config.py	2012-06-07 21:03:22 +0000
@@ -79,7 +79,7 @@
         config.publishers.append(pub_2)
         report = {}
         self.assertEqual(['1', '2'], config.publish(report))
-        self.assertEqual({'id': '2'}, report)
+        self.assertEqual('2', report['id'])
 
     def test_publish_filters_first(self):
         calls = []
@@ -101,7 +101,7 @@
         self.assertEqual(['ok', 'block'], calls)
 
     def test_publishers_returning_not_published_no_change_to_report(self):
-        # If a publisher returns non-zero (indicating it did not publish) no
+        # If a publisher returns a falseish value (indicating it did not publish) no
         # change is made to the report - this permits chaining publishers as a
         # primary and fallback: The fallback can choose to do nothing if there
         # is an id already assigned (and returns None to signal what it did);
@@ -115,4 +115,26 @@
         config.publishers.append(pub_noop)
         report = {}
         self.assertEqual(['1', None], config.publish(report))
-        self.assertEqual({'id': '1'}, report)
+        self.assertEqual({'id': '1', 'published': True}, report)
+
+    def test_publisher_returning_not_published_doesnt_set_published(self):
+        # If a publisher indicates that it didn't publish then the
+        # 'published' key is not set in the report
+        def pub_noop(report):
+            return ''
+        config = Config()
+        config.publishers.append(pub_noop)
+        report = {}
+        config.publish(report)
+        self.assertEqual({}, report)
+
+    def test_publisher_returning_published_sets_published(self):
+        # If a publisher indicates that it did publish then the
+        # 'published' key is set in the report
+        def pub_succeed(report):
+            return '1'
+        config = Config()
+        config.publishers.append(pub_succeed)
+        report = {}
+        config.publish(report)
+        self.assertEqual({'id': '1', 'published': True}, report)

=== modified file 'oops/tests/test_publishers.py'
--- oops/tests/test_publishers.py	2011-11-13 21:24:42 +0000
+++ oops/tests/test_publishers.py	2012-06-07 21:03:22 +0000
@@ -18,20 +18,25 @@
 __metaclass__ = type
 
 import testtools
-from testtools.matchers import Is
 
 from oops import publish_new_only
 
 class TestPublishers(testtools.TestCase):
 
-    def test_publish_new_only_id_in_report(self):
+    def test_publish_new_only_published_in_report(self):
         def pub_fail(report):
             self.fail('publication called')
         publisher = publish_new_only(pub_fail)
-        publisher({'id': 'foo'})
+        publisher({'published': True})
 
-    def test_publish_new_only_no_id_in_report(self):
+    def test_publish_new_only_no_published_in_report(self):
         calls = []
         publisher = publish_new_only(calls.append)
         publisher({'foo': 'bar'})
         self.assertEqual([{'foo': 'bar'}], calls)
+
+    def test_publish_new_only_no_published_false_in_report(self):
+        calls = []
+        publisher = publish_new_only(calls.append)
+        publisher({'published': False})
+        self.assertEqual([{'published': False}], calls)