A Bunch of Banadies (Phoebe, Mary, Abinnet, Mariko) - Ampers #17
A Bunch of Banadies (Phoebe, Mary, Abinnet, Mariko) - Ampers #17marikoja wants to merge 487 commits intoAda-C9:masterfrom
Conversation
…t already exists in the cart
…tems on order page
…s nil/{}. Guests can make orders
…unable to fix yellow cherry edit product failed test
|
|
||
| describe "index" do | ||
| it "redirects to root path if user is not a merchant" do | ||
| user = nil |
There was a problem hiding this comment.
this line just makes a local variable named user and assigns the value to it as nil-- this doesn't affect the app at all
Think about what you're trying to test here and how you would make the "user" in the app not-exist... or "logged out"
| @@ -0,0 +1,341 @@ | |||
| /*! normalize.css v8.0.0 | MIT License | github.com/necolas/normalize.css */ | |||
| flash[:status] = :success | ||
| flash[:result_text] = "#{@user.name} logged in successfully" | ||
| session[:uid] = @user.uid | ||
| session[:user_id] = @user.id |
There was a problem hiding this comment.
There's a lot of setting on session the values uid and user_id, and from what I can tell, it's mostly that the code didn't conform to using one or the other, even though they represent the same thing. You'll prevent bugs and confusion if you conform to one standard here!
| def create | ||
| auth_hash = request.env['omniauth.auth'] | ||
| if auth_hash['uid'] | ||
| @user = User.find_by(uid: auth_hash[:uid], provider: 'github') |
There was a problem hiding this comment.
You don't need to make user an instance variable in this method
| @@ -0,0 +1,3 @@ | |||
| class ApplicationRecord < ActiveRecord::Base | |||
| self.abstract_class = true | |||
There was a problem hiding this comment.
Interesting, I'm not sure what this does
|
|
||
| <% if flash[:result_text] or flash[:messages] %> | ||
| <section class="callout <%= flash[:status] %>"> | ||
| <p><%= flash[:status] == :failure ? "A problem occurred: " : "" %><%= flash[:result_text] %></> |
There was a problem hiding this comment.
I think you meant to close a paragraph tag instead of </>?
| paid_items << item | ||
| end | ||
| end | ||
| return paid_items |
There was a problem hiding this comment.
Code like this works, but you could definitely make this so much shorter with using enumerables methods!
def order_item_paid(order_items)
return order_items.select { |item| item.status == 'paid' }
endand for the rest of the methods below this!
| if @user | ||
| @order_items = OrderItem.where(order_id: @order.id) | ||
| else | ||
| product_ids = @user.products.map{ |i| i.id } |
There was a problem hiding this comment.
When would the code ever get into this branch?
If @user is falsey (is nil) and gets into this else block, why would we be able to call product_ids = @user.products.map{ |i| i.id }?
| if @user | ||
| else | ||
| @user = User.find(1) | ||
| end |
There was a problem hiding this comment.
try cleaner code style, like
if !@user
@user = User.find(1)
| end | ||
|
|
||
| def index | ||
| @user = User.find_by(id: params[:user_id]) |
There was a problem hiding this comment.
@user doesn't need to be an instance variable in this case. Variables only need to be instance variables (with the @) in rails when you need to share that variable between the Controller and the View... since the value of user isn't important for the products#index, you can just assume it's safe as a local variable
| end | ||
| if @product | ||
| @product.order_items.each do |i| | ||
| i.destroy |
There was a problem hiding this comment.
I normally would personally discourage using variable names like i and r, but it's pretty clear in this context what it means.
| return User.new(uid: auth_hash[:uid], provider: auth_hash[:provider], email: auth_hash[:info][:email], name: auth_hash[:info][:nickname]) | ||
| end | ||
|
|
||
| MERCHANT_PICS = ['https://www.icreativeideas.com/wp-content/uploads/2014/08/Creative-Animals-Made-of-Fruits-And-Vegetables-4.jpg', 'https://www.icreativeideas.com/wp-content/uploads/2014/08/Creative-Animals-Made-of-Fruits-And-Vegetables-26.jpg', 'https://www.icreativeideas.com/wp-content/uploads/2014/08/Creative-Animals-Made-of-Fruits-And-Vegetables-27.jpg', 'https://www.icreativeideas.com/wp-content/uploads/2014/08/Creative-Animals-Made-of-Fruits-And-Vegetables-28.jpg', 'https://www.icreativeideas.com/wp-content/uploads/2014/08/Creative-Animals-Made-of-Fruits-And-Vegetables-29.jpg', 'https://www.icreativeideas.com/wp-content/uploads/2014/08/Creative-Animals-Made-of-Fruits-And-Vegetables-30.jpg', 'https://www.icreativeideas.com/wp-content/uploads/2014/08/Creative-Animals-Made-of-Fruits-And-Vegetables-34.jpg', 'https://www.icreativeideas.com/wp-content/uploads/2014/08/Creative-Animals-Made-of-Fruits-And-Vegetables-32.jpg'] |
There was a problem hiding this comment.
I like declaring my constants at the top of the file!
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.Overall fruEtsy is a pretty successful app; I didn't come across any major broken features. It has a lot of features and it's clear there was a lot of work, coordination, styling, etc that was put into it The code is quite a thing to look through! I have one major specific comment on one specific piece of code: This block of code to me signals some big warning signs for me: the more that this code base grows, the more obscure code like this will be. Developers in the future will ask: What does this mean? Does it have to be this way? Could I make it better? But there are no tests, how can I be sure that it isn't broken? Overall, I saw a fair bit of messiness in the code: there were a lot of unused local variables, a lot of duplicated code, a lot of unused methods, etc. ... These things added up, and end up creating a small cloud of confusion in the code. I know a lot of the feedback you all gave in your comprehension questions was around needing more design to be discussed in the beginning. A lot of my comments focused on smaller questions about code style and code quality, and I think that really working on that too will be really beneficial as well. That being said, this is an ambitious project and you all pulled through. Good work (random comments: The merchant images of the snail orange and banana dolphins and stuff are very cute....... the rating bananas are so weird (and cute)......) |
5 participants
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