Add webhook management API

Review Request #7483 — Created July 3, 2015 and submitted

Information

Review Board
release-2.5.x
0db4571...

Reviewers

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

'Repository' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Missing a period.

chipx86chipx86

Missing a Returns block in the docstring.

chipx86chipx86

This error isn't going to be very obvious to the caller without access to the code. I'm also not sure …

chipx86chipx86

Should have a "Returns" section.

chipx86chipx86

Can you add a docstring to this?

chipx86chipx86

This will fail if the WebHookTarget isn't already saved, since there's no ID to work off of. We'll need to …

chipx86chipx86

The "\" shouldn't be there.

chipx86chipx86

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

chipx86chipx86

Let's make true a literal.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

You can use the multiple choice form in type: 'type': ('all', 'selected repositories', 'file attachments only') That said, I'm not …

chipx86chipx86

Can also use the constants form.

chipx86chipx86

Space before the ending quote.

chipx86chipx86

Shouldn't be a dict.

chipx86chipx86

Space before the ending quote.

chipx86chipx86

Wondering if we need this. Clients should probably be able to infer based on the custom_payload value instead, right? We …

chipx86chipx86

Space before the ending quote.

chipx86chipx86

I'd recommend we add an is_accessible_by() function to WebHookTarget that does these checks, and an equivalent query function in WebHookTarget's …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

These will appear in the generated API docs. We should flesh them out a bit. These methods are exempt from …

chipx86chipx86

Like above, this should use a choice tuple.

chipx86chipx86

Space before the ending quote.

chipx86chipx86

So should this.

chipx86chipx86

Space before the ending quote.

chipx86chipx86

Missing a period.

chipx86chipx86

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

chipx86chipx86

The """ is indented too far.

chipx86chipx86

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

chipx86chipx86

We can use the local_site passed to the handler instead.

chipx86chipx86

These are all indented too far.

chipx86chipx86

'LocalSite' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

extra_data is going to be set to whatever is returned. You can change this to: return self.cleaned_data['extra_data'] or {}

chipx86chipx86

I wouldn't use bool for this. Instead, I'd explicitly check that we have a valid value. Helps keep the logic …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

We should probably localize this and the errors below.

chipx86chipx86

Can you add Args and Returns here?

chipx86chipx86

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, …

chipx86chipx86

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

chipx86chipx86

You should only save here if modifying extra_data, since it's the only thing after the initial create + save that …

chipx86chipx86

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

chipx86chipx86

"attachment-only"

chipx86chipx86

Since consumers may not know what a "local site" is (and may wrongly assume it just means a local install), …

chipx86chipx86

This list, and the values within, are hard-coded in multiple places. We should have constants for each and a constant …

chipx86chipx86

Let's define an ALL_ENCODINGS list in WebHookTarget containing these (well, the constants for these) and reference that here, to avoid …

chipx86chipx86

This should be a list in the payload.

chipx86chipx86

Since the contents are going to be of a particular resource type, you can do: 'type': [RepositoryResource], Or, if that …

chipx86chipx86

No blank line here.

chipx86chipx86

We should have a manager method that returns accessible objects, given a user and a LocalSite, much like we do …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This function needs to check if the user has permission to create a WebHook. For repositories, we have a can_create …

chipx86chipx86

This can be: form_data['use_custom_content'] = (form_data.get('custom_content', '') != '')

chipx86chipx86

Same as above.

chipx86chipx86

Same as above.

chipx86chipx86

This should be consistent with above.

chipx86chipx86

These blank lines can be removed.

chipx86chipx86

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 …

chipx86chipx86

"keyword" Here and elsewhere.

chipx86chipx86

Parents around the comparison.

chipx86chipx86

Should have a trailing comma.

chipx86chipx86

Here too.

chipx86chipx86

Should have parens around the comparison.

chipx86chipx86

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

chipx86chipx86

Trailing comma.

chipx86chipx86

Here too.

chipx86chipx86

'ValidationError' imported but unused

reviewbotreviewbot

'logging' imported but unused

reviewbotreviewbot

'get_repository_item_url' imported but unused

reviewbotreviewbot

local variable 'repository_pks' is assigned to but never used

reviewbotreviewbot
reviewbot
  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
    
    
  2. reviewboard/notifications/forms.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W391 blank line at end of file
    
  3. reviewboard/webapi/resources/webhook.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. reviewboard/webapi/resources/webhook.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  5. Show all issues
     'Repository' imported but unused
    
  6. Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  7. Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  8. Show all issues
    Col: 1
     W391 blank line at end of file
    
  9. reviewboard/webapi/tests/urls.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  10. 
      
brennie
reviewbot
  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
    
    
  2. 
      
brennie
reviewbot
  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
    
    
  2. 
      
chipx86
  1. 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:

    1. We find a better way to do form updates, or we do the hard work of building an initial payload that we then update.
    2. We scrap the form method, and do what other resources do, which is create/update the WebHookTarget directly.
  2. reviewboard/notifications/forms.py (Diff revision 3)
     
     
    Show all issues

    Missing a period.

  3. reviewboard/notifications/forms.py (Diff revision 3)
     
     
     
     
    Show all issues

    Missing a Returns block in the docstring.

  4. reviewboard/notifications/forms.py (Diff revision 3)
     
     
    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.

  5. reviewboard/notifications/forms.py (Diff revision 3)
     
     
    Show all issues

    Should have a "Returns" section.

  6. reviewboard/testing/testcase.py (Diff revision 3)
     
     
    Show all issues

    Can you add a docstring to this?

  7. reviewboard/testing/testcase.py (Diff revision 3)
     
     
    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().

  8. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
    Show all issues

    The "\" shouldn't be there.

  9. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
     
     
     
     
    Show all issues

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

  10. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
    Show all issues

    Let's make true a literal.

  11. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
     
     
    Show all issues

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

  12. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
    Show all issues

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

  13. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
     
     
     
     
    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?

  14. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
     
     
     
    Show all issues

    Can also use the constants form.

  15. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
    Show all issues

    Space before the ending quote.

  16. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
     
    Show all issues

    Shouldn't be a dict.

  17. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
    Show all issues

    Space before the ending quote.

  18. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
    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.

  19. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
    Show all issues

    Space before the ending quote.

  20. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
     
     
     
    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)
    
  21. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
     
     
     
    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.

    1. There is no local_site parameter passed here.

  22. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
    Show all issues

    This must be last in the list of decorators.

    Same below.

  23. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
    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.

  24. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    Like above, this should use a choice tuple.

  25. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
    Show all issues

    Space before the ending quote.

  26. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    So should this.

  27. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
    Show all issues

    Space before the ending quote.

  28. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
    Show all issues

    Missing a period.

  29. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
     
     
     
     
    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.

  30. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
    Show all issues

    The """ is indented too far.

  31. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
    Show all issues

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

  32. reviewboard/webapi/resources/webhook.py (Diff revision 3)
     
     
     
    Show all issues

    We can use the local_site passed to the handler instead.

  33. reviewboard/webapi/tests/test_webhook.py (Diff revision 3)
     
     
     
     
     
    Show all issues

    These are all indented too far.

  34. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
