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

[Chris Johnston]

Add ticket complete API call to the TS

- Update docs
- Remove sourcepackage from BinaryPackage object as it isn't needed
- Add a updateticketstatus/complete/ api url.

What this new url does:

This URL can be called by a get api call. When called it does a few things.

- It takes all binaries that the owner of the ticket added as 'added binaries' and adds them to the 'master list' of binaries.
- It takes all binaries that the owner of the ticket added as 'removed binaries' and removes them from the 'master list' of binaries.
- Marks the ticket workflow step and status as complete.

[Andy Doan (doanac)]

do we really want this API to be an HTTP GET and not a PATCH?

[Chris Johnston (cjohnston)]

On Wed, Jan 22, 2014 at 11:36 AM, Andy Doan <email address hidden>wrote:

> do we really want this API to be an HTTP GET and not a PATCH?
>
You aren't actually patching anything. I'm not sure though?

[Andy Doan (doanac)]

On 01/22/2014 10:54 AM, Chris Johnston wrote:
> On Wed, Jan 22, 2014 at 11:36 AM, Andy Doan <email address hidden>wrote:
>
>> do we really want this API to be an HTTP GET and not a PATCH?
>>
> You aren't actually patching anything. I'm not sure though?

Things are being changed:

219 + queryset.current_workflow_step = TicketWorkflowStep.COMPLETED.value
220 + queryset.status = TicketWorkflowStepStatus.COMPLETED.value
221 + queryset.save()

Normally a GET implies a read-only operation. This is doing writes. If
we move to a secure model someday - we could easily forget about this
method.

[lp:~cjohnston/ubuntu-ci-services-itself/ticket-complete]

126. By Chris Johnston on 2014-01-23

Update per review

127. By Chris Johnston on 2014-01-23

Merge trunk

[Chris Johnston (cjohnston)]

review: Needs Resubmitting

[lp:~cjohnston/ubuntu-ci-services-itself/ticket-complete]

128. By Chris Johnston on 2014-01-23

Fix tests

[Andy Doan (doanac)]

191 + if (
192 + update["current_workflow_step"] == "1000" and
193 + update["status"] == "1000"
194 + ):

you forgot to update to using the new str functions you implemented

24 + curl --dump-header - http://localhost:8000/api/v1/updateticketstatus/1/complete/

you need to update the doc for the updated usage

[lp:~cjohnston/ubuntu-ci-services-itself/ticket-complete]

129. By Chris Johnston on 2014-01-24

Update per review

[Chris Johnston (cjohnston)]

review: Needs Resubmitting

[lp:~cjohnston/ubuntu-ci-services-itself/ticket-complete]

130. By Chris Johnston on 2014-01-24

Merge trunk

131. By Chris Johnston on 2014-01-24

Merge trunk

[Andy Doan (doanac)]

review: Approve

=== modified file 'docs/components/ticket-system.rst'
--- docs/components/ticket-system.rst	2014-01-24 20:33:49 +0000
+++ docs/components/ticket-system.rst	2014-01-24 20:49:45 +0000
@@ -204,7 +204,7 @@
 
  curl --dump-header - http://localhost:8000/api/v1/ticketstatus/
 
-*New*
+*Queued*
 
 ::
 
@@ -256,6 +256,15 @@
 
  curl --dump-header - -H "Content-Type: application/json" -X PATCH --data '{"current_workflow_step": "100", "status": "000"}' http://localhost:8000/api/v1/updateticketstatus/1/
 
+Mark ticket complete
+~~~~~~~~~~~~~~~~~~~~
+
+To be used by the lander. The act of marking a ticket complete will modify the master list of binary packages which are tested by adding to the list any of the 'added_binaries' from the ticket and removing any of the 'removed_binaries' from the ticket.
+
+::
+
+ curl --dump-header - -H "Content-Type: application/json" -X PATCH --data '{"current_workflow_step": "1000", "status": "1000"}' http://localhost:8000/api/v1/updateticketstatus/1/
+
 Update subticket status
 ~~~~~~~~~~~~~~~~~~~~~~~
 

