Water && Fire @ THE BESTIES: Sandy, Ida, Beatrice and Hanh Solo #62
Conversation
Product crud
Oauth users
Productcontroller
Sandy branch 03
Sandy tests branch
order item tests
Added spacing to homepage title for better presentation Added border to homepage pictures to match products page Added more pics of Churro
Added spacing to homepage title for better presentation Added border to homepage pictures to match products page Added more pics of Churro
Sandy bootstrap form
There was a problem hiding this comment.
Great job! Your project made me super nostalgic which I think was exactly what you were setting out for.
While there were few rough edges you worked together great as a team and overcame a number of personal difficulties together, congratulations on making a working online store!
This project was intentionally a huge lift and you rose to the challenge admirably!
Also, don't take the rubric too seriously it's mostly to just make sure we look at everything important. (There's a reason we don't give grades depending on the number of check marks...)
I listed you as yellow mostly because of missing reviews, but a yellow on bEtsy is still an impressive accomplishment!
bEtsy
Functional Requirements: Manual Testing
| Workflow | yes / no |
|---|---|
| Before logging in | |
| Browse all products, by category, by merchant | Categories not implemented. |
| Leave a review | Reviews not implemented. |
| Verify unable to create a new product | ✔️ |
| After logging in | |
| Create a category | Categories not implemented. |
| Create a product in that category with stock 10 | ✔️ |
| Add the product you created to your cart | ✔️ |
| Add it again (should update quantity) | ✔️ |
| Verify unable to increase quantity beyond stock | Not implemented. |
| Add another merchant's product | ✔️ |
| Check out | ✔️ |
| Check that stock was reduced | Not implemented. |
| Change order-item's status on dashboard | ✔️ |
| Verify unable to leave a review for your own product | Reviews not implemented. |
| Verify unable to edit another merchant's product by manually editing URL | |
| Verify unable to see another merchant's dashboard by manually editing URL | ✔️ |
Major Learning Goals/Code Review
| Criteria | yes / no |
|---|---|
| 90% reported coverage for all controller and model classes using SimpleCov | ✔️ 98.72%! 🎉 |
| Routes | |
No un-needed routes generated (check reviews) |
✔️ |
| Routes not overly-nested (check products and merchants) | ✔️ |
| Merchant dashboard and cart page use a non-parameterized routes (should pull merchant or cart ID from session) | ✔️ |
| Controllers | |
| Controller-filter to require login is applied to all merchant-specific actions (update/add item, add category, view merchant dashboard, etc.) - filter method is not duplicated across multiple files | ✔️ |
| Helper methods or filters to find logged-in user, cart, product, etc | ✔️ |
| No excessive business logic | ✔️ |
| Business logic that ought to live in the model | |
| Add / remove / update product on order | Not implemented. |
| Checkout -> decrease inventory | Not implemented. |
| Merchant's total revenue | ✔️ |
Find all orders for this merchant (instance method on Merchant) |
✔️ |
| Selected Model Tests | |
| Add item to cart: - Can add a good product - Can't add a product w/o enough stock - Can't add a retired product - Can't add to an order that's not in cart mode - Logic specific to this implementation |
Some edge cases missing. |
| Get orders for this merchant: - Includes all orders from this merchant - Doesn't include orders from another merchant - Orders are not included more than once - Does something reasonable when there are no orders for this merchant |
Some edge cases missing. |
| Selected Controller Tests | |
| Add item to cart: - Empty cart (should be created) - Cart already exists (should add to same order) - Product already in cart (should update quantity) - Bad product ID, product is retired, quantity too high, or something like that (error) |
Some edge cases missing. |
| Leave a review: - Works when not logged in - Works when logged in as someone other than the product's merchant - Doesn't work if logged in as this product's merchant - Doesn't work if validations fail |
Reviews not implemented. |
Code Style Bonus Awards
Was the code particularly impressive in code style for any of these reasons (or more...?)
| Quality | Yes? |
|---|---|
| Perfect Indentation | ✅ |
| Elegant/Clever | ✅ |
| Descriptive/Readable | ✅ |
| Concise | ✅ |
| Logical/Organized | ✅ |
Only the person who submitted the PR will get an email about this feedback. Please let the rest of your team know about it.
| /*! | ||
| * Bootstrap v4.5.3 (https://getbootstrap.com/) | ||
| * Copyright 2011-2020 The Bootstrap Authors | ||
| * Copyright 2011-2020 Twitter, Inc. | ||
| * Licensed under MIT (https://github.com/twbs/bootstrap/blob/main/LICENSE) | ||
| */ |
There was a problem hiding this comment.
You shouldn't have needed to copy/paste Boostrap into here. It was already imported on line 16.
| @@ -0,0 +1,28 @@ | |||
| class ApplicationController < ActionController::Base | |||
| # before_action :require_login | |||
There was a problem hiding this comment.
You should try to remember to clean up commented out code before pushing in the future.
| if @product.nil? | ||
| flash[:error] = "Product to add to cart not found" | ||
| redirect_to products_path | ||
| return | ||
| end |
There was a problem hiding this comment.
This logic should probably have lived in a controller filter, or at least a helper method (since you likely want to share this logic with edit_quantity and destroy).
|
|
||
| private | ||
|
|
||
| def find_order |
There was a problem hiding this comment.
This just being named find_order makes me expect it will do it by id. I would probably have named it something like: find_order_for_shopper
| def find_product | ||
| @product = Product.find_by(id: params[:id]) | ||
| end |
There was a problem hiding this comment.
The flash and redirect behavior you rolled one off above should probably have been moved into here:
| def find_product | |
| @product = Product.find_by(id: params[:id]) | |
| end | |
| def find_product | |
| @product = Product.find_by(id: params[:id]) | |
| if @product.nil? | |
| flash[:error] = "Cannot find product" | |
| redirect_to products_path | |
| return | |
| end | |
| end |
| <h3 class ="spotlight-padding">Your cart is empty!</h3> | ||
| <h3 class = "spotlight-padding"><%= link_to "Keep Shopping?", products_path %></h3> |
There was a problem hiding this comment.
You shouldn't use h3s for styling like this. You should be able to take all of the headings from a page and make a table of contents.
It's not something that's often done by sighted people but is really important if someone is trying to access your site with a screen reader.
| if @order_item = OrderItem.find_by(product_id: @product.id, order_id: @shopper) | ||
| # Add to the existing quantity instead of creating a new item | ||
| @order_item.add_quantity(params["quantity"].to_i) | ||
| @order_item.save |
There was a problem hiding this comment.
| @order_item.save | |
| @order_item.save! |
| order.products.each do |product| | ||
| return true if self.products.include?(product) | ||
| end |
There was a problem hiding this comment.
I think this loop always returns true.
I suspect there should have been a comparison to the merchant in here somewhere.
|
|
||
| # validates_associated :order_items # one way street, infinite loop otherwise | ||
| validates :name, format: { :with => /\A[a-zA-Z]+\z/, :message => "Only letters allowed" }, :allow_nil => true | ||
| validates :email, format: { with: /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\z/i }, :allow_nil => true |
There was a problem hiding this comment.
Nice email regex. I assume you grabbed it form somewhere.
Fun fact, there's an input type for email that will do client side validation for you: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/email
(You still need server side validation because of stuff like Postman though.)
| spotlight_for_all_products = Product.all.sample | ||
| return spotlight_for_all_products |
There was a problem hiding this comment.
You don't need to assign the result to a variable here since you immediately return it:
| spotlight_for_all_products = Product.all.sample | |
| return spotlight_for_all_products | |
| return Product.all.sample |
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