-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
J'All (Jocelyn Gonzalez, Averi Kitsch, Laura Robertson, Lauren Cardella) -- Carets #60
base: master
Are you sure you want to change the base?
Conversation
…ontroller tests for edit, update, and delete. Add edit action to order_products_controller. Edit action test passing, other tests failing.
… order index view with delete link method.
…redundant route. Put custom routes under standard resources for readability where appropriate.
bEtsyWhat We're Looking For
IMPORTANT: Whoever submitted the PR (and thus will get the notification about this feedback) should share this with their teammates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work, a few comments below.
def cancel | ||
if find_user | ||
if @order_product.product.user.id == session[:user_id] | ||
if !@order_product.shipped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good candidate of content to move to the model.
describe Category do | ||
let(:category) { Category.new } | ||
describe "validations" do | ||
it "category without name is invalid" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also useful to verify that category.errors.keys.must)include(:name)
end | ||
|
||
it "doesn't create a new category with different capitalization" do | ||
proc {Category.new(name: "PoTions")}.must_change 'Category.count', 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're missing a save here!
cat = Category.first | ||
cat.must_respond_to :products | ||
cat.products << prd | ||
cat.products[0].must_equal products(:one) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make this more generic I would use:
cat.products[0].must_include products(:one)
@order.valid?.must_equal false | ||
end | ||
|
||
["09/18","11/20","9/18"].each do |num| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a good idea for a generic test that uses the current date rather than hard-coded dates.
@@ -0,0 +1,158 @@ | |||
require "test_helper" | |||
|
|||
describe OrderProductsController do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some negative tests you're missing here. Like if the item being shipped or deleted is already gone.
end | ||
|
||
describe "order show" do | ||
it "should get show if logged in" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good testing here, but this looks like 3 tests not one.
must_respond_with :not_found | ||
|
||
end | ||
it "should get show" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of your it
blocks should be more specific, like this one which should say, "will redirect guest users on show actions."
|
||
Order.last.status.must_equal 'pending' | ||
|
||
put order_path(Order.last), params: {order: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the two arrays above you could use a loop to set up this hash.
must_respond_with :success | ||
end | ||
|
||
it "can get found" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test makes no sense.
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