Skip to content
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.

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

Fixed #26729 -- Allowed overriding the label of a tabular inline form field #6804

Closed
wants to merge 1 commit into from

Conversation

czpython
Copy link
Contributor

@@ -279,7 +279,7 @@ def fields(self):
'help_text': help_text_for_field(field_name, self.opts.model),
}
else:
form_field = self.formset.form.base_fields[field_name]
form_field = self.formset.empty_form.fields[field_name]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat trick.

@charettes charettes changed the title Fixed #26729 -- Allow overriding the label of a tabular inline form field Fixed #26729 -- Allowed overriding the label of a tabular inline form field Jun 19, 2016
@timgraham
Copy link
Member

Could you try to reuse an existing set of models? We try to avoid new models unless absolutely necessary since they slow the test suite.

Also, if it's possible to write the assertion by accessing the appropriate object in response.context, that also makes it easier to debug the test in the event of a failure.

@czpython
Copy link
Contributor Author

@timgraham
You got it. Regarding your second comment, I'm not sure I follow.. what do you mean by "accessing the appropriate object"? The code in question is here

@timgraham
Copy link
Member

Something like response.context['inline_admin_formset'].fields if that works.

@czpython
Copy link
Contributor Author

@timgraham
Sure however it looks a bit implementation specific

response = self.client.get(reverse('admin:admin_inlines_anotherparentmodel_add'))
field = list(response.context['inline_admin_formset'].fields())[0]
self.assertEqual(field['label'], 'new label')

@timgraham
Copy link
Member

I think it's an improvement since that gives some idea of where to look in the event of a test breakage. With assertContains, you really don't have much of an idea. I guess if you want to put the assertContains after those other lines, it would be okay too.

@czpython
Copy link
Contributor Author

@timgraham Thanks for review. I've updated the pr to reuse model and do more explicit test.

@timgraham
Copy link
Member

merged in 9c2d5a8, thanks!

I removed the ticket references. For future reference, we typically include them for obscure issues where the additional context of the ticket is helpful in understanding the issue.

@timgraham timgraham closed this Jun 21, 2016
@czpython czpython deleted the ticket_26729 branch June 21, 2016 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants