Fire - Earth DREAM TEAM (Mona, Kalki, Emily, Anya) #72
Conversation
…eleted margin in header
categort model test added
anyatokar
changed the title
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 | ✔️ Consider a drop down for merchants similar to categories. |
| Leave a review | ✔️ |
| Verify unable to create a new product | ✔️ I was able to view the new page, but not able to create product. Consider using require_login filter to keep guest user from seeing this page. |
| 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) | the quantity was not updated |
| Verify unable to increase quantity beyond stock | ✔️ |
| Add another merchant's product | ✔️ |
| Check out | ✔️ |
| Check that stock was reduced | stock was not reduced |
| Change order-item's status on dashboard | ✔️ |
| Verify unable to leave a review for your own product | ✔️ |
| 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 Gocode als/Code Review
| Criteria | yes / no |
|---|---|
| 90% reported coverage for all controller and model classes using SimpleCov | model test coverage is low |
| 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) | Instead of using the users show page, we recommend creating a custom route for the merchant dashboard |
| 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 | ✔️ yes! but you don't use the require login feature. |
| Helper methods or filters to find logged-in user, cart, product, etc | ✔️ There are some of these. One that could be useful is find_product |
| No excessive business logic | ✔️ |
| Business logic that ought to live in the model | |
| Add / remove / update product on order | ✔️ See in-line comment for small bug = => += |
| Checkout -> decrease inventory | ✔️ See in-line comment. It looks like update_stock is never called |
| 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 |
✔️ ✔️ ✔️ ✔️ It looks like there are a few started tests that were not completed |
| 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 |
✔️ ✔️ ✔️ Missing tests for edge cases |
| 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) |
✔️ This is specifically not tested This feature is not working ✔️ |
| 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 |
✔️ |
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 wrote clear model methods to encapsulate key functionality. Furthermore you've made good use of controller filters in your application controllers to DRY up your code. The user experience on the site is quite nice, and it's beautifully styled.
I do see some room for improvement around thoroughly testing both controller and model methods. It seems that you may have run out of time as I saw a few tests commented out. I encourage you to review the simplecov code coverage report and consider how the code that is not covered could be tested. There were also a few bugs that affected the functionality of your program (one in the seeds, and a couple others for updating quantity and stock). To work out these bugs, it's best to go through all the requirements from the rubric one last time to insure your code is working as expected.
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!
Mona and Kalki I provided in-line feedback for your requested feedback.
Emily requested feedback: The styling is beautiful! I know you learned a lot. I'm not a CSS pro, but I thought I'd share one example of using bootstrap to create nice order forms: https://mdbootstrap.com/snippets/jquery/pjoter-2-0/747937
Anya requested feedback: When testing edge cases it can be very helpful to rely on your simplecov code coverage report. The code in else clauses often represent edge cases. What happens when a product or order is nil, for instance. In addition common edge cases are how does a particilar method behave when the array is empty such as a user has no products or orders.
Only the person who submitted the PR will get an email about this feedback. Please let the rest of your team know about it.
| Find your soulmate,Sammy D'Amore,1,true | ||
| Buy an island,Jesse Luettgen,3,true | ||
| Bottle of joy,Amb. Glenda Legros,1,false | ||
| Bottle of love,Elmer Shields,1,true |
There was a problem hiding this comment.
There's no 'bottle of love' product, so I removed these from the seeds in a few places so that it would fully seed.
| } | ||
| } | ||
| } | ||
| describe "users(Guest)" do |
There was a problem hiding this comment.
Mona requested feedback: Good work dividing the tests into logged in users and guests. Also good use of fixtures.
| post product_reviews_path(product_id), params: review_data | ||
| }.must_differ "Review.count", 0 | ||
| must_respond_with :redirect | ||
| end |
There was a problem hiding this comment.
An additional edge case to consider is not being able to create a view for a retired product.
| def update_stock | ||
| self.order_items.each do |item| | ||
| item.product.stock -= item.quantity | ||
| product.stock.update_attribute(:stock, item.product.stock) |
There was a problem hiding this comment.
It looks like this method is never called. That is probably why stock is not updating
| current_item = OrderItem.find_by(product_id: product.id, order_id: self.id ) | ||
|
|
||
| if current_item | ||
| current_item.quantity = quantity |
There was a problem hiding this comment.
Quantity is not updated. This looks like a small bug.
| current_item.quantity = quantity | |
| current_item.quantity += quantity |
| expect(OrderItem.count).must_equal 5 | ||
| end | ||
|
|
||
| it "cant add item to cart if its retired" do # TODO perhaps move this to order_items controller |
There was a problem hiding this comment.
This is a great test to have. It seems that you ran out of time to complete this?
| # TODO perhaps test for an unexpected value for status | ||
| end | ||
|
|
||
| describe "total_orders_by_status" do |
There was a problem hiding this comment.
Consider the edge case for user that has no orders.
| end | ||
| end | ||
|
|
||
| describe "sorted_products" do |
There was a problem hiding this comment.
Consider the edge case for user that has no products.
| self.quantity * self.product.price | ||
| end | ||
|
|
||
| def stock_update |
| validates :zip_code, length: { is: 5, message: "Enter valid 5-digit zip code" }, :on => :update# doesn't check for digits only | ||
|
|
||
|
|
||
| def add_product(product, quantity) |
There was a problem hiding this comment.
Kalki requested feedback: Good work moving this logic into custom model methods rather than the controller. There are just a couple small bugs outlined below that keep functionality such as updating quantity and stock from working correctly.
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