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!