Conversation
…correct on the webpage.
tildeee
left a comment
tildeee
left a comment
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 |
✔️ |
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 | Does not test these actions with invalid task ids |
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
Rachael!! Good work on this project.
Largely, all of the functionality we asked for on this project is there and working! I'm happy that all of the Rails conventions were pretty much followed.
I feel like I can tell that this project maybe got messy at some point? There are a few areas where I thought, "hm, this line of code shouldn't be here, it should be deleted or moved or refactored in a way that you started." Also, I think if you had more time, I think you would have gotten to the CSS styling and just a few missing tests.
I've left a bunch of comments to help point out a few things-- they're mostly suggestions to take with you in the future! Let me know if you have any questions on them.
Otherwise, great work on this; keep up the good 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 |
| @@ -0,0 +1,100 @@ | |||
| TASKS = ["be cool", "be calm", "be collected", "lose your ever-loving mind"] | |||
There was a problem hiding this comment.
Also, this code never gets used, so it'd be better to delete this line of code
| if @task.nil? | ||
| redirect_to tasks_path | ||
| return | ||
| head :not_found | ||
| return |
There was a problem hiding this comment.
What's going on in this code? :) It reads: if the task is nil, redirect to the tasks_path, then return.... and then head :not_found and then another return? Remember, return statements ALWAYS make the code exit the method. Lines 14-15 will never execute under any circumstance, so it's better to just delete these lines of code.
| @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 required for this project, but I encourage you to use a strong params method in the future! You already have one defined below, it'd be best to refactor this line to use it :)
| else # save failed :( | ||
| render :edit | ||
| return | ||
| end |
| time = Time.now | ||
| @task.update(completed_at: time) |
There was a problem hiding this comment.
Minor suggestion: since time is only used this one time, immediately on the next line, and it still is fairly readable, it might read better to only have one line: @task.update(completed_at: Time.now)
| expect(task.name).must_equal new_task_hash[:task][:name] | ||
| expect(task.description).must_equal new_task_hash[:task][:description] | ||
| expect(task.completed_at).must_equal new_task_hash[:task][:completed_at] |
| task = Task.new(name: 'test', description: 'testy test', completed_at: '') | ||
| id = task.id | ||
|
|
||
| task.save | ||
|
|
||
| expect { | ||
| patch markup_task_path(id) | ||
|
|
||
| expect(task.completed_at).wont_equal ''} | ||
| # must_respond_with :redirect | ||
| # must_redirect_to tasks_path |
There was a problem hiding this comment.
It works out, but I think technically the syntax you want is this:
| task = Task.new(name: 'test', description: 'testy test', completed_at: '') | |
| id = task.id | |
| task.save | |
| expect { | |
| patch markup_task_path(id) | |
| expect(task.completed_at).wont_equal ''} | |
| # must_respond_with :redirect | |
| # must_redirect_to tasks_path | |
| task = Task.new(name: 'test', description: 'testy test', completed_at: '') | |
| task.save | |
| id = task.id | |
| patch markup_task_path(id) | |
| task = Task.find_by(id: id) | |
| expect(task.completed_at).wont_equal '' |
- You should move
id = task.idbelowtask.save. Recall, tasks don't have an ID until they're stored in the database-- so you should getidafter saving to the database. - You'll probably want the typical syntax for making a patch request:
patch markup_task_path(id), without the leadingexpect {on line 168. - You'll need to find
taskagain after the patch request and before theexpectpart, so you can find the task withtask = Task.find_by(id: id)
2 participants
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions