Use slug in wishlist url #164
Conversation
…sing parameter slug
…direct to wishlist_by_slug_path
There was a problem hiding this comment.
Have you heard of the ActiveRecord #to_param method? (docs) According to the documentation:
You can override to_param in your model to make user_path construct a path using the user’s name instead of the user’s id:
class User < ActiveRecord::Base def to_param # overridden name end end user = User.find_by_name('Phusion') user_path(user) # => "/users/Phusion"
You can also access the param by calling @object.param, which is handy for the controller specs.
It looks like that would cut down on the number of custom methods you're using.
|
@leesharma oh, not heard about that method, thanks for that. I will edit my PR |
|
@leesharma I updated my PR. |
There was a problem hiding this comment.
Nice work! It looks like this is almost there. 😄
I made some comments below, but here's the summary:
- We need to handle the case of attempted slug duplication. Since we're using them for urls, they can't be allowed; we should handle that gracefully.
- Since the slug is used to find wishlist items, a unique index should be added to the db. It should also not be nullable.
- This change needs some unit tests: creating a wishlist should set a slug, updating a wishlist should set a slug, and attempted slug duplication should be handled reasonably.
Let me know if you have any questions/thoughts!
config/routes.rb
Outdated
| @@ -11,7 +11,7 @@ | |||
| resource :amazon_search, controller: :amazon_search, | |||
| only: [:new, :show] | |||
| end | |||
|
|
|||
| @@ -0,0 +1,5 @@ | |||
| class AddColumnSlugToWishlists < ActiveRecord::Migration[5.1] | |||
| def change | |||
| add_column :wishlists, :slug, :string | |||
There was a problem hiding this comment.
Let's add a unique index to this column since we search by it so often. We should probably disallow nulls too.
| private | ||
|
|
||
| def create_slug | ||
| self.slug = name.parameterize |
There was a problem hiding this comment.
What happens when an admin tries to add/update a valid wishlist that results in a duplicate slug? For example, "Wishlist 1" and "wishlist-1" are unique names but would result in the same slug. Since we're using them as the param, slugs must be unique.
Validations are run before the before_save callback, so a simple validation won't work. Maybe a before_validation callback or some sort of slug incrementing would work? I bet someone's solved this and written a blog post.
| @@ -104,12 +104,11 @@ | |||
|
|
|||
| scenario "I can update an existing wishlist" do | |||
There was a problem hiding this comment.
This change is fine, but let's really test this change in the unit tests (spec/models/wishlist_spec.rb).
For the model specs, you'll want to verify that the slug gets set correctly after a save or an update. We'll also need to test how wishlist items with duplicate slugs are handled (see above). We probably don't need to test input edge cases since String#parameterize/ActiveSupport are well-tested already.
|
@leesharma what if I concat the id and the slug, 1-dc-general ? |
Resolves #156