Move Review Board's search to a SearchForm + generic SearchView

Review Request #9105 — Created July 31, 2017 and submitted

Information

Review Board
release-3.0.x
0227500...

Reviewers

This patch updates Review Board's RBSearchView, which was previously
using Haystack's deprecated SearchView with the generic version added
in 2.4 that builds on top of existing Django forms infrastructure. This
means that we now require a minimum version of 2.4.0 for Haystack.

The actual search code has been split out into a form that does the
actual searching. This will allow searches to be performed without
setting up a bunch of view rendering infrastructure that we won't need
each time (e.g., from the API).

Ran unit tests.

Description From Last Updated

Can you add unit tests to check for access to the search page under the standard conditions, and for when …

chipx86chipx86

Swap these.

chipx86chipx86

This won't link right. Just say the parent form.

chipx86chipx86

"If no filter is provided"

chipx86chipx86

self.user gets accessed a lot, so let's remove the attribute lookup and pull it in as a local variable.

chipx86chipx86

No blank line.

chipx86chipx86

reviewboard.accounts is in the wrong place.

chipx86chipx86

This sentence kinda ends strangely.

chipx86chipx86

Can you add , unused to these?

chipx86chipx86

There's something wrong here. Make sure results with multiple pages are tested thoroughly.

chipx86chipx86

W292 no newline at end of file

reviewbotreviewbot

Can we call this something other than filter, since that's a reserved word?

daviddavid

Should this validate that filter is actually a list of unicode?

daviddavid

We don't need the fallback here, since it's done in clean_filter. Right?

daviddavid
chipx86
  1. This looks really nice. Most of my comments are small doc/style issues.

  2. Can you add unit tests to check for access to the search page under the standard conditions, and for when search is disabled?

  3. reviewboard/search/forms.py (Diff revision 1)
     
     
     

    Swap these.

  4. reviewboard/search/forms.py (Diff revision 1)
     
     

    This won't link right. Just say the parent form.

  5. reviewboard/search/forms.py (Diff revision 1)
     
     

    "If no filter is provided"

  6. reviewboard/search/forms.py (Diff revision 1)
     
     

    self.user gets accessed a lot, so let's remove the attribute lookup and pull it in as a local variable.

  7. reviewboard/search/forms.py (Diff revision 1)
     
     
     
     

    No blank line.

  8. reviewboard/search/views.py (Diff revision 1)
     
     
     
     

    reviewboard.accounts is in the wrong place.

  9. reviewboard/search/views.py (Diff revision 1)
     
     

    This sentence kinda ends strangely.

  10. reviewboard/search/views.py (Diff revision 1)
     
     
     
     
     
     

    Can you add , unused to these?

  11. reviewboard/search/views.py (Diff revision 1)
     
     

    There's something wrong here.

    Make sure results with multiple pages are tested thoroughly.

  12. 
      
brennie
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
david
  1. 
      
  2. reviewboard/search/forms.py (Diff revision 4)
     
     

    Can we call this something other than filter, since that's a reserved word?

  3. reviewboard/search/forms.py (Diff revision 4)
     
     

    Should this validate that filter is actually a list of unicode?

    1. The fact that its a MultipleChoiceField should handle that. If it isnt a list, or the contents arent members of the choices, it will error.

  4. reviewboard/search/forms.py (Diff revision 4)
     
     

    We don't need the fallback here, since it's done in clean_filter. Right?

    1. If the field is not submitted, it won't be cleaned.

  5. 
      
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (a7ec5cf)
Loading...