Skip to content
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions app/controllers/wishlist_items_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ def create
authorize wishlist.wishlist_items.build

@wishlist_item = wishlist.wishlist_items.create!(wishlist_item_create_params)
redirect_to wishlist_path(@wishlist_item.wishlist), notice: "Added #{@wishlist_item.name}."
redirect_to wishlist_by_slug_path(@wishlist_item.wishlist.slug), notice: "Added #{@wishlist_item.name}."
end

def edit
Expand All @@ -22,7 +22,7 @@ def update
authorize @wishlist_item

if @wishlist_item.update(wishlist_item_params)
redirect_to @wishlist_item.wishlist, notice: 'Wishlist item was successfully updated.'
redirect_to wishlist_by_slug_path(@wishlist_item.wishlist.slug), notice: 'Wishlist item was successfully updated.'
else
render :edit
end
Expand All @@ -34,7 +34,7 @@ def destroy
wishlist = @wishlist_item.wishlist
@wishlist_item.destroy

redirect_to wishlist, notice: 'Item was successfully removed from wishlist.'
redirect_to wishlist_by_slug_path(wishlist.slug), notice: 'Item was successfully removed from wishlist.'
end

private
Expand Down
6 changes: 3 additions & 3 deletions app/controllers/wishlists_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ class WishlistsController < ApplicationController

def show
skip_authorization
@wishlist = Wishlist.includes(wishlist_items: :item).find(params[:id])
@wishlist = Wishlist.includes(wishlist_items: :item).find_by_slug(params[:slug])
@site_managers = @wishlist.users
@wishlist_items = @wishlist.wishlist_items.priority_order
end
Expand All @@ -25,7 +25,7 @@ def create
attach_site_managers

if @wishlist.save
redirect_to @wishlist, notice: 'Wishlist was successfully created.'
redirect_to wishlist_by_slug_path(@wishlist.slug), notice: 'Wishlist was successfully created.'
else
render :new
end
Expand All @@ -35,7 +35,7 @@ def update
authorize @wishlist
attach_site_managers
if @wishlist.update(wishlist_params)
redirect_to @wishlist, notice: 'Wishlist was successfully updated.'
redirect_to wishlist_by_slug_path(@wishlist.slug), notice: 'Wishlist was successfully updated.'
else
render :edit
end
Expand Down
9 changes: 9 additions & 0 deletions app/models/wishlist.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,13 @@ class Wishlist < ApplicationRecord

validates :name, presence: true,
uniqueness: true

before_save :create_slug
before_update :create_slug

private

def create_slug
self.slug = name.parameterize

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

end
end
2 changes: 1 addition & 1 deletion app/views/partials/_primary_nav.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

<div class="dropdown-menu" id="wishlists">
<% @wishlists.each do |wishlist| %>
<%= link_to wishlist.name, wishlist_path(wishlist),
<%= link_to wishlist.name, wishlist_by_slug_path(wishlist.slug),
class: "wishlist dropdown-item" %>
<% end %>
</div>
Expand Down
2 changes: 1 addition & 1 deletion app/views/pledges/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
<dd class="col-sm-10"><%= pledge.wishlist_item.name %></dd>

<dt class="col-sm-2">Wishlist</dt>
<dd class="col-sm-10"><%= link_to pledge.wishlist.name, pledge.wishlist %></dd>
<dd class="col-sm-10"><%= link_to pledge.wishlist.name, wishlist_by_slug_path(pledge.wishlist.slug) %></dd>

<dt class="col-sm-2">Pledging user</dt>
<dd class="col-sm-10">
Expand Down
2 changes: 1 addition & 1 deletion app/views/pledges/_pledge.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
<dl class="row">
<dt class="col-md-3">Wishlist</dt>
<dd class="col-md-9">
<%= link_to pledge.wishlist_name, pledge.wishlist %>
<%= link_to pledge.wishlist_name, wishlist_by_slug_path(pledge.wishlist.slug) %>
</dd>

<dt class="col-md-3">Number pledged</dt>
Expand Down
2 changes: 1 addition & 1 deletion app/views/pledges/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,6 @@
</p>
<% end %>

<%= link_to "Back to #{@pledge.wishlist.name}", @pledge.wishlist %> |
<%= link_to "Back to #{@pledge.wishlist.name}", wishlist_by_slug_path(@pledge.wishlist.slug) %> |
<%= link_to "Back to user page", @pledge.user %>
</div>
2 changes: 1 addition & 1 deletion app/views/wishlists/edit.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@

<%= render 'form', wishlist: @wishlist %>

<%= link_to 'Back to Wishlist', @wishlist %>
<%= link_to 'Back to Wishlist', wishlist_by_slug_path(@wishlist.slug) %>
</div>
4 changes: 3 additions & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@
end

# Wishlists & Items
resources :wishlists, except: [:index] do
resources :wishlists, except: [:index, :show] do
resources :wishlist_items, shallow: true,
only: [:create, :edit, :update, :destroy]
resource :amazon_search, controller: :amazon_search,
only: [:new, :show]
end

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Whitespace! 😮

get 'wishlists/:slug', to: 'wishlists#show', as: :wishlist_by_slug