=== modified file 'ticket_system/project/admin.py'
--- ticket_system/project/admin.py	2013-12-20 19:41:18 +0000
+++ ticket_system/project/admin.py	2014-01-24 20:49:45 +0000
@@ -23,8 +23,8 @@
 
 
 class BinaryPackageAdmin(admin.ModelAdmin):
-    search_fields = ['name', 'sourcepackage']
-    list_display = ('name', 'sourcepackage')
+    search_fields = ['name']
+    list_display = ('name',)
 
 admin.site.register(SourcePackage, SourcePackageAdmin)
 admin.site.register(BinaryPackage, BinaryPackageAdmin)

=== modified file 'ticket_system/project/api.py'
--- ticket_system/project/api.py	2013-12-16 18:52:37 +0000
+++ ticket_system/project/api.py	2014-01-24 20:49:45 +0000
@@ -13,10 +13,8 @@
 # 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 tastypie import fields
 from tastypie.authorization import Authorization
 from tastypie.resources import ModelResource
-from tastypie.constants import ALL
 from project.models import SourcePackage, BinaryPackage
 
 
@@ -32,8 +30,6 @@
 
 
 class BinaryPackageResource(ModelResource):
-    sourcepackage = fields.ToOneField(SourcePackageResource, 'sourcepackage',
-                                      full=True)
 
     class Meta:
         queryset = BinaryPackage.objects.all()
@@ -41,5 +37,4 @@
         authorization = Authorization()
         filtering = {
             "name": ('exact', 'iexact', 'startswith', 'istartswith'),
-            "seeded": ALL,
         }

=== modified file 'ticket_system/project/migrations/0001_initial.py'
--- ticket_system/project/migrations/0001_initial.py	2014-01-22 16:33:53 +0000
+++ ticket_system/project/migrations/0001_initial.py	2014-01-24 20:49:45 +0000
@@ -19,7 +19,6 @@
         db.create_table('binarypackage', (
             (u'id', self.gf('django.db.models.fields.AutoField')(primary_key=True)),
             ('name', self.gf('django.db.models.fields.CharField')(max_length=4096)),
-            ('sourcepackage', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['project.SourcePackage'])),
         ))
         db.send_create_signal(u'project', ['BinaryPackage'])
 
@@ -36,8 +35,7 @@
         u'project.binarypackage': {
             'Meta': {'object_name': 'BinaryPackage', 'db_table': "'binarypackage'"},
             u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
-            'name': ('django.db.models.fields.CharField', [], {'max_length': '4096'}),
-            'sourcepackage': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['project.SourcePackage']"})
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '4096'})
         },
         u'project.sourcepackage': {
             'Meta': {'object_name': 'SourcePackage', 'db_table': "'sourcepackage'"},

=== modified file 'ticket_system/project/models.py'
--- ticket_system/project/models.py	2014-01-23 17:31:52 +0000
+++ ticket_system/project/models.py	2014-01-24 20:49:45 +0000
@@ -31,7 +31,6 @@
         db_table = "binarypackage"
 
     name = models.CharField(max_length=4096)
-    sourcepackage = models.ForeignKey("SourcePackage")
 
     def __unicode__(self):
         return self.name

=== modified file 'ticket_system/project/tests.py'
--- ticket_system/project/tests.py	2014-01-24 15:10:17 +0000
+++ ticket_system/project/tests.py	2014-01-24 20:49:45 +0000
@@ -43,8 +43,7 @@
 
     def setUp(self):
         self.sourcepackage = sourcepackage_recipe.make()
-        self.binarypackage = mommy.make('BinaryPackage',
-                                        sourcepackage=self.sourcepackage)
+        self.binarypackage = mommy.make('BinaryPackage')
 
     def test_creating_binarypackage(self):
         binarypackage_in_database = BinaryPackage.objects.all()
@@ -122,25 +121,15 @@
     def setUp(self):
         super(BinaryPackageResourceTest, self).setUp('/api/v1')
         self.sourcepackage = sourcepackage_recipe.make()
-        self.binarypackage = mommy.make('BinaryPackage',
-                                        sourcepackage=self.sourcepackage)
+        self.binarypackage = mommy.make('BinaryPackage')
         self.detail_url = 'binarypackage/{0}/'.format(self.binarypackage.pk)
-        self.post_binarypackage_data = {
-            'name': 'ubuntu-ci',
-            'sourcepackage': '/api/v1/sourcepackage/{0}/'.format(
-                self.sourcepackage.pk)
-        }
+        self.post_binarypackage_data = {'name': 'ubuntu-ci'}
 
     def test_get_binarypackage_list_json(self):
         obj = self.getResource('binarypackage/')
         self.assertEqual(obj['objects'][0], {
             u'id': self.binarypackage.pk,
             u'name': unicode(self.binarypackage.name),
-            u'sourcepackage': {
-                u'id': self.sourcepackage.pk,
-                u'name': unicode(self.sourcepackage.name),
-                u'resource_uri': u'/api/v1/sourcepackage/{0}/'.format(
-                    self.sourcepackage.pk)},
             u'resource_uri': u'/api/v1/binarypackage/{0}/'.format(
                 self.binarypackage.pk)
         })
@@ -149,7 +138,7 @@
         obj = self.getResource(self.detail_url)
 
         # We use ``assertKeys`` here to just verify the keys, not all the data.
-        self.assertKeys(obj, ['id', 'name', 'resource_uri', 'sourcepackage'])
+        self.assertKeys(obj, ['id', 'name', 'resource_uri'])
         self.assertEqual(obj['name'], self.binarypackage.name)
 
     def test_post_binarypackage(self):

=== modified file 'ticket_system/ticket/api.py'
--- ticket_system/ticket/api.py	2014-01-24 16:39:35 +0000
+++ ticket_system/ticket/api.py	2014-01-24 20:49:45 +0000
@@ -13,6 +13,10 @@
 # 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/>.
 
+import json
+
+from django.shortcuts import get_object_or_404
+
 from tastypie import fields
 from tastypie.constants import ALL
 from tastypie.authorization import Authorization
@@ -23,6 +27,7 @@
                            TicketWorkflowStep, TicketWorkflowStepStatus,
                            get_enum_title)
 
+from project.models import BinaryPackage
 from project.api import SourcePackageResource
 
 
@@ -161,6 +166,38 @@
             "id": ('exact'),
         }
 
+    def patch_detail(self, request, **kwargs):
+        update = json.loads(request.body)
+        if (
+            update["current_workflow_step"] == str(
+                TicketWorkflowStep.COMPLETED.value) and
+            update["status"] == str(TicketWorkflowStepStatus.COMPLETED.value)
+        ):
+            queryset = get_object_or_404(Ticket, pk=kwargs['pk'])
+            added_binaries = queryset.added_binaries
+            if added_binaries is not None:
+                added_binaries = added_binaries.split(",")
+            else:
+                added_binaries = []
+            removed_binaries = queryset.removed_binaries
+            if removed_binaries is not None:
+                removed_binaries = removed_binaries.split(",")
+            else:
+                removed_binaries = []
+            for binary in added_binaries:
+                #Use get or create incase the binary already exists in the list
+                BinaryPackage.objects.get_or_create(name=binary)
+            for binary in removed_binaries:
+                try:
+                    # If the binary specified exists in the list, delete it
+                    bp = BinaryPackage.objects.get(name=binary)
+                    bp.delete()
+                except BinaryPackage.DoesNotExist:
+                    # If the binary specified does not exist, move on
+                    pass
+        return super(TicketUpdateStatusResource, self).patch_detail(request,
+                                                                    **kwargs)
+
 
 class TicketStatusResource(TicketTranslatedResource):
 

=== modified file 'ticket_system/ticket/models.py'
--- ticket_system/ticket/models.py	2014-01-24 20:33:49 +0000
+++ ticket_system/ticket/models.py	2014-01-24 20:49:45 +0000
@@ -31,6 +31,9 @@
     PKG_BUILDING = Item(200, "Package building")
     COMPLETED = Item(1000, "Completed")
 
+    def __str__(self):
+        return str(self.value)
+
 
 class SubTicketWorkflowStepStatus(DBEnumeratedType):
 
@@ -40,6 +43,9 @@
     PKG_BUILDING_COMPLETED = Item(230, "Completed")
     PKG_BUILDING_FAILED = Item(240, "Failed")
 
+    def __str__(self):
+        return str(self.value)
+
 
 class TicketWorkflowStep(DBEnumeratedType):
 
@@ -52,6 +58,9 @@
     FAILED = Item(999, "Failed")
     COMPLETED = Item(1000, "Completed")
 
+    def __str__(self):
+        return str(self.value)
+
 
 class TicketWorkflowStepStatus(DBEnumeratedType):
 
@@ -74,6 +83,9 @@
     PKG_PUBLISHING_FAILED = Item(540, "Package Publishing Failed")
     COMPLETED = Item(1000, "Completed")
 
+    def __str__(self):
+        return str(self.value)
+
 
 def _choices(enum):
     return [

=== modified file 'ticket_system/ticket/tests/test_write_api.py'
--- ticket_system/ticket/tests/test_write_api.py	2014-01-24 17:44:14 +0000
+++ ticket_system/ticket/tests/test_write_api.py	2014-01-24 20:49:45 +0000
@@ -21,6 +21,11 @@
                            SubTicketWorkflowStep, SubTicketWorkflowStepStatus,
                            Ticket, TicketWorkflowStep, SubTicketArtifact,
                            TicketWorkflowStepStatus)
+from project.models import BinaryPackage
+
+
+def create_ticket(added=None, removed=None):
+    return mommy.make('Ticket', removed_binaries=removed, added_binaries=added)
 
 
 sourcepackage_recipe = Recipe(SourcePackage, name=seq('foobar'))
@@ -30,7 +35,7 @@
 
     def setUp(self):
         super(APICreateTicketResourceTest, self).setUp('/api/v1')
-        self.ticket = mommy.make('Ticket')
+        self.ticket = create_ticket()
         self.resource = 'ticket/'
         self.detail_url = self.resource + '{0}/'.format(self.ticket.pk)
         self.post_ticket_data = {
@@ -264,21 +269,38 @@
 
     def setUp(self):
         super(APIUpdateTicketStatuses, self).setUp('/api/v1')
-        self.ticket = mommy.make('Ticket')
+        mommy.make('BinaryPackage', name='binary3')
+        mommy.make('BinaryPackage', name='binary4')
+        mommy.make('BinaryPackage', name='binary5')
+        self.ticket = create_ticket(added='binary1,binary2,binary6',
+                                    removed='binary3,binary4')
         self.ticket_detail_url = 'updateticketstatus/{0}/'.format(
             self.ticket.pk)
 
     def test_patch_ticket_status(self):
         new_data = {
-            'current_workflow_step': int(TicketWorkflowStep.IMAGE_BUILDING),
-            'status': int(TicketWorkflowStepStatus.IMAGE_BUILDING_COMPLETED),
+            'current_workflow_step': str(
+                TicketWorkflowStep.IMAGE_BUILDING.value),
+            'status': str(
+                TicketWorkflowStepStatus.IMAGE_BUILDING_COMPLETED.value),
         }
         self.assertEqual(Ticket.objects.count(), 1)
-        self.patch('/api/v1/' + self.ticket_detail_url,
-                   new_data)
+        self.assertEqual(BinaryPackage.objects.count(), 3)
+        binaries = BinaryPackage.objects.all()
+        self.assertQuerysetEqual(binaries,
+                                 ["<BinaryPackage: binary3>",
+                                  "<BinaryPackage: binary4>",
+                                  "<BinaryPackage: binary5>"], ordered=False)
+        self.patch('/api/v1/' + self.ticket_detail_url, new_data)
         # Make sure the count hasn't changed & we did an update.
         self.assertEqual(Ticket.objects.count(), 1)
         # Check for updated data.
+        self.assertEqual(BinaryPackage.objects.count(), 3)
+        binaries = BinaryPackage.objects.all()
+        self.assertQuerysetEqual(binaries,
+                                 ["<BinaryPackage: binary3>",
+                                  "<BinaryPackage: binary4>",
+                                  "<BinaryPackage: binary5>"], ordered=False)
         self.assertEqual(Ticket.objects.get(
             pk=self.ticket.pk).current_workflow_step,
             int(TicketWorkflowStep.IMAGE_BUILDING))
@@ -286,6 +308,128 @@
             Ticket.objects.get(pk=self.ticket.pk).status,
             int(TicketWorkflowStepStatus.IMAGE_BUILDING_COMPLETED))
 
+    def test_update_ticket_status_complete(self):
+        new_data = {
+            "current_workflow_step": str(TicketWorkflowStep.COMPLETED.value),
+            "status": str(TicketWorkflowStepStatus.COMPLETED.value),
+        }
+        self.assertEqual(Ticket.objects.count(), 1)
+        self.assertEqual(BinaryPackage.objects.count(), 3)
+        binaries = BinaryPackage.objects.all()
+        self.assertQuerysetEqual(binaries,
+                                 ["<BinaryPackage: binary3>",
+                                  "<BinaryPackage: binary4>",
+                                  "<BinaryPackage: binary5>"], ordered=False)
+        self.assertEqual(Ticket.objects.get(
+            pk=self.ticket.pk).current_workflow_step,
+            self.ticket.current_workflow_step)
+        self.assertEqual(Ticket.objects.get(
+            pk=self.ticket.pk).status,
+            self.ticket.status)
+        self.patch('/api/v1/' + self.ticket_detail_url, new_data)
+        self.assertEqual(Ticket.objects.count(), 1)
+        self.assertEqual(BinaryPackage.objects.count(), 4)
+        binaries = BinaryPackage.objects.all()
+        self.assertQuerysetEqual(binaries,
+                                 ["<BinaryPackage: binary1>",
+                                  "<BinaryPackage: binary2>",
+                                  "<BinaryPackage: binary5>",
+                                  "<BinaryPackage: binary6>"], ordered=False)
+        self.assertEqual(Ticket.objects.get(
+            pk=self.ticket.pk).current_workflow_step,
+            int(TicketWorkflowStep.COMPLETED))
+        self.assertEqual(Ticket.objects.get(
+            pk=self.ticket.pk).status,
+            int(TicketWorkflowStepStatus.COMPLETED))
+
+    def test_try_adding_binary_already_in_list(self):
+        """
+        Test what happens when a ticket attempts to add a binary to the master
+        list that is already in the master list
+        """
+        new_data = {
+            "current_workflow_step": str(TicketWorkflowStep.COMPLETED.value),
+            "status": str(TicketWorkflowStepStatus.COMPLETED.value),
+        }
+        self.ticket_2 = create_ticket(added='binary1,binary2,binary3')
+        self.assertEqual(Ticket.objects.count(), 2)
+        self.assertEqual(BinaryPackage.objects.count(), 3)
+        binaries = BinaryPackage.objects.all()
+        self.assertQuerysetEqual(binaries,
+                                 ["<BinaryPackage: binary3>",
+                                  "<BinaryPackage: binary4>",
+                                  "<BinaryPackage: binary5>"], ordered=False)
+        self.client.patch('/api/v1/updateticketstatus/{0}/'.format(
+                          self.ticket_2.pk), data=new_data)
+        self.assertEqual(BinaryPackage.objects.count(), 5)
+        binaries = BinaryPackage.objects.all()
+        self.assertQuerysetEqual(binaries,
+                                 ["<BinaryPackage: binary1>",
+                                  "<BinaryPackage: binary2>",
+                                  "<BinaryPackage: binary3>",
+                                  "<BinaryPackage: binary4>",
+                                  "<BinaryPackage: binary5>"], ordered=False)
+
+    def test_try_removing_binary_not_in_list(self):
+        """
+        Test what happens when a ticket attempts to remove a binary from the
+        master list when it isn't in the master list
+        """
+        new_data = {
+            "current_workflow_step": str(TicketWorkflowStep.COMPLETED.value),
+            "status": str(TicketWorkflowStepStatus.COMPLETED.value),
+        }
+        self.ticket_2 = create_ticket(removed='binary2,binary3')
+        self.assertEqual(Ticket.objects.count(), 2)
+        self.assertEqual(BinaryPackage.objects.count(), 3)
+        binaries = BinaryPackage.objects.all()
+        self.assertQuerysetEqual(binaries,
+                                 ["<BinaryPackage: binary3>",
+                                  "<BinaryPackage: binary4>",
+                                  "<BinaryPackage: binary5>"], ordered=False)
+        self.client.patch('/api/v1/updateticketstatus/{0}/'.format(
+                          self.ticket_2.pk), data=new_data)
+        binaries = BinaryPackage.objects.all()
+        self.assertEqual(BinaryPackage.objects.count(), 2)
+        self.assertQuerysetEqual(binaries,
+                                 ["<BinaryPackage: binary4>",
+                                  "<BinaryPackage: binary5>"], ordered=False)
+
+    def test_update_ticket_status_complete_404(self):
+        """
+        Test trying to complete the status of a ticket that doesn't exist
+        """
+        new_data = {
+            "current_workflow_step": str(TicketWorkflowStep.COMPLETED.value),
+            "status": str(TicketWorkflowStepStatus.COMPLETED.value),
+        }
+        self.assertEqual(Ticket.objects.count(), 1)
+        self.assertEqual(BinaryPackage.objects.count(), 3)
+        binaries = BinaryPackage.objects.all()
+        self.assertQuerysetEqual(binaries,
+                                 ["<BinaryPackage: binary3>",
+                                  "<BinaryPackage: binary4>",
+                                  "<BinaryPackage: binary5>"], ordered=False)
+        self.assertEqual(Ticket.objects.get(
+            pk=self.ticket.pk).current_workflow_step,
+            self.ticket.current_workflow_step)
+        self.assertEqual(Ticket.objects.get(
+            pk=self.ticket.pk).status,
+            self.ticket.status)
+        resp = self.client.patch('updateticketstatus/17394/', data=new_data)
+        self.assertEqual(Ticket.objects.count(), 1)
+        self.assertEqual(BinaryPackage.objects.count(), 3)
+        binaries = BinaryPackage.objects.all()
+        self.assertQuerysetEqual(binaries,
+                                 ["<BinaryPackage: binary3>",
+                                  "<BinaryPackage: binary4>",
+                                  "<BinaryPackage: binary5>"], ordered=False)
+        self.assertEqual(resp.status_code, 404)
+
+    def test_delete_ticket_not_allowed(self):
+        resp = self.delete(resource=self.ticket_detail_url)
+        self.assertHttpMethodNotAllowed(resp)
+
 
 class APIUpdateSubTicketStatuses(TastypieTestCase):
 
@@ -316,3 +460,7 @@
                          int(SubTicketWorkflowStep.PKG_BUILDING))
         self.assertEqual(SubTicket.objects.get(pk=self.subticket.pk).status,
                          int(SubTicketWorkflowStepStatus.PKG_BUILDING_WAITING))
+
+    def test_delete_subticket_not_allowed(self):
+        resp = self.delete(resource=self.subticket_detail_url)
+        self.assertHttpMethodNotAllowed(resp)