Leaves - Janice, Ga-Young, Mariya, and Julia K #85
Conversation
…updated strong params so that it will work with session[:merchant_id], updated new form to include categories
…nt. some work on adding to cart
made changes to make oauth authorize correctly
Update nav
Order creation take two
…o order-creation-take-two
…ty rather than lists another item in cart
Gyjin/manage store style
Styling tweaks
…howing up on Bark Obama.
removed break lines
added checkout button
fix cart styling--ordered list, lining up
Styling tweaks
cleaned up code
File cleanup
cleaned up products files
bEtsyWhat We're Looking ForManual testing
Code Review
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!. I am particularly impressed by the way you designed your user experience. It is nearly flawless! I do see some room for improvement around taking "business" logic out of the controller and the views. Requested focused feedback is below: Janice: I'd appreciate targeted feedback on my tests and styling. Consider using fixtures for your order_test.rb. Your validation tests are very thorough. Because you are using rails built in validations, it's necessary to be quite so thorough. You can trust that rails did that work for you! Ga-Young: I would like targeted feedback on my merchant's show view page. The view page shows a different view depending on if a merchant is signed in or not and I'm not sure if this is good practice or if there is a better way to do this.. Yes -- this is a good amount of logic to put in the controller. The design was robust so that even if a nefarious user tried to access a route that the user interface didn't provide, they weren't able to perform the action Mariya: I'd like targeted feedback on my tests (products & categories). For the product model test you did a great job using fixtures where possible. For your controller test, it is good practice to divide your tests into "logged in" and "logged out" tests. As such, you will only have to Julia K: I'd like feedback on the application.html.erb view (contains navbar, flash messaging, and footer) and the home page (homepages index.html.erb). These are two separate views but I think of them as a single, entry-into-website package. The application.html.erb view uses great semantic html and it produces a great user experience. As I said, the user experience is flawless! The amount of conditional logic that's used for changing the view based on what's in session is appropriate. I noticed that the homepages index.html.erb is sectioned mostly by s. You make good uses of classes, but I wonder if there's a way to infiltrate some more semantics into that view.
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. |
| end | ||
|
|
||
| def destroy | ||
| if session[:merchant_id] == nil |
There was a problem hiding this comment.
You should technically be able to logout even if you're logged in. There could be a UI bug where you double click the log out button. You want this action to be idempotent
| end | ||
|
|
||
| if session[:merchant_id].nil? | ||
| flash[:warning] = "You must be logged in as a merchant to edit a product." |
There was a problem hiding this comment.
The work of requiring login is a good candidate for a controller filter.
| end | ||
| # if there is already an order going and desired product is already in the cart, locates that orderproduct | ||
| cur_orderproduct = Orderproduct.find_by(order_id: @order.id, product_id: @product.id) | ||
| if cur_orderproduct != nil |
There was a problem hiding this comment.
Some of this work could be done by a custom validation in the model.
|
|
||
| def update | ||
| if @order.update(order_params) | ||
| @order.orderproducts.each do |orderproduct| |
| <th> Subtotal </th> | ||
| </tr> | ||
| </thead> | ||
| <% total = 0 %> |
There was a problem hiding this comment.
The work of calculating the total should be a model method that can be tested.
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