Skip to content
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

Martin Chavez #9

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

Chavezgm
Copy link

I felt confident about going into the IC! The practice does really help! I made sure to make my test as robust as possible!

@@ -0,0 +1,8 @@
class DishIngredientsController < ApplicationController
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of the joins controller!

has_many :ingredients, through: :dish_ingredients

def unique_ingredients
ingredients.distinct.pluck(:name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Adding the additional association makes this logic much easier. Great job remembering the distinct!

has_many :ingredients, through: :dish_ingredients

def total_calories
ingredients.sum(:calories)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice encapsulation of this logic in a model method!


<p>Add Ingredient</p>

<%= form_with(url: dish_ingredients_path(@dish), method: :post) do |form| %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful form implementation! Great RESTful path and method

end

resources :chefs, only: [:show,] do
resources :ingredients, only: [:index]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great RESTful routing! Another option here could be to use a nested controller since the ingredients are nested under chefs (you could add controller: "chefs/ingredients" at the end of line 12 to force the controller)

expect(page).to have_content('Cheese')
# And I see a list of ingredients for that dish

expect(page).to have_content('Calorie Count: 500')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job tying this calculation to the text it should appear next to!

RSpec.describe Ingredient, type: :model do
it { should have_many :dish_ingredients }
it { should have_many(:dishes).through(:dish_ingredients) }
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent unit testing in your model specs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants