Conversation
|
I still need to work on the 'mark as completed' link, and debug the two errors. But I chose to stop last night and just turn it in. |
There was a problem hiding this comment.
As an initial note, I am really glad that you made the choice to turn in what you had even though it isn't complete so you can move forward. Hopefully the comments here will be helpful as you get more into Ride Share Rails. :)
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 | ✔️ I love that you labeled messages with a wave number but also included a descriptive message |
| Routes follow RESTful conventions | ✔️ Yes, though it could be cleaned up by using the resources syntax |
Uses named routes (like _path) |
✔️ |
| Creates Models and migrations | ✔️ |
| Creates styled views | |
| Handles errors like nonexistant tasks | Not in an appropriate way, for the show action. See my inline comment in show action. Other actions do handle it with an appropriate redirect |
Uses form_with to render forms in Rails |
✔️ |
Functional Requirements/Manual Testing
| Functional Requirement | yes/no |
|---|---|
| Successfully handles index & show | ✔️ For the most part yes, but it is limiting that there isn't any way for a user to click a link from the index page to get to the show page. (It would be appropriate to make the tasks click-able.) |
| 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 | See inline comments for more details |
| Tests for Destroy & Task Complete include tests for valid and 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 |
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 |
| end | ||
| end | ||
|
|
||
| def destroy |
There was a problem hiding this comment.
As mentioned in the routes.rb comment, this action is not being reached because the name in routes.rb is not the same as it is here.
| it 'will destroy to permanently remove a task' do | ||
| task | ||
| expect { | ||
| destroy task_path(task.id) |
There was a problem hiding this comment.
This test and the test below are failing because the proper HTTP verb is "delete" rather than "destroy".
| destroy task_path(task.id) | |
| delete task_path(task.id) |
There was a problem hiding this comment.
Hence I get the following error
NoMethodError: undefined method `destroy'
| get '/tasks/:id/edit', to: 'tasks#edit', as: 'edit_task' | ||
| patch '/tasks/:id', to: 'tasks#update' | ||
| get '/tasks/:id/confirm_delete', to: 'tasks#confirm', as: 'confirm_task' | ||
| delete '/tasks/:id', to: 'tasks#delete', as: 'delete_task' |
There was a problem hiding this comment.
The action is referred to as delete here but it is named destroy in the controller. It is important that the names line up for Rails to be about to route the request correctly.
Convention would be to name it destroy but as long as they match, it will work.
Also, as we talked about during roundtable today, using as on this line is unnecessary because the path is already named on line 8.
| delete '/tasks/:id', to: 'tasks#delete', as: 'delete_task' | |
| delete '/tasks/:id', to: 'tasks#destroy' |
|
|
||
| def destroy | ||
| @task = Task.find_by(id: params[:id]) | ||
| task_params = params.require(:task).permit(:name, :description, :completed_at) |
There was a problem hiding this comment.
Once you fix the above, you will see the following error:
ParameterMissing: param is missing or the value is empty: task
This is because none of these params are given during a delete request. Only the id is given, so this line is unnecessary.
| task_params = params.require(:task).permit(:name, :description, :completed_at) |
|
|
||
| def update | ||
| @task = Task.find_by(id: params[:id]) | ||
| task_params = params.require(:task).permit(:name, :description, :completed_at) |
There was a problem hiding this comment.
This line is used in several methods and could be DRYed up by creating a private task_params method like we demonstrate with AdaBooks
2 participants
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions