• 
      

    SCM Web Hooks for reviewboard : Phase 1

    Review Request #5510 — Created Feb. 19, 2014 and submitted

    Information

    Review Board
    master

    Reviewers

    Generate url infrastructure for hosting services so that they can be called to access web hooks. Added repo_urlpatterns to the service base class. Modified registration and unregistration of hosting services to add and remove urlpatterns respectively.

    Wrote test case HostingServiceUrlPatternTests in hostingsvcs.tests.py. I've tested the addition and deletion of url patterns during registration and unregistration, respectively. Added tests for cases when the url_patterns are not defined. Also tested adding bitbucket url patterns (see screenshot). All tests have passed. :)


    Description From Last Updated

    Always do absolute imports. 'urls' could be anything. You also need to have this below with the project imports.

    chipx86chipx86

    Imports must always be alphabetical.

    chipx86chipx86

    Isn't this already in alphabetical order ? django... djblets.....

    B. b.ramnani

    Name this repository_url_patterns.

    chipx86chipx86

    The summary has to be one line without wrapping.

    chipx86chipx86

    "thr" -> "the"

    chipx86chipx86

    Trailing whitespace.

    chipx86chipx86

    "URL" instead of "url"

    chipx86chipx86

    This can just be if cls.repo_urlpatterns.

    anselinaanselina

    The parameters to patterns need to align. Spaces around the +

    chipx86chipx86

    Trailing whitespace.

    chipx86chipx86

    Alphabetical order.

    chipx86chipx86

    This must be one import statement, using parens, and one per line to prevent going over the line limit.

    chipx86chipx86

    Alphabetical.

    chipx86chipx86

    Two blank lines.

    chipx86chipx86

    This doesn't meet our formatting rules, and you're missing url(). It should be: repository_url_patterns = patterns( '', url('^hooks/pre-commit/$', ...), ) …

    chipx86chipx86

    Two blank lines.

    chipx86chipx86

    This is kind of hard to read. Can you group it into logical sections, with comments about what you're testing …

    chipx86chipx86

    You don't need to create your own client. You can just use self.client.

    chipx86chipx86

    URLs should always have a trailing /. Same below.

    chipx86chipx86

    Two blank lines.

    chipx86chipx86

    Two blank lines.

    chipx86chipx86

    You should just include dynamic_urls, and not do a new patterns(...)

    chipx86chipx86

    If I do that I get the following error: The included urlconf <DynamicURLResolver [] (None:None) > doesn't have any patterns …

    B. b.ramnani

    This looks like a merge failure.

    chipx86chipx86

    Remove the trailing whitespace. In fact, revert this file.

    chipx86chipx86

    Merge failure. Revert this file.

    chipx86chipx86

    Revert this file too.

    chipx86chipx86

    Imports must always be alphabetical.

    chipx86chipx86

    No blank line between these.

    chipx86chipx86

    This needs to be a patterns(...)

    chipx86chipx86

    This should just be dummyView (which needs to be renamed to something more specific, and not use camelCase).

    chipx86chipx86

    You're missing the trailing slashes on these URLs, which will prevent you from getting to the right view.

    chipx86chipx86

    Review Board imports should go after the third-party module (Django, Djblets, etc.) imports, separated by a blank line.

    anselinaanselina

    This should be in alphabetical order - include should be before patterns, and the line needs to be moved to …

    anselinaanselina

    URL instead of url.

    anselinaanselina

    These should be in alphabetical order, before from django.utils.import six.

    anselinaanselina

    URL instead of url.

    anselinaanselina

    URL instead of url.

    anselinaanselina

    This should be formatted like this instead: repository_url_patterns = patterns( '', url(r'^hooks/pre-commit/$', hosting_service_url_test_view) )

    anselinaanselina

    Comments should start with a # and a single space, like: # This is a comment. Also, URL instead of …

    anselinaanselina

    include should be before patterns.

    anselinaanselina

    "URL pattern" "class"

    chipx86chipx86

    "KeyError"

    chipx86chipx86

    The '' should go on the next line. Spaces around the +

    chipx86chipx86

    I'd suggest moving this code after the try block. It's best to keep the bodies of a try/except as simple …

    chipx86chipx86

    Docstring summaries cannot wrap unless they are a unit test function. I'd actually recommend moving this into DummyService. You should …

    chipx86chipx86

    Should be 2 blank lines.

    chipx86chipx86

    This should be formatted like: repository_url_patterns = patterns( '', url(r'.....', view_name), )

    chipx86chipx86

    "URL" Same any time it appears in a comment or string.

    chipx86chipx86

    Make sure you have this as the first line (with a blank line after): from __future__ import unicode_literals

    chipx86chipx86

    Blank line between these.

    chipx86chipx86

    Space after '' in patterns()

    chipx86chipx86

    This import isn't used.

    anselinaanselina

    "URL"

    anselinaanselina

    The last parenthesis should be on the next line, aligned with cls_urlpatterns on line 363.

    anselinaanselina

    These lines are indented an extra level.

    anselinaanselina

    "registering"

    anselinaanselina

    This import isn't used.

    anselinaanselina

    There should be a space after the comma, not before (so it should be ...patterns('', dynamic_urls))

    anselinaanselina

    There is extra whitespace before the parenthesis.

    anselinaanselina

    Actually, this should just be removed entirely from the BitBucket hosting service, no?

    daviddavid

    These lines are indented four too-many spaces.

    daviddavid
    B.
    B.
    B.
    B.
    B.
    chipx86
    1. 
        
    2. reviewboard/hostingsvcs/service.py (Diff revision 2)
       
       
      Show all issues

      Always do absolute imports. 'urls' could be anything.

      You also need to have this below with the project imports.

    3. reviewboard/hostingsvcs/service.py (Diff revision 2)
       
       
       
      Show all issues

      Imports must always be alphabetical.

    4. reviewboard/hostingsvcs/service.py (Diff revision 2)
       
       
      Show all issues

      Name this repository_url_patterns.

    5. reviewboard/hostingsvcs/service.py (Diff revision 2)
       
       
       
       
      Show all issues

      The summary has to be one line without wrapping.

    6. reviewboard/hostingsvcs/service.py (Diff revision 2)
       
       
      Show all issues

      "thr" -> "the"

    7. reviewboard/hostingsvcs/service.py (Diff revision 2)
       
       
      Show all issues

      Trailing whitespace.

    8. reviewboard/hostingsvcs/service.py (Diff revision 2)
       
       
      Show all issues

      "URL" instead of "url"

    9. reviewboard/hostingsvcs/service.py (Diff revision 2)
       
       
       
      Show all issues

      The parameters to patterns need to align.

      Spaces around the +

    10. reviewboard/hostingsvcs/service.py (Diff revision 2)
       
       
      Show all issues

      Trailing whitespace.

    11. reviewboard/hostingsvcs/tests.py (Diff revision 2)
       
       
       
       
       
      Show all issues

      Alphabetical order.

    12. reviewboard/hostingsvcs/tests.py (Diff revision 2)
       
       
       
       
      Show all issues

      This must be one import statement, using parens, and one per line to prevent going over the line limit.

    13. reviewboard/hostingsvcs/tests.py (Diff revision 2)
       
       
      Show all issues

      Alphabetical.

    14. reviewboard/hostingsvcs/tests.py (Diff revision 2)
       
       
       
       
      Show all issues

      Two blank lines.

    15. reviewboard/hostingsvcs/tests.py (Diff revision 2)
       
       
       
       
      Show all issues

      This doesn't meet our formatting rules, and you're missing url(). It should be:

      repository_url_patterns = patterns(
          '',
      
          url('^hooks/pre-commit/$', ...),
      )
      

      Also note the /$ that you need at the end.

    16. reviewboard/hostingsvcs/tests.py (Diff revision 2)
       
       
       
       
      Show all issues

      Two blank lines.

    17. reviewboard/hostingsvcs/tests.py (Diff revision 2)
       
       
       
       
       
       
       
       
       
      Show all issues

      This is kind of hard to read. Can you group it into logical sections, with comments about what you're testing in each case?

    18. reviewboard/hostingsvcs/tests.py (Diff revision 2)
       
       
      Show all issues

      URLs should always have a trailing /.

      Same below.

    19. reviewboard/hostingsvcs/urls.py (Diff revision 2)
       
       
       
       
      Show all issues

      Two blank lines.

    20. reviewboard/hostingsvcs/urls.py (Diff revision 2)
       
       
       
       
      Show all issues

      Two blank lines.

    21. reviewboard/hostingsvcs/urls.py (Diff revision 2)
       
       
      Show all issues

      You should just include dynamic_urls, and not do a new patterns(...)

    22. reviewboard/static/rb/css/reviews.less (Diff revision 2)
       
       
       
       
       
       
      Show all issues

      This looks like a merge failure.

    23. reviewboard/static/rb/css/reviews.less (Diff revision 2)
       
       
      Show all issues

      Remove the trailing whitespace.

      In fact, revert this file.

    24. reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 2)
       
       
       
       
       
       
       
      Show all issues

      Merge failure. Revert this file.

    25. Show all issues

      Revert this file too.

    26. reviewboard/urls.py (Diff revision 2)
       
       
       
       
      Show all issues

      Imports must always be alphabetical.

    27. reviewboard/urls.py (Diff revision 2)
       
       
       
       
      Show all issues

      No blank line between these.

    28. 
        
    anselina
    1. I don't really have much to add to Christian's review, but I'll do another review on your next update!

    2. reviewboard/hostingsvcs/service.py (Diff revision 2)
       
       
      Show all issues

      This can just be if cls.repo_urlpatterns.

    3. 
        
    B.
    1. 
        
    2. reviewboard/hostingsvcs/urls.py (Diff revision 2)
       
       
      Show all issues

      If I do that I get the following error:

      The included urlconf <DynamicURLResolver [] (None:None) > doesn't have any patterns in it

    3. 
        
    B.
    chipx86
    1. 
        
    2. reviewboard/hostingsvcs/tests.py (Diff revision 2)
       
       
      Show all issues

      You don't need to create your own client. You can just use self.client.

    3. 
        
    B.
    chipx86
    1. 
        
    2. reviewboard/hostingsvcs/tests.py (Diff revision 3)
       
       
      Show all issues

      This needs to be a patterns(...)

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

      This should just be dummyView (which needs to be renamed to something more specific, and not use camelCase).

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

      You're missing the trailing slashes on these URLs, which will prevent you from getting to the right view.

    5. 
        
    B.
    1. 
        
    2. reviewboard/hostingsvcs/service.py (Diff revision 2)
       
       
       
      Show all issues

      Isn't this already in alphabetical order ?

      django...
      djblets.....

    3. 
        
    B.
    anselina
    1. Here are just some minor style issues (a lot of which are duplicates)!

      1. Thanks for the review. Will make the changes.

    2. reviewboard/hostingsvcs/service.py (Diff revision 4)
       
       
      Show all issues

      Review Board imports should go after the third-party module (Django, Djblets, etc.) imports, separated by a blank line.

    3. reviewboard/hostingsvcs/service.py (Diff revision 4)
       
       
      Show all issues

      This should be in alphabetical order - include should be before patterns, and the line needs to be moved to being before from django.utils import six.

    4. reviewboard/hostingsvcs/service.py (Diff revision 4)
       
       
       
      Show all issues

      URL instead of url.

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

      These should be in alphabetical order, before from django.utils.import six.

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

      URL instead of url.

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

      URL instead of url.

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

      This should be formatted like this instead:

      repository_url_patterns = patterns(
          '',
          url(r'^hooks/pre-commit/$', hosting_service_url_test_view)
      )
      
    9. reviewboard/hostingsvcs/tests.py (Diff revision 4)
       
       
      Show all issues

      Comments should start with a # and a single space, like:
      # This is a comment.

      Also, URL instead of url.

    10. reviewboard/hostingsvcs/urls.py (Diff revision 4)
       
       
      Show all issues

      include should be before patterns.

    11. 
        
    B.
    chipx86
    1. Home stretch! A few things to fix up now that the bulk of this is done.

    2. reviewboard/hostingsvcs/service.py (Diff revision 5)
       
       
      Show all issues

      "URL pattern"

      "class"

    3. reviewboard/hostingsvcs/service.py (Diff revision 5)
       
       
      Show all issues

      "KeyError"

    4. reviewboard/hostingsvcs/service.py (Diff revision 5)
       
       
       
      Show all issues

      The '' should go on the next line.

      Spaces around the +

    5. reviewboard/hostingsvcs/service.py (Diff revision 5)
       
       
       
       
      Show all issues

      I'd suggest moving this code after the try block. It's best to keep the bodies of a try/except as simple as possible.

    6. reviewboard/hostingsvcs/tests.py (Diff revision 5)
       
       
       
      Show all issues

      Docstring summaries cannot wrap unless they are a unit test function.

      I'd actually recommend moving this into DummyService. You should be abe to make this a @classmethod.

      1. I had actually tried that with this code:

        class DummyService(HostingService):
        name = 'DummyService'

            @classmethod
            def hosting_service_url_test_view(cls, request, repo_id):
                """View to test URL pattern addition while registration
                of a hosting service"""
                return HttpResponse(str(repo_id))
        
            repository_url_patterns = patterns(
                '',
                url(r'^hooks/pre-commit/$',
                hosting_service_url_test_view)
            )
        

        but it gave me the following error. That's why I kept it outside.

        AttributeError: 'classmethod' object has no attribute 'rindex'

      2. Do you have a full traceback for that AttributeError?

        Also, since you're not using the class at all, this could even be @staticmethod.

      3. When classmethod is used: http://pastie.org/8966222
        When staticmethod is used: http://pastie.org/8966224

      4. Hmm, ok, looks like django is doing some magic internally that doesn't like that. Keeping it as a normal method is fine.

    7. reviewboard/hostingsvcs/tests.py (Diff revision 5)
       
       
       
       
      Show all issues

      Should be 2 blank lines.

    8. reviewboard/hostingsvcs/tests.py (Diff revision 5)
       
       
       
       
       
       
      Show all issues

      This should be formatted like:

      repository_url_patterns = patterns(
          '',
      
          url(r'.....', view_name),
      )
      
    9. reviewboard/hostingsvcs/tests.py (Diff revision 5)
       
       
      Show all issues

      "URL"

      Same any time it appears in a comment or string.

    10. reviewboard/hostingsvcs/urls.py (Diff revision 5)
       
       
      Show all issues

      Make sure you have this as the first line (with a blank line after):

      from __future__ import unicode_literals
      
    11. reviewboard/hostingsvcs/urls.py (Diff revision 5)
       
       
       
      Show all issues

      Blank line between these.

    12. reviewboard/hostingsvcs/urls.py (Diff revision 5)
       
       
      Show all issues

      Space after '' in patterns()

    13. 
        
    B.
    CH
    1. Ship It!

    2. 
        
    anselina
    1. I've tested this out with my GitHub hook, and it works! :) While doing this, I noticed a few more really minor things.

    2. reviewboard/hostingsvcs/service.py (Diff revision 6)
       
       
      Show all issues

      This import isn't used.

    3. reviewboard/hostingsvcs/service.py (Diff revision 6)
       
       
      Show all issues

      "URL"

    4. reviewboard/hostingsvcs/service.py (Diff revision 6)
       
       
      Show all issues

      The last parenthesis should be on the next line, aligned with cls_urlpatterns on line 363.

    5. reviewboard/hostingsvcs/service.py (Diff revision 6)
       
       
       
       
      Show all issues

      These lines are indented an extra level.

    6. reviewboard/hostingsvcs/tests.py (Diff revision 6)
       
       
      Show all issues

      "registering"

    7. reviewboard/hostingsvcs/urls.py (Diff revision 6)
       
       
      Show all issues

      This import isn't used.

    8. reviewboard/hostingsvcs/urls.py (Diff revision 6)
       
       
      Show all issues

      There should be a space after the comma, not before (so it should be ...patterns('', dynamic_urls))

    9. reviewboard/hostingsvcs/urls.py (Diff revision 6)
       
       
      Show all issues

      There is extra whitespace before the parenthesis.

    10. 
        
    B.
    david
    1. There's a typo in your change description: "called ro access web hooks"

    2. reviewboard/hostingsvcs/bitbucket.py (Diff revision 7)
       
       
       
       
       
      Show all issues

      These lines are indented four too-many spaces.

      1. This file does not need to be part of the commit.

    3. 
        
    david
    1. 
        
      1. Given that this is still tagged "WIP", can you clarify what work is left to be done?

      2. Oh Sorry. There isn't any from this review request. I've removed the WIP tag.

    2. reviewboard/hostingsvcs/bitbucket.py (Diff revision 7)
       
       
       
       
       
       
      Show all issues

      Actually, this should just be removed entirely from the BitBucket hosting service, no?

      1. Yeah. This is just to show what testing I did on the bitbucket class. I'll just remove this file completely from the commit.

    3. 
        
    B.
    B.
    david
    B.
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-2.0.x (473766c). Thanks!