# Users
resources :users
Expand Down
5 changes: 5 additions & 0 deletions db/migrate/20171023124712_add_column_slug_to_wishlists.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddColumnSlugToWishlists < ActiveRecord::Migration[5.1]
def change
add_column :wishlists, :slug, :string

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's add a unique index to this column since we search by it so often. We should probably disallow nulls too.

end
end
3 changes: 2 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 20170908012526) do
ActiveRecord::Schema.define(version: 20171023124712) do

# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
Expand Down Expand Up @@ -76,6 +76,7 @@
t.text "name"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.string "slug"
end

add_foreign_key "pledges", "users"
Expand Down
6 changes: 3 additions & 3 deletions spec/controllers/wishlists_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
describe "GET #show" do
it "returns a success response" do
wishlist = create(:wishlist, valid_attributes)
get :show, params: {id: wishlist.to_param}, session: {}
get :show, params: {slug: wishlist.slug}, session: {}
expect(response).to be_success
end
end
Expand Down Expand Up @@ -182,7 +182,7 @@
it "redirects to the created wishlist" do
post :create, params: {wishlist: valid_attributes},
session: admin_session
expect(response).to redirect_to(Wishlist.last)
expect(response).to redirect_to wishlist_by_slug_path(Wishlist.last.slug)
end
end

Expand Down Expand Up @@ -284,7 +284,7 @@
put :update, params: {id: wishlist.to_param,
wishlist: valid_attributes},
session: admin_session
expect(response).to redirect_to(wishlist)
expect(response).to redirect_to wishlist_by_slug_path(wishlist.slug)
end
end

Expand Down
2 changes: 1 addition & 1 deletion spec/features/browsing_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
visit "/"
click_link "St. Joseph's"

expect(current_path).to eq wishlist_path(st_josephs)
expect(current_path).to eq wishlist_by_slug_path(st_josephs.slug)
end

context "when there are wishlist items" do
Expand Down
17 changes: 8 additions & 9 deletions spec/features/managing_items_and_wishlists_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
let(:wishlist) { site_manager.wishlists.first }

scenario "I can add a new item to my wishlist", :external do
visit wishlist_path(wishlist)
visit wishlist_by_slug_path(wishlist.slug)
click_link "Add to Wishlist"
fill_in "search_field", with: "corgi"
click_button "Search Amazon"
Expand All @@ -34,7 +34,7 @@
end

scenario "My search can't be blank", :external do
visit wishlist_path(wishlist)
visit wishlist_by_slug_path(wishlist.slug)
click_link "Add to Wishlist"
click_button "Search Amazon"

Expand All @@ -44,7 +44,7 @@
scenario "I can edit a wishlist item on my wishlist" do
create(:wishlist_item, wishlist: wishlist)

visit wishlist_path(wishlist)
visit wishlist_by_slug_path(wishlist.slug)
within ".wishlist-item-tools" do
click_link "Edit"
end
Expand All @@ -58,7 +58,7 @@

scenario "I can remove an item from my wishlist" do
create(:wishlist_item, wishlist: wishlist, staff_message: "Another item.")
visit wishlist_path(wishlist)
visit wishlist_by_slug_path(wishlist.slug)

within ".wishlist-item-tools" do
click_link "Remove"
Expand All @@ -69,16 +69,16 @@
end

scenario "I cannot delete my wishlist" do
visit wishlist_path(wishlist)
visit wishlist_by_slug_path(wishlist.slug)

visit wishlist_path(wishlist)
visit wishlist_by_slug_path(wishlist.slug)
within "#wishlist-actions" do
expect(page).not_to have_link "Destroy"
end
end

scenario "I cannot edit my wishlist" do
visit wishlist_path(wishlist)
visit wishlist_by_slug_path(wishlist.slug)
expect(page).not_to have_link "Edit Wishlist"

visit edit_wishlist_path(wishlist)
Expand All @@ -104,12 +104,11 @@

scenario "I can update an existing wishlist" do

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

click_link "DC General"
dc_general_path = current_path
click_link "Edit Wishlist"
fill_in("wishlist_name", with: "VA General")
click_button "Update Wishlist"

expect(current_path).to eq dc_general_path
expect(current_path).to eq wishlist_by_slug_path('va-general')
expect(page).to have_text "Wishlist was successfully updated."
expect(page).to have_text "VA General"
end
Expand Down
2 changes: 1 addition & 1 deletion spec/features/pledging_an_item_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@
end

scenario "I can re-pledge an item to increment its quantity" do
visit wishlist_path(pledge.wishlist)
visit wishlist_by_slug_path(pledge.wishlist.slug)
click_button "Pledge to Donate"
expect(page).to have_text "Number pledged 2"
end
Expand Down
3 changes: 1 addition & 2 deletions spec/routing/wishlists_routing_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
end

it "routes to #show" do
expect(:get => "/wishlists/1").to route_to("wishlists#show", :id => "1")
expect(:get => "/wishlists/dc-general").to route_to("wishlists#show", :slug => "dc-general")
end

it "routes to #edit" do
Expand All @@ -30,6 +30,5 @@
it "routes to #destroy" do
expect(:delete => "/wishlists/1").to route_to("wishlists#destroy", :id => "1")
end

end
end