launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/respect-form-save-commit-param into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/respect-form-save-commit-param/+merge/117372

This problem was discovered as part of my work on making Node.nodegroup mandatory.  It complicates things, because saving a form with commit=False seems to be the only way to inject data into a model object created by the form.  And with the NOT NULL constraint on this column, we found that an unwanted model save() can be harmful for more than just performance.  Not saving is a necessary feature in Django, and where we can't retain it, we shouldn't pretend to.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/respect-form-save-commit-param/+merge/117372
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/respect-form-save-commit-param into lp:maas.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-07-27 14:52:44 +0000
+++ src/maasserver/forms.py	2012-07-31 04:37:06 +0000
@@ -210,7 +210,9 @@
     def save(self, *args, **kwargs):
         mac = super(MACAddressForm, self).save(commit=False)
         mac.node = self.node
-        return mac.save(*args, **kwargs)
+        if kwargs.get('commit', True):
+            mac.save(*args, **kwargs)
+        return mac
 
 
 class SSHKeyForm(ModelForm):
@@ -421,7 +423,8 @@
         new_email = self.cleaned_data.get('email', None)
         if new_email is not None:
             user.email = new_email
-        user.save()
+        if commit:
+            user.save()
         return user
 
     def clean_email(self):

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-06-11 07:34:55 +0000
+++ src/maasserver/tests/test_forms.py	2012-07-31 04:37:06 +0000
@@ -13,6 +13,7 @@
 __all__ = []
 
 from django import forms
+from django.contrib.auth.models import User
 from django.core.exceptions import (
     PermissionDenied,
     ValidationError,
@@ -462,6 +463,31 @@
 
 class TestNewUserCreationForm(TestCase):
 
+    def test_saves_to_db_by_default(self):
+        password = factory.make_name('password')
+        params = {
+            'email': '%s@xxxxxxxxxxx' % factory.getRandomString(),
+            'username': factory.make_name('user'),
+            'password1': password,
+            'password2': password,
+        }
+        form = NewUserCreationForm(params)
+        form.save()
+        self.assertIsNotNone(User.objects.get(username=params['username']))
+
+    def test_does_not_save_to_db_if_commit_is_False(self):
+        password = factory.make_name('password')
+        params = {
+            'email': '%s@xxxxxxxxxxx' % factory.getRandomString(),
+            'username': factory.make_name('user'),
+            'password1': password,
+            'password2': password,
+        }
+        form = NewUserCreationForm(params)
+        form.save(commit=False)
+        self.assertItemsEqual(
+            [], User.objects.filter(username=params['username']))
+
     def test_fields_order(self):
         form = NewUserCreationForm()
 
@@ -481,6 +507,21 @@
         self.assertTrue(
             MACAddress.objects.filter(node=node, mac_address=mac).exists())
 
+    def test_saves_to_db_by_default(self):
+        node = factory.make_node()
+        mac = factory.getRandomMACAddress()
+        form = MACAddressForm(node=node, data={'mac_address': mac})
+        form.save()
+        self.assertEqual(
+            mac, MACAddress.objects.get(mac_address=mac).mac_address)
+
+    def test_does_not_save_to_db_if_commit_is_False(self):
+        node = factory.make_node()
+        mac = factory.getRandomMACAddress()
+        form = MACAddressForm(node=node, data={'mac_address': mac})
+        form.save(commit=False)
+        self.assertItemsEqual([], MACAddress.objects.filter(mac_address=mac))
+
     def test_MACAddressForm_displays_error_message_if_mac_already_used(self):
         mac = factory.getRandomMACAddress()
         node = factory.make_mac_address(address=mac)