Merge lp:~jtv/maas/bug-1374388 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Work around an “impedance mismatch” between Django's BooleanField in forms, and our use of forms for both UI and API.

The Node.disable_ipv4 checkbox could be enabled in the UI, but never disabled. In the API it could go both ways. The reason is that Django never submits a False value for an unchecked checkbox; it just doesn't include the value in the form submission. Our forms are rigged to interpret missing fields as ones that should stay unchanged. As a result, once disable_ipv4 was set to True, the UI simply ignored attempts to set it back to False.

The workaround adds an invisible input field to tell a UI-originated form submission from an API-originated one. A UI form shows all fields, so a missing boolean field means “set to False.” An API form must deal with callers omitting fields that were left out in order to keep them unchanged, so a missing boolean field does what it does now. This is particularly important with fields that are added in later versions.

The solution is one we can re-use in NodeGroup.default_disable_ipv4 as well. I had to store the un-processed version of the form's submitted data, because we previously just discarded it.

Jeroen

[Raphaël Badin (rvb)]

Nice! Although I understand the immediate goal is this fix the bug linked to this branch, I'm thinking about how we could generalize this so that similar problems can be fixed with minimal changes to the code (and ideally no change to the templates).

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks for the suggestion. I'll see if I can do that in a follow-up branch. I'll start by renaming our ModelForm so that the difference from Django's ModelForm becomes more visible. That was one of those things that hid the problem from me today.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Did I just vote Approve instead of marking the branch as Approved? Death march symptoms. :(

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-09-25 21:24:03 +0000
+++ src/maasserver/forms.py	2014-09-29 10:01:07 +0000
@@ -188,10 +188,13 @@
     First it ensures that missing fields are not errors, then it removes
     None values from cleaned data. This means that missing fields result
     in no change instead of an error.
+
+    :ivar submitted_data: The `data` as originally submitted.
     """
 
     def full_clean(self):
         """For missing fields, default to the model's existing value."""
+        self.submitted_data = self.data
         self.data = get_overridden_query_dict(
             self.initial, self.data, self.fields)
         super(APIEditMixin, self).full_clean()
@@ -385,6 +388,22 @@
     def clean_distro_series(self):
         return clean_distro_series_field(self, 'distro_series', 'osystem')
 
+    def clean_disable_ipv4(self):
+        # Boolean fields only show up in UI form submissions as "true" (if the
+        # box was checked) or not at all (if the box was not checked).  This
+        # is different from API submissions which can submit "false" values.
+        # Our forms are rigged to interpret missing fields as unchanged, but
+        # that doesn't # work for the UI.  A form in the UI always submits all
+        # its fields, so in that case, no value means False.
+        #
+        # To kludge around this, the UI form submits a hidden input field named
+        # "ui-submission" that doesn't exist in the API.  If this field is
+        # present, go with the UI-style behaviour.
+        form_data = self.submitted_data
+        if 'ui-submission' in form_data and 'disable_ipv4' not in form_data:
+            self.cleaned_data['disable_ipv4'] = False
+        return self.cleaned_data['disable_ipv4']
+
     def clean(self):
         cleaned_data = super(NodeForm, self).clean()
         if self.new_node and self.data.get('disable_ipv4') is None:

=== modified file 'src/maasserver/templates/maasserver/node_edit.html'
--- src/maasserver/templates/maasserver/node_edit.html	2014-06-09 19:58:06 +0000
+++ src/maasserver/templates/maasserver/node_edit.html	2014-09-29 10:01:07 +0000
@@ -77,6 +77,7 @@
         </a>
       </li>
     </ul>
+    <input name="ui-submission" type="hidden" value="Submitted from the UI" />
     <input type="submit" value="Save node" class="right" />
     <a class="link-button" href="{% url 'node-view' node.system_id %}">Cancel</a>
   </form>

=== modified file 'src/maasserver/tests/test_forms_node.py'
--- src/maasserver/tests/test_forms_node.py	2014-09-23 14:16:33 +0000
+++ src/maasserver/tests/test_forms_node.py	2014-09-29 10:01:07 +0000
@@ -369,6 +369,25 @@
         node = form.save()
         self.assertEqual(setting, node.disable_ipv4)
 
+    def test_takes_missing_disable_ipv4_as_False_in_UI(self):
+        form = NodeForm(
+            instance=factory.make_Node(disable_ipv4=True),
+            data={
+                'architecture': make_usable_architecture(self),
+                'ui-submission': True,
+                })
+        node = form.save()
+        self.assertFalse(node.disable_ipv4)
+
+    def test_takes_missing_disable_ipv4_as_Unchanged_in_API(self):
+        form = NodeForm(
+            instance=factory.make_Node(disable_ipv4=True),
+            data={
+                'architecture': make_usable_architecture(self),
+                })
+        node = form.save()
+        self.assertTrue(node.disable_ipv4)
+
     def test_takes_True_disable_ipv4_from_cluster_by_default(self):
         setting = True
         cluster = factory.make_NodeGroup(default_disable_ipv4=setting)