Redsy - Edges #20
Redsy - Edges #20
Conversation
updated databases
merging commits from mc-model-tests branch; fixtures
… mo-merch-show
fixed order viewing page
Mc last min bugs
hover background image update for heroku
… mo-merch-show
made css changes to order summary page for merchant
… mo-merch-show
fixed order form validations
fixed submit order form
updated login button
Mo merch show
… dionisia-work
fixed issues with products error messages
added styling to forms
bEtsyDionisia - I worked on some of the testing since I still a little shaky on that. I worked on the categories controller, merchant model testing, order model testing, product model testing. and Melissa - The order summary methods.
Michelle - cart.
Layla - I appreciate targeted feedback on ways to improve the review process and how to best incorporate how to style reviews on a products detail page.
Overall:
Only the person who submitted the PR will get an email about this feedback. Please let the rest of your team know about it. |
| merchant_viewing = Merchant.find_by(id: params[:id]) | ||
|
|
||
| # this allows the order summary page to work | ||
| if merchant_viewing.nil? |
There was a problem hiding this comment.
If you're going to require that the merchant in the URL match the merchant in the session, why bother putting one in the URL? It would be easier to make it /merchants/dashboard or something similar, and show the account for only the logged in user. Then you don't even have to make this check, since there's no way they could give you the wrong ID.
| #TODO: translate into strong params | ||
|
|
||
| @order_item = OrderItem.new(product_id: params[:product_id].to_i, quantity: params[:quantity].to_i, order_id: current_order) | ||
| session[:order_id] = @order_item.order.id |
There was a problem hiding this comment.
Yes, you should use strong params here.
On line 9, the session should already be set (since this relies in the current_order method).
| def change_shipping_status | ||
| @order_item = OrderItem.find_by(id: params[:order_item_id]) | ||
|
|
||
| @order_item.update(shipped: params[:shipped]) |
There was a problem hiding this comment.
You should have some authorization here. As-is, anyone could use a tool like postman to change the shipping status of any order.
| # @order_item = OrderItem.new(product_id: params[:product_id].to_i, quantity: params[:quantity].to_i, order_id: params[:order_id]) | ||
| # # order_id = OrderItem.add_order_item_to_order(@order_item) | ||
| # # @order_item.order_id = order_id | ||
| # |
There was a problem hiding this comment.
This is a lot of commented out code! You should remove it - it makes this file much more difficult to read. It will still be there in the Git history if you find you need it.
| def update | ||
| if @login_user | ||
|
|
||
| params = product_params |
There was a problem hiding this comment.
You should not just check that the user is logged in, but that they are logged in as the owner of this product. As-is, anyone can edit anyone else's products. See https://redsy.herokuapp.com/products/1
| def destroy | ||
| if @login_user | ||
|
|
||
| product = Product.find_by(id: params[:id]) |
There was a problem hiding this comment.
Again, authorization is only half-done here.
| if @login_user | ||
| # && session.id == @product.login_user.id | ||
| flash[:status] = :failure | ||
| flash[:result_text] = "You cannot review your own product." |
There was a problem hiding this comment.
Line 26 is close. You probably want
if @login_user && @login_user.id == product.merchant_id
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