Redo the Review Bot extension configuration.
Review Request #8487 — Created Oct. 24, 2016 and submitted
Most of the Review Bot "configuration" now exists in integration configs (which
specify which tools should run under which conditions), but there are two
pieces of global configuration for the extension that still need to happen.
Specifically, Review Bot needs a user account (to use for posting reviews and
other API requests), and the URL of the AMQP broker.We had an existing settings form that was capable of doing that, but it was
kind of terrible. It used a<select>widget pre-populated with all the users
on the system, which isn't scalable, plus it required people to manually create
a user account through the database UI first. It also had no feedback about
whether the broker URL was correct.This change replaces that with a new form which is submitted asynchronously.
The user selector uses selectize with similar styling to the new multi-user
select field that we added recently, and includes a "create" option for making
a new Review Bot user. Once the settings are configured, it will attempt to
refresh the tools list on each worker, to show some feedback. Right now the
feedback includes the connection state and the list of workers (but not what
tools are available). If everything is configured correctly (the user and
broker are set, the broker is available, and there are running workers), this
will then link to the integration list to set up individual tool
configurations.
- Loaded the form with various states of configuration. Saw that the form
fields were always properly populated. - Created a reviewbot user through the form and saw that the User was created
and properly set in the extension settings. - Updated the user through the form and saw that the extension settings were
changed. - Filled out the broker URL field and saw that the extension settings were
changed. - Verified that the broker status was updated on initial page load and any
time the configuration settings changed. - Tested broker status (and worker status) feedback for the different cases of
"unable to connect", "connected without workers", and "connected with
workers".
| Description | From | Last Updated |
|---|---|---|
|
The usage of the pencil instead of a text field in these forms makes it feel a bit alien, compared … |
 chipx86
|
|
|
I don't think we need to cast here, do we? Worth leaving this as logging.exception maybe? |
chipx86
|
|
|
Same as above. |
chipx86
|
|
|
Alphabetical order. |
chipx86
|
|
|
Docstring? |
chipx86
|
|
|
Properties are documented as attributes, and aren't really intended to have things like "Returns". Unfortunately, there's no equivalent for specifying … |
chipx86
|
|
|
"Model Attributes" |
chipx86
|
|
|
Did dedent get added somewhere? |
chipx86
|
|
|
No need for type="text/javascript" these days. |
chipx86
|
|
|
No trailing comma. |
chipx86
|
|
|
No trailing comma. |
chipx86
|
|
|
This should just use ReviewBotExtension.instance. |
chipx86
|
|
|
This can be: user = get_object_or_none(User, pk=exetnsion.settings['user']) get_object_or_none is provided by Djblets. |
chipx86
|
|
|
ReviewBotExtension.instance |
chipx86
|
|
|
Maybe just combine these? |
chipx86
|
|
|
ReviewBotExtension.instance |
chipx86
|
|
|
ReviewBotExtension.instance |
chipx86
|
|
|
get_object_or_none |
chipx86
|
|
|
get_object_or_none |
chipx86
|
|
|
ReviewBotExtension.instance |
chipx86
|
|
|
ReviewBotExtension.instance |
chipx86
|
|
|
ReviewBotExtension.instance |
chipx86
|
|
|
This should actually be: avatar_services.for_user(user) That way it will return the configured avatar service for that user. If there isn't … |
 brennie
