search

bittner / django-apptemplates

Django template loader that allows you to load and override a template from a specific Django application.
https://pypi.python.org/pypi/django-apptemplates
MIT License
51 stars 12 forks source link

fix Origin missing loader and template_name attrs #5

Closed bmon closed 6 years ago

bmon commented 7 years ago

This pull fixes issues encountered when using apptemplates with django-compressor and django.template.loaders.cached.Loader, as loader and template_name are both expected attributes of an Origin in that loader.

bittner commented 7 years ago

Thanks Brendan for your observation and the PR!

Do you think you could add a test that proves that the old code fails and your changes fix the behavior? We need to verify that this change fixes the problem across all Django versions once and forever.

bmon commented 7 years ago

I'm not sure how I would even start to write a test, could you point me in the right direction?

bittner commented 7 years ago

In theory, all we need to do is install a minimal application (apptemplates.test is something like that) including django-compressor, and run whatever action you run that reproduces the issue.

If this is not easily done with the current test setup you could take a look at the acceptance tests (BDD) in my painless-continuous-delivery repository, functional tests, really. The fundamental part of those BDD tests is really text in Gherkin syntax, that makes things very clear.

If you are willing to give that a shot I'm happy to support you with whatever questions you have!

bittner commented 7 years ago

As a side-comment, your build fails for a missing Python 3.3 on Travis, which is odd. Are our friends in Berlin messing with the build image again?

Maybe if you add a python3.3 to the installation candidates on the Travis setup that can be fixed.

dracos commented 6 years ago

I've just hit this bug upgrading a codebase from Django 1.8 to 1.11. We aren't using django-compressor, only using the cached loader (which is on by default in 1.11 if DEBUG is off!) is enough. The original PR in https://github.com/bittner/django-apptemplates/pull/1 did include passing loader etc to Origin, but it wasn't included in the commit that was included, https://github.com/bittner/django-apptemplates/commit/21c15824a08fd28ea588b4a32b2ca724de0be198 . I will try and add a regression test now, as otherwise our only option is to pin to this PR or release a fork.

dracos commented 6 years ago

"Do you think you could add a test that proves that the old code fails and your changes fix the behavior?" - I have submitted a new PR #6 that includes the code from this, plus wraps the xample app in the cached loader. This is enough to show the failures without the code from this PR.

bittner commented 6 years ago

Awesome, @dracos, thanks for the effort! Let us get this all into the master branch in no time. :+1:

dracos commented 6 years ago

Now #6 is merged, this PR can be closed :) Do you think you will be able to do a new release on pypi soon? (Sorry to badger, just waiting on this to be able to deploy our Django 1.11 upgrade :) ).

bittner commented 6 years ago

Sure! I will add Django 2.0 to the test matrix, and then push a new release to PyPI.

bittner commented 6 years ago

Thanks @bmon for your initial attempt to fix this issue!

bittner commented 6 years ago

Version 1.4 is now released. :tada: