Fix search filter pagination and number of search results displaying.
Review Request #10687 — Created Sept. 6, 2019 and updated
Information | |
---|---|
nicolelisa | |
Review Board | |
master | |
4820 | |
Reviewers | |
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 |
---|---|
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 … |
|
|
Can you remove the first line of your description, since it is basically the same as your summary? |
|
|
Can you wrap the remaining bit of your description at 72 chars? |
|
|
Can you fill in the testing done field to the effect that you verified that this works? |
|
|
Looks like this final revision just contains the most recent commit. If you look at the diff viewer, you'll see … |
|
|
These should be in alphabetical order, for easier maintenance. |
|
|
This is repeated over and over, and it means that we could end up in a situation later where one … |
|
|
I do not believe that active_filters can ever be empty. On lines 130-131 we pull it out of form.cleaned_data with … |
|
|
Style nit: we use single quotes for strings elsewhere, so lets be consistent. Additionally, the printf-style %-formatting is the fastest … |
|
|
Same here re: single quotes. |
|
-
-
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.
-
reviewboard/search/views.py (Diff revision 1) These should be in alphabetical order, for easier maintenance.
-
reviewboard/templates/search/results.html (Diff revision 1) 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 calledextra_url_query
, which we can just tack right onto the URL at the end of the string.
Description: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||||
Diff: |
Revision 2 (+26 -14) |
Checks run (2 succeeded)
-
A few minor things. Otherwise, this looks great. Thanks :)
-
-
-
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 ofform.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. -
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]
-
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+4 -4) |
Checks run (2 succeeded)
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+2 -12) |
Checks run (2 succeeded)
-
-
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.
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+28 -18) |