Fix Local Site and validation issues with post-commit hooks.

Review Request #6303 — Created Sept. 8, 2014 and submitted

Information

Review Board
release-2.0.x
856eba4...

Reviewers

The post-commit hook support in hosting services had a few issues that
could lead to unintentionally closing the wrong review requests on
other repositories. They also weren't compatible with Local Sites.

The URL for the hook identified the repository, and the URL also
contained the hosting service ID, but we wouldn't use either to properly
validate the repositories. That meant that if a hook was set up to point
to repository 1 on GitHub, and then had a commit referencing a review
request on repository 2 on Bitbucket, things would break.

We now capture the hosting service ID and pass it, the local site name,
and the repository ID to the function that handles closing review
requests. That function will then use that information to narrow down
the list of repositories being processed, in order to avoid operating on
some other review request. The function has also been rewritten to
minimize the number of SQL queries needed.

Local Sites were broken because nothing bothered to check for them. The
URLs were added to the root urlpatterns, and not to
localsite_urlpatterns. There was some code that did something weird with
trying to get a Local Site from a resolved URL, but that didn't really
do anything. Now everything is wired up to allow for hooks on Local
Sites and to restrict operations to review requests on the Local Site.

There's a few other changes here and there to help consistency and to
minimize code. For instance, we had different error responses for
invalid payloads, and they weren't really correct. We just use HTTP 400
now.

Unit tests were added for the three services that support post-commit
hooks. They test hooks with and without Local Sites, and test invalid
repository IDs, Local Site names, and service IDs.

The unit test for testing service registration was also split up into
several tests, since it was testing several things.

Unit tests pass. Previously, the validation checking tests failed,
as did the Local Site tests. Only the ones with good data going to
the right review requests would pass.

Description From Last Updated

undefined name 'logging'

reviewbotreviewbot

'Resolver404' imported but unused

reviewbotreviewbot

'resolve' imported but unused

reviewbotreviewbot

'Http404' imported but unused

reviewbotreviewbot

'_find_review_request_object' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/hostingsvcs/urls.py
        reviewboard/urls.py
        reviewboard/hostingsvcs/googlecode.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/hostingsvcs/bitbucket.py
        reviewboard/hostingsvcs/hook_utils.py
        reviewboard/hostingsvcs/tests.py
    
    
  2. reviewboard/hostingsvcs/bitbucket.py (Diff revision 1)
     
     
    Show all issues
     undefined name 'logging'
    
  3. reviewboard/hostingsvcs/hook_utils.py (Diff revision 1)
     
     
    Show all issues
     'Resolver404' imported but unused
    
  4. reviewboard/hostingsvcs/hook_utils.py (Diff revision 1)
     
     
    Show all issues
     'resolve' imported but unused
    
  5. reviewboard/hostingsvcs/hook_utils.py (Diff revision 1)
     
     
    Show all issues
     'Http404' imported but unused
    
  6. reviewboard/hostingsvcs/hook_utils.py (Diff revision 1)
     
     
    Show all issues
     '_find_review_request_object' imported but unused
    
  7. 
      
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/hostingsvcs/urls.py
        reviewboard/urls.py
        reviewboard/hostingsvcs/googlecode.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/hostingsvcs/bitbucket.py
        reviewboard/hostingsvcs/hook_utils.py
        reviewboard/hostingsvcs/tests.py
    
    
  2. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/hostingsvcs/urls.py
        reviewboard/urls.py
        reviewboard/hostingsvcs/googlecode.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/hostingsvcs/bitbucket.py
        reviewboard/hostingsvcs/hook_utils.py
        reviewboard/hostingsvcs/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/hostingsvcs/urls.py
        reviewboard/urls.py
        reviewboard/hostingsvcs/googlecode.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/github.py
        reviewboard/hostingsvcs/bitbucket.py
        reviewboard/hostingsvcs/hook_utils.py
        reviewboard/hostingsvcs/tests.py
    
    
  2. 
      
david
  1. Ship It!

  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (6a21f60)
Loading...