Summary

Add webhook management API

Review Request #7483 — Created July 3, 2015 and submitted July 14, 2015, 11:07 a.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

~~7493~~, ~~7492~~, ~~7491~~

Commit

0db4571...

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
Col: 1 W391 blank line at end of file	reviewbot	July 3, 2015, 10:46 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	July 3, 2015, 10:46 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	July 3, 2015, 10:46 p.m.
'Repository' imported but unused	reviewbot	July 3, 2015, 10:58 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	July 3, 2015, 10:58 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	July 3, 2015, 10:58 p.m.
Col: 1 W391 blank line at end of file	reviewbot	July 3, 2015, 10:59 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	July 3, 2015, 10:59 p.m.
Missing a period.	chipx86	July 4, 2015, 5:32 p.m.
Missing a Returns block in the docstring.	chipx86	July 4, 2015, 5:32 p.m.
This error isn't going to be very obvious to the caller without access to the code. I'm also not sure …	chipx86	July 4, 2015, 5:32 p.m.
Should have a "Returns" section.	chipx86	July 4, 2015, 5:32 p.m.
Can you add a docstring to this?	chipx86	July 4, 2015, 5:33 p.m.
This will fail if the WebHookTarget isn't already saved, since there's no ID to work off of. We'll need to …	chipx86	July 4, 2015, 5:34 p.m.
The "\" shouldn't be there.	chipx86	July 4, 2015, 5:34 p.m.
Put these in sentence casing, and remove the ; at the end. They should stand alone as their own list …	chipx86	July 4, 2015, 5:35 p.m.
Let's make true a literal.	chipx86	July 4, 2015, 5:55 p.m.
Let's have these as their own list items, without appearing as part of a sentence.	chipx86	July 4, 2015, 5:36 p.m.
"file-attachment-only" or maybe just "attachment-only"	chipx86	July 4, 2015, 5:37 p.m.
You can use the multiple choice form in type: 'type': ('all', 'selected repositories', 'file attachments only') That said, I'm not …	chipx86	July 4, 2015, 5:38 p.m.
Can also use the constants form.	chipx86	July 4, 2015, 5:39 p.m.
Space before the ending quote.	chipx86	July 4, 2015, 5:39 p.m.
Shouldn't be a dict.	chipx86	July 4, 2015, 5:39 p.m.
Space before the ending quote.	chipx86	July 4, 2015, 5:39 p.m.
Wondering if we need this. Clients should probably be able to infer based on the custom_payload value instead, right? We …	chipx86	July 5, 2015, 2:24 a.m.
Space before the ending quote.	chipx86	July 4, 2015, 5:40 p.m.
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	July 4, 2015, 5:44 p.m.
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	July 4, 2015, 5:46 p.m.
This must be last in the list of decorators. Same below.	chipx86	July 4, 2015, 5:46 p.m.
These will appear in the generated API docs. We should flesh them out a bit. These methods are exempt from …	chipx86	July 8, 2015, 4:50 p.m.
Like above, this should use a choice tuple.	chipx86	July 4, 2015, 5:49 p.m.
Space before the ending quote.	chipx86	July 4, 2015, 5:50 p.m.
So should this.	chipx86	July 4, 2015, 5:51 p.m.
Space before the ending quote.	chipx86	July 4, 2015, 5:51 p.m.
Missing a period.	chipx86	July 4, 2015, 5:51 p.m.
Like above, I think it's cleaner if we automatically set this based on whether there's custom content set or cleared.	chipx86	July 4, 2015, 5:51 p.m.
The """ is indented too far.	chipx86	July 4, 2015, 5:51 p.m.
Let's use a regex of r',\s*'	chipx86	July 4, 2015, 5:55 p.m.
We can use the local_site passed to the handler instead.	chipx86	July 4, 2015, 5:55 p.m.
These are all indented too far.	chipx86	July 4, 2015, 5:55 p.m.
'LocalSite' imported but unused	reviewbot	July 8, 2015, 4:48 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	July 8, 2015, 4:48 p.m.
extra_data is going to be set to whatever is returned. You can change this to: return self.cleaned_data['extra_data'] or {}	chipx86	July 11, 2015, 2:36 a.m.
I wouldn't use bool for this. Instead, I'd explicitly check that we have a valid value. Helps keep the logic …	chipx86	July 11, 2015, 2:39 a.m.
Rather than this wrapping, how about pulling out apply_to into a variable?	chipx86	July 11, 2015, 2:39 a.m.
We may access local_site many times. Let's pull it out into a variable.	chipx86	July 13, 2015, 5:30 p.m.
We should probably localize this and the errors below.	chipx86	July 11, 2015, 2:42 a.m.
Can you add Args and Returns here?	chipx86	July 11, 2015, 2:44 a.m.
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	July 11, 2015, 2:45 a.m.
I know it's painful here, but can you add Args and Returns?	chipx86	July 11, 2015, 2:54 a.m.
You should only save here if modifying extra_data, since it's the only thing after the initial create + save that …	chipx86	July 11, 2015, 2:54 a.m.
We're no longer using "use_custom_content," so this should be updated.	chipx86	July 11, 2015, 2:54 a.m.
"attachment-only"	chipx86	July 11, 2015, 2:55 a.m.
Since consumers may not know what a "local site" is (and may wrongly assume it just means a local install), …	chipx86	July 11, 2015, 2:56 a.m.
This list, and the values within, are hard-coded in multiple places. We should have constants for each and a constant …	chipx86	July 11, 2015, 3:01 a.m.
Let's define an ALL_ENCODINGS list in WebHookTarget containing these (well, the constants for these) and reference that here, to avoid …	chipx86	July 11, 2015, 2:58 a.m.
This should be a list in the payload.	chipx86	July 11, 2015, 3:02 a.m.
Since the contents are going to be of a particular resource type, you can do: 'type': [RepositoryResource], Or, if that …	chipx86	July 11, 2015, 3:03 a.m.
No blank line here.	chipx86	July 11, 2015, 3:03 a.m.
We should have a manager method that returns accessible objects, given a user and a LocalSite, much like we do …	chipx86	July 11, 2015, 3:10 a.m.
We're doing these here, but not above. We should be consistent on all of them.	chipx86	July 11, 2015, 3:23 a.m.
We should store a pre-compiled regex for these on the class, and use it both here and below.	chipx86	July 11, 2015, 3:30 a.m.
Same comment as above about using a pre-defined list.	chipx86	July 11, 2015, 3:22 a.m.
This function needs to check if the user has permission to create a WebHook. For repositories, we have a can_create …	chipx86	July 11, 2015, 3:22 a.m.
This can be: form_data['use_custom_content'] = (form_data.get('custom_content', '') != '')	chipx86	July 11, 2015, 3:19 a.m.
Same as above.	chipx86	July 11, 2015, 3:18 a.m.
Same as above.	chipx86	July 11, 2015, 3:18 a.m.
This should be consistent with above.	chipx86	July 11, 2015, 3:17 a.m.
These blank lines can be removed.	chipx86	July 14, 2015, 10:49 a.m.
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	July 13, 2015, 5:31 p.m.
"keyword" Here and elsewhere.	chipx86	July 13, 2015, 5:31 p.m.
Parents around the comparison.	chipx86	July 13, 2015, 5:31 p.m.
Should have a trailing comma.	chipx86	July 13, 2015, 5:32 p.m.
Here too.	chipx86	July 13, 2015, 5:32 p.m.
Should have parens around the comparison.	chipx86	July 13, 2015, 5:32 p.m.
In this particular case, \ is preferable to ().	chipx86	July 14, 2015, 11:07 a.m.
Trailing comma.	chipx86	July 13, 2015, 5:32 p.m.
Here too.	chipx86	July 13, 2015, 5:32 p.m.
'ValidationError' imported but unused	reviewbot	July 13, 2015, 7:08 p.m.
'logging' imported but unused	reviewbot	July 13, 2015, 7:08 p.m.
'get_repository_item_url' imported but unused	reviewbot	July 13, 2015, 7:09 p.m.
local variable 'repository_pks' is assigned to but never used	reviewbot	July 13, 2015, 7:09 p.m.

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

reviewboard/notifications/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W391 blank line at end of file
```
reviewboard/webapi/resources/webhook.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/webapi/resources/webhook.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/webapi/tests/test_webhook.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'Repository' imported but unused
```
reviewboard/webapi/tests/test_webhook.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/webapi/tests/test_webhook.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/webapi/tests/test_webhook.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W391 blank line at end of file
```
reviewboard/webapi/tests/urls.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Change Summary:

Fix tests. General cleanup.

Summary:

[WIP] Add Webhook API

Add webhook management API

Description:

~		Some tests are currently failing becuase of the admin-only nature of the resource: http://pastie.org/private/vkcd29ypf4iqywbpbo6cma
	~	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.

Testing Done:

Ran unit tests.

Commit:

e1e48ee444109549d8c974cf67cab2aaa1d09c58

d0b71853720465237b09f2afec135393fd1d48f9

Diff:

Revision 2 (+744)

Show changes

	reviewboard/notifications/forms.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/resources/webhook.py
	reviewboard/webapi/tests/mimetypes.py
	reviewboard/webapi/tests/test_webhook.py
	reviewboard/webapi/tests/urls.py

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:

d0b71853720465237b09f2afec135393fd1d48f9

7b10aafc460013d5a55c3aa19699ac75ccf42f9d

Diff:

Revision 3 (+740)

Show changes

	reviewboard/notifications/forms.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/resources/webhook.py
	reviewboard/webapi/tests/mimetypes.py
	reviewboard/webapi/tests/test_webhook.py
	reviewboard/webapi/tests/urls.py

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 WebHookTarget directly.

reviewboard/notifications/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing a period.
```
reviewboard/notifications/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing a Returns block in the docstring.
```

reviewboard/notifications/forms.py (Diff revision 3)

The issue has been resolved. Show all issues

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/notifications/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Should have a "Returns" section.
```
reviewboard/testing/testcase.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can you add a docstring to this?
```

reviewboard/testing/testcase.py (Diff revision 3)

The issue has been resolved. Show all issues

This will fail if the WebHookTarget isn't already saved, since there's no ID to work off of. We'll need to do it after webhook.save().

reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
The "\" shouldn't be there.
```

reviewboard/webapi/resources/webhook.py (Diff revision 3)

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
Let's make true a literal.
```
reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Let's have these as their own list items, without appearing as part of a sentence.
```
reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"file-attachment-only" or maybe just "attachment-only"
```

reviewboard/webapi/resources/webhook.py (Diff revision 3)

The issue has been resolved. Show all issues

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_repositories with values of all, none, custom?

reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can also use the constants form.
```
reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Space before the ending quote.
```
reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Shouldn't be a dict.
```
reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Space before the ending quote.
```

reviewboard/webapi/resources/webhook.py (Diff revision 3)

The issue has been resolved. Show all issues

Wondering if we need this. Clients should probably be able to infer based on the custom_payload value instead, right? We could make sure that we set that to null if not dealing with a custom payload.

reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Space before the ending quote.
```

reviewboard/webapi/resources/webhook.py (Diff revision 3)

The issue has been resolved. Show all issues

I'd recommend we add an is_accessible_by() function to WebHookTarget that does these checks, and an equivalent query function in WebHookTarget'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_site argument we can use instead of local_site_name, simplifying the logic in get_queryset a good bit.)
Then, this function becomes:

return obj.is_accessible_by(request.user)

reviewboard/webapi/resources/webhook.py (Diff revision 3)

The issue has been resolved. Show all issues

The LocalSite should be accessible at this point, I believe through a local_site parameter. It'd match the local_site_name, so we can query from that directly.

brennie July 4, 2015, 10:22 p.m.

There is no local_site parameter passed here.

reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This must be last in the list of decorators.
Same below.
```

reviewboard/webapi/resources/webhook.py (Diff revision 3)

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
Like above, this should use a choice tuple.
```
reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Space before the ending quote.
```
reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
So should this.
```
reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Space before the ending quote.
```
reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing a period.
```

reviewboard/webapi/resources/webhook.py (Diff revision 3)

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
The """ is indented too far.
```
reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Let's use a regex of r',\s*'
```
reviewboard/webapi/resources/webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
We can use the local_site passed to the handler instead.
```
reviewboard/webapi/tests/test_webhook.py (Diff revision 3)
The issue has been resolved. Show all issues
```
These are all indented too far.
```

Change Summary:

Update à la Christian's issues.

This now depends on the new updatable form mixin logic and decorator updates to RB and Djblets.

Depends On:

~~7491 - Make webapi_request_fields pass dictionary of parsed request fields~~

~~7492 - webapi_check_local_site now passes LocalSite~~

~~7493 - Add a mixin for updating models with a ModelForm~~

Diff:

Revision 4 (+112)

Show changes

	reviewboard/notifications/forms.py
	reviewboard/notifications/models.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/tests/mimetypes.py
	reviewboard/webapi/tests/urls.py

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:

7b10aafc460013d5a55c3aa19699ac75ccf42f9d

0db4571aba8b151831ab697bd37552c3edff4816

Diff:

Revision 5 (+670)

Show changes

	reviewboard/notifications/forms.py
	reviewboard/notifications/models.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/resources/webhook.py
	reviewboard/webapi/tests/mimetypes.py
	reviewboard/webapi/tests/test_webhook.py
	1 more

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)
The issue has been resolved. Show all issues
```
 'LocalSite' imported but unused
```
reviewboard/webapi/resources/webhook.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Diff:

Revision 6 (+680)

Show changes

	reviewboard/notifications/forms.py
	reviewboard/notifications/models.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/resources/webhook.py
	reviewboard/webapi/tests/mimetypes.py
	reviewboard/webapi/tests/test_webhook.py
	1 more

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)

The issue has been resolved. Show all issues

extra_data is going to be set to whatever is returned. You can change this to:

return self.cleaned_data['extra_data'] or {}

brennie July 11, 2015, 3:17 a.m.

This is no longer required because the UpdateFormMixin ensures that extra_data is a dict before assigning.

brennie July 11, 2015, 3:54 a.m.
```
Actually, ignore that :)
```

reviewboard/notifications/forms.py (Diff revision 6)

The issue has been resolved. Show all issues

I wouldn't use bool for 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)
The issue has been resolved. Show all issues
```
Rather than this wrapping, how about pulling out apply_to into a variable?
```

reviewboard/notifications/forms.py (Diff revision 6)

The issue has been resolved. Show all issues

We may access local_site many times. Let's pull it out into a variable.

brennie July 11, 2015, 3:17 a.m.

self.cleaned_data['local_site'] is accessed only once.

chipx86 July 13, 2015, 5:15 p.m.

We're looping through all repositories in a queryset, and comparing the contents against that. Am I misreading it?

brennie July 13, 2015, 5:17 p.m.
```
I am a silly person.
```

reviewboard/notifications/forms.py (Diff revision 6)
The issue has been resolved. Show all issues
```
We should probably localize this and the errors below.
```

reviewboard/notifications/models.py (Diff revision 6)

The issue has been resolved. Show all issues

Can you add Args and Returns here?

reviewboard/notifications/models.py (Diff revision 6)

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
I know it's painful here, but can you add Args and Returns?
```

