Branches-Sara #29
Branches-Sara #29
Conversation
Task ListWhat We're Looking For
Well done on this project, Sara! Your code generally looks good, and your tests generally look good. However, you have a couple of places that I have some concern about for syntax, with possibly some concerns for understanding. I hope that when you read these comments, you'll have a better sense for ways to improve syntax, areas of concept to review, and questions for me or for any instructor.
Let me know what questions you have! Keep up the hard work! |
| a { | ||
| font-size: 12px; | ||
| text-decoration: none; | ||
| color: white; |
There was a problem hiding this comment.
Because of this style, which makes all links turn white, it was impossible for me to see the link to Mark Complete or Mark Incomplete, so I almost thought that this feature wasn't done! Watch out with your styles and think about what the user sees.
| must_redirect_to root_path | ||
|
|
||
| expect( Task.find_by(id: id) ).must_be_nil | ||
| end |
There was a problem hiding this comment.
What about a test for deleting a task that doesn't exist?
| } | ||
| } | ||
| #Act | ||
| patch task_path(existing_task.id, params: update_task_from_data) |
There was a problem hiding this comment.
This test doesn't use your complete_task_path route, and uses a completely different route/action here. In fact, it doesn't even go into the TasksController toggle_complete action here!
|
|
||
| #Assert | ||
| expect( Task.find_by(id: existing_task.id).completed).wont_equal nil | ||
| end |
There was a problem hiding this comment.
What happens if we try to toggle_complete on a task that doesn't exist? It would be awesome to have a test here on that.
|
|
||
| # patch '/tasks/update/:id', to: 'tasks#update', as: 'task_update' | ||
|
|
||
| delete '/tasks/:id', to: 'tasks#destroy', as: 'task_delete' |
There was a problem hiding this comment.
Hm, here you make a new route to delete a task. This is a route that can be generated with the resources, like the in line above with resources :tasks
|
|
||
| # get '/tasks/:id/edit', to: 'tasks#edit', as: 'edit_task' | ||
|
|
||
| # patch '/tasks/update/:id', to: 'tasks#update', as: 'task_update' |
There was a problem hiding this comment.
This file may be easier to work with if you delete the comments
|
|
||
| delete '/tasks/:id', to: 'tasks#destroy', as: 'task_delete' | ||
|
|
||
| get '/tasks/complete/:id', to: 'tasks#toggle_complete', as: 'complete_task' |
There was a problem hiding this comment.
What is the controller action tasks#toggle_complete (TasksController, toggle_complete action) supposed to do? It's supposed to change a task's data so that it's complete. Since it's going to change task data, instead of ask/query for data, we should think about the HTTP Verb for this route. The HTTP Verb you defined is get, which is usually associated with asking/querying for information. Since we're not querying, we should use the HTTP Verbs associated with changing information, like patch or put. Therefore, maybe the better version of this route may be patch '/tasks/complete/:id', to: 'tasks#toggle_complete', as: 'complete_task'
Of course, this will affect the link in the view that goes to this route.
| @@ -0,0 +1,117 @@ | |||
| class TasksController < ApplicationController | |||
| def index | |||
| @tasks = Task.all.order(:id) | |||
| @task = Task.find_by(id: task_id) | ||
|
|
||
| if task_id < 0 | ||
| flash[:Error] = "Could not find task with id: -1" |
There was a problem hiding this comment.
We haven't learned what flash is yet -- what are you doing here? :)
|
|
||
| def update | ||
| id = params[:id].to_i | ||
| if id < 0 |
There was a problem hiding this comment.
Your conditional if id < 0 is to help determine if the task was not found. However, this is not a very solid way of approaching the problem. This logic works if params[:id] is equal to -1. What if params[:id] is equal to tacocat? Then id would equal 0. Also, what if params[:id] is equal to 999, which is a valid number, but there isn't a task with that ID number?
The better thing to do would be to check if Task.find_by gives back a task or if it gives back nil.
Let me know if you have questions, because this is really important
| @task[:description] = params[:task][:description] | ||
| @task[:completed] = params[:task][:completed] | ||
|
|
||
| if @task.save |
There was a problem hiding this comment.
Instead of re-assigning each part of @task, it will be better practice to use strong params, so to change this method to
@task = Task.find_by(id: id)
if @task.update(task_params)
| id = params[:id] | ||
| @task = Task.find_by(id: id) | ||
| if @task.completed == nil | ||
| @task[:completed] = "yes" |
There was a problem hiding this comment.
Instead of using this syntax with the square brackets, the same thing can be accomplished with . and it is more Rails-y
@task.completed = "yes"
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions