Space - Lee #33
Space - Lee #33
Conversation
…ct to the root page if given an invalid id
… for comparison, tests if a flash alert is present when an invalid id is passed in
… handles invalid ids
…ly updated to an empty string
Task ListMajor Learning Goals/Code Review
Functional Requirements/Manual Testing
Overall Feedback
Additional FeedbackGreat work on this project, Lee! I'm also really glad that you got to practice strong params, partial views, and writing test cases for update/delete/custom actions! Super well done! With regards to your reflection questions: I think that you tested the toggle complete feature appropriately-- you checked the number of tasks and the contents of that task; looks great! When we introduce flash, we won't spend a lot of time testing flash, but testing the messages on flash is totally appropriate and something you can do, so I'm glad you did! I've really added only one comment about a subtle refactoring-- otherwise, your code looks great! I see you are developing a code style that insists on clarity and conciseness, and that's great and I encourage it :D Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| @@ -0,0 +1,88 @@ | |||
| class TasksController < ApplicationController | |||
| def index | |||
| @tasks = Task.order("id") # with Task.all, everytime a task was updated, it would jump to the bottom of the list | |||
| Task.create( | ||
| name: "completed task", | ||
| description: "completed description", | ||
| completed_at: Time.now | ||
| ) | ||
| end | ||
|
|
||
| let (:completed_task_hash) { | ||
| { | ||
| task: { | ||
| name: "completed task", | ||
| description: "completed task description", | ||
| completed_at: Time.now.to_s, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| it "can mark an incomplete task as complete" do | ||
| id = Task.first.id | ||
|
|
||
| expect { | ||
| patch toggle_complete_path(id) | ||
| }.wont_change "Task.count" | ||
|
|
||
| task = Task.find(id) | ||
| expect(task.completed_at).must_equal completed_task_hash[:task][:completed_at] |
There was a problem hiding this comment.
This is an extremely subtle and minor thing, but just to let it marinate in your brain for future tests:
the value of task is the Task created in your before block... and the value you test it against is the completed_at defined in the completed_task_hash let variable.
Very subtle, but that means you're checking the created Task against an unrelated variable. It ends up working out since the created Task and the completed_task_hash end up having the same values anyway.
However: Imagine a developer who didn't read through this and changes the completed_task_hash contents. Then your tests may break, but not because the feature is broken!
You may want to think about if your created task could directly rip values from the completed_task_hash, or some other way to refactor this. Let me know if you have questions! (btw this line of thinking might also apply to your edit/update tests)
There was a problem hiding this comment.
Ahh! My guess is that line 200 needs to be expect(task.completed_at).must_equal Time.now.to_s (and I don't need the completed_task_hash). After writing tests for Ride Share, I'm realizing that I made some pretty funky tests for Task List 😅. Thank you for all your feedback!
2 participants
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions
"will create a flash alert and redirect for an invalid task". This is an edge-case test because it will only happen if the user tries to make a patch or delete request with an invalid id (for example, when using a tool like Postman). Only get requests can be made through the address bar, as far as I know!create_taskmigration enters a new task into the database with the following fields: "name," "description," "completed_at," "created_at," and "updated_at."