reviewboard/testing/testcase.py (Diff revision 6)

The issue has been resolved. Show all issues

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 doing save(update_fields=['extra_data']).)

reviewboard/webapi/resources/webhook.py (Diff revision 6)
The issue has been resolved. Show all issues
```
We're no longer using "use_custom_content," so this should be updated.
```
reviewboard/webapi/resources/webhook.py (Diff revision 6)
The issue has been resolved. Show all issues
```
"attachment-only"
```

reviewboard/webapi/resources/webhook.py (Diff revision 6)

The issue has been resolved. Show all issues

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)

The issue has been resolved. Show all issues

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)

The issue has been resolved. Show all issues

Let's define an ALL_ENCODINGS list in WebHookTarget containing 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)
The issue has been resolved. Show all issues
```
This should be a list in the payload.
```

reviewboard/webapi/resources/webhook.py (Diff revision 6)

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
No blank line here.
```

reviewboard/webapi/resources/webhook.py (Diff revision 6)

The issue has been resolved. Show all issues

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)

The issue has been resolved. Show all issues

We're doing these here, but not above. We should be consistent on all of them.

reviewboard/webapi/resources/webhook.py (Diff revision 6)

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
Same comment as above about using a pre-defined list.
```

reviewboard/webapi/resources/webhook.py (Diff revision 6)

The issue has been resolved. Show all issues

This function needs to check if the user has permission to create a WebHook.
For repositories, we have a can_create method on RepositoryManager. Might be a good example.

reviewboard/webapi/resources/webhook.py (Diff revision 6)

The issue has been resolved. Show all issues

This can be:

form_data['use_custom_content'] = (form_data.get('custom_content', '') != '')

reviewboard/webapi/resources/webhook.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Same as above.
```
reviewboard/webapi/resources/webhook.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Same as above.
```

reviewboard/webapi/resources/webhook.py (Diff revision 6)

The issue has been dropped. Show all issues

This should be consistent with above.

brennie July 11, 2015, 3:17 a.m.

We cannot assume the user wants to set custom_content to '' and use_custom_content to False when not providing the custom_content in the case of a partial update.

chipx86 July 13, 2015, 5:15 p.m.
```
Ah, I understand. Makes sense.
```

Diff:

Revision 7 (+903)

Show changes

	reviewboard/notifications/forms.py
	reviewboard/notifications/managers.py
	reviewboard/notifications/models.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/resources/webhook.py
	reviewboard/webapi/tests/mimetypes.py
	2 more

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)

The issue has been resolved. Show all issues

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 way. It'll also skip doing a join (local_site__id will do a join on LocalSites and then comapre the ID, as opposed to local_site_id=, or comparing local_site= to an object).

