Merge lp:~newell-jensen/maas/fix-bug-1378936 into lp:~maas-committers/maas/trunk

[Newell Jensen]

This branch implements the behavior that if the MAAS user wants to change the OS or Release of a node, it first must be acquired.

[Blake Rouse (blake-rouse)]

I know this is WIP and you said you are having API issues, but just wanted to give the code a look over. Looks good, lets talk to day about the API.

[Newell Jensen (newell-jensen)]

Tested with self-packages and currently queued in CI.

[Newell Jensen (newell-jensen)]

Merging with trunk, this passes CI.

[Jeroen T. Vermeulen (jtv)]

Sensible change. It may be worth mentioning that this should only affect administrators, because they're the only users who can edit nodes that have no owners.

I have some nitpicks inline, and one broader design question, with a suggested alternate implementation if it turns out to touch a weak spot.

review: Approve

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-10-14 11:22:28 +0000
+++ src/maasserver/forms.py	2014-10-19 22:10:43 +0000
@@ -327,8 +327,14 @@
         # a parameter named 'request' because it is used interchangingly
         # with NodeAdminForm which actually uses this parameter.
 
+        instance = kwargs.get('instance')
+        if instance is None or instance.owner is None:
+            self.has_owner = False
+        else:
+            self.has_owner = True
+
         # Are we creating a new node object?
-        self.new_node = (kwargs.get('instance') is None)
+        self.new_node = (instance is None)
         if self.new_node:
             # Offer choice of nodegroup.
             self.fields['nodegroup'] = NodeGroupFormField(
@@ -349,7 +355,7 @@
                 self.instance.nodegroup.nodegroupinterface_set.all())
 
         self.set_up_architecture_field()
-        self.set_up_osystem_and_distro_series_fields(kwargs.get('instance'))
+        self.set_up_osystem_and_distro_series_fields(instance)
 
         if not allow_disable_ipv4:
             # Hide the disable_ipv4 field until support works properly.  The
@@ -361,6 +367,17 @@
             self.fields['disable_ipv4'] = forms.BooleanField(
                 label="", required=False, widget=forms.HiddenInput())
 
+        # We only want the license key field to render in the UI if the `OS`
+        # and `Release` fields are also present.
+        if self.has_owner:
+            self.fields['license_key'] = forms.CharField(
+                label="License Key", required=False, help_text=(
+                    "License key for operating system"),
+                max_length=30)
+        else:
+            self.fields['license_key'] = forms.CharField(
+                label="", required=False, widget=forms.HiddenInput())
+
     def set_up_architecture_field(self):
         """Create the `architecture` field.
 
@@ -387,19 +404,26 @@
         """
         osystems = list_all_usable_osystems()
         releases = list_all_usable_releases(osystems)
-        os_choices = list_osystem_choices(osystems)
-        distro_choices = list_release_choices(releases)
-        invalid_osystem_message = compose_invalid_choice_text(
-            'osystem', os_choices)
-        invalid_distro_series_message = compose_invalid_choice_text(
-            'distro_series', distro_choices)
-        self.fields['osystem'] = forms.ChoiceField(
-            label="OS", choices=os_choices, required=False, initial='',
-            error_messages={'invalid_choice': invalid_osystem_message})
-        self.fields['distro_series'] = forms.ChoiceField(
-            label="Release", choices=distro_choices,
-            required=False, initial='',
-            error_messages={'invalid_choice': invalid_distro_series_message})
+        if self.has_owner:
+            os_choices = list_osystem_choices(osystems)
+            distro_choices = list_release_choices(releases)
+            invalid_osystem_message = compose_invalid_choice_text(
+                'osystem', os_choices)
+            invalid_distro_series_message = compose_invalid_choice_text(
+                'distro_series', distro_choices)
+            self.fields['osystem'] = forms.ChoiceField(
+                label="OS", choices=os_choices, required=False, initial='',
+                error_messages={'invalid_choice': invalid_osystem_message})
+            self.fields['distro_series'] = forms.ChoiceField(
+                label="Release", choices=distro_choices,
+                required=False, initial='',
+                error_messages={
+                    'invalid_choice': invalid_distro_series_message})
+        else:
+            self.fields['osystem'] = forms.ChoiceField(
+                label="", required=False, widget=forms.HiddenInput())
+            self.fields['distro_series'] = forms.ChoiceField(
+                label="", required=False, widget=forms.HiddenInput())
         if instance is not None:
             initial_value = get_distro_series_initial(osystems, instance)
             if instance is not None:
@@ -546,11 +570,6 @@
             "does not manage DNS, then the host name as entered will be the "
             "FQDN."))
 
-    license_key = forms.CharField(
-        label="License Key", required=False, help_text=(
-            "License key for operating system"),
-        max_length=30)
-
     class Meta:
         model = Node
 
@@ -2342,7 +2361,8 @@
             error_messages={'invalid_choice': invalid_osystem_message})
         self.fields['distro_series'] = forms.ChoiceField(
             label="Release", choices=distro_choices, required=True,
-            error_messages={'invalid_choice': invalid_distro_series_message})
+            error_messages={
+                'invalid_choice': invalid_distro_series_message})
         if instance is not None:
             initial_value = get_distro_series_initial(
                 osystems, instance, with_key_required=False)

=== modified file 'src/maasserver/templates/maasserver/node_edit.html'
--- src/maasserver/templates/maasserver/node_edit.html	2014-09-29 13:21:42 +0000
+++ src/maasserver/templates/maasserver/node_edit.html	2014-10-19 22:10:43 +0000
@@ -16,20 +16,22 @@
     Y.on('load', function() {
       // Create OSDistroWidget so that the release field will be
       // updated each time the value of the os field changes.
-      var releaseWidget = new Y.maas.os_distro_select.OSReleaseWidget({
-          srcNode: '#id_distro_series'
-          });
-      releaseWidget.bindTo(Y.one('.osystem').one('select'), 'change');
-      releaseWidget.render();
-
-      // Create LicenseKeyWidget so that the license key field will be
-      // show when a release is selected that requires it.
-      var licenseKeyWidget = new Y.maas.license_key.LicenseKeyWidget({
-          srcNode: '.license_key'
-          });
-      licenseKeyWidget.bindTo(Y.one('.distro_series').one('select'), 'change');
-      licenseKeyWidget.render();
-
+      var os_release = {{ os_release }};
+      if (os_release) {
+        var releaseWidget = new Y.maas.os_distro_select.OSReleaseWidget({
+            srcNode: '#id_distro_series'
+            });
+        releaseWidget.bindTo(Y.one('.osystem').one('select'), 'change');
+        releaseWidget.render();
+
+        // Create LicenseKeyWidget so that the license key field will be
+        // show when a release is selected that requires it.
+        var licenseKeyWidget = new Y.maas.license_key.LicenseKeyWidget({
+            srcNode: '.license_key'
+            });
+        licenseKeyWidget.bindTo(Y.one('.distro_series').one('select'), 'change');
+        licenseKeyWidget.render();
+      }
       // Create LinkedContentWidget widget so that the power_parameter field
       // will be updated each time the value of the power_type field changes.
       var power_types = {{ power_types }};

=== modified file 'src/maasserver/tests/test_forms_node.py'
--- src/maasserver/tests/test_forms_node.py	2014-10-03 04:53:51 +0000
+++ src/maasserver/tests/test_forms_node.py	2014-10-19 22:10:43 +0000
@@ -69,7 +69,7 @@
                 'distro_series',
                 'license_key',
                 'disable_ipv4',
-                'nodegroup',
+                'nodegroup'
             ], list(form.fields))
 
     def test_changes_node(self):
@@ -128,33 +128,43 @@
             form.errors['architecture'])
 
     def test_accepts_osystem(self):
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
         osystem = make_usable_osystem(self)
         form = NodeForm(data={
             'hostname': factory.make_name('host'),
             'architecture': make_usable_architecture(self),
             'osystem': osystem['name'],
-            })
+            },
+            instance=node)
         self.assertTrue(form.is_valid(), form._errors)
 
     def test_rejects_invalid_osystem(self):
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
         patch_usable_osystems(self)
         form = NodeForm(data={
             'hostname': factory.make_name('host'),
             'architecture': make_usable_architecture(self),
             'osystem': factory.make_name('os'),
-            })
+            },
+            instance=node)
         self.assertFalse(form.is_valid())
         self.assertItemsEqual(['osystem'], form._errors.keys())
 
     def test_starts_with_default_osystem(self):
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
         osystems = [make_osystem_with_releases(self) for _ in range(5)]
         patch_usable_osystems(self, osystems)
-        form = NodeForm()
+        form = NodeForm(instance=node)
         self.assertEqual(
             '',
             form.fields['osystem'].initial)
 
     def test_accepts_osystem_distro_series(self):
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
         osystem = make_usable_osystem(self)
         release = osystem['default_release']
         form = NodeForm(data={
@@ -162,10 +172,13 @@
             'architecture': make_usable_architecture(self),
             'osystem': osystem['name'],
             'distro_series': '%s/%s' % (osystem['name'], release),
-            })
+            },
+            instance=node)
         self.assertTrue(form.is_valid(), form._errors)
 
     def test_rejects_invalid_osystem_distro_series(self):
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
         osystem = make_usable_osystem(self)
         release = factory.make_name('release')
         form = NodeForm(data={
@@ -173,19 +186,24 @@
             'architecture': make_usable_architecture(self),
             'osystem': osystem['name'],
             'distro_series': '%s/%s' % (osystem['name'], release),
-            })
+            },
+            instance=node)
         self.assertFalse(form.is_valid())
         self.assertItemsEqual(['distro_series'], form._errors.keys())
 
     def test_starts_with_default_distro_series(self):
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
         osystems = [make_osystem_with_releases(self) for _ in range(5)]
         patch_usable_osystems(self, osystems)
-        form = NodeForm()
+        form = NodeForm(instance=node)
         self.assertEqual(
             '',
             form.fields['distro_series'].initial)
 
     def test_rejects_mismatch_osystem_distro_series(self):
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
         osystem = make_usable_osystem(self)
         release = osystem['default_release']
         invalid = factory.make_name('invalid_os')
@@ -194,30 +212,14 @@
             'architecture': make_usable_architecture(self),
             'osystem': osystem['name'],
             'distro_series': '%s/%s' % (invalid, release),
-            })
+            },
+            instance=node)
         self.assertFalse(form.is_valid())
         self.assertItemsEqual(['distro_series'], form._errors.keys())
 
-    def test_calls_validate_license_key_without_nodegroup(self):
-        release = make_rpc_release(requires_license_key=True)
-        osystem = make_rpc_osystem(releases=[release])
-        patch_usable_osystems(self, osystems=[osystem])
-        license_key = factory.make_name('key')
-        mock_validate = self.patch(forms, 'validate_license_key')
-        mock_validate.return_value = True
-        form = NodeForm(data={
-            'hostname': factory.make_name('host'),
-            'architecture': make_usable_architecture(self),
-            'osystem': osystem['name'],
-            'distro_series': '%s/%s*' % (osystem['name'], release['name']),
-            'license_key': license_key,
-            })
-        self.assertTrue(form.is_valid())
-        self.assertThat(
-            mock_validate,
-            MockCalledOnceWith(osystem['name'], release['name'], license_key))
-
     def test_rejects_when_validate_license_key_returns_False(self):
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
         release = make_rpc_release(requires_license_key=True)
         osystem = make_rpc_osystem(releases=[release])
         patch_usable_osystems(self, osystems=[osystem])
@@ -230,12 +232,14 @@
             'osystem': osystem['name'],
             'distro_series': '%s/%s*' % (osystem['name'], release['name']),
             'license_key': license_key,
-            })
+            },
+            instance=node)
         self.assertFalse(form.is_valid())
         self.assertItemsEqual(['license_key'], form._errors.keys())
 
     def test_calls_validate_license_key_for_with_nodegroup(self):
-        node = factory.make_Node()
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
         release = make_rpc_release(requires_license_key=True)
         osystem = make_rpc_osystem(releases=[release])
         patch_usable_osystems(self, osystems=[osystem])
@@ -256,7 +260,8 @@
                 node.nodegroup, osystem['name'], release['name'], license_key))
 
     def test_rejects_when_validate_license_key_for_returns_False(self):
-        node = factory.make_Node()
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
         release = make_rpc_release(requires_license_key=True)
         osystem = make_rpc_osystem(releases=[release])
         patch_usable_osystems(self, osystems=[osystem])
@@ -274,7 +279,8 @@
         self.assertItemsEqual(['license_key'], form._errors.keys())
 
     def test_rejects_when_validate_license_key_for_raise_no_connection(self):
-        node = factory.make_Node()
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
         release = make_rpc_release(requires_license_key=True)
         osystem = make_rpc_osystem(releases=[release])
         patch_usable_osystems(self, osystems=[osystem])
@@ -292,7 +298,8 @@
         self.assertItemsEqual(['license_key'], form._errors.keys())
 
     def test_rejects_when_validate_license_key_for_raise_timeout(self):
-        node = factory.make_Node()
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
         release = make_rpc_release(requires_license_key=True)
         osystem = make_rpc_osystem(releases=[release])
         patch_usable_osystems(self, osystems=[osystem])
@@ -310,7 +317,8 @@
         self.assertItemsEqual(['license_key'], form._errors.keys())
 
     def test_rejects_when_validate_license_key_for_raise_no_os(self):
-        node = factory.make_Node()
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
         release = make_rpc_release(requires_license_key=True)
         osystem = make_rpc_osystem(releases=[release])
         patch_usable_osystems(self, osystems=[osystem])
@@ -472,7 +480,8 @@
 class TestAdminNodeForm(MAASServerTestCase):
 
     def test_AdminNodeForm_contains_limited_set_of_fields(self):
-        node = factory.make_Node()
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
         form = AdminNodeForm(instance=node)
 
         self.assertEqual(

=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py	2014-10-10 17:15:45 +0000
+++ src/maasserver/views/nodes.py	2014-10-19 22:10:43 +0000
@@ -658,6 +658,12 @@
     def get_form_class(self):
         return get_node_edit_form(self.request.user)
 
+    def get_has_owner(self):
+        node = self.get_object()
+        if node is None or node.owner is None:
+            return mark_safe("false")
+        return mark_safe("true")
+
     def get_form_kwargs(self):
         # This is here so the request can be passed to the form. The
         # form needs it because it sets error messages for the UI.
@@ -673,6 +679,9 @@
         context = super(NodeEdit, self).get_context_data(**kwargs)
         context['power_types'] = generate_js_power_types(
             self.get_object().nodegroup)
+        # 'os_release' lets us know if we should render the `OS`
+        # and `Release` choice fields in the UI.
+        context['os_release'] = self.get_has_owner()
         return context