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': '%[email protected]' % 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': '%[email protected]' % 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)
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
