Implement drag and drop on admin site (back end)

Review Request #5528 — Created Feb. 22, 2014 and submitted

Information

Review Board
master

Reviewers

The user can now customize their dashboard by dragging and dropping widgets to move them around.

I opened Review Board on two different browsers, Chrome and Firefox, and logged in as the same user on both browsers. On the Chrome browser, I visited the admin dashboard and changed the ordering of the widgets with drag-and-drop. On the Firefox browser, I also visited the admin dashboard, and I checked to make sure that the positions of the widgets were the same on both browsers, indicating that the positions had been saved on the back end side.

Description From Last Updated

These lines are too long. They should be no longer than 79 characters. You don't need the list comprehension, though. …

chipx86chipx86

Should keep the trailing comma.

chipx86chipx86

Two blank lines between these.

chipx86chipx86

Doc strings must be in the following format: """One-line summary.""" or: """One-line summary. Multi-line description. """ The summary cannot wrap …

chipx86chipx86

No blank line.

chipx86chipx86

This is basically the same code repeated twice. How about: if widget_type == 'primary': positions_key = 'primary_widget_positions' widgets = primary_widgets …

chipx86chipx86

It's best not to have to fetch this later on every time. I'd suggest doing: positions = profile_data.setdefault(positions_key, {}) Then …

chipx86chipx86

No space after range. Should be: for i in range(len(primary_widgets)):

chipx86chipx86

How does this uniquely represent the widget? Don't we want to operate on the widget_id instead?

chipx86chipx86

No blank line here. The elif is part of the same conditional group.

chipx86chipx86

Should default to None. I'd also make this the very first attribute, and update all the other widgets below to …

chipx86chipx86

Missing a var statement and a semicolon.

chipx86chipx86

Variable names must be lowercase.

chipx86chipx86

Multi-line comments must be in this format: /* * Line * Another line */

chipx86chipx86

Same comments as above.

chipx86chipx86

This is all basically repeated from above. You should factor that out.

chipx86chipx86

Same as above with the casing.

chipx86chipx86

Same as above for comment style.

chipx86chipx86

One statement per line.

chipx86chipx86

Remove this blank line.

daviddavid

These lines have some trailing whitespace, plus the mix of indentation will make some linters unhappy. Let's align it like …

daviddavid

While this isn't part of the API, I think we should change this to use POST or PUT for the …

daviddavid

You shouldn't need to include any arguments to HttpResponse if you don't need to return any content.

daviddavid

This looks like it's from your previous change. You probably need to rebase your branch onto the latest master.

daviddavid

Too many spaces between start: and function.

daviddavid

