Skip to content

fix fail-on-template-vars errors#2899

Open
dantagg wants to merge 6 commits into
DemocracyClub:masterfrom
dantagg:fail-on-template-vars
Open

fix fail-on-template-vars errors#2899
dantagg wants to merge 6 commits into
DemocracyClub:masterfrom
dantagg:fail-on-template-vars

Conversation

@dantagg

@dantagg dantagg commented Oct 29, 2020

Copy link
Copy Markdown

Fixes nearly all errors from running pytest --fail-on-template-vars as requested in issue #2802.

string_if_invalid
when setting pytest --fail-on-template-vars, pytest-django overwrites the template option string_if_invalid to an exception. This causes a lot of errors of the form
'string_if_invalid' in TEMPLATES OPTIONS must be a string but got: ... 33e3abf removes this error by not running the system check templates.E002 when testing.

required_css_class
The dc_base_theme repository's field.html template expects the form to have a required_css_class property. eb15152 and 6a98de9 add those properties to the relevant forms. It may be that a better solution is to remove the expectation of that property from the template.

CANONICAL_URL
The base_full.html template expects CANONICAL_URL in its context. 0af9439 creates a context_processor to add CANONICAL_URL to context. 9330d48 adds this context processor to the list of context processors and adds the setting variable CANONICAL_URL to base_settings.py. This will need to be overridden in production.

Undefined template variable 'media'
The outstanding error is Failed: Undefined template variable 'media' in '.../site-packages/pipeline/templates/pipeline/css.html'.

@GeoWill GeoWill self-requested a review November 13, 2020 15:12

@GeoWill GeoWill left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for this @dantagg.

It essentially looks good, but there are a couple of changes before merging. If you don't have time to do them I'm happy to, just let me know 🙂 .

  • First off, until I understand it better I'd drop 33e3ab. We're not running checks regularly with --fail-on-template so I don't want to risk hiding errors that we'd want to surface. If this is a problem with how pytest-django overwrites string_if_invalid then I'd rather raise an issue there. Saying that I haven't wrapped my head around it properly so happy to hear otherwise.
  • I think the CANONICAL_URL variable should point to our domain root. Though as I said in my comment perhaps we should bin the tag entirely.

Style and GNit picks:

  • Squash f1fd3d into eb1515
  • Make sure Travis passes - this will be black and pyflakes issues. You can run black . and pytest --flakes locally.
  • Reformat commits so that the first line is 50 characters and the body wraps at 72.

Thanks again, and let me know if any of that doesn't make sense, or how to do it.

if len(sys.argv) > 1 and sys.argv[1] in ["test", "harvest"]:
from .testing import * # noqa

CANONICAL_URL= '' No newline at end of file

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this should line should be:

CANONICAL_URL= 'https://wheredoivote.co.uk'

However this is all so that we can see how well our links are shared on fb, which I'm not sure we'll ever do. Anyway for now at least having the right url seems sensible.

@dantagg

dantagg commented Nov 13, 2020 via email

Copy link
Copy Markdown
Author

@GeoWill

GeoWill commented Nov 17, 2020

Copy link
Copy Markdown
Collaborator

send another pull request.

Feel free to force push to this one 🙂

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants