Allow Admin Widget Integration through ReviewBoard Extensions

Review Request #6573 — Created Nov. 9, 2014 and submitted

justy777
Review Board
master
6597
dc89704...
reviewboard, students

There's now a AdminWidgetHook that extensions can use to register administartion widgets. This handles registering, and unregistering Admin widgets.

Unit tests have been written to make sure the AdminWidgetHook properly registers and unregisters widgets. A third unit test checks that the Widget subclass being registered ends up in primary_widgets list, when primary is set to True in the hook initalization.

Description From Last Updated

This test should verify that the widget is correctly registered (not just checking that there are no exceptions)

daviddavid

If you want to test something that something doesn't throw an exception, wrap it in a try..except, like so: try: ...

brenniebrennie

This test should verify that the widget is correctly unregistered (not just checking that there are no exceptions)

daviddavid

This test should verify that the widget is correctly registered (not just checking that there are no exceptions)

daviddavid

There's really no reason to mark this string for translation.

daviddavid

Can you swap the order of these two methods?

daviddavid

No blank line here.

chipx86chipx86

"adding a new widget ..."

chipx86chipx86

This would be better using assertIn(TestWidget, secondary_widgets). Same below.

chipx86chipx86

Should be a ValueError.

chipx86chipx86

Col: 24 E128 continuation line under-indented for visual indent

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/extensions/hooks.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/extensions/hooks.py
    
    
  2. 
      
justy777
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/extensions/tests.py (Diff revision 2)
     
     

    If you want to test something that something doesn't throw an exception, wrap it in a try..except, like so:

    try:
        some_fn()
    except SomeException:
        self.fail('some_fn raised SomeException')
    

    Same for test_unregister and test_register_with_primary

    1. David advised me that it is not necessary. If an exception is thrown then it causes an error, and the test still fails.

  3. 
      
david
  1. 
      
  2. reviewboard/extensions/tests.py (Diff revision 2)
     
     

    This test should verify that the widget is correctly registered (not just checking that there are no exceptions)

  3. reviewboard/extensions/tests.py (Diff revision 2)
     
     

    This test should verify that the widget is correctly unregistered (not just checking that there are no exceptions)

  4. reviewboard/extensions/tests.py (Diff revision 2)
     
     

    This test should verify that the widget is correctly registered (not just checking that there are no exceptions)

  5. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/extensions/tests.py (Diff revision 3)
     
     

    There's really no reason to mark this string for translation.

  3. reviewboard/extensions/tests.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Can you swap the order of these two methods?

  4. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
    
    
  2. 
      
chipx86
  1. This looks great! Just a couple small things left.

  2. reviewboard/admin/widgets.py (Diff revision 4)
     
     
     
     

    No blank line here.

  3. reviewboard/extensions/hooks.py (Diff revision 4)
     
     

    "adding a new widget ..."

  4. reviewboard/extensions/tests.py (Diff revision 4)
     
     

    This would be better using assertIn(TestWidget, secondary_widgets).

    Same below.

  5. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/admin/widgets.py (Diff revision 5)
     
     

    Should be a ValueError.

  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
    
    
  2. reviewboard/admin/widgets.py (Diff revision 6)
     
     
    Col: 24
     E128 continuation line under-indented for visual indent
    
  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/extensions/hooks.py
        reviewboard/extensions/tests.py
    
    
  2. 
      
justy777
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (71383b5)
Loading...