Angela, Cara, Dikla, Karinna - pEtsy fEels - Octos#21
Angela, Cara, Dikla, Karinna - pEtsy fEels - Octos#21karinnainiguez wants to merge 289 commits intoAda-C9:masterfrom
Conversation
karinnainiguez
changed the title
bEtsyWhat We're Looking For
Only the person who submitted the PR will get an email about this feedback. Please let the rest of your team know about it. |
| get '/cartitems', to: 'cartitems#index', as: 'cart' | ||
| delete '/cartitems/:id', to: 'cartitems#destroy', as: 'delete_cartitem' | ||
| patch '/cartitems/:id', to: 'cartitems#update', as: 'update_cartitem' | ||
| patch '/cartitems/ship/:id', to: 'cartitems#ship', as: 'ship' |
There was a problem hiding this comment.
The restful route would have the ID between the resource and the action - /cartitems/:id/ship
| @@ -0,0 +1,69 @@ | |||
| class ProductsController < ApplicationController | |||
| before_action :find_product, only: [:show, :edit, :update, :destroy, :retire] | |||
| before_action :require_login, except: [:index, :show] | |||
There was a problem hiding this comment.
Nice job using except here as the list gets long
| t.datetime "created_at", null: false | ||
| t.datetime "updated_at", null: false | ||
| t.bigint "user_id" | ||
| t.string "product_status" |
There was a problem hiding this comment.
I would not choose product_status as a column on the products table - the extra "product" part of the column name is unnecessary
|
|
||
| def index | ||
| if session[:order_id] == nil | ||
| order = Order.create!(state: 'pending') |
There was a problem hiding this comment.
I think that it is a good idea to create the order if it doesn't yet exist - but what if there is an issue here with creating the order?
| else | ||
| @cart_item = Cartitem.new(cartitem_params) | ||
| # find the product from path | ||
| @cart_item.product = Product.find(params[:product_id]) |
| quantity: 3 | ||
| ) | ||
|
|
||
| result = cart_item.valid? |
There was a problem hiding this comment.
Checking the validity of the item is not checking that it can be created. You still need to call .save or .create at some point.
| Order.count.must_equal old_order_count | ||
|
|
||
|
|
||
| # with future date more than 5 years |
There was a problem hiding this comment.
These are all great test cases, and I'd recommend that they'd all be their own test. When trying to figure out if things should be their own test, I try to think: Is there more than one specific set of code that would need to change to fail this test? If so, it might be good to split it out.
| @order.name.wont_equal @order_data[:name] | ||
|
|
||
|
|
||
| # without zipcode |
There was a problem hiding this comment.
This is the same fundamental test and with pieces of the data changed. Can you think of how you could consolidate to be able to use a loop rather than repeating the tests each time?
| user_id: User.first.id | ||
| } | ||
| # Assumptions | ||
| product = Product.new(@product_data1) |
There was a problem hiding this comment.
This is a controller test that is never executing a controller action
| @@ -0,0 +1,45 @@ | |||
| require "test_helper" | |||
|
|
|||
| describe ReviewsController do | |||
There was a problem hiding this comment.
It'd be good to check that a user can't review their own products
5 participants
bEtsy
Congratulations! You're submitting your assignment! These comprehension questions should be answered by all members of your team, not by a single teammate.
Comprehension Questions