don't modify the data dict param

KP · KP · commit 9c79f668ef74 · 2012-09-18T01:42:50.000-05:00
always return thumbnail url when uploaded instead of id.

data parameter shouldn't be modified since it's a reference to the dict passed in from the client. A deep copy isn't necessary so just use copy().

thumbnail ids are pretty useless. urls are more standard so let's just return those. Since we're providing a thumbnail in create()/update() return data for them automatically to match the behavior of other fields.
diff --git a/shotgun_api3/shotgun.py b/shotgun_api3/shotgun.py
@@ -536,6 +536,7 @@ def create(self, entity_type, data, return_fields=None):
         :returns: dict of the requested fields.
         """
 
+        data = data.copy()
         if not return_fields:
             return_fields = ["id"]
 
@@ -562,22 +563,15 @@ def create(self, entity_type, data, return_fields=None):
         if upload_image:
             image_id = self.upload_thumbnail(entity_type, result['id'],
                                              upload_image)
-            result['image_id'] = image_id
-        # end if
-
-        if "image" in return_fields:
             image = self.find_one(entity_type, [['id', 'is', result.get('id')]],
                                   fields=['image'])
             result['image'] = image.get('image')
 
         if upload_filmstrip_image:
             filmstrip_id = self.upload_filmstrip_thumbnail(entity_type, result['id'], upload_filmstrip_image)
-            result['filmstrip_image_id'] = filmstrip_id
-
-        if "filmstrip_image" in return_fields:
             filmstrip = self.find_one(entity_type,
-                                      [['id', 'is', result.get('id')]],
-                                      fields=['filmstrip_image'])
+                                     [['id', 'is', result.get('id')]],
+                                     fields=['filmstrip_image'])
             result['filmstrip_image'] = filmstrip.get('filmstrip_image')
 
         return result
@@ -595,10 +589,10 @@ def update(self, entity_type, entity_id, data):
         id added.
         """
 
+        data = data.copy()
         upload_image = None
         if 'image' in data and data['image'] is not None:
             upload_image = data.pop('image')
-
         upload_filmstrip_image = None
         if 'filmstrip_image' in data:
             if not self.server_caps.version or self.server_caps.version < (3, 1, 0):
@@ -620,14 +614,12 @@ def update(self, entity_type, entity_id, data):
         if upload_image:
             image_id = self.upload_thumbnail(entity_type, entity_id,
                                              upload_image)
-            result['image_id'] = image_id
             image = self.find_one(entity_type, [['id', 'is', result.get('id')]],
                                   fields=['image'])
             result['image'] = image.get('image')
 
         if upload_filmstrip_image:
             filmstrip_id = self.upload_filmstrip_thumbnail(entity_type, result['id'], upload_filmstrip_image)
-            result['filmstrip_image_id'] = filmstrip_id
             filmstrip = self.find_one(entity_type,
                                      [['id', 'is', result.get('id')]],
                                      fields=['filmstrip_image'])
diff --git a/tests/test_api.py b/tests/test_api.py
@@ -204,7 +204,6 @@ def test_upload_thumbnail_in_create(self):
         self.assertTrue(isinstance(new_version.get('id'), int))
         self.assertEqual(new_version.get('type'), 'Version')
         self.assertEqual(new_version.get('project'), self.project)
-        self.assertTrue(isinstance(new_version.get('image_id'), int))
         self.assertTrue(new_version.get('image') is not None)
         self.assertTrue( re.match("http:\/\/%s\/files\/0000\/0000\/\d{4}/232/sg_logo.jpg.jpg" % (self.server_address), new_version.get('image')) )
 
@@ -217,9 +216,7 @@ def test_upload_thumbnail_in_create(self):
         self.assertTrue(isinstance(new_version.get('id'), int))
         self.assertEqual(new_version.get('type'), 'Version')
         self.assertEqual(new_version.get('project'), self.project)
-        self.assertTrue(isinstance(new_version.get('filmstrip_image_id'), int))
         self.assertTrue(new_version.get('filmstrip_image') is not None)
-        # http://trunk.shotgun.vm/files/0000/0000/0170/sg_logo.jpg
         self.assertTrue( re.match("http:\/\/%s\/files\/0000\/0000\/\d{4}/sg_logo.jpg" % (self.server_address), new_version.get('filmstrip_image')) )
     # end test_upload_thumbnail_in_create
 
