Fix rendering new widgets in admin dashboard

Review Request #10165 — Created Sept. 22, 2018 and submitted

skaefer143
Review Board
master
4747
61483f7...
reviewboard, students

The dashboard on /admin/ crashed when a brand new widget was added
to the page. The expected output is that the new widget, whether
it's a primary or secondary widget, should appear on the dashboard
with the other widgets.

This was because there was a check to find the new widgets position,
before it actually had been given one. This raised a KeyError,
which crashed the page.

I added some .get() checks, so that if the widget position is not in
the dictionary, a None value is returned.

I also refactored code on how new widgets are rendered in the
dashboard, so that the dashboard() function in admin/views.py
is much easier to read.

I ran all tests using
./tests/runtests.py and no errors resulted.

I also used the JS-tests on /js-tests/, and there were no issues.

I created a Unit Test class named WidgetTests, that can be run
with ./tests/runtests.py reviewboard.admin.tests:WidgetTests.
This test checks that adding a new primary widget and a new secondary
widget on the dashboard does not raise a KeyError, and gives the two
widgets a position.

  • 0
  • 0
  • 70
  • 0
  • 70
Description From Last Updated
skaefer143
brennie
  1. 
      
  2. Can you rewrite your summary in the imperitive mood, i.e., as if it were a command or order?

    The following sentence should make sense if you substitute in your summary:

    "This patch will <summary>"

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

    dict.get accepts a default argument if the key is not present so this can be rewritten as:

    lambda y: primary_widget_positions.get(y.widget_id, len(primary_widget_positions)
    

    Also can we rename y to something like widget? y is not a good variable name.

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

    Same as above.

  5. 
      
chipx86
  1. 
      
  2. This is a good opportunity to get comfortable with unit tests.

    You'll need to write a unit test that sets up some state in the profile for the widget positions, performs a HTTP GET to the admin UI dashboard, and then checks the resulting template context to see if your fixes did what you'd expect them to do.

    This will be done with Django's TestClient, which works like:

    def test_blah(self):
        ...
    
        response = self.client.get(reverse('admin-dashboard'), ...)
        self.assertEqual(response.context['key'], expected_value)
    
  3. 
      
skaefer143
skaefer143
Review request changed

Change Summary:

Add unit test to check rendering new widget doesn't raise KeyError

Testing Done:

   

I ran the admin/tests.py file using

    ./tests/runtests.py reviewboard.admin.tests

   
~  

I also used the JS-tests on /js-tests/, and there were no

  ~

I also used the JS-tests on /js-tests/, and there were no issues.

-   (new) issues.

   
~  

I did a thorough test by adding my patch onto a branch off

~   release-4.0.x that created a new widget. Ensuring I initially
~   loaded the dashboard with release-4.0.x, and then reloaded the
~   dashboard after applying my patch on the new widget branch, the
  ~

I created a Unit Test class named WidgetTests, that can be run

  ~ with ./tests/runtests.py reviewboard.admin.tests:WidgetTests.
  ~ This test checks that adding a new widget on the dashboard
  ~ does not raise a KeyError, and gives the widget a position.

-   page was able to sucessfully load the new widget.

Commit:

-4610e71104cbf7970e4ae6438169faeb76f130a7
+dabda4a3a6bae541c3fb00db9066d47b86a244db

Diff:

Revision 2 (+100 -6)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

skaefer143
Review request changed

Commit:

-dabda4a3a6bae541c3fb00db9066d47b86a244db
+165d9058b2dbea04b15a06a0946288b67a359eb7

Diff:

Revision 3 (+100 -6)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

skaefer143
brennie
  1. 
      
  2. Please run all tests, e.g., ./tests/runtests.py. They take a few minutes, but sometimes things cascade into other suites and break things.

  3. In your description:

    `KeyError`
    

    over

    "KeyError"
    
  4. reviewboard/admin/tests.py (Diff revision 4)
     
     
     
     

    We generally avoid \\ where we can:

    total_initial_widgets = (
        len(response.context['selected_secondary_widgets']) +
        len(response.context['selected_primary_widgets']))
    
  5. reviewboard/admin/tests.py (Diff revision 4)
     
     
     

    You can wrap this without the \\:

    profile = Profile.objects.get_or_create(
        user=response.context['user'])[0]
    
  6. reviewboard/admin/tests.py (Diff revision 4)
     
     
     

    blank line between these.

  7. reviewboard/admin/tests.py (Diff revision 4)
     
     
     
     
     
     

    You are registering widgets outside of the try..finally.

    Lets do it this way.

    try:
        register_admin_widget(TestPrimaryWidget)
        register_admin_widget(TestSecondaryWIdget)
    
        # lines 298 thru 321 without the try/finally.
    
    finally:
        try:
            unregister_admin_widget(TestPrimaryWidget)
        except KeyError:
            pass
    
        try:
            unregister_admin_widget(TestSecondaryWidget)
        except KeyError:
            pass
    

    We need the nested try in the finally clause because soemthing could go wrong with registering either of the widgets and we would fall into the finally clause.

  8. reviewboard/admin/tests.py (Diff revision 4)
     
     
     

    blank line between these.

  9. reviewboard/admin/tests.py (Diff revision 4)
     
     
     
     
     

    We can pull out the selections into a intermediate variables to make things easier to read.

        primary_selections = profile.extra_data['primary_widget_selections']
        secondary_selections = profile.extra_data['secondaary_widget_selections']
    
        primary_selections[TestPrimaryWidget.widget_id] = '1'
        secondary_selections[TestSecondaryWidget.widget_id] = '1'
    
  10. reviewboard/admin/tests.py (Diff revision 4)
     
     
     
     

    Again, we can wrap this without \ as above.

  11. reviewboard/admin/tests.py (Diff revision 4)
     
     
     

    Blank line between expression and block statement.

  12. reviewboard/admin/tests.py (Diff revision 4)
     
     
     
     

    This should actually be:

    self.assertNotIn(TestPrimaryWidget, response.context['selected_primary_widgets'])
    

    Otherwise the error message we would get when running tests (True is not False) is not helpful. This way, it will repr() print the contents of response.context['selected_primary_widgets'] to aid in debugging.

  13. reviewboard/admin/tests.py (Diff revision 4)
     
     
     

    Blank line between block statements.

  14. reviewboard/admin/tests.py (Diff revision 4)
     
     
     
     

    As per my comment above, this should actually be:

    self.assertNotIn(TestSecondaryWidget, response.context['selected_secondary_widgets'])
    
  15. reviewboard/admin/views.py (Diff revision 4)
     
     
     

    While you're here, blank line between these.

  16. reviewboard/admin/views.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Since we're doing the same operations essentially twice with different variables, maybe we should extract this into two functions so that we could do:

    selected_primary_widgets, unselected_primary_widgets = _get_widget_selections(
        primary,
        profile_data.get('primary_widget_selections'))
    
    sorted_primary_widgets = _sort_widgets(
        selected_primary_widgets,
        profile_data.get('primary_widget_positions'))
    
    selected_secondary_widgets, unselected_secondary_widgets = _get_widget_selections(
        secondary_widgets,
        profile_data.get('secondary_widget_selections'))
    
    sorted_secondary_widgets = _sort_widgets(
        selected_secondary_widgets,
        profile_data.get('secondary_widget_positions'))
    

    where _get_widget_selections and _sort_widgets are new functions that encapsulate the current behaviour. This refactor would go a long way towards shortening the function bodies, encapsulating repeated logic, and making this code more readable.

  17. reviewboard/admin/views.py (Diff revision 4)
     
     
     
     
     
     

    Lets use widget over p here.

  18. reviewboard/admin/views.py (Diff revision 4)
     
     
     

    While you're here, blank line between these.

  19. reviewboard/admin/views.py (Diff revision 4)
     
     
     
     
     
     

    Lets use widget over s here.

  20. reviewboard/admin/views.py (Diff revision 4)
     
     
     

    While you're here, blank line between these.

  21. reviewboard/admin/views.py (Diff revision 4)
     
     
     
     

    This shouldn't need the parentheses around the lambda, should it?

  22. reviewboard/admin/views.py (Diff revision 4)
     
     
     

    While you're here, blank line between these.

  23. reviewboard/admin/views.py (Diff revision 4)
     
     
     
     

    This shouldn't need the parentheses around the lambda, should it?

  24. 
      
skaefer143
Review request changed

Change Summary:

Refactor admin dashboard widget code, and update code style

Description:

   

The dashboard on /admin/ crashed when a brand new widget was added

    to the page. The expected output is that the new widget, whether
    it's a primary or secondary widget, should appear on the dashboard
    with the other widgets.

   
   

This was because there was a check to find the new widgets position,

~   before it actually had been given one. This raised a "KeyError",
  ~ before it actually had been given one. This raised a KeyError,
    which crashed the page.

   
   

I added some .get() checks, so that if the widget position is not in

    the dictionary, a None value is returned.

  +
  +

I also refactored code on how new widgets are rendered in the

  + dashboard, so that the dashboard() function in admin/views.py
  + is much easier to read.

Testing Done:

~  

I ran the admin/tests.py file using

~   ./tests/runtests.py reviewboard.admin.tests

  ~

I ran all tests using

  ~ ./tests/runtests.py and no errors resulted.

   
   

I also used the JS-tests on /js-tests/, and there were no issues.

   
   

I created a Unit Test class named WidgetTests, that can be run

    with ./tests/runtests.py reviewboard.admin.tests:WidgetTests.
~   This test checks that adding a new widget on the dashboard
~   does not raise a KeyError, and gives the widget a position.

  ~ This test checks that adding a new primary widget and a new secondary
  ~ widget on the dashboard does not raise a KeyError, and gives the two
  + widgets a position.

Commit:

-3c53d2d3b63322d66ca4ebba09a19513f9457f03
+55b376e3b2314f590a047846c77e759a6463e9ad

Diff:

Revision 5 (+168 -44)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

skaefer143
brennie
  1. 
      
  2. reviewboard/admin/tests.py (Diff revision 6)
     
     
     
     
     
     

    Our imports should be alphabetically sorted so this (a) should go after the reviewboard.admin.validation imports and (b) needs its import( ... ) list to be sorted.

  3. reviewboard/admin/tests.py (Diff revision 6)
     
     
     

    Comments should be full sentences and end in periods.

  4. reviewboard/admin/views.py (Diff revision 6)
     
     
     
     
     

    In this case just use \ to wrap to the next line since it saves a vertical line.

  5. reviewboard/admin/views.py (Diff revision 6)
     
     
     
     
     

    In this case just use \ to wrap to the next line since it saves a vertical line.

  6. reviewboard/admin/views.py (Diff revision 6)
     
     

    Missing Returns:

  7. reviewboard/admin/views.py (Diff revision 6)
     
     
     

    How about:

        A dictionary mapping widgets (:py:class:`~reviewboard.admin.widgets.
        Widget`) to whether or not they are selected (as a :py:class:`unicode`).
    
  8. reviewboard/admin/views.py (Diff revision 6)
     
     
     

    The summary should fit entirely on the first line.

    How about Return widgets sorted based on their positions.

  9. reviewboard/admin/views.py (Diff revision 6)
     
     
     

    How about:

    A dictionary mapping widget IDs to their ordinals.
    
  10. reviewboard/admin/views.py (Diff revision 6)
     
     

    Missing Returns:

  11. 
      
skaefer143
brennie
  1. After these tiny nits, I'm happy with this landing!

  2. reviewboard/admin/tests.py (Diff revision 7)
     
     
     

    Uppercase before lowercase, so this would be the first import.

  3. reviewboard/admin/tests.py (Diff revision 7)
     
     

    Single quotes.

  4. reviewboard/admin/tests.py (Diff revision 7)
     
     

    Single quotes.

  5. reviewboard/admin/tests.py (Diff revision 7)
     
     

    Single quotes.

  6. reviewboard/admin/tests.py (Diff revision 7)
     
     

    Single quotes.

  7. reviewboard/admin/tests.py (Diff revision 7)
     
     

    Can you add a comment here explaining why we're doing this? We both know why we wrap these in try..except and ignore the error, but future readers of this file might not grok it immediately.

  8. 
      
skaefer143
david
  1. 
      
  2. reviewboard/admin/tests.py (Diff revision 8)
     
     

    Should be "admin/views.py"

  3. reviewboard/admin/tests.py (Diff revision 8)
     
     
     

    This would be a little easier as:

    profile = response.context['user'].get_profile()
    
  4. reviewboard/admin/tests.py (Diff revision 8)
     
     

    This line can go away (the definition of a try block is to wrap something in case it fails).

  5. reviewboard/admin/tests.py (Diff revision 8)
     
     
     
     

    This is a little bit too verbose (I can read the words "try", "pass", and "KeyError" in the code).

    How about "If an error was encountered above, the widgets will not be registered. Ignore any errors here in that case"?

  6. reviewboard/admin/tests.py (Diff revision 8)
     
     
     

    Add a blank line here, please.

  7. reviewboard/admin/views.py (Diff revision 8)
     
     

    We usually write the type as tuple, like this:

    ```
    Returns:
    tuple of list:
    A 2-tuple containing a list of selected widgets (to display
    on the dashboard) and a list of the unselected widgets.
    """

  8. 
      
skaefer143
david
  1. Ship It!
  2. 
      
skaefer143
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (88448fc)
Loading...