reviewboard/webapi/resources/webhook.py (Diff revision 7)
The issue has been resolved. Show all issues
```
"keyword"
Here and elsewhere.
```
reviewboard/webapi/resources/webhook.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Parents around the comparison.
```
reviewboard/webapi/resources/webhook.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Should have a trailing comma.
```
reviewboard/webapi/resources/webhook.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Here too.
```
reviewboard/webapi/resources/webhook.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Should have parens around the comparison.
```
reviewboard/webapi/resources/webhook.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Trailing comma.
```
reviewboard/webapi/resources/webhook.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Here too.
```

Change Summary:

Address Christian's issues. Add new unit tests.

Diff:

Revision 8 (+1030)

Show changes

	reviewboard/notifications/forms.py
	reviewboard/notifications/managers.py
	reviewboard/notifications/models.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/resources/webhook.py
	reviewboard/webapi/tests/mimetypes.py
	2 more

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/forms.py (Diff revision 8)
The issue has been resolved. Show all issues
```
 'ValidationError' imported but unused
```
reviewboard/webapi/resources/__init__.py (Diff revision 8)
The issue has been resolved. Show all issues
```
 'logging' imported but unused
```
reviewboard/webapi/tests/test_webhook.py (Diff revision 8)
The issue has been resolved. Show all issues
```
 'get_repository_item_url' imported but unused
```
reviewboard/webapi/tests/test_webhook.py (Diff revision 8)
The issue has been resolved. Show all issues
```
 local variable 'repository_pks' is assigned to but never used
```

Diff:

Revision 9 (+1023 -2)

Show changes

	reviewboard/notifications/forms.py
	reviewboard/notifications/managers.py
	reviewboard/notifications/models.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/resources/webhook.py
	reviewboard/webapi/tests/mimetypes.py
	2 more

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

Ship it!

reviewboard/notifications/forms.py (Diff revisions 7 - 9)

The issue has been resolved. Show all issues

These blank lines can be removed.

reviewboard/webapi/resources/webhook.py (Diff revisions 7 - 9)
The issue has been resolved. Show all issues
```
In this particular case, \ is preferable to ().
```

Status:: Completed
Change Summary:: Pushed to release-2.5.x (17891bf)