Skip to content

Fixed #22745 -- Prevented reevaluation of ModelChoiceField's queryset when accesssing BoundField's attrs #2816

Closed
wants to merge 3 commits into from

4 participants

@a1tus
a1tus commented Jun 16, 2014

No description provided.

@timgraham
Django member

Do you plan to add tests?

@a1tus
a1tus commented Jul 9, 2014

Sure, I can make some tests if you think we need them. But I'm not sure what exactly to test. Simple use cases do not require tests in my opinion (e.g. calling method with int or str params). May be to write smth that checks db queries number via assertQueriesNum?

@timgraham
Django member

Yes assertNumQueries is what I had in mind.

@a1tus
a1tus commented Jul 17, 2014

Added 2 regression tests (for both widgets).
On my dev debian / python 2.7 all model_forms tests ran successfully.
Also checked these tests to fail when patch is disabled (as expected they performed 3 queries instead of 1).

@charettes
Django member

Buildbot, test this please.

@a1tus
a1tus commented Jul 22, 2014

Can this patch be included in 1.7 release?

@schmitch schmitch commented on an outdated diff Aug 1, 2014
tests/model_forms/tests.py
@@ -1459,6 +1460,20 @@ class ModelChoiceForm(forms.Form):
self.assertTrue(field1 is not ModelChoiceForm.base_fields['category'])
self.assertTrue(field1.widget.choices.field is field1)
+ def test_modelchoicefield_22745(self):
+ """
+ Regression test for ticket #22745.
@schmitch
schmitch added a note Aug 1, 2014

The PR looks good, but these line and the other docstring line needs a more verbose description.
here is a docstring from another regressiontest:

        """
        Regression test for #12913. Make sure fields with choices respect
        show_hidden_initial as a kwarg to models.Field.formfield()
        """

it describes what the Test does so that people directly know what it does ;)
+1 if the docstring gets updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@a1tus
a1tus commented Aug 1, 2014

There're a lot of tests that have only ticket id in their docstrings so I thought it's acceptable.
But still no problem with adding more verbose comments.

@schmitch
schmitch commented Aug 1, 2014

Fine ;) +1 from my side i will set the ticket to RFC.

@timgraham
Django member

merged in 5e06fa1, thanks.

@timgraham timgraham closed this Aug 4, 2014
@timgraham
Django member

The patch cannot be included in 1.7 because we are only fixing release blocking bugs (typically regressions).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.