Improved security group rule editing.

Splits rule editing and rule creation out so that
rather than being on one modal form (which is dismissed
after taking any action on the rules) they are instead
contained in their own security group detail view, with
create/delete as their own discrete forms/actions which
return to that same view.

This also reworks the form to be more explicit and
user-friendly in terms of the various options provided,
making it more responsive, and making it better documented.

Incidentally fixes some problems in the documentation.

Implements blueprint security-group-rules.

Change-Id: I866dd4fe0c74148140422aab9172be4f496689a9

doc/source/ref/forms.rst

@@ -2,10 +2,97 @@
 Horizon Forms
 =============
 
-Horizon ships with a number of generic form classes.
+Horizon ships with some very useful base form classes, form fields,
+class-based views, and javascript helpers which streamline most of the common
+tasks related to form handling.
 
-Generic Forms
+Form Classes
-=============
+============
 
-.. automodule:: horizon.forms
+.. automodule:: horizon.forms.base
     :members:
+
+Form Fields
+===========
+
+.. automodule:: horizon.forms.fields
+    :members:
+
+Form Views
+==========
+
+.. automodule:: horizon.forms.views
+    :members:
+
+Forms Javascript
+================
+
+Switchable Fields
+-----------------
+
+By marking fields with the ``"switchable"`` and ``"switched"`` classes along
+with defining a few data attributes you can programmatically hide, show,
+and rename fields in a form.
+
+The triggers are fields using a ``select`` input widget, marked with the
+"switchable" class, and defining a "data-slug" attribute. When they are changed,
+any input with the ``"switched"`` class and defining a ``"data-switch-on"``
+attribute which matches the ``select`` input's ``"data-slug"`` attribute will be
+evaluated for necessary changes. In simpler terms, if the ``"switched"`` target
+input's ``"switch-on"`` matches the ``"slug"`` of the ``"switchable"`` trigger
+input, it gets switched. Simple, right?
+
+The ``"switched"`` inputs also need to define states. For each state in which
+the input should be shown, it should define a data attribute like the
+following: ``data-<slug>-<value>="<desired label>"``. When the switch event
+happens the value of the ``"switchable"`` field will be compared to the
+data attributes and the correct label will be applied to the field. If
+a corresponding label for that value is *not* found, the field will
+be hidden instead.
+
+A simplified example is as follows::
+
+    source = forms.ChoiceField(
+        label=_('Source'),
+        choices=[
+            ('cidr', _('CIDR')),
+            ('sg', _('Security Group'))
+        ],
+        widget=forms.Select(attrs={
+            'class': 'switchable',
+            'data-slug': 'source'
+        })
+    )
+
+    cidr = fields.IPField(
+        label=_("CIDR"),
+        required=False,
+        widget=forms.TextInput(attrs={
+            'class': 'switched',
+            'data-switch-on': 'source',
+            'data-source-cidr': _('CIDR')
+        })
+    )
+
+    security_group = forms.ChoiceField(
+        label=_('Security Group'),
+        required=False,
+        widget=forms.Select(attrs={
+            'class': 'switched',
+            'data-switch-on': 'source',
+            'data-source-sg': _('Security Group')
+        })
+    )
+
+That code would create the ``"switchable"`` control field ``source``, and the
+two ``"switched"`` fields ``cidr`` and ``security group`` which are hidden or
+shown depending on the value of ``source``.
+
+
+NOTE: A field can only safely define one slug in its ``"switch-on"`` attribute.
+While switching on multiple fields is possible, the behavior is very hard to
+predict due to the events being fired from the various switchable fields in
+order. You generally end up just having it hidden most of the time by accident,
+so it's not recommended. Instead just add a second field to the form and control
+the two independently, then merge their results in the form's clean or handle
+methods at the end.

horizon/forms/views.py

@@ -49,10 +49,35 @@ class ModalFormMixin(object):
 
 
 class ModalFormView(ModalFormMixin, generic.FormView):
+    """
+    The main view class from which all views which handle forms in Horizon
+    should inherit. It takes care of all details with processing
+    :class:`~horizon.forms.base.SelfHandlingForm` classes, and modal concerns
+    when the associated template inherits from
+    `horizon/common/_modal_form.html`.
+
+    Subclasses must define a ``form_class`` and ``template_name`` attribute
+    at minimum.
+
+    See Django's documentation on the `FormView <https://docs.djangoproject.com
+    /en/dev/ref/class-based-views/generic-editing/#formview>`_ class for
+    more details.
+    """
+
     def get_object_id(self, obj):
+        """
+        For dynamic insertion of resources created in modals, this method
+        returns the id of the created object. Defaults to returning the ``id``
+        attribute.
+        """
         return obj.id
 
     def get_object_display(self, obj):
+        """
+        For dynamic insertion of resources created in modals, this method
+        returns the display name of the created object. Defaults to returning
+        the ``name`` attribute.
+        """
         return obj.name
 
     def get_form(self, form_class):

horizon/static/horizon/js/horizon.forms.js

@@ -1,16 +1,5 @@
 /* Namespace for core functionality related to Forms. */
 horizon.forms = {
-  handle_source_group: function() {
-    $("div.table_wrapper, #modal_wrapper").on("change", "#id_source_group", function (evt) {
-      var $sourceGroup = $('#id_source_group'),
-          $cidrContainer = $('#id_cidr').closest(".control-group");
-      if($sourceGroup.val() === "") {
-        $cidrContainer.removeClass("hide");
-      } else {
-        $cidrContainer.addClass("hide");
-      }
-    });
-  },
   handle_snapshot_source: function() {
     $("div.table_wrapper, #modal_wrapper").on("change", "select#id_snapshot_source", function(evt) {
       var $option = $(this).find("option:selected");
@@ -77,7 +66,6 @@ horizon.addInitFunction(function () {
   horizon.forms.init_examples($("body"));
   horizon.modals.addModalInitFunction(horizon.forms.init_examples);
 
-  horizon.forms.handle_source_group();
   horizon.forms.handle_snapshot_source();
 
   // Bind event handlers to confirm dangerous actions.
@@ -86,25 +74,36 @@ horizon.addInitFunction(function () {
     evt.preventDefault();
   });
 
-  /* Switchable fields */
+  /* Switchable Fields (See Horizon's Forms docs for more information) */
 
   // Bind handler for swapping labels on "switchable" fields.
   $(document).on("change", 'select.switchable', function (evt) {
-    var type = $(this).val();
+    var $fieldset = $(evt.target).closest('fieldset'),
-    $(this).closest('fieldset').find('input[type=text]').each(function(index, obj){
+        $switchables = $fieldset.find('.switchable');
-      var label_val = "";
+
-      if ($(obj).data(type)){
+    $switchables.each(function (index, switchable) {
-        label_val = $(obj).data(type);
+      var $switchable = $(switchable),
-      } else if ($(obj).attr("data")){
+          slug = $switchable.data('slug'),
-        label_val = $(obj).attr("data");
+          visible = $switchable.is(':visible'),
-      } else
+          val = $switchable.val();
-         return true;
+
-      $('label[for=' + $(obj).attr('id') + ']').html(label_val);
+      $fieldset.find('.switched[data-switch-on*="' + slug + '"]').each(function(index, input){
+        var $input = $(input),
+            data = $input.data(slug + "-" + val);
+
+        if (typeof data === "undefined" || !visible) {
+          $input.closest('.form-field').hide();
+        } else {
+          $('label[for=' + $input.attr('id') + ']').html(data);
+          $input.closest('.form-field').show();
+        }
+      });
     });
   });
+
   // Fire off the change event to trigger the proper initial values.
   $('select.switchable').trigger('change');
-  // Queue up the even for use in new modals, too.
+  // Queue up the for new modals, too.
   horizon.modals.addModalInitFunction(function (modal) {
     $(modal).find('select.switchable').trigger('change');
   });

openstack_dashboard/api/network.py

@@ -14,11 +14,11 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-"""Abstraction layer of networking functionalities.
+"""Abstraction layer for networking functionalities.
 
-Now Nova and Quantum have duplicated features.
+Currently Nova and Quantum have duplicated features. This API layer is
-Thie API layer is introduced to hide the differences between them
+introduced to astract the differences between them for seamless consumption by
-from dashboard implementations.
+different dashboard implementations.
 """
 
 import abc
@@ -36,16 +36,17 @@ class NetworkClient(object):
 class FloatingIpManager(object):
     """Abstract class to implement Floating IP methods
 
-    FloatingIP object returned from methods in this class
+    The FloatingIP object returned from methods in this class
     must contains the following attributes:
-    - id : ID of Floating IP
+
-    - ip : Floating IP address
+    * id: ID of Floating IP
-    - pool : ID of Floating IP pool from which the address is allocated
+    * ip: Floating IP address
-    - fixed_ip : Fixed IP address of a VIF associated with the address
+    * pool: ID of Floating IP pool from which the address is allocated
-    - port_id : ID of a VIF associated with the address
+    * fixed_ip: Fixed IP address of a VIF associated with the address
+    * port_id: ID of a VIF associated with the address
                 (instance_id when Nova floating IP is used)
-    - instance_id : Instance ID of an associated with the Floating IP
+    * instance_id: Instance ID of an associated with the Floating IP
-"""
+    """
 
     __metaclass__ = abc.ABCMeta
 
@@ -53,7 +54,7 @@ class FloatingIpManager(object):
     def list_pools(self):
         """Fetches a list of all floating IP pools.
 
-        A list of FloatingIpPool object is returned.
+        A list of FloatingIpPool objects is returned.
         FloatingIpPool object is an APIResourceWrapper/APIDictWrapper
         where 'id' and 'name' attributes are defined.
         """

openstack_dashboard/dashboards/project/access_and_security/security_groups/forms.py

@@ -58,104 +58,173 @@ class CreateGroup(forms.SelfHandlingForm):
 
 
 class AddRule(forms.SelfHandlingForm):
+    id = forms.IntegerField(widget=forms.HiddenInput())
     ip_protocol = forms.ChoiceField(label=_('IP Protocol'),
-                                    choices=[('tcp', 'TCP'),
+                                    choices=[('tcp', _('TCP')),
-                                             ('udp', 'UDP'),
+                                             ('udp', _('UDP')),
-                                             ('icmp', 'ICMP')],
+                                             ('icmp', _('ICMP'))],
                                     help_text=_("The protocol which this "
                                                 "rule should be applied to."),
-                                    widget=forms.Select(attrs={'class':
+                                    widget=forms.Select(attrs={
-                                                               'switchable'}))
+                                            'class': 'switchable',
+                                            'data-slug': 'protocol'}))
+
+    port_or_range = forms.ChoiceField(label=_('Open'),
+                                      choices=[('port', _('Port')),
+                                               ('range', _('Port Range'))],
+                                      widget=forms.Select(attrs={
+                                            'class': 'switchable switched',
+                                            'data-slug': 'range',
+                                            'data-switch-on': 'protocol',
+                                            'data-protocol-tcp': _('Open'),
+                                            'data-protocol-udp': _('Open')}))
+
+    port = forms.IntegerField(label=_("Port"),
+                              required=False,
+                              help_text=_("Enter an integer value "
+                                          "between 1 and 65535."),
+                              widget=forms.TextInput(attrs={
+                                   'class': 'switched',
+                                   'data-switch-on': 'range',
+                                   'data-range-port': _('Port')}),
+                              validators=[validate_port_range])
+
     from_port = forms.IntegerField(label=_("From Port"),
-                                   help_text=_("TCP/UDP: Enter integer value "
+                                   required=False,
-                                               "between 1 and 65535. ICMP: "
+                                   help_text=_("Enter an integer value "
-                                               "enter a value for ICMP type "
+                                               "between 1 and 65535."),
-                                               "in the range (-1: 255)"),
+                                   widget=forms.TextInput(attrs={
-                                   widget=forms.TextInput(
+                                        'class': 'switched',
-                                          attrs={'data': _('From Port'),
+                                        'data-switch-on': 'range',
-                                                 'data-icmp': _('Type')}),
+                                        'data-range-range': _('From Port')}),
                                    validators=[validate_port_range])
+
     to_port = forms.IntegerField(label=_("To Port"),
-                                 help_text=_("TCP/UDP: Enter integer value "
+                                 required=False,
-                                             "between 1 and 65535. ICMP: "
+                                 help_text=_("Enter an integer value "
-                                             "enter a value for ICMP code "
+                                             "between 1 and 65535."),
-                                             "in the range (-1: 255)"),
+                                 widget=forms.TextInput(attrs={
-                                 widget=forms.TextInput(
+                                        'class': 'switched',
-                                        attrs={'data': _('To Port'),
+                                        'data-switch-on': 'range',
-                                               'data-icmp': _('Code')}),
+                                        'data-range-range': _('To Port')}),
                                  validators=[validate_port_range])
 
-    source_group = forms.ChoiceField(label=_('Source Group'),
+    icmp_type = forms.IntegerField(label=_("Type"),
-                                     required=False,
+                                   required=False,
-                                     help_text=_("To specify an allowed IP "
+                                   help_text=_("Enter a value for ICMP type "
-                                                 "range, select CIDR. To "
+                                               "in the range (-1: 255)"),
-                                                 "allow access from all "
+                                   widget=forms.TextInput(attrs={
-                                                 "members of another security "
+                                        'class': 'switched',
-                                                 "group select Source Group."))
+                                        'data-switch-on': 'protocol',
-    cidr = fields.IPField(label=_("CIDR"),
+                                        'data-protocol-icmp': _('Type')}),
-                           required=False,
+                                   validators=[validate_port_range])
-                           initial="0.0.0.0/0",
-                           help_text=_("Classless Inter-Domain Routing "
-                                       "(e.g. 192.168.0.0/24)"),
-                           version=fields.IPv4 | fields.IPv6,
-                           mask=True)
 
-    security_group_id = forms.IntegerField(widget=forms.HiddenInput())
+    icmp_code = forms.IntegerField(label=_("Code"),
+                                   required=False,
+                                   help_text=_("Enter a value for ICMP code "
+                                               "in the range (-1: 255)"),
+                                   widget=forms.TextInput(attrs={
+                                          'class': 'switched',
+                                          'data-switch-on': 'protocol',
+                                          'data-protocol-icmp': _('Code')}),
+                                   validators=[validate_port_range])
+
+    source = forms.ChoiceField(label=_('Source'),
+                               choices=[('cidr', _('CIDR')),
+                                        ('sg', _('Security Group'))],
+                               help_text=_('To specify an allowed IP '
+                                           'range, select "CIDR". To '
+                                           'allow access from all '
+                                           'members of another security '
+                                           'group select "Security '
+                                           'Group".'),
+                               widget=forms.Select(attrs={
+                                      'class': 'switchable',
+                                      'data-slug': 'source'}))
+
+    cidr = fields.IPField(label=_("CIDR"),
+                          required=False,
+                          initial="0.0.0.0/0",
+                          help_text=_("Classless Inter-Domain Routing "
+                                      "(e.g. 192.168.0.0/24)"),
+                          version=fields.IPv4 | fields.IPv6,
+                          mask=True,
+                          widget=forms.TextInput(
+                                attrs={'class': 'switched',
+                                       'data-switch-on': 'source',
+                                       'data-source-cidr': _('CIDR')}))
+
+    security_group = forms.ChoiceField(label=_('Security Group'),
+                                       required=False,
+                                       widget=forms.Select(attrs={
+                                          'class': 'switched',
+                                          'data-switch-on': 'source',
+                                          'data-source-sg': _('Security '
+                                                              'Group')}))
 
     def __init__(self, *args, **kwargs):
         sg_list = kwargs.pop('sg_list', [])
         super(AddRule, self).__init__(*args, **kwargs)
         # Determine if there are security groups available for the
         # source group option; add the choices and enable the option if so.
-        security_groups_choices = [("", "CIDR")]
         if sg_list:
-            security_groups_choices.append(('Security Group', sg_list))
+            security_groups_choices = sg_list
-        self.fields['source_group'].choices = security_groups_choices
+        else:
+            security_groups_choices = [("", _("No security groups available"))]
+        self.fields['security_group'].choices = security_groups_choices
 
     def clean(self):
         cleaned_data = super(AddRule, self).clean()
+
+        ip_proto = cleaned_data.get('ip_protocol')
+        port_or_range = cleaned_data.get("port_or_range")
+        source = cleaned_data.get("source")
+
+        icmp_type = cleaned_data.get("icmp_type", None)
+        icmp_code = cleaned_data.get("icmp_code", None)
+
         from_port = cleaned_data.get("from_port", None)
         to_port = cleaned_data.get("to_port", None)
-        cidr = cleaned_data.get("cidr", None)
+        port = cleaned_data.get("port", None)
-        ip_proto = cleaned_data.get('ip_protocol', None)
-        source_group = cleaned_data.get("source_group", None)
 
         if ip_proto == 'icmp':
-            if from_port is None:
+            if icmp_type is None:
                 msg = _('The ICMP type is invalid.')
                 raise ValidationError(msg)
-            if to_port is None:
+            if icmp_code is None:
                 msg = _('The ICMP code is invalid.')
                 raise ValidationError(msg)
-            if from_port not in xrange(-1, 256):
+            if icmp_type not in xrange(-1, 256):
                 msg = _('The ICMP type not in range (-1, 255)')
                 raise ValidationError(msg)
-            if to_port not in xrange(-1, 256):
+            if icmp_code not in xrange(-1, 256):
                 msg = _('The ICMP code not in range (-1, 255)')
                 raise ValidationError(msg)
+            cleaned_data['from_port'] = icmp_type
+            cleaned_data['to_port'] = icmp_code
         else:
-            if from_port is None:
+            if port_or_range == "port":
-                msg = _('The "from" port number is invalid.')
+                cleaned_data["from_port"] = port
-                raise ValidationError(msg)
+                cleaned_data["to_port"] = port
-            if to_port is None:
+                if port is None:
-                msg = _('The "to" port number is invalid.')
+                    msg = _('The specified port is invalid.')
-                raise ValidationError(msg)
+                    raise ValidationError(msg)
-            if to_port < from_port:
+            else:
-                msg = _('The "to" port number must be greater than '
+                if from_port is None:
-                        'or equal to the "from" port number.')
+                    msg = _('The "from" port number is invalid.')
-                raise ValidationError(msg)
+                    raise ValidationError(msg)
+                if to_port is None:
+                    msg = _('The "to" port number is invalid.')
+                    raise ValidationError(msg)
+                if to_port < from_port:
+                    msg = _('The "to" port number must be greater than '
+                            'or equal to the "from" port number.')
+                    raise ValidationError(msg)
 
-        if source_group and cidr != self.fields['cidr'].initial:
+        if source == "cidr":
-            # Specifying a source group *and* a custom CIDR is invalid.
+            cleaned_data['security_group'] = None
-            msg = _('Either CIDR or Source Group may be specified, '
-                    'but not both.')
-            raise ValidationError(msg)
-        elif source_group:
-            # If a source group is specified, clear the CIDR from its default
-            cleaned_data['cidr'] = None
         else:
-            # If only cidr is specified, clear the source_group entirely
+            cleaned_data['cidr'] = None
-            cleaned_data['source_group'] = None
 
         return cleaned_data
 
@@ -163,17 +232,18 @@ class AddRule(forms.SelfHandlingForm):
         try:
             rule = api.nova.security_group_rule_create(
                         request,
-                        data['security_group_id'],
+                        data['id'],
                         data['ip_protocol'],
                         data['from_port'],
                         data['to_port'],
                         data['cidr'],
-                        data['source_group'])
+                        data['security_group'])
             messages.success(request,
                              _('Successfully added rule: %s') % unicode(rule))
             return rule
         except:
-            redirect = reverse("horizon:project:access_and_security:index")
+            redirect = reverse("horizon:project:access_and_security:"
+                               "security_groups:detail", args=[data['id']])
             exceptions.handle(request,
                               _('Unable to add rule to security group.'),
                               redirect=redirect)

openstack_dashboard/dashboards/project/access_and_security/security_groups/tables.py

@@ -50,8 +50,8 @@ class CreateGroup(tables.LinkAction):
 class EditRules(tables.LinkAction):
     name = "edit_rules"
     verbose_name = _("Edit Rules")
-    url = "horizon:project:access_and_security:security_groups:edit_rules"
+    url = "horizon:project:access_and_security:security_groups:detail"
-    classes = ("ajax-modal", "btn-edit")
+    classes = ("btn-edit")
 
 
 class SecurityGroupsTable(tables.DataTable):
@@ -68,6 +68,16 @@ class SecurityGroupsTable(tables.DataTable):
         row_actions = (EditRules, DeleteGroup)
 
 
+class CreateRule(tables.LinkAction):
+    name = "add_rule"
+    verbose_name = _("Add Rule")
+    url = "horizon:project:access_and_security:security_groups:add_rule"
+    classes = ("ajax-modal", "btn-create")
+
+    def get_link_url(self):
+        return reverse(self.url, args=[self.table.kwargs['security_group_id']])
+
+
 class DeleteRule(tables.DeleteAction):
     data_type_singular = _("Rule")
     data_type_plural = _("Rules")
@@ -76,7 +86,9 @@ class DeleteRule(tables.DeleteAction):
         api.nova.security_group_rule_delete(request, obj_id)
 
     def get_success_url(self, request):
-        return reverse("horizon:project:access_and_security:index")
+        sg_id = self.table.kwargs['security_group_id']
+        return reverse("horizon:project:access_and_security:"
+                       "security_groups:detail", args=[sg_id])
 
 
 def get_source(rule):
@@ -105,5 +117,5 @@ class RulesTable(tables.DataTable):
     class Meta:
         name = "rules"
         verbose_name = _("Security Group Rules")
-        table_actions = (DeleteRule,)
+        table_actions = (CreateRule, DeleteRule)
         row_actions = (DeleteRule,)

openstack_dashboard/dashboards/project/access_and_security/security_groups/tests.py

@@ -42,8 +42,11 @@ class SecurityGroupsViewTests(test.TestCase):
     def setUp(self):
         super(SecurityGroupsViewTests, self).setUp()
         sec_group = self.security_groups.first()
+        self.detail_url = reverse('horizon:project:access_and_security:'
+                                  'security_groups:detail',
+                                  args=[sec_group.id])
         self.edit_url = reverse('horizon:project:access_and_security:'
-                                'security_groups:edit_rules',
+                                'security_groups:add_rule',
                                 args=[sec_group.id])
 
     def test_create_security_groups_get(self):
@@ -96,39 +99,32 @@ class SecurityGroupsViewTests(test.TestCase):
                     'project/access_and_security/security_groups/create.html')
         self.assertContains(res, "ASCII")
 
-    def test_edit_rules_get(self):
+    def test_detail_get(self):
         sec_group = self.security_groups.first()
-        sec_group_list = self.security_groups.list()
 
         self.mox.StubOutWithMock(api.nova, 'security_group_get')
         api.nova.security_group_get(IsA(http.HttpRequest),
                                     sec_group.id).AndReturn(sec_group)
-        self.mox.StubOutWithMock(api.nova, 'security_group_list')
-        api.nova.security_group_list(
-            IsA(http.HttpRequest)).AndReturn(sec_group_list)
         self.mox.ReplayAll()
 
-        res = self.client.get(self.edit_url)
+        res = self.client.get(self.detail_url)
         self.assertTemplateUsed(res,
-                'project/access_and_security/security_groups/edit_rules.html')
+                'project/access_and_security/security_groups/detail.html')
-        self.assertItemsEqual(res.context['security_group'].name,
-                              sec_group.name)
 
-    def test_edit_rules_get_exception(self):
+    def test_detail_get_exception(self):
         sec_group = self.security_groups.first()
 
         self.mox.StubOutWithMock(api.nova, 'security_group_get')
-        self.mox.StubOutWithMock(api.nova, 'security_group_list')
-
         api.nova.security_group_get(IsA(http.HttpRequest),
                                     sec_group.id) \
-            .AndRaise(self.exceptions.nova)
+                .AndRaise(self.exceptions.nova)
+
         self.mox.ReplayAll()
 
-        res = self.client.get(self.edit_url)
+        res = self.client.get(self.detail_url)
         self.assertRedirectsNoFollow(res, INDEX_URL)
 
-    def test_edit_rules_add_rule_cidr(self):
+    def test_detail_add_rule_cidr(self):
         sec_group = self.security_groups.first()
         sec_group_list = self.security_groups.list()
         rule = self.security_group_rules.first()
@@ -147,42 +143,16 @@ class SecurityGroupsViewTests(test.TestCase):
         self.mox.ReplayAll()
 
         formData = {'method': 'AddRule',
-                    'security_group_id': sec_group.id,
+                    'id': sec_group.id,
-                    'from_port': rule.from_port,
+                    'port_or_range': 'port',
-                    'to_port': rule.to_port,
+                    'port': rule.from_port,
                     'ip_protocol': rule.ip_protocol,
                     'cidr': rule.ip_range['cidr'],
-                    'source_group': ''}
+                    'source': 'cidr'}
         res = self.client.post(self.edit_url, formData)
-        self.assertRedirectsNoFollow(res, INDEX_URL)
+        self.assertRedirectsNoFollow(res, self.detail_url)
 
-    def test_edit_rules_add_rule_cidr_and_source_group(self):
+    def test_detail_add_rule_self_as_source_group(self):
-        sec_group = self.security_groups.first()
-        sec_group_other = self.security_groups.get(id=2)
-        sec_group_list = self.security_groups.list()
-        rule = self.security_group_rules.first()
-
-        self.mox.StubOutWithMock(api.nova, 'security_group_get')
-        self.mox.StubOutWithMock(api.nova, 'security_group_list')
-        api.nova.security_group_get(IsA(http.HttpRequest),
-                                    sec_group.id).AndReturn(sec_group)
-        api.nova.security_group_list(
-                        IsA(http.HttpRequest)).AndReturn(sec_group_list)
-        self.mox.ReplayAll()
-
-        formData = {'method': 'AddRule',
-                    'security_group_id': sec_group.id,
-                    'from_port': rule.from_port,
-                    'to_port': rule.to_port,
-                    'ip_protocol': rule.ip_protocol,
-                    'cidr': "127.0.0.1/32",
-                    'source_group': sec_group_other.id}
-        res = self.client.post(self.edit_url, formData)
-        self.assertNoMessages()
-        msg = 'Either CIDR or Source Group may be specified, but not both.'
-        self.assertFormErrors(res, count=1, message=msg)
-
-    def test_edit_rules_add_rule_self_as_source_group(self):
         sec_group = self.security_groups.first()
         sec_group_list = self.security_groups.list()
         rule = self.security_group_rules.get(id=3)
@@ -202,109 +172,112 @@ class SecurityGroupsViewTests(test.TestCase):
         self.mox.ReplayAll()
 
         formData = {'method': 'AddRule',
-                    'security_group_id': sec_group.id,
+                    'id': sec_group.id,
-                    'from_port': rule.from_port,
+                    'port_or_range': 'port',
-                    'to_port': rule.to_port,
+                    'port': rule.from_port,
                     'ip_protocol': rule.ip_protocol,
                     'cidr': '0.0.0.0/0',
-                    'source_group': sec_group.id}
+                    'security_group': sec_group.id,
+                    'source': 'sg'}
         res = self.client.post(self.edit_url, formData)
-        self.assertRedirectsNoFollow(res, INDEX_URL)
+        self.assertRedirectsNoFollow(res, self.detail_url)
 
-    def test_edit_rules_invalid_port_range(self):
+    def test_detail_invalid_port_range(self):
         sec_group = self.security_groups.first()
         sec_group_list = self.security_groups.list()
         rule = self.security_group_rules.first()
 
-        self.mox.StubOutWithMock(api.nova, 'security_group_get')
-        api.nova.security_group_get(IsA(http.HttpRequest),
-                                    sec_group.id).AndReturn(sec_group)
         self.mox.StubOutWithMock(api.nova, 'security_group_list')
         api.nova.security_group_list(
             IsA(http.HttpRequest)).AndReturn(sec_group_list)
         self.mox.ReplayAll()
 
         formData = {'method': 'AddRule',
-                    'security_group_id': sec_group.id,
+                    'id': sec_group.id,
+                    'port_or_range': 'range',
                     'from_port': rule.from_port,
                     'to_port': int(rule.from_port) - 1,
                     'ip_protocol': rule.ip_protocol,
                     'cidr': rule.ip_range['cidr'],
-                    'source_group': ''}
+                    'source': 'cidr'}
         res = self.client.post(self.edit_url, formData)
         self.assertNoMessages()
         self.assertContains(res, "greater than or equal to")
 
     @test.create_stubs({api.nova: ('security_group_get',
                                    'security_group_list')})
-    def test_edit_rules_invalid_icmp_rule(self):
+    def test_detail_invalid_icmp_rule(self):
         sec_group = self.security_groups.first()
         sec_group_list = self.security_groups.list()
         icmp_rule = self.security_group_rules.list()[1]
 
-        api.nova.security_group_get(IsA(http.HttpRequest),
+        # 1st Test
-                                    sec_group.id).AndReturn(sec_group)
         api.nova.security_group_list(
             IsA(http.HttpRequest)).AndReturn(sec_group_list)
-        api.nova.security_group_get(IsA(http.HttpRequest),
+
-                                    sec_group.id).AndReturn(sec_group)
+        # 2nd Test
         api.nova.security_group_list(
             IsA(http.HttpRequest)).AndReturn(sec_group_list)
-        api.nova.security_group_get(IsA(http.HttpRequest),
+
-                                    sec_group.id).AndReturn(sec_group)
+        # 3rd Test
         api.nova.security_group_list(
             IsA(http.HttpRequest)).AndReturn(sec_group_list)
-        api.nova.security_group_get(IsA(http.HttpRequest),
+
-                                    sec_group.id).AndReturn(sec_group)
+        # 4th Test
         api.nova.security_group_list(
             IsA(http.HttpRequest)).AndReturn(sec_group_list)
+
         self.mox.ReplayAll()
 
         formData = {'method': 'AddRule',
-                    'security_group_id': sec_group.id,
+                    'id': sec_group.id,
-                    'from_port': 256,
+                    'port_or_range': 'port',
-                    'to_port': icmp_rule.to_port,
+                    'icmp_type': 256,
+                    'icmp_code': icmp_rule.to_port,
                     'ip_protocol': icmp_rule.ip_protocol,
                     'cidr': icmp_rule.ip_range['cidr'],
-                    'source_group': ''}
+                    'source': 'cidr'}
         res = self.client.post(self.edit_url, formData)
         self.assertNoMessages()
         self.assertContains(res, "The ICMP type not in range (-1, 255)")
 
         formData = {'method': 'AddRule',
-                    'security_group_id': sec_group.id,
+                    'id': sec_group.id,
-                    'from_port': icmp_rule.from_port,
+                    'port_or_range': 'port',
-                    'to_port': 256,
+                    'icmp_type': icmp_rule.from_port,
+                    'icmp_code': 256,
                     'ip_protocol': icmp_rule.ip_protocol,
                     'cidr': icmp_rule.ip_range['cidr'],
-                    'source_group': ''}
+                    'source': 'cidr'}
         res = self.client.post(self.edit_url, formData)
         self.assertNoMessages()
         self.assertContains(res, "The ICMP code not in range (-1, 255)")
 
         formData = {'method': 'AddRule',
-                    'security_group_id': sec_group.id,
+                    'id': sec_group.id,
-                    'from_port': icmp_rule.from_port,
+                    'port_or_range': 'port',
-                    'to_port': None,
+                    'icmp_type': icmp_rule.from_port,
+                    'icmp_code': None,
                     'ip_protocol': icmp_rule.ip_protocol,
                     'cidr': icmp_rule.ip_range['cidr'],
-                    'source_group': ''}
+                    'source_group': 'cidr'}
         res = self.client.post(self.edit_url, formData)
         self.assertNoMessages()
         self.assertContains(res, "The ICMP code is invalid")
 
         formData = {'method': 'AddRule',
-                    'security_group_id': sec_group.id,
+                    'id': sec_group.id,
-                    'from_port': None,
+                    'port_or_range': 'port',
-                    'to_port': icmp_rule.to_port,
+                    'icmp_type': None,
+                    'icmp_code': icmp_rule.to_port,
                     'ip_protocol': icmp_rule.ip_protocol,
                     'cidr': icmp_rule.ip_range['cidr'],
-                    'source_group': ''}
+                    'source': 'cidr'}
         res = self.client.post(self.edit_url, formData)
         self.assertNoMessages()
         self.assertContains(res, "The ICMP type is invalid")
 
-    def test_edit_rules_add_rule_exception(self):
+    def test_detail_add_rule_exception(self):
         sec_group = self.security_groups.first()
         sec_group_list = self.security_groups.list()
         rule = self.security_group_rules.first()
@@ -324,16 +297,16 @@ class SecurityGroupsViewTests(test.TestCase):
         self.mox.ReplayAll()
 
         formData = {'method': 'AddRule',
-                    'security_group_id': sec_group.id,
+                    'id': sec_group.id,
-                    'from_port': rule.from_port,
+                    'port_or_range': 'port',
-                    'to_port': rule.to_port,
+                    'port': rule.from_port,
                     'ip_protocol': rule.ip_protocol,
                     'cidr': rule.ip_range['cidr'],
-                    'source_group': ''}
+                    'source': 'cidr'}
         res = self.client.post(self.edit_url, formData)
-        self.assertRedirectsNoFollow(res, INDEX_URL)
+        self.assertRedirectsNoFollow(res, self.detail_url)
 
-    def test_edit_rules_delete_rule(self):
+    def test_detail_delete_rule(self):
         sec_group = self.security_groups.first()
         rule = self.security_group_rules.first()
 
@@ -343,11 +316,14 @@ class SecurityGroupsViewTests(test.TestCase):
 
         form_data = {"action": "rules__delete__%s" % rule.id}
         req = self.factory.post(self.edit_url, form_data)