|
|
|
avatar_url ? |
brennie
|
|
|
Properties should go before methods. |
chipx86
|
|
|
Same here. |
chipx86
|
|
|
We might want to show an alert or some inline error, instead of failing silently. |
chipx86
|
|
|
We might want to show an alert or some inline error, instead of failing silently. |
chipx86
|
|
|
Since the entire function is inside this, how about just returning if !this._rendered. |
chipx86
|
|
|
We should have an alert or inline feedback here, too. |
chipx86
|
|
|
I don't know that we want to treat this as public API. We could just return a standard 403 for … |
chipx86
|
|
|
This can be done in dispatch, so it applies to all HTTP requests on the view. |
chipx86
|
|
|
This can also go in dispatch. In fact, we should just make a mixin for this, for use in all … |
chipx86
|
-
-
The usage of the pencil instead of a text field in these forms makes it feel a bit alien, compared to other administration forms. This is the only admin form so far that's instant-apply. I see the need, since we want to be able to show the status, but I wonder if we can do this in a slightly different way.
My thought is that instead of using the inline editor here, we can use a standard text field, helping keep this consistent (visually) with other forms. Then, instead of having a Save button in the bottom-right of the form, we just put it next to the text field. While it's in a different location, it's at least the same type of UI as found in other forms, from a user's perpsective. It allows this to stay instant apply while also remaining visually consistent with other forms.
Another option (maybe even better) is to keep the Save button where it's at and have that trigger saving of both the user and the broker URL. The Status information can then appear below the form in its own Workers box. Might even be able to do more with that at some point, showing more than just the name (logs, connection errors, installed tools, etc.).
-
bot/reviewbot/tasks.py (Diff revision 1) I don't think we need to cast here, do we?
Worth leaving this as
logging.exceptionmaybe? -
-
-
-
extension/reviewbotext/extension.py (Diff revision 1) Properties are documented as attributes, and aren't really intended to have things like "Returns". Unfortunately, there's no equivalent for specifying the type for attributes in Sphinx/autodocs/napolean. (The "Attributes" section fakes it by basically doing
<i>type info</i>in the output). -
-
extension/reviewbotext/static/js/extensionConfig.es6.js (Diff revision 1) Did
dedentget added somewhere? -
extension/reviewbotext/templates/reviewbot/configure.html (Diff revision 1) No need for
type="text/javascript"these days. -
-
-
-
extension/reviewbotext/views.py (Diff revision 1) This can be:
user = get_object_or_none(User, pk=exetnsion.settings['user'])get_object_or_noneis provided by Djblets. -
-
-
-
-
-
-
-
-
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+1024 -39) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
-
Tool: Pyflakes Processed Files: bot/reviewbot/tasks.py extension/reviewbotext/extension.py extension/reviewbotext/admin_urls.py extension/reviewbotext/forms.py extension/reviewbotext/views.py extension/reviewbotext/resources.py Ignored Files: extension/reviewbotext/static/js/extensionConfig.es6.js extension/reviewbotext/templates/reviewbot/configure.html extension/reviewbotext/static/js/.jshintrc extension/reviewbotext/static/css/extension-config.less Tool: PEP8 Style Checker Processed Files: bot/reviewbot/tasks.py extension/reviewbotext/extension.py extension/reviewbotext/admin_urls.py extension/reviewbotext/forms.py extension/reviewbotext/views.py extension/reviewbotext/resources.py Ignored Files: extension/reviewbotext/static/js/extensionConfig.es6.js extension/reviewbotext/templates/reviewbot/configure.html extension/reviewbotext/static/js/.jshintrc extension/reviewbotext/static/css/extension-config.less
Change Summary:
Forgot to commit some changes.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+999 -39) |
-
Tool: Pyflakes Processed Files: bot/reviewbot/tasks.py extension/reviewbotext/extension.py extension/reviewbotext/admin_urls.py extension/reviewbotext/forms.py extension/reviewbotext/views.py extension/reviewbotext/resources.py Ignored Files: extension/reviewbotext/static/js/extensionConfig.es6.js extension/reviewbotext/templates/reviewbot/configure.html extension/reviewbotext/static/js/.jshintrc extension/reviewbotext/static/css/extension-config.less Tool: PEP8 Style Checker Processed Files: bot/reviewbot/tasks.py extension/reviewbotext/extension.py extension/reviewbotext/admin_urls.py extension/reviewbotext/forms.py extension/reviewbotext/views.py extension/reviewbotext/resources.py Ignored Files: extension/reviewbotext/static/js/extensionConfig.es6.js extension/reviewbotext/templates/reviewbot/configure.html extension/reviewbotext/static/js/.jshintrc extension/reviewbotext/static/css/extension-config.less
Change Summary:
Fix some option plumbing.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+999 -39) |
-
Tool: Pyflakes Processed Files: bot/reviewbot/tasks.py extension/reviewbotext/extension.py extension/reviewbotext/admin_urls.py extension/reviewbotext/forms.py extension/reviewbotext/views.py extension/reviewbotext/resources.py Ignored Files: extension/reviewbotext/static/js/extensionConfig.es6.js extension/reviewbotext/templates/reviewbot/configure.html extension/reviewbotext/static/js/.jshintrc extension/reviewbotext/static/css/extension-config.less Tool: PEP8 Style Checker Processed Files: bot/reviewbot/tasks.py extension/reviewbotext/extension.py extension/reviewbotext/admin_urls.py extension/reviewbotext/forms.py extension/reviewbotext/views.py extension/reviewbotext/resources.py Ignored Files: extension/reviewbotext/static/js/extensionConfig.es6.js extension/reviewbotext/templates/reviewbot/configure.html extension/reviewbotext/static/js/.jshintrc extension/reviewbotext/static/css/extension-config.less
-
-
extension/reviewbotext/views.py (Diff revision 4) This should actually be:
avatar_services.for_user(user)That way it will return the configured avatar service for that user. If there isn't one, it will return the default service or
None.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+999 -39) |
-
Tool: Pyflakes Processed Files: bot/reviewbot/tasks.py extension/reviewbotext/extension.py extension/reviewbotext/admin_urls.py extension/reviewbotext/forms.py extension/reviewbotext/views.py extension/reviewbotext/resources.py Ignored Files: extension/reviewbotext/static/js/extensionConfig.es6.js extension/reviewbotext/templates/reviewbot/configure.html extension/reviewbotext/static/js/.jshintrc extension/reviewbotext/static/css/extension-config.less Tool: PEP8 Style Checker Processed Files: bot/reviewbot/tasks.py extension/reviewbotext/extension.py extension/reviewbotext/admin_urls.py extension/reviewbotext/forms.py extension/reviewbotext/views.py extension/reviewbotext/resources.py Ignored Files: extension/reviewbotext/static/js/extensionConfig.es6.js extension/reviewbotext/templates/reviewbot/configure.html extension/reviewbotext/static/js/.jshintrc extension/reviewbotext/static/css/extension-config.less
-
This overall looks great. I just have some small nits, and some areas where we could improve error handling.
-
-
-
extension/reviewbotext/static/js/extensionConfig.es6.js (Diff revision 5) We might want to show an alert or some inline error, instead of failing silently.
-
extension/reviewbotext/static/js/extensionConfig.es6.js (Diff revision 5) We might want to show an alert or some inline error, instead of failing silently.
-
extension/reviewbotext/static/js/extensionConfig.es6.js (Diff revision 5) Since the entire function is inside this, how about just returning if
!this._rendered. -
extension/reviewbotext/static/js/extensionConfig.es6.js (Diff revision 5) We should have an alert or inline feedback here, too.
-
extension/reviewbotext/views.py (Diff revision 5) I don't know that we want to treat this as public API. We could just return a standard 403 for now, maybe work toward having Review Board be smart when encountering a 403 without a payload so we can standardize things there without the import.
-
extension/reviewbotext/views.py (Diff revision 5) This can be done in
dispatch, so it applies to all HTTP requests on the view. -
extension/reviewbotext/views.py (Diff revision 5) This can also go in
dispatch.In fact, we should just make a mixin for this, for use in all the views that need it.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+1012 -39) |
-
Tool: Pyflakes Processed Files: bot/reviewbot/tasks.py extension/reviewbotext/extension.py extension/reviewbotext/admin_urls.py extension/reviewbotext/forms.py extension/reviewbotext/views.py extension/reviewbotext/resources.py Ignored Files: extension/reviewbotext/static/js/extensionConfig.es6.js extension/reviewbotext/templates/reviewbot/configure.html extension/reviewbotext/static/js/.jshintrc extension/reviewbotext/static/css/extension-config.less Tool: PEP8 Style Checker Processed Files: bot/reviewbot/tasks.py extension/reviewbotext/extension.py extension/reviewbotext/admin_urls.py extension/reviewbotext/forms.py extension/reviewbotext/views.py extension/reviewbotext/resources.py Ignored Files: extension/reviewbotext/static/js/extensionConfig.es6.js extension/reviewbotext/templates/reviewbot/configure.html extension/reviewbotext/static/js/.jshintrc extension/reviewbotext/static/css/extension-config.less
Tool: Pyflakes Processed Files: bot/reviewbot/tasks.py extension/reviewbotext/extension.py extension/reviewbotext/admin_urls.py extension/reviewbotext/forms.py extension/reviewbotext/views.py extension/reviewbotext/resources.py Ignored Files: extension/reviewbotext/static/js/extensionConfig.es6.js extension/reviewbotext/templates/reviewbot/configure.html extension/reviewbotext/static/js/.jshintrc extension/reviewbotext/static/css/extension-config.less Tool: PEP8 Style Checker Processed Files: bot/reviewbot/tasks.py extension/reviewbotext/extension.py extension/reviewbotext/admin_urls.py extension/reviewbotext/forms.py extension/reviewbotext/views.py extension/reviewbotext/resources.py Ignored Files: extension/reviewbotext/static/js/extensionConfig.es6.js extension/reviewbotext/templates/reviewbot/configure.html extension/reviewbotext/static/js/.jshintrc extension/reviewbotext/static/css/extension-config.less