New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed #14642 -- Fixed generic inline formsets crash when using save_as_new=True. #9385

Merged

timgraham merged 1 commit into django:master from r3m0t:generic-inline-save-new

Dec 30, 2017

Contributor

r3m0t commented Nov 24, 2017 •

edited by timgraham

https://code.djangoproject.com/ticket/14642

r3m0t force-pushed the generic-inline-save-new branch from 10d1eed to 9f7a524 Compare

November 24, 2017 15:56

timgraham changed the title ~~Fix #28840 - Admin save as new doesn't work with GenericTabularInline~~ Fixed #14642 -- Fixed save_as_new with generic inline formsets.

timgraham reviewed

View reviewed changes

docs/releases/2.1.txt Outdated

@@ @@ -35,6 +35,8 @@ Minor features @@
               * :attr:`.ModelAdmin.search_fields` now accepts any lookup such as
                 ``field__exact``.
+              * Generic inlines now work correctly with "save as new".

Member

timgraham Nov 27, 2017

A release note isn't needed as this is a bug fix, not a feature.

tests/generic_inline_admin/tests.py Outdated

+                      self.assertEqual(Media.objects.get(pk=self.mp3_media_pk).url, new_mp3_url)
+                      self.assertEqual(Media.objects.get(pk=self.png_media_pk).url, new_png_url)
+                  def test_basic_edit_POST_as_new(self):

Member

timgraham Nov 27, 2017

I think the test belongs at the form layer (rather than admin) as that's the code that's fixed.

Contributor Author

r3m0t Nov 29, 2017

I added another test similar to the one for modelformset_factory. save_as_new isn't actually documented anywhere for forms though, it's only used by the admin.

Member

timgraham Dec 28, 2017

Did you forget to commit that test?

Contributor Author

r3m0t Dec 28, 2017

Looks like I lost it in git, but I have fished it out again.

r3m0t force-pushed the generic-inline-save-new branch from 9f7a524 to a4f61a1 Compare

November 29, 2017 23:24

r3m0t force-pushed the generic-inline-save-new branch 2 times, most recently from f611f7d to 2696416 Compare

December 17, 2017 18:33

timmartin reviewed

View reviewed changes

tests/generic_inline_admin/tests.py Outdated

+                  def test_basic_edit_POST_as_new(self):
+                      """
+                      A smoke test to ensure POST on edit_view works.

Contributor

timmartin Dec 20, 2017

If this needs a docstring, it should probably say something to differentiate this from the above test.

I know you copied this from the other test's comment, but I'm not sure "smoke test" is a useful description of what this test is for.

r3m0t force-pushed the generic-inline-save-new branch from 2696416 to 9093e9b Compare

December 28, 2017 18:29

timgraham reviewed

View reviewed changes

tests/generic_relations/tests.py Outdated

+                          'generic_relations-taggeditem-content_type-object_id-TOTAL_FORMS': '3',
+                          'generic_relations-taggeditem-content_type-object_id-INITIAL_FORMS': '2',
+                          'generic_relations-taggeditem-content_type-object_id-MAX_NUM_FORMS': '',
+                          'generic_relations-taggeditem-content_type-object_id-0-id': '1',

Member

timgraham Dec 28, 2017

The test fails on MySQL/PostgreSQL. Hardcoded id values might be the problem.

tests/generic_relations/tests.py Outdated

+                      quartz = Mineral.objects.create(name="Quartz", hardness=7)
+                      GenericFormSet = generic_inlineformset_factory(TaggedItem, extra=1)
+                      # An immutable QueryDict simulates request.POST.

Member

timgraham Dec 28, 2017

I think a plain dict should be fine here. The QueryDict approach was because of a different issue (362fba8).

tests/generic_relations/tests.py Outdated

@@ @@ -513,6 +514,55 @@ def test_subclasses_with_parent_gen_rel(self): @@
                       TaggedItem.objects.create(content_object=bear, tag='orange')
                       self.assertEqual(Carrot.objects.get(tags__tag='orange'), bear)
+                  def test_generic_inline_formsets_initial_count(self):
+                      """
+                      Test for initial form count considering save_as_new.

Member

timgraham Dec 28, 2017

Docstrings should state the expected behavior and omit prefixes like "Tests for" since all tests test things.

tests/generic_relations/tests.py Outdated

@@ @@ -513,6 +514,55 @@ def test_subclasses_with_parent_gen_rel(self): @@
                       TaggedItem.objects.create(content_object=bear, tag='orange')
                       self.assertEqual(Carrot.objects.get(tags__tag='orange'), bear)
+                  def test_generic_inline_formsets_initial_count(self):

Member

timgraham Dec 28, 2017

Please move these tests to test_forms.py following bcadabe.

tests/generic_relations/tests.py Outdated

+                      """
+                      Test for initial form count considering save_as_new.
+                      """
+                      quartz = Mineral.objects.create(name="Quartz", hardness=7)

Member

timgraham Dec 28, 2017

Use single quotes throughout, please.

tests/generic_inline_admin/tests.py Outdated

    
            @@ -72,24 +72,58 @@ def test_basic_add_POST(self):
          
                  def test_basic_edit_POST(self):

                      """

                      A smoke test to ensure POST on edit_view works.

                      A test to ensure POST on edit_view saves changes to the database.

Member

timgraham Dec 28, 2017

I think the changes to this file could be omitted. At least the cosmetic cleanups shouldn't be included. There are a number of docstrings that say, "A smoke test" which could be cleaned up together.

timgraham force-pushed the generic-inline-save-new branch 2 times, most recently from 028c4f5 to 3318683 Compare

December 30, 2017 16:18

timgraham changed the title ~~Fixed #14642 -- Fixed save_as_new with generic inline formsets.~~ Fixed #14642 -- Fixed generic inline formsets crash when using save_as_new=True.


          Fixed django#14642 -- Fixed generic inline formsets crash when using …

9bc4d90

…save_as_new=True.

timgraham force-pushed the generic-inline-save-new branch from 3318683 to 9bc4d90 Compare

December 30, 2017 16:42

timgraham merged commit 9bc4d90 into django:master

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