This would be slightly more readable as: primary_widget_positions = profile_data.get('primary_widget_positions') if primary_widget_positions: sorted_primary_widgets = sorted( ...

daviddavid

Same here.

daviddavid

Trailing whitespace.

daviddavid

How about adding axis: 'y', to the options for sortable?

daviddavid

Neither of the parameters here are used in the function, which can make some linters sad. Can you just remove …

daviddavid

How about adding axis: 'y', to the options for sortable?

daviddavid

This shouldn't have a comma after it (some browsers, including IE, don't react well to trailing commas in lists of …

daviddavid
HE
HE
HE
HE
chipx86
  1. 
      
  2. reviewboard/admin/views.py (Diff revision 1)
     
     
     
    Show all issues

    These lines are too long. They should be no longer than 79 characters.

    You don't need the list comprehension, though. You can just use sorted(...). If you need it to be a list, do list(sorted(...)).

  3. reviewboard/admin/views.py (Diff revision 1)
     
     
    Show all issues

    Should keep the trailing comma.

  4. reviewboard/admin/views.py (Diff revision 1)
     
     
     
     
    Show all issues

    Two blank lines between these.

  5. reviewboard/admin/views.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    Doc strings must be in the following format:

    """One-line summary."""
    

    or:

    """One-line summary.
    
    Multi-line description.
    """
    

    The summary cannot wrap or exceed the line limit.

  6. reviewboard/admin/views.py (Diff revision 1)
     
     
     
     
    Show all issues

    No blank line.

  7. reviewboard/admin/views.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This is basically the same code repeated twice. How about:

    if widget_type == 'primary':
        positions_key = 'primary_widget_positions'
        widgets = primary_widgets
    else:
        # Same for secnodary
    

    # All your other code here. Now it can operate on that data instead of re-calculating it.
    
  8. reviewboard/admin/views.py (Diff revision 1)
     
     
     
    Show all issues

    It's best not to have to fetch this later on every time. I'd suggest doing:

    positions = profile_data.setdefault(positions_key, {})
    

    Then you can just do:

    positions[widget] = i
    
  9. reviewboard/admin/views.py (Diff revision 1)
     
     
    Show all issues

    No space after range.

    Should be:

    for i in range(len(primary_widgets)):
    
  10. reviewboard/admin/views.py (Diff revision 1)
     
     
    Show all issues

    How does this uniquely represent the widget? Don't we want to operate on the widget_id instead?

    1. The post will look like "...widget1=user-activity-widget&widget2=...," so the widget's index uniquely represents it. Alternatively, I think I could reverse it to "...user-activity-widget=1..." if that would look cleaner.

  11. reviewboard/admin/views.py (Diff revision 1)
     
     
     
     
    Show all issues

    No blank line here. The elif is part of the same conditional group.

  12. reviewboard/admin/widgets.py (Diff revision 1)
     
     
    Show all issues

    Should default to None.

    I'd also make this the very first attribute, and update all the other widgets below to also have this appear first.

  13. reviewboard/static/rb/js/admin.js (Diff revision 1)
     
     
    Show all issues

    Missing a var statement and a semicolon.

  14. reviewboard/static/rb/js/admin.js (Diff revision 1)
     
     
    Show all issues

    Variable names must be lowercase.

  15. reviewboard/static/rb/js/admin.js (Diff revision 1)
     
     
     
    Show all issues

    Multi-line comments must be in this format:

    /*
     * Line
     * Another line
     */
    
  16. reviewboard/static/rb/js/admin.js (Diff revision 1)
     
     
    Show all issues

    Same comments as above.

  17. reviewboard/static/rb/js/admin.js (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    This is all basically repeated from above. You should factor that out.

  18. reviewboard/static/rb/js/admin.js (Diff revision 1)
     
     
    Show all issues

    Same as above with the casing.

  19. reviewboard/static/rb/js/admin.js (Diff revision 1)
     
     
     
    Show all issues

    Same as above for comment style.

  20. reviewboard/static/rb/js/admin.js (Diff revision 1)
     
     
    Show all issues

    One statement per line.

  21. 
      
HE
HE
HE
HE
david
  1. 
      
  2. reviewboard/admin/views.py (Diff revision 5)
     
     
    Show all issues

    Remove this blank line.

  3. reviewboard/admin/views.py (Diff revision 5)
     
     
     
     
     
    Show all issues

    These lines have some trailing whitespace, plus the mix of indentation will make some linters unhappy. Let's align it like this:

    sorted_primary_widgets = sorted(
        primary_widgets,
        key=lambda y: ...)
    
  4. reviewboard/admin/views.py (Diff revision 5)
     
     
    Show all issues

    While this isn't part of the API, I think we should change this to use POST or PUT for the HTTP request method, instead of GET.

    1. Can you explain why it should be POST instead of GET? I'm asking because I based this off the widget toggle functionality, which also uses GET (see line 130).

    2. So the basic difference is that POST requests are sending new data to the server, while GET is supposed to fetch. This means that GET requests can be affected by things like the browser's caching. There's a lot of additional reasons listed here: http://www.w3schools.com/tags/ref_httpmethods.asp

      We should probably change the toggle to use POST as well.

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

    You shouldn't need to include any arguments to HttpResponse if you don't need to return any content.

  6. reviewboard/static/rb/css/common.less (Diff revision 5)
     
     
    Show all issues

    This looks like it's from your previous change. You probably need to rebase your branch onto the latest master.

  7. reviewboard/static/rb/js/admin.js (Diff revision 5)
     
     
    Show all issues

    Too many spaces between start: and function.

  8. 
      
HE
HE
david
  1. This is looking pretty good! Just a couple small suggestions.

  2. reviewboard/admin/views.py (Diff revision 7)
     
     
     
    Show all issues

    This would be slightly more readable as:

    primary_widget_positions = profile_data.get('primary_widget_positions')
    if primary_widget_positions:
       sorted_primary_widgets = sorted(
       ...
    
  3. reviewboard/admin/views.py (Diff revision 7)
     
     
     
    Show all issues

    Same here.

  4. reviewboard/static/rb/js/admin.js (Diff revision 7)
     
     
    Show all issues

    Trailing whitespace.

  5. 
      
HE
david
  1. After applying your patch and trying this out, it's pretty awesome. I did notice a few more things, though. Almost there!

  2. reviewboard/static/rb/js/admin.js (Diff revision 8)
     
     
    Show all issues

    How about adding axis: 'y', to the options for sortable?

  3. reviewboard/static/rb/js/admin.js (Diff revision 8)
     
     
    Show all issues

    Neither of the parameters here are used in the function, which can make some linters sad. Can you just remove the parameter names and make this take no arguments?

  4. reviewboard/static/rb/js/admin.js (Diff revision 8)
     
     
    Show all issues

    How about adding axis: 'y', to the options for sortable?

  5. reviewboard/static/rb/js/admin.js (Diff revision 8)
     
     
    Show all issues

    This shouldn't have a comma after it (some browsers, including IE, don't react well to trailing commas in lists of things).

  6. 
      
HE
david
  1. Ship It!

  2. 
      
HE
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (3695f02). Nice work!