-
Notifications
You must be signed in to change notification settings - Fork 47
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
Yolotzin - Time #54
base: master
Are you sure you want to change the base?
Yolotzin - Time #54
Conversation
…laceholder header in the application view
…nks in Work show view
…er, with session routes and ERB login form and current view
Media RankerFunctional Requirements: Manual Testing
Major Learning Goals/Code Review
Previous Rails learning, Building Complex Model Logic, DRYing up Rails Code
Testing Rails Apps
Overall Feedback
Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?) SummaryTo be clear your testing is very rough. You have a lot of things to work on there. You did get the basic functionality working. Take a look at my inline comments. I'd like you to particularly pay attention to my comments on testing and see what you can do to do better there in the bEtsy project. Maybe take on this responsibility with your team so that you can improve. |
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.
Note: You are indenting with 4 spaces instead of 2. At Ada our standard indentation is 2 spaces.
it "can get the new_work_path" do | ||
get new_work_path | ||
|
||
must_respond_with :success | ||
end |
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 because you're requiring the user to log in first before they can get to the form.
This would also require you provide a user fixture.
it "can get the new_work_path" do | |
get new_work_path | |
must_respond_with :success | |
end | |
it "can get the new_work_path" do | |
perform_login() | |
get new_work_path | |
must_respond_with :success | |
end |
end | ||
end | ||
|
||
describe "create" do |
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.
These tests also require the user log in first (See above). Also you should test what happens when a guest user tries to create a work.
# Arrange | ||
invalid_work_id = -1 | ||
# Act | ||
get "/works/#{invalid_work_id}" |
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.
get "/works/#{invalid_work_id}" | |
get work_path(invalid_work_id) |
end | ||
|
||
def find_work | ||
@work = Work.find_by_id(params[:id]) |
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.
You need to do something if the work does not exist.
@work = Work.find_by_id(params[:id]) | |
@work = Work.find_by_id(params[:id]) | |
if @work.nil? | |
flash[:error] = "That work does not exist" | |
redirect_to root_path | |
end |
}.must_differ 'Work.count', -1 | ||
|
||
must_respond_with :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.
end |
fixtures :all | ||
|
||
# Add more helper methods to be used by all tests here... | ||
def perform_login(user = nil) |
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.
👍
<% @work.errors.each do |column, message| %> | ||
<li> | ||
<strong><%= column.capitalize %></strong> <%= message %> | ||
</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.
This is better using a flash notice
<% @work.errors.each do |column, message| %> | ||
<li> | ||
<strong><%= column.capitalize %></strong> <%= message %> | ||
</li> | ||
<% end %> |
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.
Again better using a flash notice.
@@ -0,0 +1,30 @@ | |||
class Work < ApplicationRecord | |||
has_many :votes |
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.
If you want to destroy any votes for this work
has_many :votes | |
has_many :votes, dependent: :destroy |
belongs_to :work | ||
belongs_to :user | ||
|
||
validates :user_id, uniqueness: { scope: :work_id } |
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.
👍
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
session
andflash
? What is the difference between them?Assignment Submission: Media Ranker
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection
session
andflash
? What is the difference between them?