Add a new selector for related users.

Review Request #8173 — Created May 18, 2016 and submitted

Information

Review Board
release-2.5.x
61d0cc5...

Reviewers

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.

  • 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.

Description From Last Updated

I thought we were moving away from self = this ?

brenniebrennie

You can do $item = $items.eq(i)

brenniebrennie

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

chipx86chipx86

Missing period.

chipx86chipx86

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

chipx86chipx86

"Gravatar"

chipx86chipx86

Djblets

brenniebrennie

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

chipx86chipx86

If all we need to do here is override the widget, we can do so like: class MyForm(forms.ModelForm): class Meta: …

chipx86chipx86

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

chipx86chipx86

Selectize is getting all kinds of maintenance done. Assuming we're matching values is scary to me. We should probably override …

chipx86chipx86

This would be a really nice thing to move into Djblets. Maybe for 3.0?

chipx86chipx86

Can this be one line, like with <select>?

chipx86chipx86

Let's put <input> in double backticks, so it's not rendered as HTML.

chipx86chipx86

Can we use events: {...} for this?

chipx86chipx86

The options should all be indented one more level.

chipx86chipx86

Double backticks.

chipx86chipx86

Double backticks.

chipx86chipx86

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

chipx86chipx86

Formatting's a bit strange, because of the lengthy name. Could we maybe do the calculation outside of the conditional, and …

chipx86chipx86

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

chipx86chipx86

compressed_js should be indented one.

chipx86chipx86

It was already like this before, but compressed_js should be indented one.

chipx86chipx86

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

chipx86chipx86

Missing period.

brenniebrennie

Can we say the version?

chipx86chipx86

Classes before tags.

brenniebrennie

Classes before tags.

brenniebrennie

Classes before tags.

brenniebrennie

Can we list the default?

chipx86chipx86

We should probably use |escapejs here.

brenniebrennie

and here.

brenniebrennie
reviewbot
  1. 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
    
    
  2. 
      
david
  1. NB: Since I made the video, I've made the field a little wider to accomodate longer real/usernames without wrapping

  2. 
      
brennie
  1. 
      
  2. Show all issues

    I thought we were moving away from self = this ?

    1. 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.

  3. Show all issues

    You can do $item = $items.eq(i)

  4. 
      
david
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/admin/form_widgets.py (Diff revision 2)
     
     
    Show all issues

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

  3. reviewboard/admin/form_widgets.py (Diff revision 2)
     
     
    Show all issues

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

  4. reviewboard/admin/form_widgets.py (Diff revision 2)
     
     
    Show all issues

    "Gravatar"

  5. reviewboard/admin/form_widgets.py (Diff revision 2)
     
     
     
    Show all issues

    Blank line between these.

    We also should return something when value is falsy.

  6. reviewboard/reviews/forms.py (Diff revision 2)
     
     
     
     
     
     
    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.

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

  7. Show all issues

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

  8. 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.

    1. 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?

    2. 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.

  9. Show all issues

    This would be a really nice thing to move into Djblets. Maybe for 3.0?

    1. Sure, that's probably doable.

  10. Show all issues

    Can this be one line, like with <select>?

  11. Show all issues

    Let's put <input> in double backticks, so it's not rendered as HTML.

  12. Show all issues

    Can we use events: {...} for this?

    1. This was actually dead code, since the click handler set up in _onItemSelected takes precedence.

  13. Show all issues

    The options should all be indented one more level.

  14. Show all issues

    Double backticks.

  15. Show all issues

    Double backticks.

  16. Show all issues

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

    1. localeCompare is only available in IE11+

    2. 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.

  17. 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.

    1. linters hate that.

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

    3. > 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
      
  18. reviewboard/templates/admin/base_site.html (Diff revision 2)
     
     
     
    Show all issues

    compressed_js should be indented one.

  19. reviewboard/templates/admin/dashboard.html (Diff revision 2)
     
     
     
    Show all issues

    It was already like this before, but compressed_js should be indented one.

  20. 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?

  21. 
      
brennie
  1. 
      
  2. reviewboard/admin/form_widgets.py (Diff revision 2)
     
     
    Show all issues

    Djblets

  3. 
      
david
reviewbot
  1. 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
    
    
  2. 
      
david
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/admin/form_widgets.py (Diff revisions 2 - 4)
     
     
    Show all issues

    Missing period.

  3. 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.

  4. Show all issues

    Can we say the version?

  5. Show all issues

    Can we list the default?

  6. 
      
brennie
  1. 
      
  2. reviewboard/scmtools/forms.py (Diff revision 4)
     
     
    Show all issues

    Missing period.

    1. We don't have periods for any of the other "section markers" here.

  3. Show all issues

    Classes before tags.

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

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

    3. 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.

  4. Show all issues

    Classes before tags.

  5. Show all issues

    Classes before tags.

  6. Show all issues

    We should probably use |escapejs here.

  7. Show all issues

    and here.

  8. 
      
david
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.5.x (fd0e7e3)