Holy Holly -Tram, Beauttie, Ana, Gessica #83
Conversation
There was a problem hiding this comment.
bEtsy
Functional Requirements: Manual Testing
| Workflow | yes / no |
|---|---|
| Before logging in | |
| Browse all products, by category, by merchant | ✔️ |
| Leave a review | |
| Verify unable to create a new product | ✔️ |
| After logging in | |
| Create a category | ✔️ |
| 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 | 😭, I now have 15 orders of "Fluff" in my cart and 10 in inventory! |
| Add another merchant's product | ✔️ |
| Check out | ✔️, However if I break a validation I have to re-enter the entire order form from scratch. You should have just rendered the view. |
| Check that stock was reduced | ✔️ |
| Change order-item's status on dashboard | ✔️, Yep, but why is "shipped" a link? If I click on it it does nothing. |
| Verify unable to leave a review for your own product | ✔️, Can't leave a review period. |
| 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.25% |
| 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 | ✔️ |
| Checkout -> decrease inventory | ✔️ |
| 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 |
✔️, No test for adding an item to an order that's shipped for example |
| 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 |
|
| 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) |
✔️ |
| 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 |
NA |
Code Style Bonus Awards
Was the code particularly impressive in code style for any of these reasons (or more...?)
| Quality | Yes? |
|---|---|
| Perfect Indentation | ✅ |
| Descriptive/Readable | ✅ |
| Concise | ✅ |
| Logical/Organized | ✅ |
Overall Feedback
Great 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 you did authorization with controller filters. I also like your model methods. Finally you had some good tests in there. Nice work!
Areas for improvement:
- You would need some more through testing. I made a few notifications there.
- There are some small styling things like lists overlapping the footer bar and your product images are not clickable. People click on shiny pictures of cookies!
- your routes are not as dry and well-formatted as they could be
- Check your coverage report, you're not testing the product index by category and by merchant!
Overall this is a very solid submission and y'all did a great job and should be very proud of the work you turned in.
Only the person who submitted the PR will get an email about this feedback. Please let the rest of your team know about it.
|
|
||
| resources :categories, only: [:index, :show, :new, :create] | ||
| resources :products | ||
| resources :order_items, only: [:destroy, :update, :create] |
There was a problem hiding this comment.
Do you need the create route here, since you have the nested route.
| @@ -0,0 +1,144 @@ | |||
| require "test_helper" | |||
|
|
|||
| describe Merchant do | |||
| @@ -0,0 +1,100 @@ | |||
| require "test_helper" | |||
|
|
|||
| describe OrderItem do | |||
There was a problem hiding this comment.
👍 , but... you should have a test to verify that you can't create an order item for an order that's not in cart mode. I shouldn't be able to add items to an order after it's shipped for example.
| end | ||
| end | ||
|
|
||
| describe "update_stock" do |
| <% Product.recently_added.each do |product| %> | ||
| <div class="col-md-4"> | ||
| <div class="thumbnail"> | ||
| <%= image_tag(product.photo_url, size: "250x250", alt: "product image" )%> |
There was a problem hiding this comment.
I would also make this image a link.
| end | ||
| end | ||
|
|
||
| # describe "destroy" do |
There was a problem hiding this comment.
These are good tests, and they work if you put the destroy method back in.
However you should include tests for:
- You have to be logged in and the owner of the product to destroy it.
- If you're not logged in you can't delete a product
- If you're logged in and not the owner you can't delete the product.
| def find_product | ||
| product = Product.find_by(id: self.product_id) | ||
| return product | ||
| end |
There was a problem hiding this comment.
Do you need this? Someone could just do order_item.product since it belongs to a product. Ditto for find_order
| @@ -0,0 +1,51 @@ | |||
| class Order < ApplicationRecord | |||
| # # product = products(:product_one) | ||
| # # expect(product.merchant).must_equal merchants(:merch_one) |
There was a problem hiding this comment.
You had never set the product to belong to the merchant in the before block.
| describe 'index' do | ||
| it "gets index" do | ||
| get products_path | ||
| must_respond_with :success | ||
| end | ||
| end |
There was a problem hiding this comment.
😱
You're missing tests for product-by-category and product-by-merchant!
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