-
Notifications
You must be signed in to change notification settings - Fork 49
Ports - Shamira #44
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 - Shamira #44
Conversation
Task ListWhat We're Looking For
|
|
|
||
| def create | ||
| task = Task.new(task_params) | ||
| if task.name == "" || task.description == "" || task.completion_date == "" |
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.
This is a little weird. .save is a method, you shouldn't assign it a value.
It also crashes rails
| if is_successful | ||
| redirect_to task_path(task.id) | ||
| else | ||
| head :temporary_redirect |
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.
Why temporary redirect? why not server error or something?
| <%= f.label :completion_date %> | ||
| <%= f.text_field :completion_date%> | ||
|
|
||
| <%= f.submit action_name, class:"task-form_submit-btn"%> |
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.
👍
| <%= link_to task.name, task_path(task.id)%> | ||
| Details: <%= task.description%>, due <%= task.completion_date%> <%= link_to "Mark Complete", toggle_path(task.id), method: :patch, data: { confirm: "Mark Complete?" } %> | ||
| </li> | ||
| <li><%= link_to "Delete", task_path(task.id), method: :delete, data: { confirm: "Are you sure you want to delete?" } %></li> |
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.
Nice confirmation
| def toggle | ||
| @complete_task = Task.find_by(id: params[:id]) | ||
| # is_complete = complete_task.update(status ) | ||
| if @complete_task.update(:status => false) |
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.
This if statement is checking to see if the update method works or not, it doesn't check to see what complete_task.status is.
Instead I would do:
@complete_task = Task.find_by(id: params[:id])
if @complete_task
@complete_task.status = ! @complete_task.status
if @complete_task.save
redirect_to tasks_path
else
head :server_error
end
else
head :not_found
end| patch toggle_path(complete_task.id) | ||
| toggle = Task.find_by(id: complete_task.id) | ||
| # Assert | ||
| expect(toggle.status).must_equal true |
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.
This test is failing... Also you need to test that once you toggle a task to complete, you can toggle it incomplete.
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions