💤 Team 1Z - Iris, Kal, Ringo, and Sophia #80
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Nov23part2
Nov23part2
Merchant avatar
Merchant avatar
pull in Iris' pull request
changes to nav bar
Il/nov25night
deleted comments
completed review validations
Rma1125am2
troubleshoot product and orderitemscontroller tests
kashenafi
changed the title
kaidamasaki
reviewed
Dec 6, 2020
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) | Adds a second copy. |
| Verify unable to increase quantity beyond stock | ✔️ (Though adding a second copy can confuse this logic.) |
| 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 | Can view another merchant's dashboard. |
Major Learning Goals/Code Review
| Criteria | yes / no |
|---|---|
| 90% reported coverage for all controller and model classes using SimpleCov | 71.68% |
| Routes | |
No un-needed routes generated (check reviews) |
Reviews has routes for edit and update but no corresponding actions. |
| 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) | Merchants uses parameterized routes. |
| 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 | Some of the cart logic lives in the controller. |
| 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 (logic in controller) |
| 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 some. |
| 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) |
No (order controller tests commented out) |
| 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 |
No reviews controller tests. |
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 made excellent use of partials.
I do see some room for improvement around test coverage. As you may have learned throughout this project changing untested code is way scarier than changing code that has tests.
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.
Comment on lines
+3
to
+6
| before_action :set_current_order | ||
| before_action :current_merchant | ||
| before_action :find_merchants | ||
| before_action :find_categories |
There was a problem hiding this comment.
I see that you made great use of controller filters, that's awesome!
| @order = Order.find_by(id: params[:order_id]) | ||
| elsif params[:id] | ||
| @order = Order.find_by(id: params[:id]) | ||
| else session[:order_id] && session[:order_id] != nil |
There was a problem hiding this comment.
I think you meant this to be an elsif as well since you listed a condition.
| end | ||
| end | ||
|
|
||
| def is_this_your_cart? |
| end | ||
|
|
||
| def construct_error_messages(errors) | ||
| return errors ? errors.messages.map{|error_type, msg| "#{error_type.to_s.gsub('_', ' ')}: #{msg.join(" - ")}" unless msg.empty?} : [] |
There was a problem hiding this comment.
I would have liked to have seen this split this across multiple lines this line is really dense.
Comment on lines
+26
to
+27
| merchant = Merchant.find_by(uid: auth_hash[:uid], | ||
| provider: params[:provider]) |
Comment on lines
+7
to
+18
| def acceptable_avatar | ||
| return unless avatar.attached? | ||
|
|
||
| unless avatar.byte_size <= 1.megabyte | ||
| errors.add(:avatar, "must be under 1MB") | ||
| end | ||
|
|
||
| acceptable_types = ["image/png", "image/jpeg"] | ||
| unless acceptable_types.include?(avatar.content_type) | ||
| errors.add(:avatar, "must be a JPEG or PNG") | ||
| end | ||
| end |
| <p>Sorry, no products!</p> | ||
|
|
||
| <% else %> | ||
| <div class="card-columns" style="margin-top:30px"> |
There was a problem hiding this comment.
This is a really intense partial. Well done!
| <p>Name: <%= order.name %></p> | ||
| <p>Email Address: <%= order.email %></p> | ||
| <p>Mailing Address: <%= order.address %></p> | ||
| <p>Credit Card: **** **** **** <%= order.credit_card_number.to_s[-4..-1]%></p> |
Comment on lines
+8
to
+9
| <th style="width: 17%">Quantity</th> | ||
| <th style="width: 17%">Price</th> |
|
|
||
| require "test_helper" | ||
|
|
||
| describe OrdersController do |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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