Water && Earth (Marj, Maha, Jessica, Ting-Yi) #74
Conversation
created cart view and route
draft create action for order_item (do we need price for order_item?)
Updated User show with retired/shipped/cancelled/edit/delete; added working methods/tests for destroy/retired for products & shipped/cancelled for order_items
fixed create action for order_item
revised quantity calculation and stock for product
… and payment view to include cc_cvv
unskipped edit user test
Cart tests
created validation tests for user model
added cc_cvv to orders table, added validates and revised order controller and payment view to include cc_cvv
add tests for orderitem validates & relations
tests for new/update/edit/update/destray for review
Product controller tests
created category controller tests
added destroy tests for orderitem
tested custom model methods for order
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 | ✔️ |
| Add another merchant's product | ✔️ |
| Check out | ✔️ |
| Check that stock was 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 Goals/Code Review
| Criteria | yes / no |
|---|---|
| 90% reported coverage for all controller and model classes using SimpleCov | ✔️? |
| 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) | Rather than using the user show page for the merchant dashboard, it would be good to use a custom non-parameterized route that pulls the user_id from session (just as you have for cart). |
| 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 | See in-line comment |
| 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 | See in-line comment |
| 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 |
✔️ (logic in order items controller) ✔️ (logic in order items controller) I don't see this specific test. It looks like the section the controller that handles order_items that fail validations is not tested. see above ✔️ |
| 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 |
✔️ ✔️ ✔️ ✔️ |
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 | ✅ |
| 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 used controller filters, thoroughly tested your code, and beautifully styled your site. The user experience is very nice. Furthermore, good work going the extra step of deploying your site with OAuth.
One place that could benefit from refactoring is moving some of the order_item logic that's in the controller into custom model methods. Remember that manipulation of data is business, and is best put in the model.
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!
Only the person who submitted the PR will get an email about this feedback. Please let the rest of your team know about it.
| if session[:user_id].nil? | ||
| flash[:error] = "You must be logged in to retire this item" | ||
| redirect_to root_path | ||
| return |
There was a problem hiding this comment.
This logic is in a couple places. Consider moving it to a controller filter require_login.
There was a problem hiding this comment.
Jessica requested feedback: I would consider adding the require_login method to the applications controller because this functionality is used in multiple controllers.
| @order_item.quantity += params[:quantity].to_i | ||
| @order_item.price += chosen_product.price * params[:quantity].to_i | ||
| qty_limit = chosen_product.stock - @order_item.quantity |
There was a problem hiding this comment.
This logic is a bit "businessy". Consider how you could move it to model methods in orderItem.rb.
| @order_item.quantity = params[:quantity].to_i | ||
| @order_item.price = product.price * params[:quantity].to_i |
There was a problem hiding this comment.
This logic is repeated from above which is a good indication that it would be best to encapsulate this functionality in a model method.
There was a problem hiding this comment.
Ting-Yi: This comment applies to your requested feedback.
| </div> | ||
| </div> | ||
|
|
||
| <div class="form-row"> |
There was a problem hiding this comment.
Marj requested feedback: The styles are beautiful! One thing I would consider is moving the make new category functionality into the same form as the make new product. The user experience is a little funny since if you enter all the information for a new product and then want to add a new category, when you click the make new category button, your product information is cleared.
| </ul> | ||
| <% end %> | ||
|
|
||
| <%= form_with model: @order,url: complete_path(@order), method: :patch, class: 'order-form' do |f| %> |
There was a problem hiding this comment.
Maha requested feedback: I see that the class order-form is applied to the payment form, but I don't see any styles that correspond to this class. I believe this is from boostrap, but it seems that the rest of your html doesn't make use of bootstrap classes to make a justified form. I would recommend looking at a few more examples like this: https://mdbootstrap.com/snippets/jquery/pjoter-2-0/747937
| before_action :order_cart | ||
| before_action :find_user |
There was a problem hiding this comment.
Jessica requested feedback: These are great controller filters to add to the applications controller.
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