Summary

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

Review Request #9105 — Created July 31, 2017 and submitted Aug. 30, 2017, 12:51 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Blocks

9116

Commit

0227500...

Reviewers

Groups

reviewboard

People

Description

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).

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
Can you add unit tests to check for access to the search page under the standard conditions, and for when …	chipx86	Aug. 28, 2017, 2:15 p.m.
Swap these.	chipx86	Aug. 21, 2017, 2:22 p.m.
This won't link right. Just say the parent form.	chipx86	Aug. 21, 2017, 2:22 p.m.
"If no filter is provided"	chipx86	Aug. 21, 2017, 2:22 p.m.
self.user gets accessed a lot, so let's remove the attribute lookup and pull it in as a local variable.	chipx86	Aug. 21, 2017, 2:23 p.m.
No blank line.	chipx86	Aug. 21, 2017, 2:23 p.m.
reviewboard.accounts is in the wrong place.	chipx86	Aug. 21, 2017, 2:24 p.m.
This sentence kinda ends strangely.	chipx86	Aug. 21, 2017, 2:24 p.m.
Can you add , unused to these?	chipx86	Aug. 21, 2017, 2:24 p.m.
There's something wrong here. Make sure results with multiple pages are tested thoroughly.	chipx86	Aug. 21, 2017, 2:24 p.m.
W292 no newline at end of file	reviewbot	Aug. 29, 2017, 3:20 p.m.
Can we call this something other than filter, since that's a reserved word?	david	Aug. 30, 2017, 11:18 a.m.
Should this validate that filter is actually a list of unicode?	david	Aug. 30, 2017, 11:17 a.m.
We don't need the fallback here, since it's done in clean_filter. Right?	david	Aug. 30, 2017, 11:15 a.m.

flake8 passed.

JSHint passed.

This looks really nice. Most of my comments are small doc/style issues.

The issue has been resolved. Show all issues

Can you add unit tests to check for access to the search page under the standard conditions, and for when search is disabled?

reviewboard/search/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Swap these.
```
reviewboard/search/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This won't link right. Just say the parent form.
```
reviewboard/search/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"If no filter is provided"
```

reviewboard/search/forms.py (Diff revision 1)

The issue has been resolved. Show all issues

self.user gets accessed a lot, so let's remove the attribute lookup and pull it in as a local variable.

reviewboard/search/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No blank line.
```
reviewboard/search/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
reviewboard.accounts is in the wrong place.
```
reviewboard/search/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This sentence kinda ends strangely.
```
reviewboard/search/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you add , unused to these?
```

reviewboard/search/views.py (Diff revision 1)

The issue has been resolved. Show all issues

There's something wrong here.

Make sure results with multiple pages are tested thoroughly.

Change Summary:

Address most of Christian's issues; tests pending

Diff:

Revision 2 (+360 -228)

Show changes

	reviewboard/dependencies.py
	reviewboard/reviews/managers.py
	reviewboard/search/forms.py
	reviewboard/search/tests.py
	reviewboard/search/urls.py
	reviewboard/search/views.py
	reviewboard/search/search_backends/registry.py
	reviewboard/templates/search/results.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

added tests

Commit:

82aad1570e3c9bc1cd01da7eaff3758c8e71601a

Diff:

Revision 3 (+418 -228)

Show changes

	reviewboard/dependencies.py
	reviewboard/reviews/managers.py
	reviewboard/search/forms.py
	reviewboard/search/tests.py
	reviewboard/search/urls.py
	reviewboard/search/views.py
	reviewboard/search/search_backends/registry.py
	reviewboard/templates/search/results.html

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/search/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W292 no newline at end of file
```

Commit:

82aad1570e3c9bc1cd01da7eaff3758c8e71601a

3d54a9ec23ec51c7343afba90c02914a80b88c9d

Diff:

Revision 4 (+418 -228)

Show changes

	reviewboard/dependencies.py
	reviewboard/reviews/managers.py
	reviewboard/search/forms.py
	reviewboard/search/tests.py
	reviewboard/search/urls.py
	reviewboard/search/views.py
	reviewboard/search/search_backends/registry.py
	reviewboard/templates/search/results.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/search/forms.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Can we call this something other than filter, since that's a reserved word?
```

reviewboard/search/forms.py (Diff revision 4)

The issue has been dropped. Show all issues

Should this validate that filter is actually a list of unicode?

brennie Aug. 30, 2017, 11:18 a.m.

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.

reviewboard/search/forms.py (Diff revision 4)

The issue has been dropped. Show all issues

We don't need the fallback here, since it's done in clean_filter. Right?

brennie Aug. 30, 2017, 11:15 a.m.

If the field is not submitted, it won't be cleaned.

Change Summary:

Addressed David's issues

Commit:

3d54a9ec23ec51c7343afba90c02914a80b88c9d

02275003d441274926f2f0305f757ac1ce68e948

Diff:

Revision 5 (+418 -228)

Show changes

	reviewboard/dependencies.py
	reviewboard/reviews/managers.py
	reviewboard/search/forms.py
	reviewboard/search/tests.py
	reviewboard/search/urls.py
	reviewboard/search/views.py
	reviewboard/search/search_backends/registry.py
	reviewboard/templates/search/results.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-3.0.x (a7ec5cf)