Refactors search and adds some tests.

Refactors search and adds some tests.

Search views have been moved to their own view file.
The actual search machinery has been moved into its own method, do_search, as
it did essentially the same thing in both views.
Tests are added using a mock for do_search and a result fake for the result
type the search should return.

https://code.launchpad.net/~jcsackett/charmworld/search-refactor/+merge/143182

(do not edit description out of merge proposal)

[j.c.sackett, 2013-01-14 20:45:28]

Please take a look.

[rharding, 2013-01-15 13:37:25]

lgtm but with suggestions on using Mock and fixing a small UX issue around the
code changed.

https://codereview.appspot.com/7092052/diff/1/charmworld/views/search.py
File charmworld/views/search.py (right):

https://codereview.appspot.com/7092052/diff/1/charmworld/views/search.py#newc...
charmworld/views/search.py:38: if not "search_text" in request.params:
This should set a flash message so that an error can be displayed on the search
field. 

http://pyramid.readthedocs.org/en/latest/narr/sessions.html?highlight=flash#f...

Maybe we can change the placeholder of the search input when the flash is set
with a "Please enter search terms" or something vs just Search.

https://codereview.appspot.com/7092052/diff/1/charmworld/views/search.py#newc...
charmworld/views/search.py:43: # `results.matches_human_readable_estimate`
Please file a bug to track with this.

https://codereview.appspot.com/7092052/diff/1/charmworld/views/tests/test_sea...
File charmworld/views/tests/test_search.py (right):

https://codereview.appspot.com/7092052/diff/1/charmworld/views/tests/test_sea...
charmworld/views/tests/test_search.py:28: super(SearchTests, self).setUp()
I think you can just use mock.patch to do this and not track the old/new
relationship.

I'd just use the decorator over the test so that it's clear when looking at a
single test.

from charmworld.views import search


patched_search_ret = {
    'results': ResultFake(results),
    'search_time': search_time,
    'charm_total': len(results),
}

@patch('search.do_search', return_value=patched_search_ret)
def test_search(self, search_mock):
    .......Might be a trick to import the search.do_search correctly so that
it's patched in the view itself. The import as name matters to matching.
    search_mock.assert_called_with...

[j.c.sackett, 2013-01-15 13:43:11]

https://codereview.appspot.com/7092052/diff/1/charmworld/views/search.py
File charmworld/views/search.py (right):

https://codereview.appspot.com/7092052/diff/1/charmworld/views/search.py#newc...
charmworld/views/search.py:38: if not "search_text" in request.params:
On 2013/01/15 13:37:26, rharding wrote:
> This should set a flash message so that an error can be displayed on the
search
> field. 
> 
>
http://pyramid.readthedocs.org/en/latest/narr/sessions.html?highlight=flash#f...
> 
> Maybe we can change the placeholder of the search input when the flash is set
> with a "Please enter search terms" or something vs just Search.

I'll set that up.

https://codereview.appspot.com/7092052/diff/1/charmworld/views/search.py#newc...
charmworld/views/search.py:43: # `results.matches_human_readable_estimate`
On 2013/01/15 13:37:26, rharding wrote:
> Please file a bug to track with this.

Sure.

https://codereview.appspot.com/7092052/diff/1/charmworld/views/tests/test_sea...
File charmworld/views/tests/test_search.py (right):

https://codereview.appspot.com/7092052/diff/1/charmworld/views/tests/test_sea...
charmworld/views/tests/test_search.py:28: super(SearchTests, self).setUp()
On 2013/01/15 13:37:26, rharding wrote:
> I think you can just use mock.patch to do this and not track the old/new
> relationship.
> 
> I'd just use the decorator over the test so that it's clear when looking at a
> single test.
> 
> from charmworld.views import search
> 
> 
> patched_search_ret = {
>     'results': ResultFake(results),
>     'search_time': search_time,
>     'charm_total': len(results),
> }
> 
> @patch('search.do_search', return_value=patched_search_ret)
> def test_search(self, search_mock):
>     .......Might be a trick to import the search.do_search correctly so that
> it's patched in the view itself. The import as name matters to matching.
>     search_mock.assert_called_with...

Two things: using the decorator may lose autospeccing, which is handy in a
wrapper that may develop over time. Possibly the patch decorator accepts an
autospec command.

More important, I tried this, as well as the with patch(target) syntax with
about every variation of the import chain for the target I could think of. In
each case it either didn't patch do_search or failed to find a valid target.
Thus this current state. I'll give it another try, but the patch target stuff
has frequently been a bit dodgy in my experience.

[j.c.sackett, 2013-01-15 15:27:25]

Please take a look.

[rharding, 2013-01-15 15:31:42]

lgtm thanks for the update.

[j.c.sackett, 2013-01-15 17:14:40]

*** Submitted:

Refactors search and adds some tests.

Search views have been moved to their own view file.
The actual search machinery has been moved into its own method, do_search, as
it did essentially the same thing in both views.
Tests are added using a mock for do_search and a result fake for the result
type the search should return.

R=rharding
CC=
https://codereview.appspot.com/7092052