- Description:
-
Please ignore all diffs except reviewboard/urls.py, hostingsvcs/urls.py, hostingsvcs/service.py. I was unable to rebase because of some merge conflicts and am now having trouble with that. I'll update the diffs as soon as I resolve them.
Generate url infrastructure for hosting services so that they can be called ro access web hooks. Added repo_urlpatterns to the service base class. Modified registration and unregistration of hosting services to add and remove urlpatterns respectively.
- - I need some more information before moving forward:
- - 1) ChipX86 mentioned that the url patterns must follow the below format:
- repos/<repo_id>/<hostingservice_name>/hooks/precommit,postcommit,etc. Please correct me if I'm wrong. - - 2) Now all service classes have a property repo_urlpatterns. Will this be initialized in the constructor of individual classes, including the <hostingservices_name> part ?
- Testing Done:
-
- - I was having trouble testing this functionality. What is the workflow of registering a new hosting service ? Does it happen through the web UI ? Can you point me to any documentation related to this ?
- - I saw the tests.py to find tests related to registering and unregistering a service. But couldn't find that. I was thinking of adding/modifying those test cases itself, so as to test this new property.
- - Should I test with an existing Hosting service like github or create a dummy hosting service ?
SCM Web Hooks for reviewboard : Phase 1
Review Request #5510 — Created Feb. 19, 2014 and submitted
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. |
chipx86 | |
Imports must always be alphabetical. |
chipx86 | |
Isn't this already in alphabetical order ? django... djblets..... |
B. b.ramnani | |
Name this repository_url_patterns. |
chipx86 | |
The summary has to be one line without wrapping. |
chipx86 | |
"thr" -> "the" |
chipx86 | |
Trailing whitespace. |
chipx86 | |
"URL" instead of "url" |
chipx86 | |
This can just be if cls.repo_urlpatterns. |
anselina | |
The parameters to patterns need to align. Spaces around the + |
chipx86 | |
Trailing whitespace. |
chipx86 | |
Alphabetical order. |
chipx86 | |
This must be one import statement, using parens, and one per line to prevent going over the line limit. |
chipx86 | |
Alphabetical. |
chipx86 | |
Two blank lines. |
chipx86 | |
This doesn't meet our formatting rules, and you're missing url(). It should be: repository_url_patterns = patterns( '', url('^hooks/pre-commit/$', ...), ) … |
chipx86 | |
Two blank lines. |
chipx86 | |
This is kind of hard to read. Can you group it into logical sections, with comments about what you're testing … |
chipx86 | |
You don't need to create your own client. You can just use self.client. |
chipx86 | |
URLs should always have a trailing /. Same below. |
chipx86 | |
Two blank lines. |
chipx86 | |
Two blank lines. |
chipx86 | |
You should just include dynamic_urls, and not do a new patterns(...) |
chipx86 | |
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. |
chipx86 | |
Remove the trailing whitespace. In fact, revert this file. |
chipx86 | |
Merge failure. Revert this file. |
chipx86 | |
Revert this file too. |
chipx86 | |
Imports must always be alphabetical. |
chipx86 | |
No blank line between these. |
chipx86 | |
This needs to be a patterns(...) |
chipx86 | |
This should just be dummyView (which needs to be renamed to something more specific, and not use camelCase). |
chipx86 | |
You're missing the trailing slashes on these URLs, which will prevent you from getting to the right view. |
chipx86 | |
Review Board imports should go after the third-party module (Django, Djblets, etc.) imports, separated by a blank line. |
anselina | |
This should be in alphabetical order - include should be before patterns, and the line needs to be moved to … |
anselina | |
URL instead of url. |
anselina | |
These should be in alphabetical order, before from django.utils.import six. |
anselina | |
URL instead of url. |
anselina | |
URL instead of url. |
anselina | |
This should be formatted like this instead: repository_url_patterns = patterns( '', url(r'^hooks/pre-commit/$', hosting_service_url_test_view) ) |
anselina | |
Comments should start with a # and a single space, like: # This is a comment. Also, URL instead of … |
anselina | |
include should be before patterns. |
anselina | |
"URL pattern" "class" |
chipx86 | |
"KeyError" |
chipx86 | |
The '' should go on the next line. Spaces around the + |
chipx86 | |
I'd suggest moving this code after the try block. It's best to keep the bodies of a try/except as simple … |
chipx86 | |
Docstring summaries cannot wrap unless they are a unit test function. I'd actually recommend moving this into DummyService. You should … |
chipx86 | |
Should be 2 blank lines. |
chipx86 | |
This should be formatted like: repository_url_patterns = patterns( '', url(r'.....', view_name), ) |
chipx86 | |
"URL" Same any time it appears in a comment or string. |
chipx86 | |
Make sure you have this as the first line (with a blank line after): from __future__ import unicode_literals |
chipx86 | |
Blank line between these. |
chipx86 | |
Space after '' in patterns() |
chipx86 | |
This import isn't used. |
anselina | |
"URL" |
anselina | |
The last parenthesis should be on the next line, aligned with cls_urlpatterns on line 363. |
anselina | |
These lines are indented an extra level. |
anselina | |
"registering" |
anselina | |
This import isn't used. |
anselina | |
There should be a space after the comma, not before (so it should be ...patterns('', dynamic_urls)) |
anselina | |
There is extra whitespace before the parenthesis. |
anselina | |
Actually, this should just be removed entirely from the BitBucket hosting service, no? |
david | |
These lines are indented four too-many spaces. |
david |
- Groups:
- Change Summary:
-
Finished implementation of addition and deletion of urlpatterns in hosting services. added a test case. need some help in passing the test cases.
- Description:
-
~ Please ignore all diffs except reviewboard/urls.py, hostingsvcs/urls.py, hostingsvcs/service.py. I was unable to rebase because of some merge conflicts and am now having trouble with that. I'll update the diffs as soon as I resolve them.
~ Please ignore all diffs except reviewboard/urls.py, hostingsvcs/urls.py, hostingsvcs/service.py, hostingsvcs/tests.py.py. I was unable to rebase because of some merge conflicts and am now having trouble with that. I'll update the diffs as soon as I resolve them.
Generate url infrastructure for hosting services so that they can be called ro access web hooks. Added repo_urlpatterns to the service base class. Modified registration and unregistration of hosting services to add and remove urlpatterns respectively.
- Testing Done:
-
~ Working on writing unit tests now.
~ Wrote test case HostingServiceUrlPatternTests in hostingsvcs.tests.py. I've tested the addition and deletion of url patterns using shell. The patterns are getting added in the dynamic urls. However, I get a 404 error when trying to access it. I can't figure out why. Will need some help there. I've added the urlpatterns in the reviewboard/urls.py as well as hostingsvcs.urls.py.
- Branch:
-
new-branch-bhushanmaster
- Diff:
-
Revision 2 (+1315 -1238)
-
-
Always do absolute imports. 'urls' could be anything.
You also need to have this below with the project imports.
-
-
-
-
-
-
-
-
-
-
This must be one import statement, using parens, and one per line to prevent going over the line limit.
-
-
-
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. -
-
This is kind of hard to read. Can you group it into logical sections, with comments about what you're testing in each case?
-
-
-
-
-
-
-
-
-
-
- Testing Done:
-
Wrote test case HostingServiceUrlPatternTests in hostingsvcs.tests.py. I've tested the addition and deletion of url patterns using shell. The patterns are getting added in the dynamic urls. However, I get a 404 error when trying to access it. I can't figure out why. Will need some help there. I've added the urlpatterns in the reviewboard/urls.py as well as hostingsvcs.urls.py.
+ + http://pastie.org/8824987#7,15
- Change Summary:
-
Added dummyView to the tests file.
- Branch:
-
new-branch-bhushanmaster
- Diff:
-
Revision 3 (+1322 -1238)
- Change Summary:
-
Fixed the test case for registration and unregistration of url patterns
- Description:
-
- Please ignore all diffs except reviewboard/urls.py, hostingsvcs/urls.py, hostingsvcs/service.py, hostingsvcs/tests.py.py. I was unable to rebase because of some merge conflicts and am now having trouble with that. I'll update the diffs as soon as I resolve them.
- Generate url infrastructure for hosting services so that they can be called ro access web hooks. Added repo_urlpatterns to the service base class. Modified registration and unregistration of hosting services to add and remove urlpatterns respectively.
- Testing Done:
-
~ Wrote test case HostingServiceUrlPatternTests in hostingsvcs.tests.py. I've tested the addition and deletion of url patterns using shell. The patterns are getting added in the dynamic urls. However, I get a 404 error when trying to access it. I can't figure out why. Will need some help there. I've added the urlpatterns in the reviewboard/urls.py as well as hostingsvcs.urls.py.
~ 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. All tests have passed. :)
- - http://pastie.org/8824987#7,15
-
Here are just some minor style issues (a lot of which are duplicates)!
-
Review Board imports should go after the third-party module (Django, Djblets, etc.) imports, separated by a blank line.
-
This should be in alphabetical order -
include
should be beforepatterns
, and the line needs to be moved to being beforefrom django.utils import six
. -
-
-
-
-
This should be formatted like this instead:
repository_url_patterns = patterns( '', url(r'^hooks/pre-commit/$', hosting_service_url_test_view) )
-
Comments should start with a # and a single space, like:
# This is a comment.
Also, URL instead of url.
-
-
Home stretch! A few things to fix up now that the bulk of this is done.
-
-
-
-
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.
-
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
. -
-
-
-
Make sure you have this as the first line (with a blank line after):
from __future__ import unicode_literals
-
-
-
I've tested this out with my GitHub hook, and it works! :) While doing this, I noticed a few more really minor things.
-
-
-
-
-
-
-
-
- Change Summary:
-
Made the changes as reviewed by heapify. Added Bitbucket test.
- Testing Done:
-
~ 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. All tests have passed. :)
~ 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. :)
- Diff:
-
Revision 7 (+131 -3)
- Added Files:
- Summary:
-
[WIP] SCM Web Hooks for reviewboard : Phase 1SCM Web Hooks for reviewboard : Phase 1
- Description:
-
~ Generate url infrastructure for hosting services so that they can be called ro access web hooks. Added repo_urlpatterns to the service base class. Modified registration and unregistration of hosting services to add and remove urlpatterns respectively.
~ 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.