-
Notifications
You must be signed in to change notification settings - Fork 51
Earth/Taylor #46
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?
Earth/Taylor #46
Conversation
|
TaskList Feedback Task ListMajor Learning Goals/Code ReviewComprehension questions: In response to your last note: Running migrations is only something to do after you've created a new migration that you haven't yet run. Running
Functional Requirements/Manual Testing
Overall FeedbackGreat work! You made a fully-functional multi-page Ruby on Rails website!!! In just a week! And all the code is easy to read and understand! 👏👏👏
Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| def show | ||
| @task = Task.find_by(id: params[:id]) | ||
| if @task.nil? == true | ||
| 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.
This doesn't actually trigger a redirect so the user just ends up seeing a blank page. The redirect_to method on line 23 does though
| it "successfully deletes an existing Task" do | ||
| existing_task = Task.last | ||
|
|
||
| expect { | ||
| delete task_path(existing_task.id) | ||
| }.must_change "Task.count", -1 |
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.
| it "successfully deletes an existing Task" do | |
| existing_task = Task.last | |
| expect { | |
| delete task_path(existing_task.id) | |
| }.must_change "Task.count", -1 | |
| it "successfully deletes an existing Task" do | |
| task.save | |
| expect { | |
| delete task_path(task.id) | |
| }.must_change "Task.count", -1 |
I noticed this test and another were erroring out for the same reason and I didn't understand why so I decided to look into it.
It turns out let syntax does not properly update the test database so that task from the let block isn't there.
If you change the code to what I have here, the test passes because the task is actually added to the test database.
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.
I hope you didn't spend too much time on this. It's a really funky thing that we didn't ever explain!
It would really be better if the test we gave you had used a before block instead because then the test database would've been populated as you expected.
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions