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)
     
     

    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)
     
     
     

    Imports must always be alphabetical.

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

    Name this repository_url_patterns.

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

    The summary has to be one line without wrapping.

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

    "thr" -> "the"

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

    Trailing whitespace.

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

    "URL" instead of "url"

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

    The parameters to patterns need to align.

    Spaces around the +

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

    Trailing whitespace.

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

    Alphabetical order.

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

    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)
     
     

    Alphabetical.

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

    Two blank lines.

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

    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)
     
     
     
     

    Two blank lines.

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

    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)
     
     

    URLs should always have a trailing /.

    Same below.

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

    Two blank lines.

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

    Two blank lines.

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

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

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

    This looks like a merge failure.

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

    Remove the trailing whitespace.

    In fact, revert this file.

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

    Merge failure. Revert this file.

  25. Revert this file too.

  26. reviewboard/urls.py (Diff revision 2)
     
     
     
     

    Imports must always be alphabetical.

  27. reviewboard/urls.py (Diff revision 2)
     
     
     
     

    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)
     
     

    This can just be if cls.repo_urlpatterns.

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

    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)
     
     

    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)
     
     

    This needs to be a patterns(...)

  3. reviewboard/hostingsvcs/tests.py (Diff revision 3)
     
     

    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)
     
     

    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)
     
     
     

    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)
     
     

    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)
     
     

    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)
     
     
     

    URL instead of url.

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

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

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

    URL instead of url.

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

    URL instead of url.

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

    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)
     
     

    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)
     
     

    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)
     
     

    "URL pattern"

    "class"

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

    "KeyError"

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

    The '' should go on the next line.

    Spaces around the +

  5. reviewboard/hostingsvcs/service.py (Diff revision 5)
     
     
     
     

    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)
     
     
     

    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)
     
     
     
     

    Should be 2 blank lines.

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

    This should be formatted like:

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

    "URL"

    Same any time it appears in a comment or string.

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

    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)
     
     
     

    Blank line between these.

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

    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)
     
     

    This import isn't used.

  3. reviewboard/hostingsvcs/service.py (Diff revision 6)
     
     
  4. reviewboard/hostingsvcs/service.py (Diff revision 6)
     
     

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

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

    These lines are indented an extra level.

  6. reviewboard/hostingsvcs/tests.py (Diff revision 6)
     
     

    "registering"

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

    This import isn't used.

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

    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)
     
     

    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)
     
     
     
     
     

    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)
     
     
     
     
     
     

    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: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (473766c). Thanks!
Loading...