Summary

Add a new selector for related users.

Review Request #8173 — Created May 18, 2016 and submitted June 15, 2016, 12:37 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

Commit

61d0cc5...

Reviewers

Groups

reviewboard

People

Description

We're faced with a conundrum in the admin UI when it comes to models that have
relations to other tables which can contain tons of entries. We can either use
one of django's filter widgets, which query all rows and pre-populate the form
field completely, or we can use django's "raw ID field", which has a
comma-separated list of PKs and a pop-up window search to add items. In the
first case, when there are a very large number of users (such as on public
servers), we can actually crash the web server processes. In the second case,
it's efficient but totally unusable.

This change introduces a new, custom widget, which is based on selectize. This
widget provides a list for the currently selected objects, plus a searchable
drop-down to add new items. Whenever something is selected, it gets put into a
list which shows exactly what is already selected.

This has the benefits of the filter widget with some extra sauce on top
(avatars, better searching), plus it's much more efficient in its queries to
the database. This is currently only implemented for relations to User (since
there's custom display and API interface code per type), but that's the big
offender anyway.

Testing Done


Loaded the form with no users selected and saw an empty list.
Loaded the form with existing users selected and saw them show up correctly.
Clicked into the drop-down and saw it populate with the first page of results

  from the API.
Started typing in a name and saw that the drop-down searched for that name

  using the API.
Searched for a name which was already in the selected list and saw that it was

  properly hidden from the results.
Used the keyboard to navigate to and accept a result.
Removed items from the selected list.
Saved the form in various states and verified that the resulting database

  entries were correct.

Files

Issues

Description	From	Last Updated
I thought we were moving away from self = this ?	brennie	May 18, 2016, 7:24 p.m.
You can do $item = $items.eq(i)	brennie	May 19, 2016, 10:21 a.m.
Can you add a docstring to the top of the file? Helps with the doc generation when browsing modules.	chipx86	May 26, 2016, 7:03 p.m.
Missing period.	chipx86	June 14, 2016, 10:45 a.m.
<select> should be wrapped in double backticks, so it doesn't turn into HTML when generated.	chipx86	May 26, 2016, 2:51 p.m.
"Gravatar"	chipx86	May 26, 2016, 2:51 p.m.
Djblets	brennie	May 26, 2016, 2:51 p.m.
Blank line between these. We also should return something when value is falsy.	chipx86	May 26, 2016, 2:52 p.m.
If all we need to do here is override the widget, we can do so like: class MyForm(forms.ModelForm): class Meta: …	chipx86	May 26, 2016, 7:04 p.m.
Can we wrap each computation in parens, just so it's very clear? (Since CSS is so inconsistent about arguments.)	chipx86	May 26, 2016, 3:07 p.m.
Selectize is getting all kinds of maintenance done. Assuming we're matching values is scary to me. We should probably override …	chipx86	May 26, 2016, 3:09 p.m.
This would be a really nice thing to move into Djblets. Maybe for 3.0?	chipx86	May 26, 2016, 3:09 p.m.
Can this be one line, like with <select>?	chipx86	May 26, 2016, 3:10 p.m.
Let's put <input> in double backticks, so it's not rendered as HTML.	chipx86	May 26, 2016, 3:10 p.m.
Can we use events: {...} for this?	chipx86	May 26, 2016, 6:35 p.m.
The options should all be indented one more level.	chipx86	May 26, 2016, 3:11 p.m.
Double backticks.	chipx86	May 26, 2016, 3:11 p.m.
Double backticks.	chipx86	May 26, 2016, 3:11 p.m.
We should use localeCompare for string sorting, to handle rules in other languages.	chipx86	June 6, 2016, 12:49 p.m.
Formatting's a bit strange, because of the lengthy name. Could we maybe do the calculation outside of the conditional, and …	chipx86	June 14, 2016, 10:47 a.m.
Might as well include the parameter, even if it's unused. That'll at least ensure it'll be shown in the function …	chipx86	June 14, 2016, 2:46 p.m.
compressed_js should be indented one.	chipx86	May 26, 2016, 3:22 p.m.
It was already like this before, but compressed_js should be indented one.	chipx86	May 26, 2016, 3:22 p.m.
Instead of defining one of these inline every time we need it, can we make a proper subclass as a …	chipx86	May 26, 2016, 6:36 p.m.
Missing period.	brennie	June 14, 2016, 10:50 a.m.
Can we say the version?	chipx86	June 14, 2016, 10:46 a.m.
Classes before tags.	brennie	June 14, 2016, 3:23 p.m.
Classes before tags.	brennie	June 14, 2016, 3:23 p.m.
Classes before tags.	brennie	June 14, 2016, 3:23 p.m.
Can we list the default?	chipx86	June 14, 2016, 10:46 a.m.
We should probably use \|escapejs here.	brennie	June 14, 2016, 10:50 a.m.
and here.	brennie	June 14, 2016, 10:50 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/reviews/forms.py
    reviewboard/admin/form_widgets.py
    reviewboard/reviews/admin.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/templates/admin/dashboard.html
    reviewboard/static/rb/css/pages/admin.less
    reviewboard/templates/admin/base_site.html
    reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
    reviewboard/templates/admin/related_user_widget.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/reviews/forms.py
    reviewboard/admin/form_widgets.py
    reviewboard/reviews/admin.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/templates/admin/dashboard.html
    reviewboard/static/rb/css/pages/admin.less
    reviewboard/templates/admin/base_site.html
    reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
    reviewboard/templates/admin/related_user_widget.html

NB: Since I made the video, I've made the field a little wider to accomodate longer real/usernames without wrapping

reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js (Diff revision 1)

The issue has been dropped. Show all issues

I thought we were moving away from self = this ?

david May 18, 2016, 7:16 p.m.

Well, I think in a lot of cases fat arrow functions (which I'll make some use of in the 3.0.x port of this) obsolete that, but in this case we need both the lexical this and the this bound by selectize.

reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
You can do $item = $items.eq(i)
```

Commit:

0e6fde1d83fe33b82b2ddb8a9ce2e0ddd91c27f3

9e86bc117cb1181a01b42397520ea6da1f855cff

Diff:

Revision 2 (+551 -27)

Show changes

	reviewboard/staticbundles.py
	reviewboard/admin/form_widgets.py
	reviewboard/reviews/admin.py
	reviewboard/reviews/forms.py
	reviewboard/scmtools/forms.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
	reviewboard/templates/admin/base_site.html
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/reviews/forms.py
    reviewboard/admin/form_widgets.py
    reviewboard/reviews/admin.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/templates/admin/dashboard.html
    reviewboard/static/rb/css/pages/admin.less
    reviewboard/templates/admin/base_site.html
    reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
    reviewboard/templates/admin/related_user_widget.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/reviews/forms.py
    reviewboard/admin/form_widgets.py
    reviewboard/reviews/admin.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/templates/admin/dashboard.html
    reviewboard/static/rb/css/pages/admin.less
    reviewboard/templates/admin/base_site.html
    reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
    reviewboard/templates/admin/related_user_widget.html

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

The issue has been resolved. Show all issues

Can you add a docstring to the top of the file? Helps with the doc generation when browsing modules.

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

The issue has been resolved. Show all issues

<select> should be wrapped in double backticks, so it doesn't turn into HTML when generated.

reviewboard/admin/form_widgets.py (Diff revision 2)
The issue has been resolved. Show all issues
```
"Gravatar"
```

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

The issue has been resolved. Show all issues

Blank line between these.
We also should return something when value is falsy.

reviewboard/reviews/forms.py (Diff revision 2)

The issue has been dropped. Show all issues

If all we need to do here is override the widget, we can do so like:

class MyForm(forms.ModelForm):
    class Meta:
        widgets = {
            'people': RelatedUserWidget(),
            ...
        }


etc.
Same elsewhere.

david May 26, 2016, 7:06 p.m.

Unfortunately with that we still get extra gunk injected in from the field, like some meaningless help text and "+" buttons to add new users.

reviewboard/static/rb/css/pages/admin.less (Diff revision 2)

The issue has been resolved. Show all issues

Can we wrap each computation in parens, just so it's very clear? (Since CSS is so inconsistent about arguments.)

reviewboard/static/rb/css/pages/admin.less (Diff revision 2)

The issue has been resolved. Show all issues

Selectize is getting all kinds of maintenance done. Assuming we're matching values is scary to me. We should probably override both Selectize and this, to be absolutely sure.

david May 26, 2016, 7:06 p.m.

If they're making internal or styling changes, it seems like overriding their CSS is probably even worse from a future-proofing standpoint. How about I add a README.selectize that gives instructions on what to check when updating it?

chipx86 May 28, 2016, 11:21 p.m.

Yeah, that'd be a good thing to have (the README).

FWIW, we're doing this exact thing in Splat (setting the values in their theme and in ours to make them consistent). It should be safe, because Selectize has a concept of themes, and we're free to set new values for things like the padding. So long as they retain the theme support, we're good. This would just be about making sure the theme's style and ours are consistent (set to the same constant), in case one side of it ever changes.

reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js (Diff revision 2)
The issue has been dropped. Show all issues
```
This would be a really nice thing to move into Djblets. Maybe for 3.0?
```
1. david May 26, 2016, 7:06 p.m.
  Sure, that's probably doable.
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Can this be one line, like with <select>?
```
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Let's put <input> in double backticks, so it's not rendered as HTML.
```
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we use events: {...} for this?
```
1. david May 26, 2016, 7:06 p.m.
  This was actually dead code, since the click handler set up in _onItemSelected takes precedence.
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
The options should all be indented one more level.
```
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Double backticks.
```
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Double backticks.
```

reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js (Diff revision 2)

The issue has been resolved. Show all issues

We should use localeCompare for string sorting, to handle rules in other languages.

david May 26, 2016, 7:06 p.m.

localeCompare is only available in IE11+

chipx86 May 28, 2016, 11:21 p.m.

We can conditionally use it. If it exists, compare using that, to get locale-safe string comparisons, and fall back on what we're doing here.

reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js (Diff revision 2)

The issue has been dropped. Show all issues

Might as well include the parameter, even if it's unused. That'll at least ensure it'll be shown in the function signature of any docs.

david May 26, 2016, 7:06 p.m.
```
linters hate that.
```

chipx86 May 28, 2016, 11:21 p.m.

Which? jshint's fine with it. We do this in plenty of places already.

david June 1, 2016, 9:40 a.m.

> jshint reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js: line 186, col 28, 'item' is defined but never used.

1 error

reviewboard/templates/admin/base_site.html (Diff revision 2)
The issue has been resolved. Show all issues
```
compressed_js should be indented one.
```
reviewboard/templates/admin/dashboard.html (Diff revision 2)
The issue has been resolved. Show all issues
```
It was already like this before, but compressed_js should be indented one.
```

reviewboard/templates/admin/related_user_widget.html (Diff revision 2)

The issue has been resolved. Show all issues

Instead of defining one of these inline every time we need it, can we make a proper subclass as a JavaScript file?

reviewboard/admin/form_widgets.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Djblets
```

Commit:

9e86bc117cb1181a01b42397520ea6da1f855cff

c729f0d02fc68ed1b9bf444de2e2e4502d988709

Diff:

Revision 3 (+601 -27)

Show changes

	reviewboard/staticbundles.py
	reviewboard/admin/form_widgets.py
	reviewboard/reviews/admin.py
	reviewboard/reviews/forms.py
	reviewboard/scmtools/forms.py
	reviewboard/static/lib/js/README.selectize
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
	4 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/reviews/forms.py
    reviewboard/admin/form_widgets.py
    reviewboard/reviews/admin.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/admin/views/relatedUserSelectorView.js
    reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
    reviewboard/static/lib/js/README.selectize
    reviewboard/templates/admin/dashboard.html
    reviewboard/templates/admin/base_site.html
    reviewboard/templates/admin/related_user_widget.html
    reviewboard/static/rb/css/pages/admin.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/reviews/forms.py
    reviewboard/admin/form_widgets.py
    reviewboard/reviews/admin.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/admin/views/relatedUserSelectorView.js
    reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
    reviewboard/static/lib/js/README.selectize
    reviewboard/templates/admin/dashboard.html
    reviewboard/templates/admin/base_site.html
    reviewboard/templates/admin/related_user_widget.html
    reviewboard/static/rb/css/pages/admin.less

Change Summary:

Optimistically use localeCompare

Commit:

c729f0d02fc68ed1b9bf444de2e2e4502d988709

8062aa716c23ca745caf0dcc8d5d9642a1d24e2c

Diff:

Revision 4 (+629 -27)

Show changes

	reviewboard/staticbundles.py
	reviewboard/admin/form_widgets.py
	reviewboard/reviews/admin.py
	reviewboard/reviews/forms.py
	reviewboard/scmtools/forms.py
	reviewboard/static/lib/js/README.selectize
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
	4 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/reviews/forms.py
    reviewboard/admin/form_widgets.py
    reviewboard/reviews/admin.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/admin/views/relatedUserSelectorView.js
    reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
    reviewboard/static/lib/js/README.selectize
    reviewboard/templates/admin/dashboard.html
    reviewboard/templates/admin/base_site.html
    reviewboard/templates/admin/related_user_widget.html
    reviewboard/static/rb/css/pages/admin.less



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/reviews/forms.py
    reviewboard/admin/form_widgets.py
    reviewboard/reviews/admin.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/admin/views/relatedUserSelectorView.js
    reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
    reviewboard/static/lib/js/README.selectize
    reviewboard/templates/admin/dashboard.html
    reviewboard/templates/admin/base_site.html
    reviewboard/templates/admin/related_user_widget.html
    reviewboard/static/rb/css/pages/admin.less

reviewboard/admin/form_widgets.py (Diff revisions 2 - 4)
The issue has been resolved. Show all issues
```
Missing period.
```

reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js (Diff revisions 2 - 4)

The issue has been resolved. Show all issues

Formatting's a bit strange, because of the lengthy name. Could we maybe do the calculation outside of the conditional, and then check the value? Or, pull out a reference to compareStrings before the loop, so we don't have to resolve RB.RelatedObjectSelectorView.compareStrings every iteration.

reviewboard/static/lib/js/README.selectize (Diff revision 4)
The issue has been resolved. Show all issues
```
Can we say the version?
```
reviewboard/static/rb/js/admin/views/relatedUserSelectorView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Can we list the default?
```

reviewboard/scmtools/forms.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Missing period.
```
1. david June 14, 2016, 10:50 a.m.
  We don't have periods for any of the other "section markers" here.

reviewboard/static/rb/css/pages/admin.less (Diff revision 4)

The issue has been dropped. Show all issues

Classes before tags.

chipx86 June 13, 2016, 7:15 p.m.

Other way around. Tags, then IDs, then classes.

david June 14, 2016, 10:50 a.m.

Wait, what?? We should follow the same order as the cascade (ID, then class, then tag)

chipx86 June 14, 2016, 11:11 a.m.

We've been doing it this way for like 10 years. It shouldn't come as a surprise, as it's also the order shown in your code.

We're not listing them in cascade order. If we were, all the CSS we have that looks like:

.something { ... }
.something.blah { ... }

would be this instead:

.something.blah { ... }
.something { ... }

It's a pretty common ordering. Bootstrap uses it as well, for example, and they were hardly the first.

reviewboard/static/rb/css/pages/admin.less (Diff revision 4)
The issue has been dropped. Show all issues
```
Classes before tags.
```
reviewboard/static/rb/css/pages/admin.less (Diff revision 4)
The issue has been dropped. Show all issues
```
Classes before tags.
```
reviewboard/templates/admin/related_user_widget.html (Diff revision 4)
The issue has been resolved. Show all issues
```
We should probably use |escapejs here.
```
reviewboard/templates/admin/related_user_widget.html (Diff revision 4)
The issue has been resolved. Show all issues
```
and here.
```

Change Summary:

Make requested changes.

Commit:

8062aa716c23ca745caf0dcc8d5d9642a1d24e2c

61d0cc5bc46c38636ee26f8b9f1cea8cb4c52a27

Diff:

Revision 5 (+4685 -27)

Show changes

	reviewboard/staticbundles.py
	reviewboard/admin/form_widgets.py
	reviewboard/reviews/admin.py
	reviewboard/reviews/forms.py
	reviewboard/scmtools/forms.py
	reviewboard/static/lib/css/selectize.default-0.12.1.css
	reviewboard/static/lib/js/README.selectize
	reviewboard/static/lib/js/selectize-0.12.1.js
	6 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/reviews/forms.py
    reviewboard/admin/form_widgets.py
    reviewboard/reviews/admin.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/admin/views/relatedUserSelectorView.js
    reviewboard/static/lib/css/selectize.default-0.12.1.css
    reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
    reviewboard/static/lib/js/README.selectize
    reviewboard/static/rb/css/pages/admin.less
    reviewboard/templates/admin/base_site.html
    reviewboard/templates/admin/related_user_widget.html
    reviewboard/static/lib/js/selectize-0.12.1.js
    reviewboard/templates/admin/dashboard.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/reviews/forms.py
    reviewboard/admin/form_widgets.py
    reviewboard/reviews/admin.py
    reviewboard/scmtools/forms.py

Ignored Files:
    reviewboard/static/rb/js/admin/views/relatedUserSelectorView.js
    reviewboard/static/lib/css/selectize.default-0.12.1.css
    reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
    reviewboard/static/lib/js/README.selectize
    reviewboard/static/rb/css/pages/admin.less
    reviewboard/templates/admin/base_site.html
    reviewboard/templates/admin/related_user_widget.html
    reviewboard/static/lib/js/selectize-0.12.1.js
    reviewboard/templates/admin/dashboard.html

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.5.x (fd0e7e3)