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

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

Barret Rennie
Review Board
release-3.0.x
9116
0227500...
reviewboard

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.

  • 0
  • 0
  • 12
  • 2
  • 14
Description From Last Updated
Christian Hammond
  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. 
      
Barret Rennie
Barret Rennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Barret Rennie
David Trowbridge
  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. 
      
Barret Rennie
David Trowbridge
  1. Ship It!
  2. 
      
Barret Rennie
Review request changed

Status: Closed (submitted)

Change Summary:

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