-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Branches - Fruit Loops (Paige, Xinran, Vi, Emily) #96
base: master
Are you sure you want to change the base?
Conversation
user total earnings/ cart subtotal / order total
added links to product images
…rder through params
…ck to use 'find_order' method, refactored the orders controller tests
Emily product
Emily product
Emily product
final css stylings
bEtsyWhat We're Looking ForManual testing
Code Review
Selected Model Tests | Overall FeedbackGreat 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!. Vi and Paige, the OrderItems controller and model look great! Xinran, the search and filter_order_items methods look great! They're both so succinct and clear. Emily, hopefully the feedback I gave throughout help to give input on code cleanliness/efficiency. I am particularly impressed by the way that you implemented a search! Also the row of fruits at the top looks so good and adds a lot to the feel of the website! I do see some room for improvement around checking edge cases (ie. uploading a product with an invalid image) and validating all features on your final version. (I assume editing a product and the "All fruits" page both worked at some point.) I am going to mark this as "approaches standard" because of the number of things that didn't work when I tested them. 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. |
@order.order_items.each do |item| | ||
item.product.stock = item.product.update_quantity(item.quantity, @order.status) | ||
item.product.save |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be moved to a model method.
As a rule of thumb, when I see more than a line or two that isn't related to control flow (render, redirect, etc.) or defining variables for views, I'm going to feel inclined to put the code in a model.
@order.order_items.each do |item| | ||
item.product.stock = item.product.update_quantity(item.quantity, @order.status) | ||
item.product.save |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also be moved to the model
existing_quantity = self.quantity | ||
new_quantity = existing_quantity + quantity | ||
|
||
return self.update(quantity: new_quantity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where it'd be important to check the quantity against the quantity in stock.
I looked at it again with y'all and found that when things weren't broken due to the url issue the following things worked:
Because of all of these things working and the acknowledgement that it was only one edge case that was causing problems, I will change the color grade to reflect that. |
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