Improve Review Bot's admin interface
Review Request #6999 — Created Feb. 28, 2015 and discarded
As a part of the project "Improve Review Bot's admin interface" in https://reviewboard.hackpad.com/Student-Project-Ideas-bWzTgtBtq9f, this change improves UI in "Tool Change" view in admin site.
The styling of the form is done in https://reviews.reviewboard.org/r/6998/
Works done are:
- implemented JSONDictWidget
this is the widget to be used for profile.tool_settings- linked profile model admin with ProfileForest and ProfileForm
- made ProfileForm use JSONDictWidget for 'tool_settings' field
Now, tool_settings part is rendered as a set of widgets which are declared in "reviewbot/tools/{tool_name}.py" for each tool.
Browser test done:
1. go to http://localhost:8080/admin/extensions/reviewbotext.extension.ReviewBotExtension/db/reviewbotext/tool/
2. click one of the tools. for example, "PEP8 Style Checker"
3. see if existing profiles' "Tool settings" part is displayed well
4. click "Add another profile"
5. fill out the form and save
6. click the same tool
7. see if newly added profile is displayed well
8. try deleting and check againSome possible edge cases are:
1. there's no existing profiles
2. there's exactly one porfile
Description | From | Last Updated |
---|---|---|
'Field' imported but unused |
reviewbot | |
'Widget' imported but unused |
reviewbot | |
You should combine this with the other reviewbotext.forms import. |
brennie | |
No comma on this line. |
brennie | |
Move this to the previous line. |
brennie | |
Move this to the previous line. |
brennie | |
Blank line between these. |
brennie | |
It seems very un-Pythonic to be doing very strict type checking. |
brennie | |
You can do if class_path. |
brennie | |
Why not just '.'.join(class_path) ? This will make another list. |
brennie | |
This should be in the second import group. |
brennie | |
Our imports are split into three groups: Python standard library imports Third party library imports Project library imports. Each import … |
brennie | |
This should go in the third import group. It should also be an absolute import, e.g. from reviewbotext.utils import get_class |
brennie | |
This should go in the first import group. |
brennie | |
Add a docstring. |
brennie | |
Move this to the previous line. |
brennie | |
Period. |
brennie | |
Period. |
brennie | |
Use a period. Can this be clarified? |
brennie | |
Comma on this line. |
brennie | |
Period. |
brennie | |
Is there any reason that we have to check if this is a Widget? Are we expecting to ever not … |
brennie | |
Period. |
brennie | |
Period. |
brennie | |
Move this to the previous line. |
brennie | |
You should do from django.utils.six.moves import zip The six zip creates a generator instead of a list. |
brennie | |
Blank line between these. |
brennie | |
String concatenation is expensive. You should store all the parts in an array and use ''.join(string_parts) |
brennie | |
Move this to the previous line. |
brennie | |
Period. |
brennie | |
Period. |
brennie | |
Use a generator expression instead of a list comprehension (i.e., use ( and ) instead of [ and ]. It … |
brennie | |
We don't need the enclosing [ and ]; it is more efficient to use a generator expression than a list … |
brennie | |
'Widget' imported but unused |
reviewbot | |
Can be in alphabetical order. |
XU xuanyi | |
'Field' imported but unused |
reviewbot | |
Does one blank line after class docstring apply to this? I understand that most Widget class did have a blank … |
XU xuanyi | |
Add trailing comma. |
VT VTL-Developer | |
Leftover debug output? |
david | |
Since this is now unused, we should just get rid of it. |
david | |
typo: "defiend" |
david | |
It might be nicer to do the join here, rather than below. |
david |
- Change Summary:
-
deleted unused imports and uploaded screenshots
- Commit:
-
7076828d972d8f05691abc9781a5b61cd3fa51e773e5cdc234e3fb63acb9d3b7c13a8652a55fa47d
- Diff:
-
Revision 2 (+140 -134)
- Added Files:
-
Tool: PEP8 Style Checker Processed Files: extension/reviewbotext/utils.py extension/reviewbotext/forms.py extension/reviewbotext/widgets.py extension/reviewbotext/admin.py Tool: Pyflakes Processed Files: extension/reviewbotext/utils.py extension/reviewbotext/forms.py extension/reviewbotext/widgets.py extension/reviewbotext/admin.py
- Change Summary:
-
wrote comments
- Description:
-
As a part of the project "Improve Review Bot's admin interface" in https://reviewboard.hackpad.com/Student-Project-Ideas-bWzTgtBtq9f, this change improves UI in "Tool Change" view in admin site.
The styling of the form is done in https://reviews.reviewboard.org/r/6998/
Works done are:
- implemented JSONDictWidget
this is the widget to be used for profile.tool_settings
- linked profile model admin with ProfileForest and ProfileForm
- made ProfileForm use JSONDictWidget for 'tool_settings' field
Now, tool_settings part is rendered as a set of widgets which are declared in "reviewbot/tools/{tool_name}.py" for each tool.
- - Documentation is not done yet => still WIP
- implemented JSONDictWidget
- Commit:
-
73e5cdc234e3fb63acb9d3b7c13a8652a55fa47ddac257bf8fedfbcd02dd2dee775f7a55787e9d8f
-
Tool: PEP8 Style Checker Processed Files: extension/reviewbotext/utils.py extension/reviewbotext/forms.py extension/reviewbotext/widgets.py extension/reviewbotext/admin.py Tool: Pyflakes Processed Files: extension/reviewbotext/utils.py extension/reviewbotext/forms.py extension/reviewbotext/widgets.py extension/reviewbotext/admin.py
-
-
-
-
-
-
-
-
-
-
-
Our imports are split into three groups:
- Python standard library imports
- Third party library imports
- Project library imports.
Each import group should be separated by a blank line.
-
This should go in the third import group. It should also be an absolute import, e.g.
from reviewbotext.utils import get_class
-
-
-
-
-
-
-
-
-
Is there any reason that we have to check if this is a
Widget
? Are we expecting to ever not get aWidget
passed in?This seems very un-Pythonic.
-
-
-
-
You should do
from django.utils.six.moves import zip
The
six
zip creates a generator instead of a list. -
-
String concatenation is expensive. You should store all the parts in an array and use
''.join(string_parts)
-
-
-
-
Use a generator expression instead of a list comprehension (i.e., use
(
and)
instead of[
and]
. It doesn't require building a new list and you are just iterating over it. -
We don't need the enclosing
[
and]
; it is more efficient to use a generator expression than a list comprehension.
-
Tool: Pyflakes Processed Files: extension/reviewbotext/utils.py extension/reviewbotext/forms.py extension/reviewbotext/widgets.py extension/reviewbotext/admin.py Tool: PEP8 Style Checker Processed Files: extension/reviewbotext/utils.py extension/reviewbotext/forms.py extension/reviewbotext/widgets.py extension/reviewbotext/admin.py
-
-
-
Tool: PEP8 Style Checker Processed Files: extension/reviewbotext/utils.py extension/reviewbotext/forms.py extension/reviewbotext/widgets.py extension/reviewbotext/admin.py Tool: Pyflakes Processed Files: extension/reviewbotext/utils.py extension/reviewbotext/forms.py extension/reviewbotext/widgets.py extension/reviewbotext/admin.py
-
Tool: Pyflakes Processed Files: extension/reviewbotext/utils.py extension/reviewbotext/forms.py extension/reviewbotext/widgets.py extension/reviewbotext/admin.py Tool: PEP8 Style Checker Processed Files: extension/reviewbotext/utils.py extension/reviewbotext/forms.py extension/reviewbotext/widgets.py extension/reviewbotext/admin.py