Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[WIP] Views cleanup #812

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

stenington commented Apr 1, 2013

A long overdue cleanup of the views directory.

Amongst general cleanup and renaming, the following changes are particularly noteworthy:

  • c54a08d adds a small helper app under test/views that lets you easily render views with dummy data.
  • 7c7256c introduces a new piece of middleware that lets static pages like tou.html, privacy.html, and vpat.html extend a layout so they won't get out of sync with style changes so easily.

stenington added some commits Mar 29, 2013

vpat.html, tou.html, and privacy.html extend layout
Our legal/vpat pages were previously completely static, meaning it was
easy for them to get out of synch with layout changes. This introduces
a new middleware method, staticViews, that works as a catch-all to
render views that don't need template data. That way each of those
pages can extend layouts and stay up to date much more easily.
Organizing view directory
Prefer dash-separated over camelCase view names
Add "template" or "partial" to filenames as appropriate
  Subdirs might be nicer, but need to make sure it doesn't
  break client-side rendering.

@toolness toolness commented on the diff Apr 15, 2013

middleware.js
@@ -173,6 +173,28 @@ exports.less = function less(env) {
return lessMiddleware(_.defaults(base, config));
};
+exports.staticViews = function staticViews(env) {
@toolness

toolness Apr 15, 2013

Contributor

Would it be helpful to put a comment above this explaining the rationale behind staticViews? Or perhaps change the name to something a bit more descriptive, like staticTemplateViews?

@stenington

stenington Apr 18, 2013

Contributor

PROBABLY!

@toolness toolness commented on the diff Apr 15, 2013

middleware.js
@@ -173,6 +173,28 @@ exports.less = function less(env) {
return lessMiddleware(_.defaults(base, config));
};
+exports.staticViews = function staticViews(env) {
+ return function (req, res, next) {
+ var match;
+ if(match = /^\/(.+\.html)$/.exec(req.path)) {
@toolness

toolness Apr 15, 2013

Contributor

Hmm, because this regexp is so liberal, I'm concerned that it might eventually be the source of some kind of security vulnerability. Would it be possible to make the .+ a bit more restrictive, e.g. only match alphanumeric characters and slashes?

@stenington

stenington Apr 18, 2013

Contributor

Ok, sure.

@toolness toolness commented on the diff Apr 15, 2013

middleware.js
@@ -173,6 +173,28 @@ exports.less = function less(env) {
return lessMiddleware(_.defaults(base, config));
};
+exports.staticViews = function staticViews(env) {
+ return function (req, res, next) {
+ var match;
+ if(match = /^\/(.+\.html)$/.exec(req.path)) {
+ var view = match[1];
+ try {
+ env.getTemplate(view);
+ }
+ catch (e) {
@toolness

toolness Apr 15, 2013

Contributor

I am a little concerned about this blanket catch statement, since it can potentially be used to silently swallow a real error that we might want reported. One solution, which solves both this and the aforementioned security concern, is to have a separate directory for static views, e.g. /views/static, read the filenames in that directory at startup, and whitelist against them in this middleware. That way you wouldn't need the blanket catch, and the aforementioned security risk will be mitigated since you're using a whitelist. Aside from that, a separate subdirectory for static views might also be helpful organizationally.

@stenington

stenington Apr 18, 2013

Contributor

I think this is somewhat achievable already by passing a different nunjucks env that only knows about a restricted subset of templates. The catch is just the "nunjucks has no idea what template you're talking about" case, so I don't think there are errors there we care about. I'll look over it again and see if it can be improved.

@toolness

toolness Apr 18, 2013

Contributor

Ah cool. Is there a way we can test the caught exception to see if it's the "nunjucks has no idea what template you're talking about" error, and throw if it's not?

@stenington

stenington Apr 18, 2013

Contributor

Yeah, I think that's probably doable.

Contributor

toolness commented on c54a08d Apr 15, 2013

Hmm, I like the idea of this, but there's a number of things that are confusing about it for me--for instance, a lot of the app views throw exceptions or just look weird, presumably b/c they're partials or expect some context stuff. I also am not entirely sure how to use that withUser setup method documented on its main page, or what views to use it with.

I think this could be really cool if there were a form next to each template view, pre-filled with sample data that the view needs, or perhaps just links to them with the proper querystring arguments already filled in.

I'm also a bit concerned about how easily this might bit-rot. And how "discoverable" it is by newcomers--the fakeissuer is already hard enough to find, and it makes me wonder if we should just have a single test app that combines the fakeissuer and this viewer into a single app, where each mini-app is linked to from a helpful index page. Perhaps that's asking too much though.

Anyways, unless you feel particularly strong about it, I think it might be better to leave this out of pull request #812 for now, and submit it as a separate request.

Contributor

stenington commented Apr 18, 2013

I'm closing this to break it out into smaller pull requests.

@stenington stenington closed this Apr 18, 2013

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