-
-
-
reviewboard/webapi/resources/webhook.py (Diff revision 1) Col: 1 E302 expected 2 blank lines, found 1
-
reviewboard/webapi/resources/webhook.py (Diff revision 1) Col: 1 W293 blank line contains whitespace
-
-
reviewboard/webapi/tests/test_webhook.py (Diff revision 1) Col: 1 E302 expected 2 blank lines, found 1
-
reviewboard/webapi/tests/test_webhook.py (Diff revision 1) Col: 1 E302 expected 2 blank lines, found 1
-
-
Add webhook management API
Review Request #7483 — Created July 3, 2015 and submitted
Webhooks are now able to be created, retrieved, updated, or deleted via
the API. All webhook APIs are limited to superusers and local site
admins (for local site webhooks). Unit tests have been added to test
the basic CRUD functionality of the resource.
Ran unit tests.
| Description | From | Last Updated |
|---|---|---|
|
Col: 1 W391 blank line at end of file |
 reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
'Repository' imported but unused |
reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
Col: 1 W391 blank line at end of file |
reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
Missing a period. |
 chipx86
|
|
|
Missing a Returns block in the docstring. |
chipx86
|
|
|
This error isn't going to be very obvious to the caller without access to the code. I'm also not sure … |
chipx86
|
|
|
Should have a "Returns" section. |
chipx86
|
|
|
Can you add a docstring to this? |
chipx86
|
|
|
This will fail if the WebHookTarget isn't already saved, since there's no ID to work off of. We'll need to … |
chipx86
|
|
|
The "\" shouldn't be there. |
chipx86
|
|
|
Put these in sentence casing, and remove the ; at the end. They should stand alone as their own list … |
chipx86
|
|
|
Let's make true a literal. |
chipx86
|
|
|
Let's have these as their own list items, without appearing as part of a sentence. |
chipx86
|
|
|
"file-attachment-only" or maybe just "attachment-only" |
chipx86
|
|
|
You can use the multiple choice form in type: 'type': ('all', 'selected repositories', 'file attachments only') That said, I'm not … |
chipx86
|
|
|
Can also use the constants form. |
chipx86
|
|
|
Space before the ending quote. |
chipx86
|
|
|
Shouldn't be a dict. |
chipx86
|
|
|
Space before the ending quote. |
chipx86
|
|
|
Wondering if we need this. Clients should probably be able to infer based on the custom_payload value instead, right? We … |
chipx86
|
|
|
Space before the ending quote. |
chipx86
|
|
|
I'd recommend we add an is_accessible_by() function to WebHookTarget that does these checks, and an equivalent query function in WebHookTarget's … |
chipx86
|
|
|
The LocalSite should be accessible at this point, I believe through a local_site parameter. It'd match the local_site_name, so we … |
chipx86
|
|
|
This must be last in the list of decorators. Same below. |
chipx86
|
|
|
These will appear in the generated API docs. We should flesh them out a bit. These methods are exempt from … |
chipx86
|
|
|
Like above, this should use a choice tuple. |
chipx86
|
|
|
Space before the ending quote. |
chipx86
|
|
|
So should this. |
chipx86
|
|
|
Space before the ending quote. |
chipx86
|
|
|
Missing a period. |
chipx86
|
|
|
Like above, I think it's cleaner if we automatically set this based on whether there's custom content set or cleared. |
chipx86
|
|
|
The """ is indented too far. |
chipx86
|
|
|
Let's use a regex of r',\s*' |
chipx86
|
|
|
We can use the local_site passed to the handler instead. |
chipx86
|
|
|
These are all indented too far. |
chipx86
|
|
|
'LocalSite' imported but unused |
reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
extra_data is going to be set to whatever is returned. You can change this to: return self.cleaned_data['extra_data'] or {} |
chipx86
|
|
|
I wouldn't use bool for this. Instead, I'd explicitly check that we have a valid value. Helps keep the logic … |
chipx86
|
|
|
Rather than this wrapping, how about pulling out apply_to into a variable? |
chipx86
|
|
|
We may access local_site many times. Let's pull it out into a variable. |
chipx86
|
|
|
We should probably localize this and the errors below. |
chipx86
|
|
|
Can you add Args and Returns here? |
chipx86
|
|
|
You can short-circuit this by doing: return (user.is_superuser or (user.is_authenticated() and (local_site ......))) Also, better to do self.local_site_id == local_site.pk, … |
chipx86
|
|
|
I know it's painful here, but can you add Args and Returns? |
chipx86
|
|
|
You should only save here if modifying extra_data, since it's the only thing after the initial create + save that … |
chipx86
|
|
|
We're no longer using "use_custom_content," so this should be updated. |
chipx86
|
|
|
"attachment-only" |
chipx86
|
|
|
Since consumers may not know what a "local site" is (and may wrongly assume it just means a local install), … |
chipx86
|
|
|
This list, and the values within, are hard-coded in multiple places. We should have constants for each and a constant … |
chipx86
|
|
|
Let's define an ALL_ENCODINGS list in WebHookTarget containing these (well, the constants for these) and reference that here, to avoid … |
chipx86
|
|
|
This should be a list in the payload. |
chipx86
|
|
|
Since the contents are going to be of a particular resource type, you can do: 'type': [RepositoryResource], Or, if that … |
chipx86
|
|
|
No blank line here. |
chipx86
|
|
|
We should have a manager method that returns accessible objects, given a user and a LocalSite, much like we do … |
chipx86
|
|
|
We're doing these here, but not above. We should be consistent on all of them. |
chipx86
|
|
|
We should store a pre-compiled regex for these on the class, and use it both here and below. |
chipx86
|
|
|
Same comment as above about using a pre-defined list. |
chipx86
|
|
|
This function needs to check if the user has permission to create a WebHook. For repositories, we have a can_create … |
chipx86
|
|
|
This can be: form_data['use_custom_content'] = (form_data.get('custom_content', '') != '') |
chipx86
|
|
|
Same as above. |
chipx86
|
|
|
Same as above. |
chipx86
|
|
|
This should be consistent with above. |
chipx86
|
|
|
These blank lines can be removed. |
chipx86
|
|
|
This is more efficient as: return self.filter(local_site=local_site) It'll choose the most appropriate query based on the value of local_site that … |
chipx86
|
|
|
"keyword" Here and elsewhere. |
chipx86
|
|
|
Parents around the comparison. |
chipx86
|
|
|
Should have a trailing comma. |
chipx86
|
|
|
Here too. |
chipx86
|
|
|
Should have parens around the comparison. |
chipx86
|
|
|
In this particular case, \ is preferable to (). |
chipx86
|
|
|
Trailing comma. |
chipx86
|
|
|
Here too. |
chipx86
|
|
|
'ValidationError' imported but unused |
reviewbot
|
|
|
'logging' imported but unused |
reviewbot
|
|
|
'get_repository_item_url' imported but unused |
reviewbot
|
|
|
local variable 'repository_pks' is assigned to but never used |
reviewbot
|
Change Summary:
Fix tests. General cleanup.
Summary: |
|
|||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||
Testing Done: |
|
|||||||||||||||
Commit: |
|
|||||||||||||||
Diff: |
Revision 2 (+744) |
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/forms.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/forms.py
Change Summary:
Remove print statements.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+740) |
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/forms.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/forms.py
-
Thanks for taking this on!
A lot of these are small issues. Missing spaces, missing periods, things like that. There's some comments on validation errors, reducing query counts, where some code should live, little things about the API that can help with validation, etc.
One bigger topic is the approach of modifying the form for webhooks to manipulate the list of fields. I'm not sure this is the way to go, but I want to open a discussion on it. I'll try to articulate what about that makes me uncomfortable, and why I'm not sure whether it should or not.
A form is generally built to take a whole set of input at once (through a form post) and can then generate a result or a validation error. So that makes sense, and maybe we should rethink more API resources in those terms. However, there are annoyances with forms that you're having to work around, and I think we need a differnt solution for them. This has to do with field exclusions.
Excluding fields based on user input provides too many avenues for logic to break in more complex forms, or for some crucial bit of validation/permission checking to fail. (I'm speaking generally, since this design should be able to apply to future resources.) I want to avoid this situation, to simplify the forms avoid such situations.
For creation, we should never have to remove fields from the form. We should require that every field the form needs is either provided by the caller, or provided by the API handler (based on provided data). That part is easy.
Updating is harder, because a form expects that all fields will be provided in the payload. So what we'd have to do there is build a payload based on the object state, and then update it with what's provided by the caller. That's annoying and lengthy, though, and at that point, we might as well loop on all possible fields and set any provided on the object like we do in most other resources.
So, options:
- We find a better way to do form updates, or we do the hard work of building an initial payload that we then update.
- We scrap the form method, and do what other resources do, which is create/update the
WebHookTargetdirectly.
-
-
-
reviewboard/notifications/forms.py (Diff revision 3) This error isn't going to be very obvious to the caller without access to the code. I'm also not sure we want to leak information about the local sites to the caller.
Instead, let's just fail with a single validation error if the repository isn't an accessible repository, informating the user that it's not a valid repository.
-
-
-
reviewboard/testing/testcase.py (Diff revision 3) This will fail if the
WebHookTargetisn't already saved, since there's no ID to work off of. We'll need to do it afterwebhook.save(). -
-
reviewboard/webapi/resources/webhook.py (Diff revision 3) Put these in sentence casing, and remove the
;at the end. They should stand alone as their own list items. -
-
reviewboard/webapi/resources/webhook.py (Diff revision 3) Let's have these as their own list items, without appearing as part of a sentence.
-
reviewboard/webapi/resources/webhook.py (Diff revision 3) "file-attachment-only" or maybe just "attachment-only"
-
reviewboard/webapi/resources/webhook.py (Diff revision 3) You can use the multiple choice form in
type:'type': ('all', 'selected repositories', 'file attachments only')That said, I'm not sure about these strings as values. I'd prefer something without spaces. Maybe call this
apply_to_repositorieswith values ofall,none,custom? -
-
-
-
-
reviewboard/webapi/resources/webhook.py (Diff revision 3) Wondering if we need this. Clients should probably be able to infer based on the
custom_payloadvalue instead, right? We could make sure that we set that tonullif not dealing with a custom payload. -
-
reviewboard/webapi/resources/webhook.py (Diff revision 3) I'd recommend we add an
is_accessible_by()function toWebHookTargetthat does these checks, and an equivalent query function inWebHookTarget's manager, much like we have for groups and repositories and such.Once we have that, the queryset below can turn into:
return WebHookTarget.accessible(request.user, local_site=local_site)(Note that, as I'll mention below, we have a
local_siteargument we can use instead oflocal_site_name, simplifying the logic inget_queryseta good bit.)Then, this function becomes:
return obj.is_accessible_by(request.user) -
reviewboard/webapi/resources/webhook.py (Diff revision 3) The LocalSite should be accessible at this point, I believe through a
local_siteparameter. It'd match thelocal_site_name, so we can query from that directly. -
reviewboard/webapi/resources/webhook.py (Diff revision 3) This must be last in the list of decorators.
Same below.
-
reviewboard/webapi/resources/webhook.py (Diff revision 3) These will appear in the generated API docs. We should flesh them out a bit.
These methods are exempt from the doc styles we're now using elsewhere, in that these should be pluralized.
"Retrieves ..."
etc.
See the repository docs, for instance.
-
reviewboard/webapi/resources/webhook.py (Diff revision 3) Like above, this should use a choice tuple.
-
-
-
-
-
reviewboard/webapi/resources/webhook.py (Diff revision 3) Like above, I think it's cleaner if we automatically set this based on whether there's custom content set or cleared.
-
-
-
reviewboard/webapi/resources/webhook.py (Diff revision 3) We can use the
local_sitepassed to the handler instead. -
Change Summary:
Update à la Christian's issues.
This now depends on the new updatable form mixin logic and decorator updates to RB and Djblets.
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/notifications/models.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/forms.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/notifications/models.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/forms.py
Commit: |
|
||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+670)
|
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/notifications/models.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/forms.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/notifications/models.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/forms.py -
-
reviewboard/webapi/resources/webhook.py (Diff revision 5) Col: 1 E302 expected 2 blank lines, found 1
Diff: |
Revision 6 (+680)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/notifications/models.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/forms.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/notifications/models.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/forms.py
-
-
reviewboard/notifications/forms.py (Diff revision 6) extra_datais going to be set to whatever is returned. You can change this to:return self.cleaned_data['extra_data'] or {} -
reviewboard/notifications/forms.py (Diff revision 6) I wouldn't use
boolfor this. Instead, I'd explicitly check that we have a valid value. Helps keep the logic more explicit and less error-prone. -
reviewboard/notifications/forms.py (Diff revision 6) Rather than this wrapping, how about pulling out
apply_tointo a variable? -
reviewboard/notifications/forms.py (Diff revision 6) We may access
local_sitemany times. Let's pull it out into a variable. -
reviewboard/notifications/forms.py (Diff revision 6) We should probably localize this and the errors below.
-
-
reviewboard/notifications/models.py (Diff revision 6) You can short-circuit this by doing:
return (user.is_superuser or (user.is_authenticated() and (local_site ......)))Also, better to do
self.local_site_id == local_site.pk, to help prevent unnecessary lookups. -
reviewboard/testing/testcase.py (Diff revision 6) I know it's painful here, but can you add
ArgsandReturns? -
reviewboard/testing/testcase.py (Diff revision 6) You should only save here if modifying
extra_data, since it's the only thing after the initial create + save that would require re-saving. (You can also optimize by doingsave(update_fields=['extra_data']).) -
reviewboard/webapi/resources/webhook.py (Diff revision 6) We're no longer using "use_custom_content," so this should be updated.
-
-
reviewboard/webapi/resources/webhook.py (Diff revision 6) Since consumers may not know what a "local site" is (and may wrongly assume it just means a local install), we should use
:term:'Local Site'(but with backticks -- silly Markdown) to link to the glossary. -
reviewboard/webapi/resources/webhook.py (Diff revision 6) This list, and the values within, are hard-coded in multiple places. We should have constants for each and a constant for all possible values, to ensure we're never out of sync.
-
reviewboard/webapi/resources/webhook.py (Diff revision 6) Let's define an
ALL_ENCODINGSlist inWebHookTargetcontaining these (well, the constants for these) and reference that here, to avoid having a situation where they get out of sync. -
-
reviewboard/webapi/resources/webhook.py (Diff revision 6) Since the contents are going to be of a particular resource type, you can do:
'type': [RepositoryResource],Or, if that triggers import issues:
'type': ['reviewboard.webapi.resources.repository.RepositoryResource'], -
-
reviewboard/webapi/resources/webhook.py (Diff revision 6) We should have a manager method that returns accessible objects, given a user and a LocalSite, much like we do for repositories and other objects.
-
reviewboard/webapi/resources/webhook.py (Diff revision 6) We're doing these here, but not above. We should be consistent on all of them.
-
reviewboard/webapi/resources/webhook.py (Diff revision 6) We should store a pre-compiled regex for these on the class, and use it both here and below.
-
reviewboard/webapi/resources/webhook.py (Diff revision 6) Same comment as above about using a pre-defined list.
-
reviewboard/webapi/resources/webhook.py (Diff revision 6) This function needs to check if the user has permission to create a WebHook.
For repositories, we have a
can_createmethod onRepositoryManager. Might be a good example. -
reviewboard/webapi/resources/webhook.py (Diff revision 6) This can be:
form_data['use_custom_content'] = (form_data.get('custom_content', '') != '') -
-
-
Diff: |
Revision 7 (+903)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/notifications/models.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/managers.py reviewboard/notifications/forms.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/notifications/models.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/managers.py reviewboard/notifications/forms.py
-
-
reviewboard/notifications/managers.py (Diff revision 7) This is more efficient as:
return self.filter(local_site=local_site)It'll choose the most appropriate query based on the value of
local_sitethat way. It'll also skip doing a join (local_site__idwill do a join on LocalSites and then comapre the ID, as opposed tolocal_site_id=, or comparinglocal_site=to an object). -
-
-
-
-
-
-
Change Summary:
Address Christian's issues. Add new unit tests.
Diff: |
Revision 8 (+1030)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/notifications/models.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/managers.py reviewboard/notifications/forms.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/notifications/models.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/managers.py reviewboard/notifications/forms.py -
-
-
reviewboard/webapi/tests/test_webhook.py (Diff revision 8) 'get_repository_item_url' imported but unused
-
reviewboard/webapi/tests/test_webhook.py (Diff revision 8) local variable 'repository_pks' is assigned to but never used
Diff: |
Revision 9 (+1023 -2)
|
|---|
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/notifications/models.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/managers.py reviewboard/notifications/forms.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/notifications/models.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/managers.py reviewboard/notifications/forms.py
-
-
-
reviewboard/webapi/resources/webhook.py (Diff revisions 7 - 9) In this particular case,
\is preferable to().
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/forms.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/__init__.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/webhook.py reviewboard/webapi/tests/test_webhook.py reviewboard/webapi/tests/mimetypes.py reviewboard/notifications/forms.py