Fixing ticket #21371 #2152

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
6 participants

pouete commented Jan 8, 2014

Added the test case for the ticket #21371 as well as updated the templates with the needed "block.super" .
All the these test cases are passing.

J

Contributor

kezabelle commented Jan 9, 2014

I feel like the {{ block.super }} in the templates should have a trailing space: {{ block.super }}, like proposed in @Resry's fork, and the assertContains should similarly include a trailing space -- by the look of it at the moment, the response is something like <body class="bodyclass_constitency_checkapp-applabel"> (rather than <body class="bodyclass_constitency_check app-applabel">, where bodyclass_constitency_check can be considered a separate template namespace), which might break existing styles, and would require an awful lot of redundant CSS targeting the variants.

Member

mjtamlyn commented Jan 9, 2014

Agreed, the whitespace can't hurt.

pouete commented Jan 9, 2014

Done !
See last commit

tests/admin_views/models.py
@@ -15,6 +15,7 @@
class Section(models.Model):
"""
+
@timgraham

timgraham Feb 5, 2014

Owner

Please avoid trivial whitespace changes in unrelated files.

+{% block branding %}
+<h1 id="site-name"><a href="{% url 'admin:index' %}">{{ site_header }}</a></h1>
+{% endblock %}
+{% block bodyclass%}bodyclass_constitency_check{% endblock %}
@timgraham

timgraham Feb 5, 2014

Owner

space before %}

also, can't this file just be:

{% extends "admin/base.html" %}

{% block bodyclass %}bodyclass_constitency_check{% endblock %}

none of the other blocks are customized, right?

tests/admin_views/tests.py
+ def testExtendedBodyclassTemplateAddUser(self):
+ """
+ Ensure that the templates uses block.super in bodyclass in AddUser
+ tests :
@timgraham

timgraham Feb 5, 2014

Owner

colon is a bit out of place -- you can end the sentence with a period if you like.

I think the sentence should be something like: "Ensure that the /admin/change_form.html template uses block.super in the bodyclass block." (similarly for the other tests)

tests/admin_views/tests.py
+ Ensure that the templates uses block.super in bodyclass in AddUser
+ tests :
+ """
+ template_dirs = settings.TEMPLATE_DIRS + (
@timgraham

timgraham Feb 5, 2014

Owner

can we make template_dirs a module level constant rather than computing it for every test?

something like:

ADMIN_VIEW_TEMPLATE_DIRS = settings.TEMPLATE_DIRS + (
    os.path.join(os.path.dirname(upath(__file__)), 'templates'),)
tests/admin_views/tests.py
+ template_dirs = settings.TEMPLATE_DIRS + (
+ os.path.join(os.path.dirname(upath(__file__)), 'templates'),)
+ with self.settings(TEMPLATE_DIRS=template_dirs):
+
@timgraham

timgraham Feb 5, 2014

Owner

remove extra newline

tests/admin_views/tests.py
+ os.path.join(os.path.dirname(upath(__file__)), 'templates'),)
+ with self.settings(TEMPLATE_DIRS=template_dirs):
+ user = User.objects.get(username='super')
+ response = self.client.get('/test_admin/%s/auth/user/%s/password/' % (self.urlbit,user.id) )
@timgraham

timgraham Feb 5, 2014

Owner

need a space after the comma. There are some other minor formatting fixes needed, but I'm going to ask you to please check your code with flake8 rather than enumerate them myself.

tests/admin_views/tests.py
+ """
+ template_dirs = settings.TEMPLATE_DIRS + (
+ os.path.join(os.path.dirname(upath(__file__)), 'templates'),)
+ with self.settings(TEMPLATE_DIRS=template_dirs):
@timgraham

timgraham Feb 5, 2014

Owner

it might be worth trying to put all these tests in a single test class so you can use override_settings on the class rather than using self.settings in every test.

tests/admin_views/tests.py
+ Ensure that the templates uses block.super in bodyclass
+ test: templates/admin/delete_confirmation.html
+ """
+ from django.contrib.auth.models import Group
@timgraham

timgraham Feb 5, 2014

Owner

imports should go at the top of the file unless they cause a circular import which shouldn't be the case here.

tests/admin_views/tests.py
+ test: templates/admin/delete_confirmation.html
+ """
+ from django.contrib.auth.models import Group
+ group = Group(name="foogroup")
@timgraham

timgraham Feb 5, 2014

Owner

use Group.objects.create(name="foogroup") rather than .save()

tests/admin_views/tests.py
+ with self.settings(TEMPLATE_DIRS=template_dirs):
+ response = self.client.get('/test_admin/%s/auth/group/%s/delete/' % (self.urlbit, group.id) )
+ self.assertContains(response, 'bodyclass_constitency_check ')
+ group.delete()
@timgraham

timgraham Feb 5, 2014

Owner

not needed I think (tests automatically clean up after themselves)

pouete commented Feb 10, 2014

Corrected following timagraham comments on commit #4b333d7

@@ -0,0 +1,2 @@
+{% extends "admin/base.html" %}
+{% block bodyclass%}bodyclass_constitency_check{% endblock %}
@vin-sha

vin-sha Feb 10, 2014

Is it correct spelling: constitency
I think i shoud be constituency

@pouete

pouete Feb 10, 2014

You are right, the spelling is actually wrong : should be "consistency"
constituency means something totally different.
I should have spotted that before :)

@timgraham

timgraham Feb 10, 2014

Owner

I'm actually in the process of committing this now so I'll make the fix, thanks!

+
+ def testExtendedBodyclassTemplateChangepass(self):
+ """
+ Ensure that the auth/user/change_password.html template uses block.super in the bodyclass block.
@timgraham

timgraham Feb 10, 2014

Owner

I've made the fix, but for future reference please wrap docstrings at 80 characters.

@@ -648,6 +649,77 @@ def test_invalid_appindex_url(self):
reverse('admin:app_list', args=('admin_views2',))
+@override_settings(TEMPLATE_DIRS=ADMIN_VIEW_TEMPLATES_DIR,)
+class AdminCustomTemplateTests(AdminViewBasicTestCase):
+ def testExtendedBodyclassTemplateAddUser(self):
@timgraham

timgraham Feb 10, 2014

Owner

For new tests, we prefer to use underscores, not camel case.

@pouete

pouete Feb 10, 2014

Should I fix that right now ? something like : admin_custom_template_tests ?

Owner

timgraham commented Feb 10, 2014

merged in f5123c7, thanks!

@timgraham timgraham closed this Feb 10, 2014

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