reviewboard.reviews.ui.ReviewUI sandboxing

Review Request #6551 — Created Nov. 2, 2014 and submitted

justy777
Review Board
master
f65c7f4...
reviewboard, students

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

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

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

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

Description From Last Updated

====================================================================== ERROR: Testing FileAttachmentReviewUI sandboxes for get_comment_thumbnail Traceback (most recent call last): File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/tests.py", line 2888, in test_get_comment_thumbnail file_attachment_comments.thumbnail File ...

justy777justy777

====================================================================== ERROR: Testing FileAttachmentReviewUI sandboxes for get_comment_link_url Traceback (most recent call last): File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/tests.py", line 2919, in test_get_comment_link_url file_attachment_comments.get_absolute_url() File ...

justy777justy777

====================================================================== ERROR: Testing FileAttachmentReviewUI sandboxes for get_comment_link_text Traceback (most recent call last): File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/tests.py", line 2950, in test_get_comment_link_text file_attachment_comments.get_link_text() File ...

justy777justy777

local variable 'comment_text_1' is assigned to but never used

reviewbotreviewbot

undefined name 'comment'

reviewbotreviewbot

Blank line between statement and block. Same below.

brenniebrennie

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

You can pass in your local review_ui variable here.

daviddavid

Same here.

daviddavid

And here.

daviddavid

Instead of "calling __init__ for", can you say "instantiating"?

daviddavid

This can use the context manager form, which makes it so you don't have to explicitly close, and makes it ...

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
    
    
  2. 
      
justy777
  1. 
      
  2. reviewboard/reviews/tests.py (Diff revision 1)
     
     

    ======================================================================
    ERROR: Testing FileAttachmentReviewUI sandboxes for get_comment_thumbnail


    Traceback (most recent call last):
    File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/tests.py", line 2888, in test_get_comment_thumbnail
    file_attachment_comments.thumbnail
    File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/models/file_attachment_comment.py", line 34, in thumbnail
    return self.file_attachment.review_ui.get_comment_thumbnail(self)
    File "/home/justin/developmentEnvironments/reviewboard/lib/python2.7/site-packages/kgb-0.5.1-py2.7.egg/kgb/spies.py", line 199, in call
    return self.func(args, *kwargs)
    TypeError: unbound method get_comment_thumbnail() must be called with SandboxReviewUI instance as first argument (got FileAttachmentComment instance instead)

  3. reviewboard/reviews/tests.py (Diff revision 1)
     
     

    ======================================================================
    ERROR: Testing FileAttachmentReviewUI sandboxes for get_comment_link_url


    Traceback (most recent call last):
    File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/tests.py", line 2919, in test_get_comment_link_url
    file_attachment_comments.get_absolute_url()
    File "/home/justin/developmentEnvironments/reviewboard/lib/python2.7/site-packages/Django-1.6.8-py2.7.egg/django/utils/functional.py", line 15, in _curried
    return _curried_func((args + moreargs), dict(kwargs, morekwargs))
    File "/home/justin/developmentEnvironments/reviewboard/lib/python2.7/site-packages/Django-1.6.8-py2.7.egg/django/db/models/base.py", line 1009, in get_absolute_url
    return settings.ABSOLUTE_URL_OVERRIDES.get('%s.%s' % (opts.app_label, opts.model_name), func)(self,
    args, kwargs)
    File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/models/file_attachment_comment.py", line 41, in get_absolute_url
    return self.file_attachment.review_ui.get_comment_link_url(self)
    File "/home/justin/developmentEnvironments/reviewboard/lib/python2.7/site-packages/kgb-0.5.1-py2.7.egg/kgb/spies.py", line 199, in call
    return self.func(*args,
    kwargs)
    TypeError: unbound method get_comment_link_url() must be called with SandboxReviewUI instance as first argument (got FileAttachmentComment instance instead)

  4. reviewboard/reviews/tests.py (Diff revision 1)
     
     

    ======================================================================
    ERROR: Testing FileAttachmentReviewUI sandboxes for get_comment_link_text


    Traceback (most recent call last):
    File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/tests.py", line 2950, in test_get_comment_link_text
    file_attachment_comments.get_link_text()
    File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/models/file_attachment_comment.py", line 48, in get_link_text
    return self.file_attachment.review_ui.get_comment_link_text(self)
    File "/home/justin/developmentEnvironments/reviewboard/lib/python2.7/site-packages/kgb-0.5.1-py2.7.egg/kgb/spies.py", line 199, in call
    return self.func(args, *kwargs)
    TypeError: unbound method get_comment_link_text() must be called with SandboxReviewUI instance as first argument (got FileAttachmentComment instance instead)

  5. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/file_attachment_comment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/file_attachment_comment.py
    
    
  2. reviewboard/reviews/tests.py (Diff revision 2)
     
     
     local variable 'comment_text_1' is assigned to but never used
    
  3. reviewboard/reviews/tests.py (Diff revision 2)
     
     
     undefined name 'comment'
    
  4. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/file_attachment_comment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/file_attachment_comment.py
    
    
  2. 
      
brennie
  1. 
      
  2. Blank line between statement and block. Same below.

  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/file_attachment_comment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/file_attachment_comment.py
    
    
  2. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/file_attachment_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/file_attachment_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
  2. reviewboard/reviews/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/file_attachment_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/file_attachment_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
  2. 
      
david
  1. 
      
  2. You can pass in your local review_ui variable here.

  3. reviewboard/reviews/ui/base.py (Diff revision 6)
     
     

    Instead of "calling __init__ for", can you say "instantiating"?

  4. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/file_attachment_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/file_attachment_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
  2. 
      
david
  1. One last thing I noticed:

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

    This can use the context manager form, which makes it so you don't have to explicitly close, and makes it exception safe.

    with open(filename, 'r') as f:
        self.file = SimpleUploadedFile(f.name, f.read(),
                                       content_type='image/png')
    
  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/file_attachment_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/file_attachment_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
  2. 
      
david
  1. Ship It!

  2. 
      
justy777
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (7ac7115)
Loading...