branches - Diana Caroline Kelsey Steph #92
Conversation
Steph team
Diana weekend - Monday work
Kelsey 1027
working on viewing products by merchant id
Kelsey 1027
Diana weekend updates to merchant dashboard
css font color
Kelsey 1031
Kelsey 1031
Caroline checkout
Caroline checkout
center left align fixed
Kelsey 1031
bEtsyWhat We're Looking ForManual testing
Code Review
Add item to cart: Overall FeedbackGreat work overall! You've built a fully functional web store from top to bottom. This represents a huge amount of work, and you should be proud of yourselves!. I am particularly impressed by the way that the website looks and feels quite usable and clean and much of the testing is quite thorough. I do see some room for improvement around clarity on what code belongs in a controller and what belongs in a model. Also, clarity on when parameterized routes are useful. bEtsy is a huge project on a very short timeline, and this feedback should not at all diminish the magnitude of what you've accomplished. Keep up the hard work! 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 "/orders/checkout", to: "orders#checkout", as: "checkout" | ||
|
|
||
| resources :orders do | ||
| resources :order_items |
There was a problem hiding this comment.
this could stand to be limited with an "only" param
| # for merchants' eyes only | ||
| # sending only the relevant order_items and orders | ||
| @order_items = OrderItem.by_merchant(session[:merchant_id]) | ||
| @orders = @order_items.map { |order_item| order_item.order } | ||
| @orders.uniq! | ||
|
|
||
|
|
||
| # if merchant wants to FILTER BY ORDER STATUS | ||
| @statuses = %w[ALL SHIPPED PAID PENDING] | ||
| if params[:status_selected] | ||
| statuses_index = params[:status_selected].to_i | ||
|
|
||
| # @orders and @order_items will get filtered depending on status selected | ||
| @database_status = @statuses[statuses_index].downcase | ||
| if @database_status == "all" | ||
| # leave @order_items as is | ||
| return | ||
| else | ||
| @order_items = @order_items.find_all { |order_item| order_item.status == @database_status } | ||
| @orders = @order_items.map { |order_item| order_item.order } | ||
| @orders.uniq! |
There was a problem hiding this comment.
Most of this is business logic that should live in models. As a rule of thumb, when I see more than a line or two that isn't related to control flow (render, redirect, etc.) or defining variables for views, I'm going to feel inclined to put the code in a model.
There was a problem hiding this comment.
Also almost all of these variable aren't used by the view and therefore shouldn't be instance variables.
There was a problem hiding this comment.
Also also, when refactoring this code to move most of it to the model, I would not just move it to one method in the model; I would figure out how to break it into several small methods.
In general, IMO, if a method is more than 30ish lines, it's worth considering how it could be broken down into smaller methods.
| it "A Product Can Have Many Reviews" do | ||
| product = products(:p1) | ||
| test_review_one = Review.create(rating: 1, comment:"It sucks!", product_id: product.id) | ||
| test_review_two = Review.create(rating: 5, comment:"wow!", product_id: product.id) | ||
| get new_product_review_path(product.id) | ||
|
|
||
| must_respond_with :success | ||
| expect(test_review_one.comment).must_include "It sucks!" | ||
| expect(test_review_two.comment).must_include "wow" | ||
| end |
There was a problem hiding this comment.
This is a model test but it's in the controller test file.
| require "test_helper" | ||
|
|
||
| describe ReviewsController do | ||
| describe "create" do |
There was a problem hiding this comment.
create could stand to have some edge case testing
| @@ -0,0 +1,164 @@ | |||
| require "test_helper" | |||
| describe OrderItemsController do | |||
There was a problem hiding this comment.
As I mention in another comment, there's a logic in this controller that belongs in the model. That said, the testing here is very well-written and thorough!
| describe "#index action" do | ||
| it "responds with success when there is at least one Product saved" do | ||
| new_product = products(:p1) | ||
|
|
||
| get products_path | ||
|
|
||
| must_respond_with :success | ||
| expect(Product.count).must_be :>, 0 | ||
| end | ||
|
|
||
| it "responds with success when there are products to show " do | ||
| get products_path | ||
|
|
||
| must_respond_with :success | ||
| expect(Product.count).must_equal 5 | ||
| end | ||
| end |
There was a problem hiding this comment.
These tests are actually testing the same condition. (The arrange in the 1st test isn't actually changing the db.) An interesting edge case would be to delete all the products and make sure the page still responds the way you want it to.
| @@ -0,0 +1,278 @@ | |||
| class OrdersController < ApplicationController | |||
There was a problem hiding this comment.
I wrote the below comment on the index action but wanted to say it applied to almost every method in this controller.
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