Bug 1309227 - Improve form error handling and other UX things

atmo/clusters/forms.py

@@ -1,44 +1,21 @@
 from django import forms
 
 from . import models
+from ..utils.forms import CreatedByFormMixin
 from ..utils.fields import PublicKeyFileField
 
 
-class ClusterFormMixin(object):
-
-    def __init__(self, user, *args, **kwargs):
-        self.created_by = user
-        super(ClusterFormMixin, self).__init__(*args, **kwargs)
-
-    def clean(self):
-        """
-        only allow deleting clusters that one created
-        """
-        super(ClusterFormMixin, self).clean()
-        if self.instance.id and self.created_by != self.instance.created_by:
-            raise forms.ValidationError(
-                "Access denied to a cluster of another user"
-            )
-
-
-class NewClusterForm(ClusterFormMixin, forms.ModelForm):
+class NewClusterForm(CreatedByFormMixin, forms.ModelForm):
 
     identifier = forms.RegexField(
         label="Cluster identifier",
         required=True,
         regex="^[\w-]{1,100}$",
         widget=forms.TextInput(attrs={
             'class': 'form-control',
-            'pattern': r'[\w-]+',
-            'data-toggle': 'popover',
-            'data-trigger': 'focus',
-            'data-placement': 'top',
-            'data-container': 'body',
-            'data-content': 'A brief description of the cluster\'s purpose, '
-                            'visible in the AWS management console.',
-            'data-validation-pattern-message': 'Valid cluster names are strings of alphanumeric '
-                                               'characters, \'_\', and \'-\'.',
-        })
+        }),
+        help_text='A brief description of the cluster\'s purpose, '
+                  'visible in the AWS management console.',
     )
     size = forms.IntegerField(
         label="Cluster size",
@@ -49,23 +26,27 @@ class NewClusterForm(ClusterFormMixin, forms.ModelForm):
             'required': 'required',
             'min': '1',
             'max': '20',
-            'data-toggle': 'popover',
-            'data-trigger': 'focus',
-            'data-placement': 'top',
-            'data-container': 'body',
-            'data-content': 'Number of workers to use in the cluster '
-                            '(1 is recommended for testing or development).',
-        })
+        }),
+        help_text='Number of workers to use in the cluster '
+                  '(1 is recommended for testing or development).'
     )
     public_key = PublicKeyFileField(
         label="Public SSH key",
         required=True,
-        widget=forms.FileInput(attrs={'required': 'required'}),
+        widget=forms.FileInput(attrs={'class': 'form-control', 'required': 'required'}),
         help_text="""\
 Upload your SSH <strong>public key</strong>, not private key!
 This will generally be found in places like <code>~/.ssh/id_rsa.pub</code>.
 """
     )
+    emr_release = forms.ChoiceField(
+        choices=models.Cluster.EMR_RELEASES_CHOICES,
+        widget=forms.Select(
+            attrs={'class': 'form-control', 'required': 'required'}
+        ),
+        label='EMR release version',
+        initial=models.Cluster.EMR_RELEASES_CHOICES_DEFAULT,
+    )
 
     def save(self, commit=False):
         # create the model without committing, since we haven't
@@ -84,22 +65,16 @@ class Meta:
         fields = ['identifier', 'size', 'public_key', 'emr_release']
 
 
-class EditClusterForm(ClusterFormMixin, forms.ModelForm):
+class EditClusterForm(CreatedByFormMixin, forms.ModelForm):
     identifier = forms.RegexField(
         required=True,
-        regex="^[\w-]{1,100}$",
+        regex=r'^[\w-]{1,100}$',
         widget=forms.TextInput(attrs={
             'class': 'form-control',
             'pattern': r'[\w-]+',
-            'data-toggle': 'popover',
-            'data-trigger': 'focus',
-            'data-placement': 'top',
-            'data-container': 'body',
-            'data-content': 'A brief description of the cluster\'s purpose, '
-                            'visible in the AWS management console.',
-            'data-validation-pattern-message': 'Valid cluster names are strings of alphanumeric '
-                                               'characters, \'_\', and \'-\'.',
-        })
+        }),
+        help_text='A brief description of the cluster\'s purpose, '
+                  'visible in the AWS management console.',
     )
 
     def save(self, commit=True):
@@ -115,11 +90,11 @@ class Meta:
         fields = ['identifier']
 
 
