-
Notifications
You must be signed in to change notification settings - Fork 49
Ports - Elle #33
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
base: master
Are you sure you want to change the base?
Ports - Elle #33
Conversation
…o timestamp. Confirmed records can be created. Updated the controller's index action you created to retrieve and show all Task objects.
…nality to create a new task. Wrote tests.
…rs. Revisit later.
…it to the index view. Also cleaned up a few things.
Task ListWhat We're Looking For
Great job with this project, Elle! Your project hits all the requirements I was looking for: following Rails best practices, RESTful routes, and CRUD. Also, I'm so happy you used strong params well! The only thing I have to comment on is your controller tests, which I think you knew would happen. That being said, your project looks great! Even the mark complete functionality -- it looks good to me. Well done! |
| elsif this_task.completed == false | ||
| this_task.completed = true | ||
| this_task[:date_completed] = Time.now | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this logic! It's accurate and has the right functionality. It can probably be refactored to something a little more slim. Don't forget-- if this_task.completed == true is the same thing as if this_task.completed, and you could use an else instead of elsif ...!
| # Arrange | ||
| change_task = Task.find_by(task_name: "Clean the bathroom") | ||
|
|
||
| change_task.update(params: new_task_hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of calling update on the task itself, you'll want to use routes to get to the controller action you are testing. The method update that's on the task itself doesn't take a params hash... but the controller update action does! That's why your other version makes sense-- you call patch task_path(id), params: new_task_hash
| # }.must_change task_to_toggle[:completed], true | ||
|
|
||
| # Act | ||
| task_to_toggle2.toggle_completed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, you are calling toggle_completed as if it were an instance method on the model task_to_toggle2. However, your model doesn't have a method by that name defined! Instead, your Act will be to execute the code in the TasksController in the action/method named toggle_compelted... And how do you get to that code? Through the router! Your act will be whatever is the route defined to get there... which will be put toggle_completed_action_path
I think fixing the rest of your incomplete tests will follow this same pattern:
- Determine which controller and controller action you are testing
- Determine which route will get to that controller and controller action (
delete task_path, maybe?) - Determine which info needs to go along with it:
- Does it have route params?
- Does it have form data?
- Write out the "Act" in this test using that route and that additional information
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions