Conversation
There was a problem hiding this comment.
Task List
Major Learning Goals/Code Review
| Criteria | yes/no, and optionally any details/lines of code to reference |
|---|---|
| At least 6 commits with meaningful commit messages | ✔️ |
| Routes follow RESTful conventions | ✔️ |
Uses named routes (like _path) |
✔️ |
| Creates Models and migrations | ✔️ |
| Creates styled views | ✔️ |
| Handles errors like nonexistant tasks | ✔️ |
Uses form_with to render forms in Rails |
You ended up using form_for instead, which is valid and works, but for this project we were expecting form_with |
Functional Requirements/Manual Testing
| Functional Requirement | yes/no |
|---|---|
| Successfully handles index & show | ✔️ |
| index & show tests pass | ✔️ |
| Successfully handles: New, Create | ✔️ |
| New, Create tests pass | ✔️ |
| Successfully handles: Edit, Update | ✔️ |
| Edit, Update tests pass with valid & invalid task ids | ✔️ |
| Successfully handles: Destroy, Task Complete | ✔️ |
| Tests for Destroy & Task Complete include tests for valid and invalid task ids | No test for Task Complete with invalid task id |
Overall Feedback
| Overall Feedback | Criteria | yes/no |
|---|---|---|
| Green (Meets/Exceeds Standards) | 5+ in Code Review && 6+ in Functional Requirements | ✔️ |
| Yellow (Approaches Standards) | 3+ in Code Review && 5+ in Functional Requirements, or the instructor judges that this project needs special attention | |
| Red (Not at Standard) | 0-2 in Code Review or 0-4 in Functional Reqs, or assignment is breaking/doesn’t run with less than 5 minutes of debugging, or the instructor judges that this project needs special attention |
Additional Feedback
Great work on this project, Noor! Your project has all the functionality I was looking for.
Your tests also look very good! I like how they're written. And I like the styling you put on your site :) !!!
There are a couple of things I'm commenting on your code-- nothing to do except think about them and let me know if you have any questions.
Also, something you should note: When creating a task, it already says that it is complete. I wouldn't expect that functionality, so it'd be interesting to figure out why it's happening!
Overall, great work!
Code Style Bonus Awards
Was the code particularly impressive in code style for any of these reasons (or more...?)
| Quality | Yes? |
|---|---|
| Perfect Indentation | |
| Elegant/Clever | |
| Descriptive/Readable | |
| Concise | ✅ |
| Logical/Organized |
| let (:new_task_hash) { | ||
| { | ||
| task: { | ||
| name: "Buy a Book", | ||
| description: "Buy a book about six sigma", | ||
| completed_at: "today" | ||
| }, | ||
| } | ||
| } |
| get update_task_path(-1) | ||
| must_redirect_to root_path |
| it "can toggle" do | ||
| task = Task.create( | ||
| name: "Buy Flowers", | ||
| description: "Buy flowers that has different colors", | ||
| completed_at: "today" | ||
| ) | ||
|
|
||
| patch uncomplete_task_path(task.id) | ||
| task = Task.find_by(id: task.id) | ||
| expect(task.completed_at).must_be_nil | ||
|
|
||
| patch complete_task_path(task.id) | ||
| task = Task.find_by(id: task.id) | ||
| expect(task.completed_at).wont_be_nil | ||
|
|
||
| end |
| @task = Task.new( | ||
| name: params[:task][:name], | ||
| description: params[:task][:description], | ||
| completed_at: params[:task][:completed_at] | ||
| ) |
There was a problem hiding this comment.
It wasn't a requirement for this project, but if you had more time on this project, I'd encourage you to use a strong params method!
| def count | ||
| count = Task.count | ||
| end |
There was a problem hiding this comment.
Does this method get used? If I comment this method out, all of your tests still pass.
If this method doesn't get used, then it's better to delete it!
| patch uncomplete_task_path(task.id) | ||
| task = Task.find_by(id: task.id) | ||
| expect(task.completed_at).must_be_nil | ||
|
|
||
| patch complete_task_path(task.id) | ||
| task = Task.find_by(id: task.id) | ||
| expect(task.completed_at).wont_be_nil |
There was a problem hiding this comment.
I love this test! I think it's pretty awesome that you did multiple "act"s and multiple "assert"s. Usually I'd like to discourage that, but in this case, the asserts are very related, so it reads very well. :)
|
Thank you Simon |
2 participants
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions