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

Col: 1
 W391 blank line at end of file

Col: 1
 E302 expected 2 blank lines, found 1

Col: 1
 W293 blank line contains whitespace

 'Repository' imported but unused

Col: 1
 E302 expected 2 blank lines, found 1

Col: 1
 E302 expected 2 blank lines, found 1

Col: 1
 W391 blank line at end of file

Col: 1
 E302 expected 2 blank lines, found 1

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

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.

Missing a period.

Missing a Returns block in the docstring.

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.

Should have a "Returns" section.

Can you add a docstring to this?

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().

The "\" shouldn't be there.

Put these in sentence casing, and remove the ; at the end. They should stand alone as their own list items.

Let's make true a literal.

Let's have these as their own list items, without appearing as part of a sentence.

"file-attachment-only" or maybe just "attachment-only"

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?

Can also use the constants form.

Space before the ending quote.

Shouldn't be a dict.

Space before the ending quote.

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.

Space before the ending quote.

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)

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.

This must be last in the list of decorators.
Same below.

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.

Like above, this should use a choice tuple.

Space before the ending quote.

So should this.

Space before the ending quote.

Missing a period.

Like above, I think it's cleaner if we automatically set this based on whether there's custom content set or cleared.

The """ is indented too far.

Let's use a regex of r',\s*'

We can use the local_site passed to the handler instead.

These are all indented too far.

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

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

 'LocalSite' imported but unused

Col: 1
 E302 expected 2 blank lines, found 1

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

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

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

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.

Rather than this wrapping, how about pulling out apply_to into a variable?

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

We should probably localize this and the errors below.

Can you add Args and Returns here?

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.

I know it's painful here, but can you add Args and Returns?

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']).)

We're no longer using "use_custom_content," so this should be updated.

"attachment-only"

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.

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.

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.

This should be a list in the payload.

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'],

No blank line here.

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.

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

We should store a pre-compiled regex for these on the class, and use it both here and below.

Same comment as above about using a pre-defined list.

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.

This can be:

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

Same as above.

Same as above.

This should be consistent with above.

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

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).

"keyword"
Here and elsewhere.

Parents around the comparison.

Should have a trailing comma.

Here too.

Should have parens around the comparison.

Trailing comma.

Here too.

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

 'ValidationError' imported but unused

 'logging' imported but unused

 'get_repository_item_url' imported but unused

 local variable 'repository_pks' is assigned to but never used

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

These blank lines can be removed.

In this particular case, \ is preferable to ().