Fix search filter pagination and number of search results displaying.

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

nicolelisa
Review Board
master
4820
reviewboard, students

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 Author
Fix search filter pagination
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. 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)
     
     
     
     
     
     

    These should be in alphabetical order, for easier maintenance.

  4. 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. Can you remove the first line of your description, since it is basically the same as your summary?

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

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

    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)
     
     

    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)
     
     

    Same here re: single quotes.

  7. 
      
nicolelisa
nicolelisa
nicolelisa
brennie
  1. LGTM

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

  3. 
      
nicolelisa
chipx86
  1. 
      
  2. 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 Author
-
removed else statement as per review
nicolelisa
+
Fix search filter pagination
nicolelisa

Diff:

Revision 5 (+28 -18)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...