flake8
passed.
JSHint
passed.
Fix search filter pagination and number of search results displaying.
Review Request #10687 — Created Sept. 6, 2019 and updated
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 |
|---|---|---|
| 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 … |
 chipx86
|
|
|
Can you remove the first line of your description, since it is basically the same as your summary? |
 brennie
|
|
|
Can you wrap the remaining bit of your description at 72 chars? |
brennie
|
|
|
Can you fill in the testing done field to the effect that you verified that this works? |
brennie
|
|
|
Looks like this final revision just contains the most recent commit. If you look at the diff viewer, you'll see … |
chipx86
|
|
|
These should be in alphabetical order, for easier maintenance. |
chipx86
|
|
|
This is repeated over and over, and it means that we could end up in a situation later where one … |
chipx86
|
|
|
I do not believe that active_filters can ever be empty. On lines 130-131 we pull it out of form.cleaned_data with … |
brennie
|
|
|
Style nit: we use single quotes for strings elsewhere, so lets be consistent. Additionally, the printf-style %-formatting is the fastest … |
brennie
|
|
|
Same here re: single quotes. |
brennie
|
-
-
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.
-
-
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:
-
~ Fix search filter pagination bug
~ Fix search filter pagination and number of search results displaying.
+ + 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.
- Commits:
-
Summary ID Author f9a73b27c813eb4257100607fa78acf13ce7139b nicolelisa b57c3d57f8df9ccf985f9f34bba121503283d4c5 nicolelisa
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
A few minor things. Otherwise, this looks great. Thanks :)
-
-
-
I do not believe that
active_filterscan ever be empty. On lines 130-131 we pull it out ofform.cleaned_datawith 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_queryOR always use&model_query={{active_filter}}where you are using{{extra_url_query}}in the template. -
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:
-
Fix search filter paginationFix search filter pagination and number of search results displaying.
- Description:
-
~ Fix search filter pagination and number of search results displaying.
~ 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. ~ 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.
~ 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.
- Commits:
-
Summary ID Author b57c3d57f8df9ccf985f9f34bba121503283d4c5 nicolelisa 2f8afd7ccaf7e83e8fac0d73434000cdc4674c73 nicolelisa - Diff:
-
Revision 3 (+4 -4)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Commits:
-
Summary ID Author 2f8afd7ccaf7e83e8fac0d73434000cdc4674c73 nicolelisa 9fc0b2c6b1a6915534dfcc025cb897664468a82c nicolelisa - Diff:
-
Revision 4 (+2 -12)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
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.