Summary

Improve Review Bot's admin interface

Review Request #6999 — Created Feb. 28, 2015 and discarded Nov. 23, 2016, 12:37 a.m.

Information

Owner

yyoud88

Repository

ReviewBot

Branch

master

Bugs

Depends On

Blocks

6998

Reviewers

Groups

reviewbot, students

People

anselina, smacleod

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.

Testing Done

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 again

Some possible edge cases are:
1. there's no existing profiles
2. there's exactly one porfile

Files

Issues

Description	From	Last Updated
'Field' imported but unused	reviewbot	March 4, 2015, 4:07 p.m.
'Widget' imported but unused	reviewbot	March 4, 2015, 4:07 p.m.
You should combine this with the other reviewbotext.forms import.	brennie	March 14, 2015, 12:44 p.m.
No comma on this line.	brennie	March 14, 2015, 12:44 p.m.
Move this to the previous line.	brennie	March 14, 2015, 12:44 p.m.
Move this to the previous line.	brennie	March 14, 2015, 12:45 p.m.
Blank line between these.	brennie	March 14, 2015, 12:45 p.m.
It seems very un-Pythonic to be doing very strict type checking.	brennie	March 14, 2015, 12:47 p.m.
You can do if class_path.	brennie	March 14, 2015, 12:47 p.m.
Why not just '.'.join(class_path) ? This will make another list.	brennie	March 14, 2015, 12:48 p.m.
This should be in the second import group.	brennie	March 14, 2015, 12:49 p.m.
Our imports are split into three groups: Python standard library imports Third party library imports Project library imports. Each import …	brennie	March 14, 2015, 12:49 p.m.
This should go in the third import group. It should also be an absolute import, e.g. from reviewbotext.utils import get_class	brennie	March 14, 2015, 12:50 p.m.
This should go in the first import group.	brennie	March 14, 2015, 12:50 p.m.
Add a docstring.	brennie	March 14, 2015, 12:52 p.m.
Move this to the previous line.	brennie	March 14, 2015, 12:52 p.m.
Period.	brennie	March 14, 2015, 12:53 p.m.
Period.	brennie	March 14, 2015, 12:53 p.m.
Use a period. Can this be clarified?	brennie	March 14, 2015, 12:53 p.m.
Comma on this line.	brennie	March 17, 2015, 5:08 p.m.
Period.	brennie	March 14, 2015, 12:54 p.m.
Is there any reason that we have to check if this is a Widget? Are we expecting to ever not …	brennie	March 14, 2015, 12:55 p.m.
Period.	brennie	March 14, 2015, 12:55 p.m.
Period.	brennie	March 14, 2015, 12:55 p.m.
Move this to the previous line.	brennie	March 14, 2015, 12:55 p.m.
You should do from django.utils.six.moves import zip The six zip creates a generator instead of a list.	brennie	March 14, 2015, 12:56 p.m.
Blank line between these.	brennie	March 14, 2015, 12:56 p.m.
String concatenation is expensive. You should store all the parts in an array and use ''.join(string_parts)	brennie	March 14, 2015, 1:01 p.m.
Move this to the previous line.	brennie	March 14, 2015, 1:02 p.m.
Period.	brennie	March 14, 2015, 1:02 p.m.
Period.	brennie	March 14, 2015, 1:02 p.m.
Use a generator expression instead of a list comprehension (i.e., use ( and ) instead of [ and ]. It …	brennie	March 14, 2015, 1:03 p.m.
We don't need the enclosing [ and ]; it is more efficient to use a generator expression than a list …	brennie	March 14, 2015, 1:03 p.m.
'Widget' imported but unused	reviewbot	March 15, 2015, 6:46 a.m.
Can be in alphabetical order.	XU xuanyi	March 15, 2015, 6:47 a.m.
'Field' imported but unused	reviewbot	March 15, 2015, 6:47 a.m.
Does one blank line after class docstring apply to this? I understand that most Widget class did have a blank …	XU xuanyi	March 15, 2015, 6:52 a.m.
Add trailing comma.	VT VTL-Developer	March 26, 2015, 10:13 p.m.
Leftover debug output?	david	May 21, 2015, 5:34 p.m.
Since this is now unused, we should just get rid of it.	david	May 21, 2015, 5:34 p.m.
typo: "defiend"	david	May 21, 2015, 5:34 p.m.
It might be nicer to do the join here, rather than below.	david	May 21, 2015, 5:34 p.m.

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

extension/reviewbotext/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'Field' imported but unused
```
extension/reviewbotext/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'Widget' imported but unused
```

Change Summary:

deleted unused imports and uploaded screenshots

Commit:

7076828d972d8f05691abc9781a5b61cd3fa51e7

73e5cdc234e3fb63acb9d3b7c13a8652a55fa47d

Diff:

Revision 2 (+140 -134)

Show changes

	extension/reviewbotext/admin.py
	extension/reviewbotext/forms.py
	extension/reviewbotext/utils.py
	extension/reviewbotext/widgets.py

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

Commit:

73e5cdc234e3fb63acb9d3b7c13a8652a55fa47d

dac257bf8fedfbcd02dd2dee775f7a55787e9d8f

Diff:

Revision 3 (+155 -134)

Show changes

	extension/reviewbotext/admin.py
	extension/reviewbotext/forms.py
	extension/reviewbotext/utils.py
	extension/reviewbotext/widgets.py

Summary:: [WIP] Improve Review Bot's admin interface
Improve Review Bot's admin interface

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

extension/reviewbotext/admin.py (Diff revision 3)
The issue has been resolved. Show all issues
```
You should combine this with the other reviewbotext.forms import.
```
extension/reviewbotext/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
No comma on this line.
```
extension/reviewbotext/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Move this to the previous line.
```
extension/reviewbotext/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Move this to the previous line.
```
extension/reviewbotext/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line between these.
```
extension/reviewbotext/utils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
It seems very un-Pythonic to be doing very strict type checking.
```
extension/reviewbotext/utils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
You can do if class_path.
```
extension/reviewbotext/utils.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Why not just '.'.join(class_path) ? This will make another list.
```
1. YY
  
  yyoud88 March 14, 2015, 1:22 p.m.
  it is to skip the last element
extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This should be in the second import group.
```

extension/reviewbotext/widgets.py (Diff revision 3)

The issue has been resolved. Show all issues

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.

extension/reviewbotext/widgets.py (Diff revision 3)

The issue has been resolved. Show all issues

This should go in the third import group. It should also be an absolute import, e.g.
from reviewbotext.utils import get_class

extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This should go in the first import group.
```
extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Add a docstring.
```
extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Move this to the previous line.
```
extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Period.
```
extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Period.
```
extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Use a period. Can this be clarified?
```
extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Comma on this line.
```
1. YY
  
  yyoud88 March 16, 2015, 2:58 p.m.
  Should I put comma for the last element as well?
2. brennie March 16, 2015, 5:28 p.m.
  This is the last element?
3. YY
  
  yyoud88 March 16, 2015, 6:12 p.m.
  yes
extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Period.
```

extension/reviewbotext/widgets.py (Diff revision 3)

The issue has been resolved. Show all issues

Is there any reason that we have to check if this is a Widget? Are we expecting to ever not get a Widget passed in?

This seems very un-Pythonic.

extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Period.
```
extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Period.
```
extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Move this to the previous line.
```

extension/reviewbotext/widgets.py (Diff revision 3)

The issue has been resolved. Show all issues

You should do
from django.utils.six.moves import zip

The six zip creates a generator instead of a list.

extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line between these.
```

extension/reviewbotext/widgets.py (Diff revision 3)

The issue has been resolved. Show all issues

String concatenation is expensive. You should store all the parts in an array and use

''.join(string_parts)

extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Move this to the previous line.
```
extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Period.
```
extension/reviewbotext/widgets.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Period.
```

extension/reviewbotext/widgets.py (Diff revision 3)

The issue has been resolved. Show all issues

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.

extension/reviewbotext/widgets.py (Diff revision 3)

The issue has been resolved. Show all issues

We don't need the enclosing [ and ]; it is more efficient to use a generator expression than a list comprehension.

Commit:

dac257bf8fedfbcd02dd2dee775f7a55787e9d8f

10eee4a1d53ba5b0cb5ddf8347e61f2c4a5ff119

Diff:

Revision 4 (+148 -134)

Show changes

	extension/reviewbotext/admin.py
	extension/reviewbotext/forms.py
	extension/reviewbotext/utils.py
	extension/reviewbotext/widgets.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

extension/reviewbotext/widgets.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 'Widget' imported but unused
```
extension/reviewbotext/widgets.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 'Field' imported but unused
```

extension/reviewbotext/widgets.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Can be in alphabetical order.
```

extension/reviewbotext/widgets.py (Diff revision 4)

The issue has been resolved. Show all issues

Does one blank line after class docstring apply to this? I understand that most Widget class did have a blank line.

Commit:

10eee4a1d53ba5b0cb5ddf8347e61f2c4a5ff119

d4117d7a2e6a16bc5569128ae269c40ed53b6f07

Diff:

Revision 5 (+148 -134)

Show changes

	extension/reviewbotext/admin.py
	extension/reviewbotext/forms.py
	extension/reviewbotext/utils.py
	extension/reviewbotext/widgets.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

Fix it!

extension/reviewbotext/forms.py (Diff revision 5)

I don't know how often empty_form would get called or when it will get called. Maybe cache it after creation? An example to look at is reviewboard/hostingsvcs/models.py in reviewboard.

YY

yyoud88 March 26, 2015, 11:07 p.m.
```
fixed.
```

extension/reviewbotext/widgets.py (Diff revision 5)
An issue was opened. Show all issues
```
Add trailing comma.
```
1. YY
  
  yyoud88 March 26, 2015, 11:07 p.m.
  should i add comma to the last element as well?

Commit:

d4117d7a2e6a16bc5569128ae269c40ed53b6f07

127bc6dedadb8eb712e698bab889da9a687807b4

Diff:

Revision 6 (+151 -134)

Show changes

	extension/reviewbotext/admin.py
	extension/reviewbotext/forms.py
	extension/reviewbotext/utils.py
	extension/reviewbotext/widgets.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

Fix it!

extension/reviewbotext/forms.py (Diff revision 6)
An issue was opened. Show all issues
```
Leftover debug output?
```
extension/reviewbotext/forms.py (Diff revision 6)
An issue was opened. Show all issues
```
Since this is now unused, we should just get rid of it.
```
extension/reviewbotext/widgets.py (Diff revision 6)
An issue was opened. Show all issues
```
typo: "defiend"
```
extension/reviewbotext/widgets.py (Diff revision 6)
An issue was opened. Show all issues
```
It might be nicer to do the join here, rather than below.
```

Status:: Discarded