launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> 
> === modified file 'lib/lp/services/webhooks/model.py'
> --- lib/lp/services/webhooks/model.py	2015-08-03 10:49:46 +0000
> +++ lib/lp/services/webhooks/model.py	2015-08-04 06:04:54 +0000
> @@ -144,6 +145,10 @@
>          updated_data['event_types'] = event_types
>          self.json_data = updated_data
>  
> +    def setSecret(self, secret):
> +        """See `IWebhook`."""
> +        self.secret = secret

The PubSubHubbub spec says that the secret "MUST be less than 200 bytes in length".  I think we should enforce that here, and document the restriction.

> +
>  
>  @implementer(IWebhookSource)
>  class WebhookSource:
> 
> === renamed file 'lib/lp/services/webhooks/tests/test_webhookjob.py' => 'lib/lp/services/webhooks/tests/test_job.py'
> --- lib/lp/services/webhooks/tests/test_webhookjob.py	2015-08-04 00:11:46 +0000
> +++ lib/lp/services/webhooks/tests/test_job.py	2015-08-04 06:04:54 +0000
> @@ -513,6 +539,31 @@
>          self.assertEqual(True, self.runJob(job))
>          self.assertEqual(JobStatus.COMPLETED, job.status)
>  
> +    def test_manual_retry_with_reset(self):
> +        # retry(reset=True) unsets date_first_sent so the automatic
> +        # retries can be resumed. This can be useful for recovering from
> +        # systemic errors the erroneously failed many deliveries.

s/the/that/

> +        hook = self.factory.makeWebhook()
> +        job = WebhookDeliveryJob.create(hook, payload={'foo': 'bar'})
> +        client = MockWebhookClient(response_status=404)
> +        self.useFixture(ZopeUtilityFixture(client, IWebhookClient))
> +
> +        # Simulate a first attempt failure.
> +        self.assertEqual(False, self.runJob(job))
> +        self.assertEqual(JobStatus.WAITING, job.status)
> +        self.assertIsNot(None, job.date_first_sent)
> +
> +        # A manual retry brings the scheduled start forward.
> +        job.retry()
> +        self.assertEqual(JobStatus.WAITING, job.status)
> +        self.assertIsNot(None, job.date_first_sent)
> +
> +        # When reset=True, date_first_sent is unset to restart the 24
> +        # hour auto-retry window.
> +        job.retry(reset=True)
> +        self.assertEqual(JobStatus.WAITING, job.status)
> +        self.assertIs(None, job.date_first_sent)
> +
>  
>  class TestViaCronscript(TestCaseWithFactory):
>  


-- 
https://code.launchpad.net/~wgrant/launchpad/webhook-api-and-active/+merge/266835
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.