-class TerminateClusterForm(ClusterFormMixin, forms.ModelForm):
+class TerminateClusterForm(CreatedByFormMixin, forms.ModelForm):
     confirmation = forms.RegexField(
         required=True,
         label='Confirm termination with cluster identifier',
-        regex="^[\w-]{1,100}$",
+        regex=r'^[\w-]{1,100}$',
         widget=forms.TextInput(attrs={
             'class': 'form-control',
         }),

atmo/clusters/jobs.py

@@ -5,8 +5,8 @@
 from django.utils import timezone
 import newrelic.agent
 
-from atmo.clusters.models import Cluster
-from atmo.utils import email
+from .models import Cluster
+from ..utils import email
 
 
 @newrelic.agent.background_task(group='RQ')

atmo/clusters/models.py

@@ -8,12 +8,15 @@
 from ..utils import provisioning
 
 
-# Default release is the last item.
-EMR_RELEASES = ('5.0.0', '4.5.0')
-
-
 class Cluster(models.Model):
     FINAL_STATUS_LIST = ('COMPLETED', 'TERMINATED', 'FAILED')
+    # Default release is the first item, order should be from latest to oldest
+    EMR_RELEASES = (
+        '5.0.0',
+        '4.5.0',
+    )
+    EMR_RELEASES_CHOICES = list(zip(*(EMR_RELEASES,) * 2))
+    EMR_RELEASES_CHOICES_DEFAULT = EMR_RELEASES[0]
 
     identifier = models.CharField(
         max_length=100,
@@ -45,7 +48,7 @@ class Cluster(models.Model):
     )
 
     emr_release = models.CharField(
-        max_length=50, choices=list(zip(*(EMR_RELEASES,) * 2)), default=EMR_RELEASES[-1],
+        max_length=50, choices=EMR_RELEASES_CHOICES, default=EMR_RELEASES_CHOICES_DEFAULT,
         help_text=('Different EMR versions have different versions '
                    'of software like Hadoop, Spark, etc')
     )

atmo/clusters/tests.py

@@ -27,7 +27,7 @@ def setUp(self, cluster_info, cluster_start):
                 'new-identifier': 'test-cluster',
                 'new-size': 5,
                 'new-public_key': io.BytesIO('ssh-rsa AAAAB3'),
-                'new-emr_release': models.EMR_RELEASES[-1]
+                'new-emr_release': models.Cluster.EMR_RELEASES_CHOICES_DEFAULT
             }, follow=True)
         self.cluster_start = cluster_start
         self.cluster = models.Cluster.objects.get(jobflow_id=u'12345')
@@ -44,7 +44,7 @@ def test_that_cluster_is_correctly_provisioned(self):
         self.assertEqual(identifier, 'test-cluster')
         self.assertEqual(size, 5)
         self.assertEqual(public_key, 'ssh-rsa AAAAB3')
-        self.assertEqual(emr_release, models.EMR_RELEASES[-1])
+        self.assertEqual(emr_release, models.Cluster.EMR_RELEASES_CHOICES_DEFAULT)
 
     def test_that_the_model_was_created_correctly(self):
         cluster = models.Cluster.objects.get(jobflow_id=u'12345')
@@ -56,7 +56,7 @@ def test_that_the_model_was_created_correctly(self):
             self.start_date <= cluster.start_date <= self.start_date + timedelta(seconds=10)
         )
         self.assertEqual(cluster.created_by, self.test_user)
-        self.assertEqual(cluster.emr_release, models.EMR_RELEASES[-1])
+        self.assertEqual(cluster.emr_release, models.Cluster.EMR_RELEASES_CHOICES_DEFAULT)
         self.assertTrue(User.objects.filter(username='john.smith').exists())
 
     @mock.patch('atmo.utils.provisioning.cluster_start', return_value=u'67890')
@@ -72,7 +72,7 @@ def test_empty_public_dns(self, cluster_info, cluster_start):
                 'new-identifier': 'test-cluster',
                 'new-size': 5,
                 'new-public_key': io.BytesIO('ssh-rsa AAAAB3'),
-                'new-emr_release': models.EMR_RELEASES[-1]
+                'new-emr_release': models.Cluster.EMR_RELEASES_CHOICES_DEFAULT
             }, follow=True)
         self.assertEqual(cluster_start.call_count, 1)
         cluster = models.Cluster.objects.get(jobflow_id=u'67890')