Allow Admin Widget Integration through ReviewBoard Extensions

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

Information

Review Board
master
dc89704...

Reviewers

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

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

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

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

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

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

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

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

    No blank line here.

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

    "adding a new widget ..."

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

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

    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)
     
     
    Show all issues
    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:
Completed
Change Summary:
Pushed to master (71383b5)