Puzzle route from numeric database id to slug

[brentvatne]

In response to issue #58 - changed the route for puzzles from /puzzles/id to /puzzles/slug. The slugging engine is adapted from University Web Slugger. Note, however, that the actual params variable referenced for puzzles did not change from id.

What I mean by this is that to find a puzzle in the PuzzlesController, we now do find_by_slug!(params[:id]) rather than find(params[:id]) If anyone knows how to do this without adding a lot of complexity to the nested puzzle/slug/comment and puzzle/slug/submission routes, that might be desirable. Thoughts?

I was also unsure how to properly test the JavaScript in the new puzzle form in test/integration/admin/puzzles_test.rb as the slug is generated via an ajax call to the SluggerController - instead I just filled in the slug value.

We should ask ourselves before we merge this to master: what should we do about the old routes? Should there be backwards compatibility so /puzzles/1 correctly leads the user to /puzzles/whatever-the-name-is-for-id-one?

[brentvatne]

We should probably also consider whether we want to add an additional filter to the slug to make it looks nicer. At the moment, it would translate puzzle names from [1] Six Degrees of Separation to [1]-six-degrees-of-separation, when perhaps it is desirable to instead have either 1-six-degrees-of-separation or six-degrees-of-separation

[brentvatne]

Also, here's a quick rake task to generate slugs for the already existing Puzzles:

task :generate_slugs => :environment do
  Puzzle.all.each do |puzzle|
    puzzle.slug = Slugger.generate(puzzle.name)
    puzzle.save!
  end
end

[madebydna]

This looks really good, Brent!

Just a few things I noticed:

1. You need to change the Admin::PuzzlesController to also find puzzles by slug instead of by id.
2. Remove the debug line from the PuzzleNode.Slugger.generate function. It causes a console is undefined in IE. Not that I'm using IE, but I the problem was brought to my attention in one of my own projects :)

The rake task you suggested works fine. You should add that too as a separate commit. With the rake task in place, I don't think we need to worry about backwards compatibility of the puzzle IDs, but I'll have Jordan weigh in on this question.

Regarding the pretty URL question, I think we should include the ordinal number of the puzzle as part of the slug, but lose the square brackets, so this version: 1-six-degrees-of-separation. You should be able to do that with an additional regex in the Slugger#generate method.

[gjp]

Just to clarify...is the ordinal entered manually at the start of the title?

[madebydna]

Yes, they are manually entered. Right now there is a confusing discrepancy between the ordinal number of the puzzle and its id that shows up in the URL. That's way this slug feature is pretty important.

[brentvatne]

@madebydna Good points. I can't run the code from here (in the Dominican Republic at a friend's wedding), but I think it should work because I overwrote to_param on Puzzle and made it return slug. This makes me wonder which method is desirable - should we explicitly use find_by_slug! from the id or puzzle_id param (as far as I know, changing this in the route to puzzle_slug would be more work than it's worth because we would have to re-write the resources routes), or if we should just use the find method, which has the same behaviour in the sense that it raises an exception when a matching record is not found. While find_by_slug! would make it more clear that we are using slugs, find is usually the method you expect to be able to use for id params, so I'm not sure which would be better.

I had left that console.log in exactly as it was in the University-web version of slugger, thinking that perhaps it was desirable somehow.

I agree on losing the square brackets, looks much nicer!

I'll get on this as soon as I'm back from my vacations, just after Christmas.

[brentvatne]

Hi folks, sorry for the delay on this - it slipped my mind until now, but I wanted to at least get @madebydna's fixes in before starting up the RMU semester. Any other changes will need to wait until February! :) All of the suggested fixes are in place if I'm not mistaken.

[jordanbyron]

@brentvatne sorry it took so long to get this merged, but I finally got around to checking your patch out and merged it into master. Thanks for working on this!