Fix search filter pagination and number of search results displaying.

Review Request #10687 — Created Sept. 6, 2019 and updated

Information

Review Board
master

Reviewers

When a filter (ie. "Review Requests" or "Users") is applied to a search,
any navigation in the pagination resulted in returning to "All Results".
This was because the active filter was not being referenced as a url
parameter when navigating the pagination links. There was also an issue
with several of the variables used throughout the template, including
the results count only displaying the number of results of the active
page, and the "last page" link referencing an empty url, causing it
to return to the first page.

The search functionality now passes the active filter to the url,
and the correct variables are being referenced for the last page link
and the number of search results.

This has been manually tested and I have verified that it is working
for all filters and pagination links.

Summary ID Author
Fix search filter pagination
3d658bbcee9e0e7f3f9b780911f843f7fe5b8d3b nicolelisa
Description From Last Updated

I commented on your other review request (/r/10686) regarding the write-up for your change. The same notes apply here. Learning …

chipx86chipx86

Can you remove the first line of your description, since it is basically the same as your summary?

brenniebrennie

Can you wrap the remaining bit of your description at 72 chars?

brenniebrennie

Can you fill in the testing done field to the effect that you verified that this works?

brenniebrennie

Looks like this final revision just contains the most recent commit. If you look at the diff viewer, you'll see …

chipx86chipx86

These should be in alphabetical order, for easier maintenance.

chipx86chipx86

This is repeated over and over, and it means that we could end up in a situation later where one …

chipx86chipx86

I do not believe that active_filters can ever be empty. On lines 130-131 we pull it out of form.cleaned_data with …

brenniebrennie

Style nit: we use single quotes for strings elsewhere, so lets be consistent. Additionally, the printf-style %-formatting is the fastest …

brenniebrennie

Same here re: single quotes.

brenniebrennie
chipx86
  1. 
      
  2. Show all issues

    I commented on your other review request (/r/10686) regarding the write-up for your change. The same notes apply here.

    Learning to write really good commit descriptions is a skill that will help you stand out from your peers, and is something most engineers are really bad at in the industry. These early review requests will be good practice.

  3. reviewboard/search/views.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    These should be in alphabetical order, for easier maintenance.

  4. Show all issues

    This is repeated over and over, and it means that we could end up in a situation later where one is updated and another forgotten. Best to avoid repeating yourself in code when it's complex enough.

    So a way we can address this is to precompute a variable that contains the &model_filter=.... In fact, we may even want to add other things like this down the road, beyond just &model_filter. So how about we define a variable in the template context called extra_url_query, which we can just tack right onto the URL at the end of the string.

  5. 
      
nicolelisa
brennie
  1. A few minor things. Otherwise, this looks great. Thanks :)

  2. Show all issues

    Can you remove the first line of your description, since it is basically the same as your summary?

  3. Show all issues

    Can you wrap the remaining bit of your description at 72 chars?

  4. reviewboard/search/views.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    I do not believe that active_filters can ever be empty. On lines 130-131 we pull it out of form.cleaned_data with a default of [form.FILTER_ALL] if it is unset.

    This is also confirmed by line 145 where we have 'active_filter': active_filters[0].

    Therefore we can git rid of this if-statement altogether and always set extra_url_query OR always use &model_query={{active_filter}} where you are using {{extra_url_query}} in the template.

    1. It is empty on the "all results" I think. When I remove the else statement, I get a 'referenced before assignment' error when I search because it is empty.

  5. reviewboard/search/views.py (Diff revision 2)
     
     
    Show all issues

    Style nit: we use single quotes for strings elsewhere, so lets be consistent.

    Additionally, the printf-style %-formatting is the fastest way to concatenate strings in Python, e.g.

    extra_url_query = '&model_filter=%s' % active_filters[0]
    
  6. reviewboard/search/views.py (Diff revision 2)
     
     
    Show all issues

    Same here re: single quotes.

  7. 
      
nicolelisa
nicolelisa
nicolelisa
brennie
  1. LGTM

  2. Show all issues

    Can you fill in the testing done field to the effect that you verified that this works?

  3. 
      
nicolelisa
chipx86
  1. 
      
  2. Show all issues

    Looks like this final revision just contains the most recent commit. If you look at the diff viewer, you'll see that the meat of the change isn't present, meaning it won't be part of the change that gets landed (and will thus break).

    To fix this, you'll need to post the entire branch for review (squashed, for now -- we can't land multiple commits yet, and prefer not to for smaller bits of work). Barret can help you sort out the posting.

  3. 
      
nicolelisa
Review request changed

Commits:

Summary ID Author
removed else statement as per review
9fc0b2c6b1a6915534dfcc025cb897664468a82c nicolelisa
Fix search filter pagination
3d658bbcee9e0e7f3f9b780911f843f7fe5b8d3b nicolelisa

Diff:

Revision 5 (+28 -18)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...