Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixed #16869: BaseGenericInlineFormSet.save_new should use form's save() method #1599

Closed
wants to merge 2 commits into from

2 participants

tests/generic_relations/tests.py
((4 lines not shown))
+ def test_save_new_uses_form_save(self):
+ """
+ Regression for #16260: save_new should call form's save
+ method.
+ """
+
+ # Custom form used to check save was called
+ class SaveTestForm(forms.ModelForm):
+ save_called = False
+
+ @classmethod
+ def __saved(cls):
+ cls.save_called = True
+
+ def save(self, *args, **kwargs):
+ SaveTestForm.__saved()
@timgraham Owner

IMO it would be simpler if we just added some custom logic here like setting an attribute on the model to a certain value

@polmuz
polmuz added a note

you are right!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
tests/generic_relations/tests.py
((20 lines not shown))
+ super(SaveTestForm, self).save(*args, **kwargs)
+
+ Formset = generic_inlineformset_factory(ForProxyModelModel,
+ fields='__all__', for_concrete_model=True,
+ form=SaveTestForm)
+
+ instance = ProxyRelatedModel.objects.create()
+
+ data = {
+ 'form-TOTAL_FORMS': '1',
+ 'form-INITIAL_FORMS': '0',
+ 'form-MAX_NUM_FORMS': '',
+ 'form-0-title': 'foo',
+ }
+
+ formset = Formset(data, instance=instance, prefix='form')
@timgraham Owner

don't think prefix needs to be specified

@polmuz
polmuz added a note

it fails if I remove the prefix ManagementForm data is missing or has been tampered with

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/generic_relations/tests.py
((23 lines not shown))
+ fields='__all__', for_concrete_model=True,
+ form=SaveTestForm)
+
+ instance = ProxyRelatedModel.objects.create()
+
+ data = {
+ 'form-TOTAL_FORMS': '1',
+ 'form-INITIAL_FORMS': '0',
+ 'form-MAX_NUM_FORMS': '',
+ 'form-0-title': 'foo',
+ }
+
+ formset = Formset(data, instance=instance, prefix='form')
+ self.assertTrue(formset.is_valid())
+
+ new_obj, = formset.save()
@timgraham Owner

extra comma?

@polmuz
polmuz added a note

I stole this code from another test, it's an unboxing trick to get the first item of a one item list, I'm changing it to use formset.save()[0]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/generic_relations/tests.py
((8 lines not shown))
+ """
+
+ # Custom form used to check save was called
+ class SaveTestForm(forms.ModelForm):
+ save_called = False
+
+ @classmethod
+ def __saved(cls):
+ cls.save_called = True
+
+ def save(self, *args, **kwargs):
+ SaveTestForm.__saved()
+ super(SaveTestForm, self).save(*args, **kwargs)
+
+ Formset = generic_inlineformset_factory(ForProxyModelModel,
+ fields='__all__', for_concrete_model=True,
@timgraham Owner

for_concrete_model doesn't need to be specified (it defaults to True)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham
Owner

merged in b11564f - thanks!

@timgraham timgraham closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 7, 2013
  1. @polmuz
  2. @polmuz

    Fixed #16869: BaseGenericInlineFormSet.save_new should use form's sav…

    polmuz authored
    …e() method
    
    Thanks to Pablo Recio (pyriku) for providing the patch
This page is out of date. Refresh to see the latest.
View
13 django/contrib/contenttypes/generic.py
@@ -415,13 +415,12 @@ def get_default_prefix(cls):
))
def save_new(self, form, commit=True):
- kwargs = {
- self.ct_field.get_attname(): ContentType.objects.get_for_model(
- self.instance, for_concrete_model=self.for_concrete_model).pk,
- self.ct_fk_field.get_attname(): self.instance.pk,
- }
- new_obj = self.model(**kwargs)
- return save_instance(form, new_obj, commit=commit)
+ setattr(form.instance, self.ct_field.get_attname(),
+ ContentType.objects.get_for_model(self.instance).pk)
+ setattr(form.instance, self.ct_fk_field.get_attname(),
+ self.instance.pk)
+ return form.save(commit=commit)
+
def generic_inlineformset_factory(model, form=ModelForm,
formset=BaseGenericInlineFormSet,
View
32 tests/generic_relations/tests.py
@@ -306,6 +306,37 @@ def test_generic_inlineformset_factory(self):
form = Formset().forms[0]
self.assertIsInstance(form['tag'].field.widget, CustomWidget)
+ def test_save_new_uses_form_save(self):
+ """
+ Regression for #16260: save_new should call form's save
+ method.
+ """
+
+ # Custom form used to check save was called
+ class SaveTestForm(forms.ModelForm):
+ def save(self, *args, **kwargs):
+ self.instance.saved_by = "custom method"
+ return super(SaveTestForm, self).save(*args, **kwargs)
+
+ Formset = generic_inlineformset_factory(
+ ForProxyModelModel, fields='__all__', form=SaveTestForm)
+
+ instance = ProxyRelatedModel.objects.create()
+
+ data = {
+ 'form-TOTAL_FORMS': '1',
+ 'form-INITIAL_FORMS': '0',
+ 'form-MAX_NUM_FORMS': '',
+ 'form-0-title': 'foo',
+ }
+
+ formset = Formset(data, instance=instance, prefix='form')
@timgraham Owner

don't think prefix needs to be specified

@polmuz
polmuz added a note

it fails if I remove the prefix ManagementForm data is missing or has been tampered with

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ self.assertTrue(formset.is_valid())
+
+ new_obj = formset.save()[0]
+
+ self.assertEqual(new_obj.saved_by, "custom method")
+
def test_save_new_for_proxy(self):
Formset = generic_inlineformset_factory(ForProxyModelModel,
fields='__all__', for_concrete_model=False)
@@ -344,7 +375,6 @@ def test_save_new_for_concrete(self):
new_obj, = formset.save()
self.assertNotIsInstance(new_obj.obj, ProxyRelatedModel)
-
class ProxyRelatedModelTest(TestCase):
def test_default_behavior(self):
"""
Something went wrong with that request. Please try again.