-        table = RulesTable(req, sec_group.rules)
+        kwargs = {'security_group_id': sec_group.id}
+        table = RulesTable(req, sec_group.rules, **kwargs)
         handled = table.maybe_handle()
-        self.assertEqual(strip_absolute_base(handled['location']), INDEX_URL)
+        self.assertEqual(strip_absolute_base(handled['location']),
+                         self.detail_url)
 
-    def test_edit_rules_delete_rule_exception(self):
+    def test_detail_delete_rule_exception(self):
+        sec_group = self.security_groups.first()
         rule = self.security_group_rules.first()
 
         self.mox.StubOutWithMock(api.nova, 'security_group_rule_delete')
@@ -358,10 +334,11 @@ class SecurityGroupsViewTests(test.TestCase):
 
         form_data = {"action": "rules__delete__%s" % rule.id}
         req = self.factory.post(self.edit_url, form_data)
-        table = RulesTable(req, self.security_group_rules.list())
+        kwargs = {'security_group_id': sec_group.id}
+        table = RulesTable(req, self.security_group_rules.list(), **kwargs)
         handled = table.maybe_handle()
         self.assertEqual(strip_absolute_base(handled['location']),
-                         INDEX_URL)
+                         self.detail_url)
 
     def test_delete_group(self):
         sec_group = self.security_groups.get(name="other_group")

openstack_dashboard/dashboards/project/access_and_security/security_groups/urls.py

@@ -20,11 +20,15 @@
 
 from django.conf.urls.defaults import patterns, url
 
-from .views import CreateView, EditRulesView
+from .views import CreateView, DetailView, AddRuleView
 
 
 urlpatterns = patterns('',
     url(r'^create/$', CreateView.as_view(), name='create'),
-    url(r'^(?P<security_group_id>[^/]+)/edit_rules/$',
+    url(r'^(?P<security_group_id>[^/]+)/$',
-        EditRulesView.as_view(),
+        DetailView.as_view(),
-        name='edit_rules'))
+        name='detail'),
+    url(r'^(?P<security_group_id>[^/]+)/add_rule/$',
+        AddRuleView.as_view(),
+        name='add_rule')
+)

openstack_dashboard/dashboards/project/access_and_security/security_groups/views.py

@@ -23,8 +23,7 @@ Views for managing instances.
 """
 import logging
 
-from django import shortcuts
+from django.core.urlresolvers import reverse_lazy, reverse
-from django.core.urlresolvers import reverse_lazy
 from django.utils.translation import ugettext_lazy as _
 
 from horizon import exceptions
@@ -39,12 +38,9 @@ from .tables import RulesTable
 LOG = logging.getLogger(__name__)
 
 
-class EditRulesView(tables.DataTableView, forms.ModalFormView):
+class DetailView(tables.DataTableView):
     table_class = RulesTable
-    form_class = AddRule
+    template_name = 'project/access_and_security/security_groups/detail.html'
-    template_name = ('project/access_and_security/security_groups/'
-                     'edit_rules.html')
-    success_url = reverse_lazy("horizon:project:access_and_security:index")
 
     def get_data(self):
         security_group_id = int(self.kwargs['security_group_id'])
@@ -54,17 +50,32 @@ class EditRulesView(tables.DataTableView, forms.ModalFormView):
             rules = [api.nova.SecurityGroupRule(rule) for
                      rule in self.object.rules]
         except:
-            self.object = None
+            redirect = reverse('horizon:project:access_and_security:index')
-            rules = []
             exceptions.handle(self.request,
-                              _('Unable to retrieve security group.'))
+                              _('Unable to retrieve security group.'),
+                              redirect=redirect)
         return rules
 
+
+class AddRuleView(forms.ModalFormView):
+    form_class = AddRule
+    template_name = 'project/access_and_security/security_groups/add_rule.html'
+
+    def get_success_url(self):
+        sg_id = self.kwargs['security_group_id']
+        return reverse("horizon:project:access_and_security:"
+                       "security_groups:detail", args=[sg_id])
+
+    def get_context_data(self, **kwargs):
+        context = super(AddRuleView, self).get_context_data(**kwargs)
+        context["security_group_id"] = self.kwargs['security_group_id']
+        return context
+
     def get_initial(self):
-        return {'security_group_id': self.kwargs['security_group_id']}
+        return {'id': self.kwargs['security_group_id']}
 
     def get_form_kwargs(self):
-        kwargs = super(EditRulesView, self).get_form_kwargs()
+        kwargs = super(AddRuleView, self).get_form_kwargs()
 
         try:
             groups = api.nova.security_group_list(self.request)
@@ -83,37 +94,6 @@ class EditRulesView(tables.DataTableView, forms.ModalFormView):
         kwargs['sg_list'] = security_groups
         return kwargs
 
-    def get_form(self):
-        if not hasattr(self, "_form"):
-            form_class = self.get_form_class()
-            self._form = super(EditRulesView, self).get_form(form_class)
-        return self._form
-
-    def get_context_data(self, **kwargs):
-        context = super(EditRulesView, self).get_context_data(**kwargs)
-        context['form'] = self.get_form()
-        if self.request.is_ajax():
-            context['hide'] = True
-        return context
-
-    def get(self, request, *args, **kwargs):
-        # Table action handling
-        handled = self.construct_tables()
-        if handled:
-            return handled
-        if not self.object:  # Set during table construction.
-            return shortcuts.redirect(self.success_url)
-        context = self.get_context_data(**kwargs)
-        context['security_group'] = self.object
-        return self.render_to_response(context)
-
-    def post(self, request, *args, **kwargs):
-        form = self.get_form()
-        if form.is_valid():
-            return self.form_valid(form)
-        else:
-            return self.get(request, *args, **kwargs)
-
 
 class CreateView(forms.ModalFormView):
     form_class = CreateGroup

openstack_dashboard/dashboards/project/access_and_security/templates/access_and_security/security_groups/_add_rule.html

@@ -0,0 +1,28 @@
+{% extends "horizon/common/_modal_form.html" %}
+{% load i18n %}
+
+{% block form_id %}create_security_group_rule_form{% endblock %}
+{% block form_action %}{% url horizon:project:access_and_security:security_groups:add_rule security_group_id %}{% endblock %}
+
+{% block modal-header %}{% trans "Add Rule" %}{% endblock %}
+{% block modal_id %}create_security_group_rule_modal{% endblock %}
+
+{% block modal-body %}
+<div class="left">
+    <fieldset>
+    {% include "horizon/common/_form_fields.html" %}
+    </fieldset>
+</div>
+<div class="right">
+    <h3>{% trans "Description" %}:</h3>
+    <p>{% blocktrans %}Rules define which traffic is allowed to instances assigned to the security group. A security group rule consists of three main parts:{% endblocktrans %}</p>
+    <p><strong>{% trans "Protocol" %}</strong>: {% blocktrans %}You must specify the desired IP protocol to which this rule will apply; the options are TCP, UDP, or ICMP.{% endblocktrans %}</p>
+    <p><strong>{% trans "Open Port/Port Range" %}</strong>: {% blocktrans %}For TCP and UDP rules you may choose to open either a single port or a range of ports. Selecting the "Port Range" option will provide you with space to provide both the starting and ending ports for the range. For ICMP rules you instead specify an ICMP type and code in the spaces provided.{% endblocktrans %}</p>
+    <p><strong>{% trans "Source" %}</strong>: {% blocktrans %}You must specify the source of the traffic to be allowed via this rule. You may do so either in the form of an IP address block (CIDR) or via a source group (Security Group). Selecting a security group as the source will allow any other instance in that security group access to any other instance via this rule.{% endblocktrans %}</p>
+</div>
+{% endblock %}
+
+{% block modal-footer %}
+  <input class="btn btn-primary pull-right" type="submit" value="{% trans "Add" %}" />
+  <a href="{% url horizon:project:access_and_security:security_groups:detail security_group_id %}" class="btn secondary cancel close">{% trans "Cancel" %}</a>
+{% endblock %}

openstack_dashboard/dashboards/project/access_and_security/templates/access_and_security/security_groups/_edit_rules.html

@@ -1,21 +0,0 @@
-{% extends "horizon/common/_modal_form.html" %}
-{% load i18n %}
-
-{% block form_id %}security_group_rule_form{% endblock %}
-{% block form_action %}{% url horizon:project:access_and_security:security_groups:edit_rules security_group.id %}{% endblock %}
-{% block form_class %}{{ block.super }} horizontal split_five{% endblock %}
-
-{% block modal_id %}security_group_rule_modal{% endblock %}
-{% block modal-header %}{% trans "Edit Security Group Rules" %}{% endblock %}
-
-{% block modal-body %}
-  <h3>{% trans "Add Rule" %}</h3>
-  <fieldset>
-  {% include "horizon/common/_form_fields.html" %}
-  </fieldset>
-{% endblock %}
-
-{% block modal-footer %}
-    <input class="btn btn-primary pull-right" type="submit" value="{% trans "Add Rule" %}" />
-    <a href="{% url horizon:project:access_and_security:index %}" class="btn secondary cancel close">{% trans "Cancel" %}</a>
-{% endblock %}

openstack_dashboard/dashboards/project/access_and_security/templates/access_and_security/security_groups/add_rule.html

@@ -0,0 +1,12 @@
+{% extends 'base.html' %}
+{% load i18n %}
+{% block title %}{% trans "Add Rule" %}{% endblock %}
+
+{% block page_header %}
+  {% include "horizon/common/_page_header.html" with title=_("Add Rule") %}
+{% endblock page_header %}
+
+{% block main %}
+  {% include 'project/access_and_security/security_groups/_add_rule.html' %}
+{% endblock %}
+

openstack_dashboard/dashboards/project/access_and_security/templates/access_and_security/security_groups/create.html

@@ -7,5 +7,5 @@
 {% endblock page_header %}
 
 {% block main %}
-    {% include 'project/access_and_security/security_groups/_create.html' %}
+  {% include 'project/access_and_security/security_groups/_create.html' %}
 {% endblock %}

openstack_dashboard/dashboards/project/access_and_security/templates/access_and_security/security_groups/detail.html

@@ -7,5 +7,5 @@
 {% endblock page_header %}
 
 {% block main %}
-  {% include "project/access_and_security/security_groups/_edit_rules.html" %}
+  {{ table.render }}
 {% endblock %}