swEtsy - Lina, Kareha, Sophie, Richelle #81
Conversation
…for add to cart method
…view page- added $ sign and round to 2 decimal places
Ld update inventory
Ld change cost datatype
…he cart show page
Ld cartitem and cart total cost
only show checkout when items in cart
update current user page for if item has no categories
…d tweaked other tests
bEtsyY'all, you did it! And your site looks SO GOOD! I hope that you show this off to people! If I were you, I'd be so proud of what y'all built together! Richelle, the user model tests are very thorough! In general, it's considered much more ok to repeat yourself in tests than it would in the app code. That said, there is some excessive thoroughness in some of the User model tests that make them more work to write and actually make them less thorough tests. See my inline comments in Sophie, for the most part the controllers look plenty DRY! There are a couple places in the controllers where I suggest areas for improvement. See my inline comments in the controllers for details. Kareha, sorry, I honestly struggle to understand what "update status cart test" meant so am not sure how to give targeted feedback. Lina, all the testing you mentioned you wanted feedback on looks great to me! Again, great work everyone! This is going to be a site that'll be fun to show off for a long time I think! Functional Requirements: Manual Testing
Major Learning Goals/Code Review
Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
Overall FeedbackOnly the person who submitted the PR will get an email about this feedback. Please let the rest of your team know about it. |
| if @cart_item.qty < product_inventory | ||
| @cart_item.qty += 1 | ||
| @cart_item.save |
There was a problem hiding this comment.
This is not a large amount of code but is is a small example of "business logic" that could have it's own model method.
| if @cart_item.qty > 1 | ||
| @cart_item.qty -= 1 | ||
| end | ||
| @cart_item.save |
There was a problem hiding this comment.
This is not a large amount of code but is is a small example of "business logic" that could have it's own model method.
| @@ -0,0 +1,72 @@ | |||
| class User < ApplicationRecord | |||
| @@ -0,0 +1,42 @@ | |||
| class Cart < ApplicationRecord | |||
| @@ -0,0 +1,49 @@ | |||
| class Product < ApplicationRecord | |||
| if pending_revenue == 0 | ||
| expect(pending_revenue).must_equal 0 | ||
| else | ||
| expect(pending_revenue).must_be_kind_of Float | ||
| end | ||
|
|
||
| if paid_revenue == 0 | ||
| expect(paid_revenue).must_equal 0 | ||
| else | ||
| expect(paid_revenue).must_be_kind_of Float | ||
| end | ||
|
|
||
| if complete_revenue == 0 | ||
| expect(complete_revenue).must_equal 0 | ||
| else | ||
| expect(complete_revenue).must_be_kind_of Float | ||
| end |
There was a problem hiding this comment.
I will say that this is definitely more work than you should need to do for a test. For testing, unlike in app code, you do have full control over the user and their current values so you should be able to setup the user in a specific way (eg. where values == 0 or values != 0) and shouldn't need to write if statements about those preconditions.
Happy to talk more about this if this brief explanation doesn't make sense.
| expect(pending_items_count).must_be_kind_of Integer | ||
| expect(paid_items_count).must_be_kind_of Integer | ||
| expect(complete_items_count).must_be_kind_of Integer | ||
|
|
||
| total_by_status = pending_items_count + paid_items_count + complete_items_count |
There was a problem hiding this comment.
This is another example where you should be able to make more explicit assumptions than this test is making. You can and should hardcode specific values for what each of these values should be based on the way the user is arranged.
The way this test is written, if the pending_items_count isn't being calculated correctly, the test will still pass.
In general you should rarely need to do any real math or if statements in tests.
| expect(pending_revenue).must_equal 0 | ||
| expect(paid_revenue).must_equal 0 | ||
| expect(complete_revenue).must_equal 0 |
There was a problem hiding this comment.
This is great! A good example of expecting very specific values. This sort of expectation can be applied to values besides 0 as well. Hope that helps
| if @current_cart.products.include?(@product) | ||
| # find the cart item | ||
| cart_item = current_cart.cartitems.find_by(product_id: @product.id) | ||
| # check if there is enough inventory | ||
| if cart_item.qty < @product.inventory | ||
| cart_item.qty += 1 | ||
| flash[:success] = "Successfully added to cart" | ||
| else | ||
| # not enough inventory | ||
| flash[:error] = "Sorry, not enough inventory" | ||
| end | ||
| else | ||
| # create a new cart item if it doesn't exist | ||
| cart_item = Cartitem.new(cart: @current_cart, product: @product, qty: 1, cost: @product.cost ) | ||
| flash[:success] = "Successfully added to cart" | ||
| end |
There was a problem hiding this comment.
This could all be a method in the cart model, something like cart.add(product). It will return true if it could successfully add it to the cart and false otherwise. The appropriate flash message could be displayed accordingly from this controller.
fix products banner
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