Tapestry

Review comments:

I pushed 1249f636f76bb7502436e10af0f4e5773456eae8 to fix up some image tags.

Beyond that:

app/views/samples/index.html.erb needs better solution than removing leading slash
app/views/samples/receive.html.erb needs better solution than removing leading slash
app/views/traitwise/index.html.erb needs better solution than removing leading slash
app/views/users/index.html.erb needs better solution than removing leading slash

public/javascripts/kit_designs.js needs better solution than removing leading slash

public/stylesheets/tapestry_default.css changing /images to ../images can't be the correct solution here, right?

config/config.defaults.yml: can you use root_url instead of hardcoding?

I agree about the better solutions: I blame the way the routing is currently, with IDs built into the URL paths, of which I'm not a big fan usually in any case. There's also something of an excess of routing, using 'match' (which accepts all HTTP verbs) and 'resources' indiscriminately. Both of these problems lead to all kinds of routes that are "open" that oughtn't to be. And as someone who's coming to Tapestry afresh, it means I am not sure which routes are open because they've actually been implemented and need to be tested and dealt with, and so. (This is part of why it won't necessarily work very efficiently for Joe to use `rake routes` to figure out what he needs to examine!)

Anyway, for example, in the samples/index.html.erb, every path needs an ID in order to use the correct named path, but we don't have them when the .erb is being interpreted, because it's Javascript that will be called later, by the client, etc... So we want routes that don't require the IDs in advance!

Personally, I am a fan of very simple, explicit routes, using 'get' and 'put' and 'post' and avoiding 'match' and 'resource' and 'resources' unless something truly has every aspect of CRUD already implemented, or unless one is diligent about specifying which verbs and actions are supported.

Really I think the best solution to the datatables issue of javascript population of rows is for Rails partials to be employed for the rows. This way we can use the correct paths. But I feel as if this should be a separate dev task. Sorather than trying to resolve all these issues, I've opted instead for adding a few redundant, simple, explicit routes, and using them in the JS, and passing parameters the old-fashioned way. So, e.g.:

  var show_sample_href = '<%= sample_show_path %>?id=' + sample_id;

Even though it's "redundant", since there's the Rails <%= sample_path(sample_id) %>. We keep them both so that the references elsewhere to sample_path(id) keep working.

I know it's tempting to use

  var show_sample_href = '<%= samples_path %>/' + sample_id;

...rather than create a new route, but I feel that this is a more perilous solution because it finds the right path using the wrong route, and relies on the samples index always being '/samples' and not something else...

Anyway, I hope you agree. I did the same thing for the public profile url and others you'll see. I don't see a huge problem with there being several synonyms for a resource, so long as they all go to the same controller and action and so the logic is centralized. But we either have to:

• Leave the javascript as is and have two synomous routes
• Change the way the javascript works so it can use the original routes
• Change the original routes, losing the prettier "/profile/phil_xx0FH456" in exchange for "/profile_public?hex=phil_xx0FH456". According to RESTful philosophies, "/profile/phil_xx0FH456" is more correct because it is a URI which identifies a resource.

Note, about samples/index.html.erb: there was a 'delete' path that could not have worked before. I am not sure how this is supposed to work, as I see a couple of different ways that destruction is handled for these samples. I feel as if this too is a separate dev task, so I've just disabled the visibility of that link.

Not sure what your take on all of this is, but anyway I've laid out the thinking behind the relative path commits.

As for the .js and .css files... As far as I know, all this improves in Rails 3.1 with the Asset Pipeline, and we shouldn't lose our heads over trying to figure out the best way to do it in Rails 3.0... For the .js I moved the code to the html.erb, and for the .css I have to say that the "../images" works for me, and we will have to this differently in Rails 3.1 (using something very much along the lines of what you pushed earlier).

Lastly, for the config.defaults.yml, one can't just use 'signup_url' or 'root_url'. I actually spent a little time trying to solve this. I discovered that the routing for the application is not known at the time that these initializers are run. I found that there might be some mildly unorthodox techniques for forcing the loading of the routes, but in the limited time I spent on this I did not find any "proper" way to handle this. I considered a couple of defaulting options that could be handled via helpers, but I'm always wary of too much helper magic. In the end you can see that I thought that since a part of configuration was setting a few important URLs in the config file, it would not cost much to have this be another one of the handful of URL configurations. I improved the situation mildly from the status quo by moving the config file entry next to the lines where the root_url and root_url_scheme are defined.

Okay, that seems like a reasonable compromise.

Just one thing - we definitely should not move away from proper REST routes.

I'm not 100% happy with the dual paths but for now that seems like a net improvement.

With regard to the ../images/pgp.gif - you are right, css url() calls are relative to the location of the stylesheet, so that's a totally valid fix.

Looks good to merge now I think!