Sandbox reviewboard.admin.widgets.Widget

Review Request #6650 — Created Nov. 27, 2014 and submitted

Information

Review Board
master
cf2b791...

Reviewers

Extensions that create a Widget subclass (using the AdminWidgetHook) can throw exceptions inside Reviewboard. The parts of Reviewboard that call those methods have been sandboxed.

Now when a Widget subclass from an extension throws an exception; Reviewboard logs the errors with enough information to find which method in the Widget subclass threw the exception.

Unit tests have been written to make sure that functions from a Widget subclass have been called, and when an exception is thrown it gets logged.

The tests fail without the sanboxing, and succeed with it.

Admin UI displays the widgets in the correct column.
When the "X" in the corner is clicked it closes the widget, but does refresh the page.


Description From Last Updated

"Testing admin widget sandboxing for ..." Same below.

chipx86chipx86

This isn't always going to be clear which went wrong. Instead, the lambda function should call a new, private function …

chipx86chipx86

This needs to return the generated data. Make sure the admin UI is working as expected after this change. It …

chipx86chipx86

No blank line.

chipx86chipx86
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/admin/tests.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/admin/tests.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/admin/tests.py (Diff revision 1)
     
     
    Show all issues

    "Testing admin widget sandboxing for ..."

    Same below.

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

    This isn't always going to be clear which went wrong.

    Instead, the lambda function should call a new, private function that calls self.generate_data and wraps that in a try/except.

    generate_cache_key should be called prior to the cache_memoize, wrapped in a try/except. The resulting key can then be passed to cache_memoize outside the try/except.

  4. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/admin/tests.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/admin/tests.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/admin/widgets.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues

    This needs to return the generated data. Make sure the admin UI is working as expected after this change.

    It should also raise an exception that can be caught later when calling cache_memoize. (The exception can then be ignored in that callback.)

    The reason for that is that we don't want cache_memoize to either store a None value (as the exception may be temporary, so we want to try again), and for the same reasons we don't want to store any other default value. An exception will guarantee that nothing is cached.

    You can just re-raise and then catch in the try/except for the cache_memoize call.

  3. reviewboard/admin/widgets.py (Diff revision 2)
     
     
     
     
    Show all issues

    No blank line.

  4. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/admin/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/admin/widgets.py
        reviewboard/admin/tests.py
    
    
  2. 
      
david
  1. Can you provide details on what testing you did manually on the admin site?

    1. Admin site was not used for testing in this patch. It was used in my patch to sandbox HostingService.

    2. Christian asked you before to "make sure the admin UI is working". Running the tests is great but I don't want there to be a regression in the functionality.

  2. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/admin/views.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/admin/views.py
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
justy777
Review request changed
Status:
Completed
Change Summary:
Pushed to master (ce3de12)