Tor, Angelica, Hannah - SuperMarket Betsy - Octos #18
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
…out paths in view, update uid to type string
…ntication merge to test with an authenticated user
… added in nested merchant routes to facilitate this
…gory route, added validation to category model to have uniqueness be true for category
…s and adding conditional validations
…s fixture to work with authentication
… in a category are retired
bEtsyWhat We're Looking For
Only the person who submitted the PR will get an email about this feedback. Please let the rest of your team know about it. |
droberts-sea
reviewed
May 3, 2018
|
|
||
| def product_params | ||
| return params.require(:product).permit(:name, :merchant_id, :stock, :price, :description, :photo_url, :category_ids => []) | ||
| end |
There was a problem hiding this comment.
Product params should not include the merchant ID - you should be taking that from the session! I was able to change the owner of a product, effective "stealing" it.
| def edit; end | ||
|
|
||
| def update | ||
| @product.assign_attributes(product_params) |
There was a problem hiding this comment.
Edit and update should both be making sure that the logged-in user is also the owner of the product. I was able to change the info on someone else's product by typing the URL into the address bar.
|
|
||
| def destroy | ||
| if @product.merchant_id == @logged_merchant.id | ||
|
|
There was a problem hiding this comment.
Here you're checking for product ownership - good work. Since you need it in multiple places, this would be a good candidate for a controller filter.
|
|
||
| order_items.each { |order_item| | ||
| order_status = Order.find_by(id: order_item.order_id).status | ||
| if order_status == status || status == "all" |
There was a problem hiding this comment.
On line 24, why not order_item.order.status?
| orders_and_items = {} | ||
| orders = self.orders | ||
| order_ids = orders.map { |order| order.id } | ||
| order_ids.each { |order_id| orders_and_items[order_id] = [] } |
There was a problem hiding this comment.
I'm not sure what this method is accomplishing. Why not start with orders.where(status: status), and then for each of those orders say order.order_items? Seems like putting it all into a hash of arrays is redundant.
| it 'sends does not update a product for invalid data' do | ||
| orderitem = { | ||
| product_id: Product.first.id, | ||
| order_id: Order.first.id, |
| describe 'ship' do | ||
| it "allows you to ship an item that exists" do | ||
| order_item = OrderItem.create(order_id: Order.first.id, product_id: Product.first.id, quantity: 1, status: "pending") | ||
| order_item.status.must_equal "pending" |
There was a problem hiding this comment.
I would probably include some test cases around auth here - what if you're not logged in? What if you're logged in as someone other than the merchant offering this product?
| describe ProductsController do | ||
| describe 'guest user' do | ||
| describe 'index' do | ||
| it 'succeeds with multiple products for a guest user' do |
There was a problem hiding this comment.
You need to include negative cases for the things a guest user / the wrong user can't do.
| it "wont' let you review if it's your own product" do | ||
| merchant = Merchant.first | ||
| merchant.products.length.must_be :>, 0 | ||
| login(merchant) |
| it "returns 0.0 for the total revenue of a merchant with zero orders" do | ||
| merchant = Merchant.new | ||
| result = merchant.total_revenue_by("all") | ||
| result.must_equal 0.0 |
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.
bEtsy
Congratulations! You're submitting your assignment! These comprehension questions should be answered by all members of your team, not by a single teammate.
Comprehension Questions