Team MKKRs - Neopetsy #78
Conversation
Validations
Merchant validation
basic review styles, stars function needs help
Last minute edits
Merge refactor
add products and categories csv
…rom routes. changed if else statement in new method to render bad request if product isn't found.
cleaned up reviews controller/tests
Semantic update
bEtsyY'all, you did it! And your site looks SO GOOD! I hope that you show this off to people! If I were you, I'd be so proud of what y'all built together! The two primary areas for improvement I see are around putting more business logic into the models (which I see a comment about, so it's good to see you're aware of that) and adding more testing in general. That said, with a shortage of time, I think y'all did get a good amount of testing done :) In general, there aren't many tests written for custom model tests. They are still marked as covered by SimpleCov but that's only because the controller tests cause those methods to get called. But adding tests for the specific custom methods is still important. Kayla, great work on the cart! It all looks like it works really well! The main area for improvement I see is around moving more of the logic into model custom methods. As a general rule, if I see more than one or two lines of code in a controller that don't deal with flash, session, or control flow (eg. render, redirect, etc.), I ask myself if I can make it into one or more separate model methods instead. Also, the testing for OrderItemsController that is there looks good. There is room for more edge case testing. I'm glad to see that you have the skeletons for more tests so you are obviously aware of more tests that could be added :) Ren, the views look really good! I left a few inline comments in places in the views that could be cleaned up a little. Madeline, the products controller and reviews controller tests look good! The only lines that aren't covered are lines 12-14 of the reviews controller, which can be seen in the SimpleCov report. This could be addressed by adding a test for a product_id that doesn't exist. Kim, the custom methods in Merchant look good! As I noted above most custom methods in the models aren't tested or aren't tested very thoroughly but I assume that's because there just wasn't enough time. Again, great work everyone! This is going to be a site that'll be fun to show off for a long time I think! Functional Requirements: Manual Testing
Major Learning Goals/Code Review
Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
Overall FeedbackOnly the person who submitted the PR will get an email about this feedback. Please let the rest of your team know about it. |
| # standard RESTFUL routes | ||
| resources :categorizations | ||
| resources :order_items #TODO: If you don't need all of these, I just need to be able to update order_item | ||
| resources :orders | ||
| resources :reviews | ||
| resources :products, except: [:delete] #TODO: I think this one is redundant |
There was a problem hiding this comment.
The routes all look good until these lines. There are several of the 7 restful routes that aren't supported by these controllers so there's no need to add routes for them.
| end | ||
|
|
||
| def checkout | ||
| @order = Order.find_by(id: session[:order_id]) |
There was a problem hiding this comment.
This would be a good candidate for a little controller filter
| if [email protected]? | ||
| @order.order_items.each do |item| | ||
| item.product.quantity += item.quantity | ||
| item.product.save | ||
| item.destroy | ||
| end | ||
| end |
There was a problem hiding this comment.
This is code that would be great as a model method.
| @cart_total = get_total | ||
| end | ||
|
|
||
| #should be in model--but...no time to refactor. |
| </section> | ||
|
|
||
| <section class="display-products-container"> | ||
| <%= render partial: 'products/product-grid', locals: { products: @categorization.products.where(active: true) } %> |
There was a problem hiding this comment.
This is one little example of something that could be cleaned up slightly by making a @products variable in the controller with the value @categorization.products.where(active: true) so that you can just reference the variable from the view.
| logged_in_merchant_has_order_items: @login_user && @order.extract_merchant_order_items(@login_user.id).any?, | ||
| merchant_items: @login_user ? @order.extract_merchant_order_items(@login_user.id) : [] |
There was a problem hiding this comment.
these two boolean values could be computed in the controller and assigned variable names to clean up this a little
|
|
||
| <section class="display-products-container"> | ||
| <% if @login_user %> | ||
| <%= render partial: 'product-grid', locals: { products: @products.where.not(merchant_id: @login_user.id, active: false) } %> |
There was a problem hiding this comment.
This is another good example where a variable assignment could happen in the controller to clean this up
Assignment Submission: bEtsy
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions. These should be answered by all members of your team, not by a single teammate.
Reflection