Summary

Sandbox reviewboard.admin.widgets.Widget

Review Request #6650 — Created Nov. 27, 2014 and submitted Dec. 22, 2014, 2:04 p.m.

Information

Owner

justy777

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

cf2b791...

Reviewers

Groups

reviewboard, students

People

Description

Extensions that create a Widget subclass (using the AdminWidgetHook) can throw exceptions inside Reviewboard. The parts of Reviewboard that call those methods have been sandboxed.

Now when a Widget subclass from an extension throws an exception; Reviewboard logs the errors with enough information to find which method in the Widget subclass threw the exception.

Testing Done

Unit tests have been written to make sure that functions from a Widget subclass have been called, and when an exception is thrown it gets logged.

The tests fail without the sanboxing, and succeed with it.

Admin UI displays the widgets in the correct column.
When the "X" in the corner is clicked it closes the widget, but does refresh the page.

Files

Issues

Description	From	Last Updated
"Testing admin widget sandboxing for ..." Same below.	chipx86	Nov. 30, 2014, 3:44 p.m.
This isn't always going to be clear which went wrong. Instead, the lambda function should call a new, private function …	chipx86	Nov. 30, 2014, 6:27 p.m.
This needs to return the generated data. Make sure the admin UI is working as expected after this change. It …	chipx86	Dec. 4, 2014, 8:39 p.m.
No blank line.	chipx86	Dec. 4, 2014, 8:39 p.m.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/widgets.py
    reviewboard/admin/tests.py



Tool: Pyflakes
Processed Files:
    reviewboard/admin/widgets.py
    reviewboard/admin/tests.py

reviewboard/admin/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"Testing admin widget sandboxing for ..."
Same below.
```

reviewboard/admin/widgets.py (Diff revision 1)

The issue has been resolved. Show all issues

This isn't always going to be clear which went wrong.
Instead, the lambda function should call a new, private function that calls self.generate_data and wraps that in a try/except.
generate_cache_key should be called prior to the cache_memoize, wrapped in a try/except. The resulting key can then be passed to cache_memoize outside the try/except.

Commit:

-	6b8e2b391f583be3eafd174e4d73617333e9ca9a
+	9086a620e9d18b12a7a6b121a68e1e2a699a4e46

Diff:

Revision 2 (+78 -3)

Show changes

	reviewboard/admin/tests.py
	reviewboard/admin/widgets.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/widgets.py
    reviewboard/admin/tests.py



Tool: Pyflakes
Processed Files:
    reviewboard/admin/widgets.py
    reviewboard/admin/tests.py

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

The issue has been resolved. Show all issues

This needs to return the generated data. Make sure the admin UI is working as expected after this change.
It should also raise an exception that can be caught later when calling cache_memoize. (The exception can then be ignored in that callback.)
The reason for that is that we don't want cache_memoize to either store a None value (as the exception may be temporary, so we want to try again), and for the same reasons we don't want to store any other default value. An exception will guarantee that nothing is cached.
You can just re-raise and then catch in the try/except for the cache_memoize call.

reviewboard/admin/widgets.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No blank line.
```

Commit:

-	9086a620e9d18b12a7a6b121a68e1e2a699a4e46
+	5e8752c31ea05cbb043ed4f7eac91779b764d3ef

Diff:

Revision 3 (+77 -3)

Show changes

	reviewboard/admin/tests.py
	reviewboard/admin/widgets.py

Tool: Pyflakes
Processed Files:
    reviewboard/admin/widgets.py
    reviewboard/admin/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/widgets.py
    reviewboard/admin/tests.py

Can you provide details on what testing you did manually on the admin site?

justy777 Dec. 12, 2014, 8:51 a.m.

Admin site was not used for testing in this patch. It was used in my patch to sandbox HostingService.

david Dec. 12, 2014, 3:40 p.m.

Christian asked you before to "make sure the admin UI is working". Running the tests is great but I don't want there to be a regression in the functionality.

Testing Done:

		Unit tests have been written to make sure that functions from a Widget subclass have been called, and when an exception is thrown it gets logged.

		The tests fail without the sanboxing, and succeed with it.
	+
	+	Admin UI displays the widgets in the correct column.
	+	When the "X" in the corner is clicked it closes the widget, but does refresh the page.

Commit:

-	5e8752c31ea05cbb043ed4f7eac91779b764d3ef
+	cf2b79145bc167891f160b5cf007a920a4b29f89

Diff:

Revision 4 (+6 -4)

Show changes

reviewboard/admin/views.py

Added Files:

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/views.py



Tool: Pyflakes
Processed Files:
    reviewboard/admin/views.py

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (ce3de12)