feedback commit #118

case-eee · 2017-01-25T16:35:42Z

feedback only - do NOT merge!

houndci-bot · 2017-01-25T16:36:07Z

spec/features/customers/visitor_can_put_items_in_cart_without_being_logged_in_spec.rb

+
+    expect(page).to have_current_path(cart_path)
+    expect(page).to have_content("#{item_1.description}")
+    expect(page).to have_content("#{item_2.description}")


Prefer to_s over string interpolation.

houndci-bot · 2017-01-25T16:36:07Z

spec/features/customers/visitor_can_put_items_in_cart_without_being_logged_in_spec.rb

+
+    expect(page).to have_current_path(cart_path)
+    expect(page).to have_content("#{item_1.description}")
+    expect(page).to have_content("#{item_2.description}")


Prefer to_s over string interpolation.

houndci-bot · 2017-01-25T16:36:08Z

spec/features/customers/visitor_can_put_items_in_cart_without_being_logged_in_spec.rb

+    create_valid_account
+
+    expect(page).to have_current_path(cart_path)
+    expect(page).to have_content("#{item_1.description}")


Prefer to_s over string interpolation.

houndci-bot · 2017-01-25T16:36:08Z

spec/features/customers/visitor_can_put_items_in_cart_without_being_logged_in_spec.rb

+    create_valid_account
+
+    expect(page).to have_current_path(cart_path)
+    expect(page).to have_content("#{item_1.description}")


Prefer to_s over string interpolation.

houndci-bot · 2017-01-25T16:36:08Z

spec/features/customers/visitor_can_put_items_in_cart_without_being_logged_in_spec.rb

+
+  scenario "the user has two items in their cart" do
+    expect(page).to have_content("#{item_1.description}")
+    expect(page).to have_content("#{item_2.description}")


Prefer to_s over string interpolation.

houndci-bot · 2017-01-25T16:36:09Z

spec/features/customers/visitor_can_put_items_in_cart_without_being_logged_in_spec.rb

+
+  scenario "the user has two items in their cart" do
+    expect(page).to have_content("#{item_1.description}")
+    expect(page).to have_content("#{item_2.description}")


Prefer to_s over string interpolation.

houndci-bot · 2017-01-25T16:36:09Z

spec/features/customers/visitor_can_put_items_in_cart_without_being_logged_in_spec.rb

+  end
+
+  scenario "the user has two items in their cart" do
+    expect(page).to have_content("#{item_1.description}")


Prefer to_s over string interpolation.

houndci-bot · 2017-01-25T16:36:09Z

spec/features/customers/visitor_can_put_items_in_cart_without_being_logged_in_spec.rb

+  end
+
+  scenario "the user has two items in their cart" do
+    expect(page).to have_content("#{item_1.description}")


Prefer to_s over string interpolation.

houndci-bot · 2017-01-25T16:36:09Z

spec/features/customers/visitor_can_put_items_in_cart_without_being_logged_in_spec.rb

+require 'rails_helper'
+
+feature 'When a visitor has items in their cart' do
+


Extra empty line detected at block body beginning.

houndci-bot · 2017-01-25T16:36:10Z

spec/features/customers/visitor_can_put_items_in_cart_without_being_logged_in_spec.rb

+require 'rails_helper'
+
+feature 'When a visitor has items in their cart' do
+


Extra empty line detected at block body beginning.

houndci-bot · 2017-01-25T16:36:10Z

spec/features/customers/visitor_does_not_successfully_create_an_account_spec.rb

+require 'rails_helper'
+
+feature "A visitor does not successfully create an account" do
+


Extra empty line detected at block body beginning.

houndci-bot · 2017-01-25T16:36:10Z

spec/features/customers/visitor_does_not_successfully_create_an_account_spec.rb

+require 'rails_helper'
+
+feature "A visitor does not successfully create an account" do
+


Extra empty line detected at block body beginning.

houndci-bot · 2017-01-25T16:36:10Z

spec/features/customers/visitor_can_create_an_account_spec.rb

+require 'rails_helper'
+
+feature 'A visitor can create an account' do
+


Extra empty line detected at block body beginning.

houndci-bot · 2017-01-25T16:36:11Z

spec/features/customers/visitor_can_create_an_account_spec.rb

+require 'rails_helper'
+
+feature 'A visitor can create an account' do
+


Extra empty line detected at block body beginning.

houndci-bot · 2017-01-25T16:36:11Z

spec/features/customers/unauthenticated_visitor_security_spec.rb

+feature "When a site visitor is unauthenticated" do
+  before do
+    customer = create(:customer)
+    @order_1, order_2, order_3 = create_list(:all_new_order, 3, customer: customer)


Useless assignment to variable - order_2. Use _ or _order_2 as a variable name to indicate that it won't be used.
Useless assignment to variable - order_3. Use _ or _order_3 as a variable name to indicate that it won't be used.

houndci-bot · 2017-01-25T16:36:11Z

spec/features/customers/unauthenticated_visitor_security_spec.rb

+feature "When a site visitor is unauthenticated" do
+  before do
+    customer = create(:customer)
+    @order_1, order_2, order_3 = create_list(:all_new_order, 3, customer: customer)


Useless assignment to variable - order_2. Use _ or _order_2 as a variable name to indicate that it won't be used.
Useless assignment to variable - order_3. Use _ or _order_3 as a variable name to indicate that it won't be used.

houndci-bot · 2017-01-25T16:36:11Z

spec/features/customers/customer_can_navigate_to_dashboard_spec.rb

+
+feature "When a visitor is logged in" do
+  before do
+    Customer.create!( first_name: "Jane",


Space inside parentheses detected.

houndci-bot · 2017-01-25T16:36:12Z

spec/features/customers/customer_can_navigate_to_dashboard_spec.rb

+
+feature "When a visitor is logged in" do
+  before do
+    Customer.create!( first_name: "Jane",


Space inside parentheses detected.

houndci-bot · 2017-01-25T16:36:12Z

spec/features/cart/user_can_add_items_to_cart_spec.rb

+  end
+
+  feature "When a customer visits the items detail page and adds that item to their cart" do
+


Extra empty line detected at block body beginning.

houndci-bot · 2017-01-25T16:36:12Z

spec/features/cart/user_can_add_items_to_cart_spec.rb

+  end
+
+  feature "When a customer visits the items detail page and adds that item to their cart" do
+


Extra empty line detected at block body beginning.

houndci-bot · 2017-01-25T16:36:12Z

spec/features/admins/admin_can_edit_items_spec.rb

+  scenario "the admin updates the item description" do
+    new_description = "Genuine lumberjack pants. Look like you do work, without actually doing it."
+    click_on_edit
+    fill_in "item[description]", with: "#{new_description}"


Prefer to_s over string interpolation.

houndci-bot · 2017-01-25T16:36:13Z

spec/features/admins/admin_can_edit_items_spec.rb

+  scenario "the admin updates the item description" do
+    new_description = "Genuine lumberjack pants. Look like you do work, without actually doing it."
+    click_on_edit
+    fill_in "item[description]", with: "#{new_description}"


Prefer to_s over string interpolation.

houndci-bot · 2017-01-25T16:36:13Z

spec/factories.rb

@@ -0,0 +1,81 @@
+FactoryGirl.define do
+


Extra empty line detected at block body beginning.

houndci-bot · 2017-01-25T16:36:13Z

spec/factories.rb

@@ -0,0 +1,81 @@
+FactoryGirl.define do
+


Extra empty line detected at block body beginning.

houndci-bot · 2017-01-25T16:36:14Z

app/models/order.rb

+  end
+
+  def items_and_quantities
+    items.reduce(Hash.new(0)) do |hash, item|


Use each_with_object instead of reduce.

houndci-bot · 2017-01-25T16:36:14Z

app/models/order.rb

+  end
+
+  def items_and_quantities
+    items.reduce(Hash.new(0)) do |hash, item|


Use each_with_object instead of reduce.

houndci-bot · 2017-01-25T16:36:14Z

app/models/cart.rb

+
+  def clean_cart
+    contents.delete_if do |item_id, quantity|
+      quantity == 0


Use quantity.zero? instead of quantity == 0.

houndci-bot · 2017-01-25T16:36:14Z

app/models/cart.rb

+
+  def clean_cart
+    contents.delete_if do |item_id, quantity|
+      quantity == 0


Use quantity.zero? instead of quantity == 0.

houndci-bot · 2017-01-25T16:36:15Z

app/models/cart.rb

+  private
+
+  def clean_cart
+    contents.delete_if do |item_id, quantity|


Unused block argument - item_id. If it's necessary, use _ or _item_id as an argument name to indicate that it won't be used.

houndci-bot · 2017-01-25T16:36:15Z

app/models/cart.rb

+  end
+
+  def items_with_quantities
+    contents.reduce({}) do |memo, (id, quantity)|


Use each_with_object instead of reduce.

houndci-bot · 2017-01-25T16:36:15Z

app/models/cart.rb

+  private
+
+  def clean_cart
+    contents.delete_if do |item_id, quantity|


Unused block argument - item_id. If it's necessary, use _ or _item_id as an argument name to indicate that it won't be used.

houndci-bot · 2017-01-25T16:36:16Z

app/models/cart.rb

+  end
+
+  def items_with_quantities
+    contents.reduce({}) do |memo, (id, quantity)|


Use each_with_object instead of reduce.

case-eee

@CPowell23 @robbie-smith @ski-climb @lucyconklin great work on this. here's some feedback 😄 sorry for the delay!

case-eee · 2017-01-25T16:36:22Z

README.md

@@ -1,24 +1,11 @@
-# README
+# Little Shop


i suggest adding an overview of what this application does here and perhaps how to pull it down and get it working locally 👍

case-eee · 2017-01-25T16:36:40Z

app/assets/javascripts/checkouts.js

+  });
+});
+
+function stripeResponseHandler(status, response) {


wooo stripe checkout 🎉

case-eee · 2017-01-25T16:37:45Z

app/controllers/admins_controller.rb

+
+  def filter_orders
+    if status_filter.nil?
+      Order.all.most_recent


if most_recent is a class method, we don't need to call all here

case-eee · 2017-01-25T16:38:42Z

app/controllers/application_controller.rb

@@ -1,3 +1,38 @@
 class ApplicationController < ActionController::Base
+  include ApplicationHelper
+  include MessageHelper


we talked about this in your eval a bit, helper modules are conventionally used for your views - their responsibility is formatting output for viewing purposes. i like how you refactored your controllers - could we make a PORO though to handle some of this logic?

case-eee · 2017-01-25T16:40:00Z

app/controllers/checkouts_controller.rb

+  def create
+    customer = current_customer.stripe_customer
+
+    begin


there's a lot of stuff going on here (i know this is all stripe logic) - i suggest pulling this out to a PORO in order to keep your controller more single responsibility 👍

case-eee · 2017-01-25T16:45:46Z

app/views/items/show.html.erb

+      <p><%= @item.description %></p>
+      <div>
+        <h4><%= number_to_currency(@item.price_in_dollars) %></h4>
+        <% if @item.retired == false %>


this is too much logic in your view. i suggest creating a method on Item called retired? that does this for you 😄

case-eee · 2017-01-25T16:46:11Z

app/views/layouts/application.html.erb

-    <%= yield %>
+    <%= render partial: 'layouts/nav' %>
+    <section class="main">
+      <%= render partial: 'shared/errors' %>


woooo shared partials 👍

case-eee · 2017-01-25T16:46:32Z

app/views/shared/_item.html.erb

+    </div>
+  <% end %>
+  <p><%= number_to_currency(item.price_in_dollars) %></p>
+  <% if item.retired == false %>


you could reuse that retired? method here!

case-eee · 2017-01-25T16:47:20Z

app/views/shared/_orders.html.erb

+          <% end %>
+        </td>
+        <td>
+          <% if order.status.paid? || order.status.ordered? %>


this is also a lot of logic in your view. can we create a method in Order that does this for us? maybe something called paid_or_ordered??

case-eee · 2017-01-25T16:47:31Z

config/database.yml

@@ -58,6 +58,9 @@ development:
 test:
  <<: *default
  database: little_shop_test
+
+  adapter: postgresql
+  database: travis_ci_test


🎉 🎉 🎉

feedback commit

e9166dc

houndci-bot reviewed Jan 25, 2017

View reviewed changes

spec/factories.rb

@@ -0,0 +1,81 @@

FactoryGirl.define do

Copy link

houndci-bot Jan 25, 2017

Choose a reason for hiding this comment

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

Extra empty line detected at block body beginning.

houndci-bot reviewed Jan 25, 2017

View reviewed changes

spec/factories.rb

@@ -0,0 +1,81 @@

FactoryGirl.define do

Copy link

houndci-bot Jan 25, 2017

Choose a reason for hiding this comment

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

Extra empty line detected at block body beginning.

houndci-bot reviewed Jan 25, 2017

View reviewed changes

app/models/order.rb

end

def items_and_quantities

items.reduce(Hash.new(0)) do |hash, item|

Copy link

houndci-bot Jan 25, 2017

Choose a reason for hiding this comment

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

Use each_with_object instead of reduce.

houndci-bot reviewed Jan 25, 2017

View reviewed changes

app/models/order.rb

end

def items_and_quantities

items.reduce(Hash.new(0)) do |hash, item|

Copy link

houndci-bot Jan 25, 2017

Choose a reason for hiding this comment

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

Use each_with_object instead of reduce.

houndci-bot reviewed Jan 25, 2017

View reviewed changes

app/models/cart.rb

def clean_cart

contents.delete_if do |item_id, quantity|

quantity == 0

Copy link

houndci-bot Jan 25, 2017

Choose a reason for hiding this comment

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

Use quantity.zero? instead of quantity == 0.

houndci-bot reviewed Jan 25, 2017

View reviewed changes

app/models/cart.rb

def clean_cart

contents.delete_if do |item_id, quantity|

quantity == 0

Copy link

houndci-bot Jan 25, 2017

Choose a reason for hiding this comment

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

Use quantity.zero? instead of quantity == 0.

houndci-bot reviewed Jan 25, 2017

View reviewed changes

app/models/cart.rb

end

def items_with_quantities

contents.reduce({}) do |memo, (id, quantity)|

Copy link

houndci-bot Jan 25, 2017

Choose a reason for hiding this comment

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

Use each_with_object instead of reduce.

houndci-bot reviewed Jan 25, 2017

View reviewed changes

app/models/cart.rb

end

def items_with_quantities

contents.reduce({}) do |memo, (id, quantity)|

Copy link

houndci-bot Jan 25, 2017

Choose a reason for hiding this comment

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

Use each_with_object instead of reduce.

case-eee commented Jan 25, 2017

View reviewed changes

		require 'rails_helper'

		feature 'When a visitor has items in their cart' do

		require 'rails_helper'

		feature "A visitor does not successfully create an account" do

		require 'rails_helper'

		feature 'A visitor can create an account' do

		end

		feature "When a customer visits the items detail page and adds that item to their cart" do

feedback commit #118

Are you sure you want to change the base?

feedback commit #118

Conversation

case-eee commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

case-eee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment