Pipes - Iuliia, Eva, Lindsey, Rebecca - Betsy#66
Pipes - Iuliia, Eva, Lindsey, Rebecca - Betsy#66julalam wants to merge 328 commits intoAda-C8:masterfrom
Conversation
Categoriesproducts
…of OrderItems to proceed
Reviews tests
Review model tests
Order merchant view
Fix total cost
fixed all the tests but one
Change layout for 404 page
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. |
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
|
|
||
| <title><%= content_for?(:title) ? yield(:title) : "Untitled" %></title> | ||
|
|
There was a problem hiding this comment.
This stuff in the <title> tag doesn't seem to have worked - the title of your page is always "Untitled".
| resources :products, only: [:index] | ||
| end | ||
|
|
||
| resources :merchants do |
There was a problem hiding this comment.
All of these resources :merchants blocks could be consolidated.
There was a problem hiding this comment.
Also, many of these routes only work for the currently logged in merchant. To me this implies that the merchant ID in the URL is redundant, and you could take this information from the session instead.
| @order.order_items.each do |item| | ||
| @product = Product.find(item.product_id) | ||
| @product.stock -= item.quantity | ||
| @product.save |
There was a problem hiding this comment.
You should be checking the return value of .save here.
| return false | ||
| else | ||
| return true | ||
| end |
There was a problem hiding this comment.
Because this method is used as a controller filter, you don't need the return here - in fact, the return value is never read. Just redirecting is enough to make sure the action won't be run.
| def consoldate_order_items(order_id, product_id, quantity) | ||
| @order_items = OrderItem.where(order_id: order_id) | ||
| product_id = product_id.to_i | ||
| quantity = quantity.to_i |
There was a problem hiding this comment.
I like that you've separated this out into its own method!
This functionality might be a good candidate for a model method, perhaps an instance method on Order. You would have to rework the way you report status to the user a bit.
| params[:order].each do |key, value| | ||
| if value == "" | ||
| flash[:status] = :failure | ||
| flash[:message] = "Any of required fields can't be empty" |
There was a problem hiding this comment.
This seems like a job for validations. Maybe a Payment needs to be a separate model, and an Order optionally has_one Payment.
| flash[:message] = "Any of required fields can't be empty" | ||
| redirect_to edit_order_path(@order) | ||
| return | ||
| end |
There was a problem hiding this comment.
Instead of doing a redirect_to here you should use render, so that data the user has entered will stay in the form to get fixed.
|
|
||
| describe CategoriesController do | ||
| describe "correct logged in user" do | ||
| before do |
There was a problem hiding this comment.
I very much like the pattern of splitting these tests into 3 sub-categories.
| #Assert | ||
| #1. gave the order a cart | ||
| OrderItem.first.order_id.wont_equal nil | ||
| #2. set the session order id |
| merchant: eva | ||
|
|
||
| two: | ||
| name: product2 |
There was a problem hiding this comment.
Pro tip: you can use ERB in your fixture files!
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