Merge lp:~cjohnston/ubuntu-ci-services-itself/write-ticket-resource into lp:ubuntu-ci-services-itself

[Chris Johnston]

Add write API and tests for the ticket resource

Add write API and tests for the ticket resource

[Andy Doan (doanac)]

On 12/19/2013 02:28 PM, Chris Johnston wrote:

> === added file 'ticket_system/ticket/tests/test_write_api.py'

> + def test_post_ticket(self):
> + # Check how many are there first.
> + self.assertEqual(Ticket.objects.count(), 1)
> + self.post(resource=self.resource, params=self.post_ticket_data)
> + # Verify a new one has been added.
> + self.assertEqual(Ticket.objects.count(), 2)

might be worth taking the ID from self.post and making sure the
post_ticket_data matches the object that got created. not a big deal though.

> + def test_patch_detail(self):
> + # Grab the current data & modify it slightly.
> + original_data = self.getResource(self.detail_url)
> + new_data = original_data.copy()
> + new_data['owner'] = '<email address hidden>'
> +
> + self.assertEqual(Ticket.objects.count(), 1)
> + resp = self.client.patch('/api/v1/' + self.detail_url, data=new_data)
> + self.assertHttpMethodNotAllowed(resp)
> + # Make sure the count hasn't changed & we did an update.
> + self.assertEqual(Ticket.objects.count(), 1)
> + # Check for updated data.
> + self.assertEqual(Ticket.objects.get(pk=self.ticket.pk).owner,
> + self.ticket.owner)

Should this assert be:
   self.assertEqual(Ticket.objects.get(pk=self.ticket.pk).owner,
                    new_data['owner'])

[Chris Johnston (cjohnston)]

On Thu, Dec 19, 2013 at 4:27 PM, Andy Doan <email address hidden>wrote:

> > + def test_patch_detail(self):
> > + # Grab the current data & modify it slightly.
> > + original_data = self.getResource(self.detail_url)
> > + new_data = original_data.copy()
> > + new_data['owner'] = '<email address hidden>'
> > +
> > + self.assertEqual(Ticket.objects.count(), 1)
> > + resp = self.client.patch('/api/v1/' + self.detail_url,
> data=new_data)
> > + self.assertHttpMethodNotAllowed(resp)
> > + # Make sure the count hasn't changed & we did an update.
> > + self.assertEqual(Ticket.objects.count(), 1)
> > + # Check for updated data.
> > + self.assertEqual(Ticket.objects.get(pk=self.ticket.pk).owner,
> > + self.ticket.owner)
>
> Should this assert be:
> self.assertEqual(Ticket.objects.get(pk=self.ticket.pk).owner,
> new_data['owner'])
>

Nope... If you note:

+ allowed_methods = ['get', 'post']

You will notice that patch is not allowed. So I am verifying that patch
didn't work:

+ resp = self.client.patch('/api/v1/' + self.detail_url,
data=new_data)
+ self.assertHttpMethodNotAllowed(resp)

and then that the owner is still the same owner that it was originally.

[Andy Doan (doanac)]

review: Approve

[Vincent Ladeuil (vila)]

74 + self.assertEqual(Ticket.objects.count(), 1)

Before the meme starts getting out of hand, *I* have a strong preference for:

 assert (expected, actual)

I *do* know there is an ongoing controversy on that topic but there is also an argument nobody was never able to properly answer: diff, patch, the good old guys, present their output (or input) in the old/new order. And nobody in their right mind will ever propose to change this.

When I read a failing test output, I expect the same order, 'old' is what was working and is what the test expect, so it's 'expected'. 'new' is why the test is failing right now, so it's 'actual'.

And we don't want to have to read the test code to know which order has been used each time we encounter a test failure, so please, let's settle on expected, actual.

=== modified file 'ticket_system/ticket/api.py'
--- ticket_system/ticket/api.py	2013-12-19 16:07:20 +0000
+++ ticket_system/ticket/api.py	2013-12-19 20:32:23 +0000
@@ -14,6 +14,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 from tastypie import fields
+from tastypie.authorization import Authorization
 from tastypie.resources import ModelResource
 from ticket.models import Ticket, SubTicket, SourcePackageUpload, Artifact
 
@@ -24,7 +25,8 @@
 
     class Meta:
         queryset = Ticket.objects.all()
-        allowed_methods = ['get']
+        allowed_methods = ['get', 'post']
+        authorization = Authorization()
 
 
 class SourcePackageUploadResource(ModelResource):

=== modified file 'ticket_system/ticket/tests/__init__.py'
--- ticket_system/ticket/tests/__init__.py	2013-12-13 04:56:17 +0000
+++ ticket_system/ticket/tests/__init__.py	2013-12-19 20:32:23 +0000
@@ -16,3 +16,4 @@
 from test_models import *
 from test_read_api import *
 from test_full_read_api import *
+from test_write_api import *

=== added file 'ticket_system/ticket/tests/test_write_api.py'
--- ticket_system/ticket/tests/test_write_api.py	1970-01-01 00:00:00 +0000
+++ ticket_system/ticket/tests/test_write_api.py	2013-12-19 20:32:23 +0000
@@ -0,0 +1,61 @@
+# Ubuntu Continuous Integration Engine
+# Copyright 2013 Canonical Ltd.
+
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU Affero General Public License version 3, as
+# published by the Free Software Foundation.
+
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU Affero General Public License for more details.
+
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+from model_mommy import mommy
+from ci_utils.tastypie.test import TastypieTestCase
+from ticket.models import Ticket
+
+
+class APICreateTicketResourceTest(TastypieTestCase):
+
+    def setUp(self):
+        super(APICreateTicketResourceTest, self).setUp('/api/v1')
+        self.ticket = mommy.make('Ticket')
+        self.resource = 'ticket/'
+        self.detail_url = self.resource + '{0}/'.format(self.ticket.pk)
+        self.post_ticket_data = {
+            'owner': '[e-mail]',
+            'title': 'My first ticket!',
+            'description': 'This if my first ticket. See what it can do',
+            'bug_id': '12345',
+            'added_binaries': 'binary-1,binary-2',
+            'removed_binaries': 'binary-3,binary-4',
+        }
+
+    def test_post_ticket(self):
+        # Check how many are there first.
+        self.assertEqual(Ticket.objects.count(), 1)
+        self.post(resource=self.resource, params=self.post_ticket_data)
+        # Verify a new one has been added.
+        self.assertEqual(Ticket.objects.count(), 2)
+
+    def test_patch_detail(self):
+        # Grab the current data & modify it slightly.
+        original_data = self.getResource(self.detail_url)
+        new_data = original_data.copy()
+        new_data['owner'] = '[e-mail]'
+
+        self.assertEqual(Ticket.objects.count(), 1)
+        resp = self.client.patch('/api/v1/' + self.detail_url, data=new_data)
+        self.assertHttpMethodNotAllowed(resp)
+        # Make sure the count hasn't changed & we did an update.
+        self.assertEqual(Ticket.objects.count(), 1)
+        # Check for updated data.
+        self.assertEqual(Ticket.objects.get(pk=self.ticket.pk).owner,
+                         self.ticket.owner)
+
+    def test_delete_ticket_not_allowed(self):
+        resp = self.delete(resource=self.detail_url)
+        self.assertHttpMethodNotAllowed(resp)