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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add frontend template debug view #5661

Merged
merged 5 commits into from
Apr 21, 2020
Merged

Add frontend template debug view #5661

merged 5 commits into from
Apr 21, 2020

Conversation

chosak
Copy link
Member

@chosak chosak commented Apr 14, 2020

This commit adds a new generic "template debug" view to the v1 app for easy debugging of Jinja templates. This work was originally developed for #5635 and has been spun out of that PR for more isolated review.

Testing

Using a local server, visit http://localhost:8000/admin/template_debug/v1/notification/.

Screenshots

image

Notes

To be discussed at the 4/14 Dev CoP.

Todos

This is just an initial implementation, and there are many potential improvements, including documentation. For example, this could be linked from the Wagtail sidebar, and it'd be nice to have a top-level page listing all templates registered this way. It'd also be nice to be able to render each template test case individually at their own URLs for isolation for e.g. visual regression testing.

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the CFPB development guidelines
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Visually tested in supported browsers and devices (see checklist below 馃憞)
  • Project documentation has been updated
  • Reviewers requested with the Reviewers tool 鉃★笍

Copy link
Member

@willbarton willbarton left a comment

Choose a reason for hiding this comment

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

I love this so much! Do you want to add some documentation on how to add a new template debug view to the docs?

@chosak
Copy link
Member Author

chosak commented Apr 15, 2020

Thanks @willbarton, I've added some docs in fbc952f.

image

Copy link
Member

@anselmbradford anselmbradford left a comment

Choose a reason for hiding this comment

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

LGTM 馃憤

template.

```py
# myapp/template_debug.py
Copy link
Member

Choose a reason for hiding this comment

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

Does it matter that you have myapp/template_debug.py here but the actual file path is myapp/views/template_debug.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

So the actual view itself lives in the v1 app, because it has to live somewhere. This is the single Django view used to render any templates form anywhere. Individual apps (including v1) also need a place to define the test data used when debugging templates, and I propose to use a convention of myapp.template_debug for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't quite call the test data module that in this PR, either though. As discussed in chat, I would suggest renaming test_cases.py to notifications.py and use a different module for each new set of test data. Otherwise this file gon' get MASSIVE. Then update this line as such:

Suggested change
# myapp/template_debug.py
# myapp/template_debug/link.py

Copy link
Member

@contolini contolini left a comment

Choose a reason for hiding this comment

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

This is exciting! If we can find a reasonable way to expose the view to non-admin users we can write visual regression tests against our templates.

@higs4281
Copy link
Member

This is a great idea, @chosak !

@chosak
Copy link
Member Author

chosak commented Apr 17, 2020

If we can find a reasonable way to expose the view to non-admin users we can write visual regression tests against our templates.

@contolini technically doing this is pretty simple - we would just have to decide on a URL pattern for these pages. Since this would be going on the public website, though, we'd probably want to block in robots.txt, and navigate any clearance concerns. Once this gets in I can open an internal issue for that conversation.

@contolini
Copy link
Member

If we can find a reasonable way to expose the view to non-admin users we can write visual regression tests against our templates.

@contolini technically doing this is pretty simple - we would just have to decide on a URL pattern for these pages. Since this would be going on the public website, though, we'd probably want to block in robots.txt, and navigate any clearance concerns. Once this gets in I can open an internal issue for that conversation.

Can we hide it behind a flag so that it only appears on beta and not prod?

Copy link
Contributor

@Scotchester Scotchester left a comment

Choose a reason for hiding this comment

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

Pretty good stuff! Just to document here the other thing I mentioned in chat was the idea of moving cfgov/v1/template_debug/__init__.py, cfgov/v1/views/template_debug.py, and the new parts of cfgov/v1/wagtail_hooks.py to the core app, but it sounds like we may wait to do that until these are made available outside the admin.

cfgov/cfgov/settings/base.py Show resolved Hide resolved
cfgov/v1/jinja2/v1/template_debug.html Show resolved Hide resolved
cfgov/v1/jinja2/v1/template_debug.html Outdated Show resolved Hide resolved
cfgov/v1/jinja2/v1/template_debug.html Outdated Show resolved Hide resolved
docs/debugging-templates.md Show resolved Hide resolved
template.

```py
# myapp/template_debug.py
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't quite call the test data module that in this PR, either though. As discussed in chat, I would suggest renaming test_cases.py to notifications.py and use a different module for each new set of test data. Otherwise this file gon' get MASSIVE. Then update this line as such:

Suggested change
# myapp/template_debug.py
# myapp/template_debug/link.py

docs/debugging-templates.md Show resolved Hide resolved
docs/debugging-templates.md Outdated Show resolved Hide resolved
This commit adds a new generic "template debug" view to the v1 app for
easy debugging of Jinja templates. Apps using this should serve this
view under /admin/template_debug/.
@chosak chosak force-pushed the feature/template-debug-view branch from 283f3f1 to 89dcbe9 Compare April 21, 2020 20:49
@chosak
Copy link
Member Author

chosak commented Apr 21, 2020

@Scotchester thanks for the thorough review; I've applied several of your suggestions.

I declined to make the suggestions around the structure of the template_debug Python code (module vs. package) - that feels like a decision best made on a case by case basis. For example, it makes sense for the job listing test cases from #5635 to live in the same file, instead of splitting up each one into separate files. If/when the v1 test cases become so numerous as to require multiple files, hopefully we can follow our general Python best practices around when and how to do that.

I agree the best place for this would be in the core app, but I think that should wait until such time as this is decoupled from the Wagtail hooks infrastructure. Currently our core app has few dependencies on Wagtail (v1 is more of our "core" Wagtail app) and it'd be nice to keep it that way.

@chosak chosak merged commit 90500f2 into master Apr 21, 2020
@chosak chosak deleted the feature/template-debug-view branch April 21, 2020 21:04
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.

None yet

6 participants