124 search bug

[McEileen]

I fixed the bug where unapproved lessons showed up in the search feature, but I'm not sure how to test it. Right now my test is failing because it's getting a redirect instead of a 200. I think this is because I didn't stub out the search feature properly, but I'm not sure how to stub it. @vkoves, @TheJhyde or @hwayne, could one of you please provide guidance?

To provide background on what research I've done, I read through the ruby on rails guide for controller testing and I looked up some stack overflow questions, though most of them were about rspec.

[vkoves]

So I realized why Travis isn't failing for this - the Travis config file only specifies to run rake db:create and rake db:migrate in the test environment, it never actually runs tests. This should probably be opened up as an issue, as that defies the whole point of using Travis. I'm investigating tests locally right now.

[vkoves]

Manual testing confirms this feature works, though I'm a bit puzzled why both tweets and lessons have an approved attribute.

I also realized the issue with your test @McEileen - you accidentally named the test TweetsControllerTest instead of LessonsControllerTest.

Change this line in lessons_controller_test.rb:

class TweetsControllerTest < ActionController::TestCase

to:

class LessonsControllerTest < ActionController::TestCase
  include Devise::Test::ControllerHelpers

Since Devise logic is done on lessons, it complained if I didn't include the Devise test helpers.

However, this still fails tests, as you expected the response body to be @approved_lessons when really it's a bunch of HTML that includes some data about @approved_lessons. I think your test might be too simple if you're looking to test the actual HTML response from the lessons controller, or you might want to switch to making a JSON request to be able to test whether the right lessons are returned more directly.

[McEileen]

Thanks @vkoves! I opened issue #126 in response to the travis bug and I don't think it will be too hard to fix. I agree that it's odd for both tweets and lessons to have an approved attribute. I'm going to think about that more. I'll also get the test to pass.

[McEileen]

@vkoves This is kind of hacky, but I tested that the unapproved lesson wasn't returned by using @assert_no_match and comparing the unapproved lesson's intro to response.body My first test was brittle because I hard-coded in the intro. I tried to fix it by undoing that commit locally (after I pushed) and then writing a new commit with the same name, but then I got a merge conflict when I tried to push. Live and learn!

[vkoves]

@McEileen - I merged master back into this branch so that Travis would be running tests properly.

Unfortunately, I don't think is a fair test. The test still passes even if you revert the functionality so that all tweets are shown. This seems to be caused by the fact that the lessons page seems to shows the tweets in a lesson and not the lesson's intro.

However, I've cooked up a simple test that should resolve this and will also test that approved tweets are being displayed:

  test "the search feature only returns lessons that are approved" do
      get :index
      assert_response :success
      assert_no_match(@unapproved_lesson.hash_tag, @response.body)
      assert_match(@approved_lessons.first.hash_tag, @response.body)
  end

I have confirmed this works with your change and breaks without it. Since this looks for the hashtag of the lesson (which should be a unique property) it should also be a fairly robust test. Let me know if this works!

[McEileen]

Hi @vkoves, thank you! I think the test you wrote is better for the index because it doesn't include a parameter of a keyword search. I wrote a test for the index based on what you wrote. Also, I confirmed that the index test fails when I take off the where(approved:true) part of line 10/11. I also fixed the search test based on the assertions you wrote. Good catch about the lessons page not showing a lesson's intro. I think this code can be merged in now. (I also merged my up-to-date local master into my local feature branch before pushing, but I still had to pull from remote 124-search-bug before pushing.... Sigh, there are many merge commits)