finished #3
finished #3
Conversation
| @@ -0,0 +1,9 @@ | |||
| class DishIngredientsController < ApplicationController | |||
There was a problem hiding this comment.
Great use of a joins controller for creating the relationship between a dish and an ingredient!
| has_many :ingredients, through: :dish_ingredients | ||
|
|
||
| def calorie_count | ||
| calories = 0 |
There was a problem hiding this comment.
Whenever you are doing Ruby .map, .each or other enumerable in a model method on an IC, consider -- is there an AR method that could help me do this more easily?
in this case: ingredients.sum(:calorie_count)
| class IngredientsController < ApplicationController | ||
| def index | ||
| @chef = Chef.find(params[:chef_id]) | ||
| @ingredients = @chef.ingredients.distinct |
There was a problem hiding this comment.
This is a minor MVC infraction (and the rubric allows for one) but doing distinct in the controller is an AR method beyond simple CRUD actions (.create, .find, .update, .destroy, .save,etc) so it can go in a model method. IF you have this model method in your chef model:
def ingredients_list
ingredients.distinct
end
then you can call @chef.ingredients_list either in the controller or from the view as well!
| @@ -1,4 +1,6 @@ | |||
| class Chef < ApplicationRecord | |||
| validates_presence_of :name | |||
| has_many :dishes | |||
| has_many :ingredients, through: :dishes | |||
There was a problem hiding this comment.
Nice work adding the additional association!
| @@ -3,4 +3,13 @@ | |||
|
|
|||
| # Defines the root path route ("/") | |||
| # root "articles#index" | |||
|
|
|||
There was a problem hiding this comment.
Excellent RESTful routing!
| resources :dishes, only: [:show] | ||
|
|
||
| resources :chefs, only: [:show] do | ||
| get 'ingredients', to: 'ingredients#index' |
There was a problem hiding this comment.
You mentioned feeling like your route was not quite right, and you might have been thinking about the possibility of a nested controller. Using the ingredients controller like you have here is reasonable, but another alternative could be to create a chefs directory in your controllers folder and adding the ingredients controller there. Then, this route would go to "chefs/ingredients#index" so you would need to add the controller: "chefs/ingredients" parameter to this line on your route. Either this or your implementation are okay for this IC. But on a project, if you needed to have an ingredients controller that showed ALL ingredients, you would want to use "ingredients#index" for that so this US for a chef's ingredients could go to "chefs/ingredients#index"
Let me know if any of this is unclear!
| expect(page).to have_content(@dish.description) | ||
| expect(page).to have_content(@ingredient.name) | ||
| expect(page).to have_content(@ingredient_1.name) | ||
| expect(page).to have_content(32) |
There was a problem hiding this comment.
Consider tying this "32" to its label "total_calorie_count" (which you've set up in your view) so that we can be sure that this number is showing up in the right place and is just a random integer from somewhere else on the page. Your expect could instead be expect(page).to have_content("total_calorie_count: 32")
| @@ -9,6 +9,23 @@ | |||
|
|
|||
| describe "relationships" do | |||
| it {should belong_to :chef} | |||
| it { should have_many :dish_ingredients} | |||
There was a problem hiding this comment.
Nice work testing relationships in this model method! MAke sure you do this with your other models as well! There should be relationship tests in the chef_spec, ingredient_spec and dish_ingredient_spec as well
i know my routes might have some issues but i didnt have enough time not the full 3 hours unfortunatelly.
thanks.