Summary

Correctly check for search backend validation in search settings form

Review Request #9824 — Created March 22, 2018 and submitted March 22, 2018, 11:43 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

4668

Depends On

Commit

efd02b6...

Reviewers

Groups

reviewboard

People

Description

The search settings form was assuming that the search_backend_id field
would be available in clean, but the value is only present if the
backend passes validation. We now check for its existence instead of
assuming that it does exist and carry on with validation accordingly.

Testing Done


Ran unit tests.
Selecting "elasticsearch" on the search settings form without it

  installed no longer results in a 500.
Enabling Elasticsearch works.
Enabling Whoosh works.

Issues

Description	From	Last Updated
I also want to see test cases for a correct backend, and info in Testing Done that you've tested all …	chipx86	March 22, 2018, 6:29 p.m.
E501 line too long (84 > 79 characters)	reviewbot	March 22, 2018, 6:19 p.m.
This should be two tests: One for a backend that doesn't exist, another for the validate() check.	chipx86	March 22, 2018, 6:26 p.m.
This line is too long.	chipx86	March 22, 2018, 6:26 p.m.
Ordering is all over the place. Can you fix this?	chipx86	March 22, 2018, 6:52 p.m.
Rather than the reset() above, can you use this pattern: register try: ... finally: unregister	chipx86	March 22, 2018, 6:52 p.m.
The "plain" version of a test should always go above the other ones. This should be first.	chipx86	March 22, 2018, 6:53 p.m.
F841 local variable 'form' is assigned to but never used	reviewbot	March 22, 2018, 6:52 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	March 22, 2018, 6:52 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	March 22, 2018, 6:52 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	March 22, 2018, 10:36 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	March 22, 2018, 10:36 p.m.
Ordering is still off here.	chipx86	March 22, 2018, 11:15 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/admin/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (84 > 79 characters)
```

Bugs:

+	4668

Commit:

-	ddb9851e7edf4af3f9d677b49822546c1fe55ff7
+	c956c13e78057ace5840ce8ea99ba60a6bfe72df

Diff:

Revision 2 (+43 -4)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/admin/tests.py (Diff revision 2)

The issue has been resolved. Show all issues

This should be two tests: One for a backend that doesn't exist, another for the validate() check.

reviewboard/admin/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This line is too long.
```

The issue has been resolved. Show all issues

I also want to see test cases for a correct backend, and info in Testing Done that you've tested all backends in there.

Testing Done:

~		Ran unit tests.
~		Selecting "elasticsearch" on the search settings form without it
~		installed no longer results in a 500.
	~	Ran unit tests.
	~	Selecting "elasticsearch" on the search settings form without it installed no longer results in a 500.
	~	Enabling Elasticsearch works.
	+	Enabling Whoosh works.

Commit:

-	c956c13e78057ace5840ce8ea99ba60a6bfe72df
+	306886b306b2e1029bd7120f11eea76d6d53a081

Diff:

Revision 3 (+64 -4)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/tests.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/admin/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F841 local variable 'form' is assigned to but never used
```
reviewboard/admin/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
reviewboard/admin/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```

reviewboard/admin/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Ordering is all over the place. Can you fix this?
```

reviewboard/admin/tests.py (Diff revision 3)

The issue has been resolved. Show all issues

Rather than the reset() above, can you use this pattern:

register

try:
    ...
finally:
    unregister

reviewboard/admin/tests.py (Diff revision 3)

The issue has been resolved. Show all issues

The "plain" version of a test should always go above the other ones. This should be first.

Commit:

-	306886b306b2e1029bd7120f11eea76d6d53a081
+	ebfc44acef0532405496f53b1bab8aa3b2235f3e

Diff:

Revision 4 (+68 -5)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/tests.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/admin/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
reviewboard/admin/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```

Commit:

-	ebfc44acef0532405496f53b1bab8aa3b2235f3e
+	89e2e2baafac2ddcd3c8395648eacc64f231f3f6

Diff:

Revision 5 (+68 -5)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/admin/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Ordering is still off here.
```

Owner:

-	brennie
+	david

Commit:

-	89e2e2baafac2ddcd3c8395648eacc64f231f3f6
+	efd02b64e6eef78a227a585232d9b3bbb0c653ac

Diff:

Revision 6 (+74 -5)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Oops.

Owner:

-	david
+	brennie

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (ce0c109)