-
Notifications
You must be signed in to change notification settings - Fork 10
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
Adds course reserve access point #144
Conversation
@@ -23,12 +23,18 @@ def to_s | |||
def catalog_index_access_points | |||
if @params[:f] | |||
format_param = @params[:f][:format] || @params[:f][:format_main_ssim] | |||
course_reserve_params = {course: @params[:f][:course], instructor: @params[:f][:instructor]} | |||
if format_param | |||
case |
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've tried a couple of things to get away from this case statement. I don't really like it but not sure how to fix it.
Coverage decreased (-0.09%) when pulling b99c31c710040cd6431f818a7c510ce658fbe6ff on apanel-cr into 1922b04 on master. |
Coverage decreased (-0.11%) when pulling 0d353e1f7c979f29b2d1291c1419a618f9d347d0 on apanel-cr into ad9a339 on master. |
response.should_receive(:docs).exactly(6).times.and_return([{crez_course_info: ["CATZ-102-|-Mice and Men-|-Cat, Tom", "SOULSTEEP-101-|-42-|-Winfrey, Oprah"]}]) | ||
expect(helper.create_course.id).to eq "CATZ-102" | ||
expect(helper.create_course.instructor).to eq "Cat, Tom" | ||
expect(helper.create_course.name).to eq "Mice and Men" |
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 are testing the return value of the helper method, however, in the view we're using the @course_info
instance variable. We should either test the instance variable or use helper method's return value in the view (and not set an instance variable at all).
That's all I got! Other than my few comments this all looks good to me. |
@jkeck your comments should all be addressed now |
Coverage remained the same when pulling 45662f3dece08b2fa838ee012bf1bb04c66c95c1 on apanel-cr into 37633ee on master. |
Adds course reserve access point
Ready to go Fixes #17