Merge lp:~mhall119/awstrial/fixes-873066 into lp:awstrial

[Michael Hall]

Get instance by user *and* campaign, not just by user, so that we can run multiple campaigns

Overview
========
Trying to start a new instance on a new campaign fails if the user has an old instance on an old campaign

Details
=======
Because the view was not filtering Instances by user *and* campaign, it would raise a MultipleObjectsReturned exception when the user had an instance on more than one campaign.

[Māris Fogels (mars)]

Nice fix. Complex test setup, but that's boto for you. A few questions:

In your test, do you need to call the first campaign before calling the second campaign? I can't tell if the first campaign call is part of the test setup, action, or verification.

Do you need to check the number of campaigns twice, once in the test setup, then at the end of the test?

Maybe not part of this review, but what happens if you set both campaigns to active?

[Michael Hall (mhall119)]

Calling call_run_instance on the first campaign is part of the setup, since we are testing what happens when a user tries to start a second instance on a different campaign.

Checking the number in the test setup is just to let us know that the setup was done properly. If there was some kind of test isolation problem, this would let us know before we got to the assertions that the test was made to check.

Whether a campaign is active or not doesn't matter, it was just written this way to directly imitate the problem we encountered.

[Matthew Nuzum (newz)]

review: Approve

[ISD Branch Mangler (isd-branches-mangler)]

Attempt to merge into lp:awstrial failed due to conflicts:

text conflict in awstrial/trial/tests.py

=== modified file 'awstrial/trial/tests.py'
--- awstrial/trial/tests.py	2011-09-29 13:50:26 +0000
+++ awstrial/trial/tests.py	2011-10-12 20:58:24 +0000
@@ -104,6 +104,14 @@
         settings.AWS_SECRET_ACCESS_KEY = self.old_secret_key
         settings.ALTERNATE_CLOUD = self.old_alternate_cloud
 
+    def call_run_instance(self, campaign):
+        post_data = {
+            'config': 'default',
+            'a': 1,
+            'launchbtn': 'Launch',
+        }
+        return self.client.post('/%s/run/' % campaign.name, post_data)
+
     @patch('boto.ec2.EC2Connection')
     def test_disallow_multiple_instances(self, mock_connection):
         """
@@ -123,11 +131,18 @@
             def __init__(self, instance_ids):
                 self.instances = [MockInstance(i) for i in instance_ids]
         
+        campaign = Campaign.objects.create(
+            name='test-campaign',
+            verbose_name='Test Campaign',
+            max_instances=5,
+            active=True,
+        )
+        
         def mock_run_instances(conn, *args, **kwargs):
             self.call_count += 1
             if self.call_count == 1:
                 # call again before returning to create the race condition
-                response = call_run_instance()
+                response = self.call_run_instance(campaign)
 
                 args, kargs = mock_connection.call_args
                 self.assertEquals('TestAccessKey', kargs['aws_access_key_id'])
@@ -138,23 +153,70 @@
                 return MockReservation(['i-test-two'])
         mock_connection.return_value.run_instances.side_effect = mock_run_instances
         
-        def call_run_instance():
-            post_data = {
-                'config': 'default',
-                'a': 1,
-                'launchbtn': 'Launch',
-            }
-            return self.client.post('/test-campaign/run/', post_data)
-            
-        campaign = Campaign.objects.create(
-            name='test-campaign',
-            verbose_name='Test Campaign',
+        user = User.objects.create_user('testuser', '[e-mail]', 'testpasswd')
+        self.client.login(username='testuser', password='testpasswd')
+        
+        response = self.call_run_instance(campaign)
+        self.assertEquals(1, Instances.objects.all().count())
+
+    @patch('boto.ec2.EC2Connection')
+    def test_allow_multiple_instances_on_multiple_campaigns(self, mock_connection):
+        """
+        Checks that a user can create multiple instances as long as they are on
+        different campaigns
+        """
+
+        settings.ALTERNATE_CLOUD = None
+        self.call_count=0
+        class MockInstance(object):
+        
+            def __init__(self, instance_id):
+                self.id = instance_id
+                
+        class MockReservation(object):
+        
+            def __init__(self, instance_ids):
+                self.instances = [MockInstance(i) for i in instance_ids]
+        
+        def mock_run_instances(conn, *args, **kwargs):
+            self.call_count += 1
+            if self.call_count == 1:
+                return MockReservation(['i-test-one'])
+            else:
+                return MockReservation(['i-test-two'])
+        mock_connection.return_value.run_instances.side_effect = mock_run_instances
+        
+        campaign1 = Campaign.objects.create(
+            name='test-campaign-1',
+            verbose_name='Test Campaign 1',
             max_instances=5,
             active=True,
         )
+        campaign2 = Campaign.objects.create(
+            name='test-campaign-2',
+            verbose_name='Test Campaign-2',
+            max_instances=5,
+            active=False,
+        )
         
         user = User.objects.create_user('testuser', '[e-mail]', 'testpasswd')
         self.client.login(username='testuser', password='testpasswd')
         
-        response = call_run_instance()
+        response = self.call_run_instance(campaign1)
+        self.assertRedirects(response, '/test-campaign-1/instance_info/')
+        response = self.client.get('/test-campaign-1/instance_info/')
+        self.assertEquals(200, response.status_code)
         self.assertEquals(1, Instances.objects.all().count())
+
+        campaign1.active=False
+        campaign1.save()
+        
+        campaign2.active=True
+        campaign2.save()
+        
+        response = self.call_run_instance(campaign2)
+        self.assertRedirects(response, '/test-campaign-2/instance_info/')
+        response = self.client.get('/test-campaign-2/instance_info/')
+        self.assertEquals(200, response.status_code)
+        self.assertEquals(2, Instances.objects.all().count())
+        

=== modified file 'awstrial/trial/views.py'
--- awstrial/trial/views.py	2011-09-13 20:47:35 +0000
+++ awstrial/trial/views.py	2011-10-12 20:58:24 +0000
@@ -127,7 +127,7 @@
 @safe_user_required
 def info_instance(request,campaign):
     """ Return details about Instance """
-    instance = Instances.objects.get(owner=request.user)
+    instance = Instances.objects.get(owner=request.user, campaign__name=campaign)
     instcfg = util.find_config(instance.config,settings.CONFIGS)
     context = {
         'instance': instance,