Fix rendering new widgets in admin dashboard

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

Information

Review Board
master
61483f7...

Reviewers

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.

Description From Last Updated

Can you rewrite your summary in the imperitive mood, i.e., as if it were a command or order? The following …

brenniebrennie

This is a good opportunity to get comfortable with unit tests. You'll need to write a unit test that sets …

chipx86chipx86

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

brenniebrennie

In your description: `KeyError` over "KeyError"

brenniebrennie

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, …

brenniebrennie

Same as above.

brenniebrennie

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E211 whitespace before '['

reviewbotreviewbot

E211 whitespace before '['

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E211 whitespace before '['

reviewbotreviewbot

E211 whitespace before '['

reviewbotreviewbot

We generally avoid \\ where we can: total_initial_widgets = ( len(response.context['selected_secondary_widgets']) + len(response.context['selected_primary_widgets']))

brenniebrennie

You can wrap this without the \\: profile = Profile.objects.get_or_create( user=response.context['user'])[0]

brenniebrennie

blank line between these.

brenniebrennie

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 …

brenniebrennie

blank line between these.

brenniebrennie

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 …

brenniebrennie

Again, we can wrap this without \ as above.

brenniebrennie

Blank line between expression and block statement.

brenniebrennie

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) …

brenniebrennie

Blank line between block statements.

brenniebrennie

As per my comment above, this should actually be: self.assertNotIn(TestSecondaryWidget, response.context['selected_secondary_widgets'])

brenniebrennie

While you're here, blank line between these.

brenniebrennie

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

brenniebrennie

Lets use widget over p here.

brenniebrennie

While you're here, blank line between these.

brenniebrennie

Lets use widget over s here.

brenniebrennie

While you're here, blank line between these.

brenniebrennie

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

brenniebrennie

While you're here, blank line between these.

brenniebrennie

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

brenniebrennie

E303 too many blank lines (2)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

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

brenniebrennie

Comments should be full sentences and end in periods.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

Missing Returns:

brenniebrennie

How about: A dictionary mapping widgets (:py:class:`~reviewboard.admin.widgets. Widget`) to whether or not they are selected (as a :py:class:`unicode`).

brenniebrennie

The summary should fit entirely on the first line. How about Return widgets sorted based on their positions.

brenniebrennie

How about: A dictionary mapping widget IDs to their ordinals.

brenniebrennie

Missing Returns:

brenniebrennie

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

brenniebrennie

Single quotes.

brenniebrennie

Single quotes.

brenniebrennie

Single quotes.

brenniebrennie

Single quotes.

brenniebrennie

Can you add a comment here explaining why we're doing this? We both know why we wrap these in try..except …

brenniebrennie

Should be "admin/views.py"

daviddavid

This would be a little easier as: profile = response.context['user'].get_profile()

daviddavid

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

daviddavid

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

daviddavid

Add a blank line here, please.

daviddavid

We usually write the type as tuple, like this: ``` Returns: tuple of list: A 2-tuple containing a list of …

daviddavid
skaefer143
brennie
  1. 
      
  2. Show all issues

    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)
     
     
     
    Show all issues

    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)
     
     
     
    Show all issues

    Same as above.

  5. 
      
chipx86
  1. 
      
  2. Show all issues

    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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

skaefer143
Review request changed
Commit:
dabda4a3a6bae541c3fb00db9066d47b86a244db
165d9058b2dbea04b15a06a0946288b67a359eb7

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

skaefer143
brennie
  1. 
      
  2. Show all issues

    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. Show all issues

    In your description:

    `KeyError`
    

    over

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

    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)
     
     
     
    Show all issues

    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)
     
     
     
    Show all issues

    blank line between these.

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

    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)
     
     
     
    Show all issues

    blank line between these.

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

    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)
     
     
     
     
    Show all issues

    Again, we can wrap this without \ as above.

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

    Blank line between expression and block statement.

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

    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)
     
     
     
    Show all issues

    Blank line between block statements.

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

    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)
     
     
     
    Show all issues

    While you're here, blank line between these.

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

    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)
     
     
     
     
     
     
    Show all issues

    Lets use widget over p here.

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

    While you're here, blank line between these.

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

    Lets use widget over s here.

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

    While you're here, blank line between these.

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

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

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

    While you're here, blank line between these.

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

    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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    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)
     
     
     
    Show all issues

    Comments should be full sentences and end in periods.

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

    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)
     
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues

    Missing Returns:

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

    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)
     
     
     
    Show all issues

    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)
     
     
     
    Show all issues

    How about:

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

    Missing Returns:

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

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

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

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

    Single quotes.

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

    Single quotes.

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

    Single quotes.

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

    Single quotes.

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

    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)
     
     
    Show all issues

    Should be "admin/views.py"

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

    This would be a little easier as:

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

    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)
     
     
     
     
    Show all issues

    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)
     
     
     
    Show all issues

    Add a blank line here, please.

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

    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:
Completed
Change Summary:
Pushed to release-3.0.x (88448fc)