brennie
reviewbot
  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
    
    
  2. reviewboard/webapi/resources/webhook.py (Diff revision 5)
     
     
    Show all issues
     'LocalSite' imported but unused
    
  3. reviewboard/webapi/resources/webhook.py (Diff revision 5)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. 
      
brennie
reviewbot
  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
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/notifications/forms.py (Diff revision 6)
     
     
     
     
     
    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 {}
    
    1. This is no longer required because the UpdateFormMixin ensures that extra_data is a dict before assigning.

    2. Actually, ignore that :)

  3. reviewboard/notifications/forms.py (Diff revision 6)
     
     
     
    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.

  4. reviewboard/notifications/forms.py (Diff revision 6)
     
     
     
    Show all issues

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

  5. reviewboard/notifications/forms.py (Diff revision 6)
     
     
    Show all issues

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

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

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

    3. I am a silly person.

  6. reviewboard/notifications/forms.py (Diff revision 6)
     
     
    Show all issues

    We should probably localize this and the errors below.

  7. reviewboard/notifications/models.py (Diff revision 6)
     
     
     
     
     
     
     
    Show all issues

    Can you add Args and Returns here?

  8. reviewboard/notifications/models.py (Diff revision 6)
     
     
     
     
    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.

  9. reviewboard/testing/testcase.py (Diff revision 6)
     
     
     
     
     
    Show all issues

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

  10. reviewboard/testing/testcase.py (Diff revision 6)
     
     
     
     
     
    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']).)

  11. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
    Show all issues

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

  12. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
    Show all issues

    "attachment-only"

  13. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
    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.

  14. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
    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.

  15. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
     
    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.

  16. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    This should be a list in the payload.

  17. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
    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'],
    
  18. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
     
     
    Show all issues

    No blank line here.

  19. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
     
     
     
     
    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.

  20. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
     
     
     
     
     
    Show all issues

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

  21. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
    Show all issues

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

  22. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
     
    Show all issues

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

  23. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
    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.

  24. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    This can be:

    form_data['use_custom_content'] = (form_data.get('custom_content', '') != '')
    
  25. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
    Show all issues

    Same as above.

  26. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
     
    Show all issues

    Same as above.

  27. reviewboard/webapi/resources/webhook.py (Diff revision 6)
     
     
     
    Show all issues

    This should be consistent with above.

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

    2. Ah, I understand. Makes sense.

  28. 
      
brennie
reviewbot
  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/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
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/notifications/managers.py (Diff revision 7)
     
     
     
     
     
    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).

  3. reviewboard/webapi/resources/webhook.py (Diff revision 7)
     
     
    Show all issues

    "keyword"

    Here and elsewhere.

  4. reviewboard/webapi/resources/webhook.py (Diff revision 7)
     
     
    Show all issues

    Parents around the comparison.

  5. reviewboard/webapi/resources/webhook.py (Diff revision 7)
     
     
    Show all issues

    Should have a trailing comma.

  6. reviewboard/webapi/resources/webhook.py (Diff revision 7)
     
     
    Show all issues

    Here too.

  7. reviewboard/webapi/resources/webhook.py (Diff revision 7)
     
     
    Show all issues

    Should have parens around the comparison.

  8. reviewboard/webapi/resources/webhook.py (Diff revision 7)
     
     
    Show all issues

    Trailing comma.

  9. reviewboard/webapi/resources/webhook.py (Diff revision 7)
     
     
    Show all issues

    Here too.

  10. 
      
brennie
reviewbot
  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/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
    
    
  2. reviewboard/notifications/forms.py (Diff revision 8)
     
     
    Show all issues
     'ValidationError' imported but unused
    
  3. Show all issues
     'logging' imported but unused
    
  4. Show all issues
     'get_repository_item_url' imported but unused
    
  5. Show all issues
     local variable 'repository_pks' is assigned to but never used
    
  6. 
      
brennie
reviewbot
  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/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
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/notifications/forms.py (Diff revisions 7 - 9)
     
     
     
     
     
     
     
     
    Show all issues

    These blank lines can be removed.

  3. reviewboard/webapi/resources/webhook.py (Diff revisions 7 - 9)
     
     
     
    Show all issues

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

  4. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.5.x (17891bf)