Add integration model and manager for third-party services framework

Review Request #6918 — Created Feb. 7, 2015 and discarded

Information

Review Board
master

Reviewers

This provides the integration base class, database model and manager for extensions to implement and register integrations.

Compared to the extension framework, these integration model will provide a way for users, such as local site administrator, to manage their own individiual integration. Third-party services will need to subclass the integration base class to provide the initialize and shutdown method for their services.

With the database model, multiple instances of the integration's configuration could be supported, and thus providing multiple configurations of the integration for a single web server.

All these configured integration will then be managed through the integration manager.

Added unit test for the integrations.

  • Test manager on creating, deleting, toggling of configured integration.
  • Test manager on handling of invalid configured integration.

Mannual testing with a dummy integration.

Unit tests pass.

Description From Last Updated

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

"Allows extensions to register and unregister service integrations."

chipx86chipx86

Two blank lines are needed here.

chipx86chipx86

Must be inserted in alphabetical order.

chipx86chipx86

When the imports can fit on one line, they should be on one line without parens. However, instead of importing …

chipx86chipx86

"registration"

chipx86chipx86

"unregistration"

chipx86chipx86

We don't want to introduce any new fixtures into the codebase. Instead, create the objects as you need them. See …

chipx86chipx86

Each of these should be documented, using this form: #: Single line comment myvar1 = ... #: Single-line summary #: …

chipx86chipx86

Blank line between these.

chipx86chipx86

Why is there a separate initialize method? Why can't subclass initialization be done in __init__?

brenniebrennie

"URL"

chipx86chipx86

Two blank lines between these.

chipx86chipx86

No blank line here.

brenniebrennie

"Return" instead of "Get".

chipx86chipx86

"Returns" instead of "Retrieves", "ID" instead of "id", and a period is needed.

chipx86chipx86

No blank line here.

chipx86chipx86

Blank line after class docstrings.

chipx86chipx86

Private functions should always come after all the public functions.

chipx86chipx86

This can be combined into one statement.

chipx86chipx86

Should use pk instead of id for model instances.

chipx86chipx86

How about just: "This configuration is already registered."

chipx86chipx86

"ID"

chipx86chipx86

This needs to not break loading the rest of the integrations. If an extension has been disabled, the ConfiguredIntegrations will …

chipx86chipx86

Missing a period.

chipx86chipx86

This would be better as: try: del self._config_instances[config_instance.pl] except KeyError: raise ... Also, note pk instead of id for model …

chipx86chipx86

Missing a trailing period.

chipx86chipx86

Trailing period.

chipx86chipx86

Change the get to a filter, to save an SQL query. This way, we don't have to fetch the entire …

chipx86chipx86

Trailing period.

chipx86chipx86

Trailing period.

chipx86chipx86

Trailing period.

chipx86chipx86

Trailing period.

chipx86chipx86

Trailing period.

chipx86chipx86

Trailing period.

chipx86chipx86

Should use six.itervalues(...) instead of calling values() on the dictionary.

chipx86chipx86

Trailing period.

chipx86chipx86

Needs a docstring.

chipx86chipx86

Must be in alphabetical order.

chipx86chipx86

Needs a docstring.

chipx86chipx86

Needs a docstring. This should use @cached_property instead of doing the hasattr trick. Search the codebase for the proper import …

chipx86chipx86

"has" access.

chipx86chipx86

Must be a single line. How about: "Return whether the user can modify this configuration."

chipx86chipx86

This needs to return something.

chipx86chipx86

Must be in alphabetical order.

chipx86chipx86

Trailing period.

chipx86chipx86

Multi-line unit test docstrings must be in this form: """First line second line. """

chipx86chipx86

No blank line here.

chipx86chipx86

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

Col: 13 E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'reviewboard' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'reviewboard' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'reviewboard' imported but unused

reviewbotreviewbot

This shouldn't be set here. Rather, Integrations should accept an extension in the constructor, but only if that specific integration …

chipx86chipx86

Each comment before an attribute should be in this form: #: Summary #: #: ... Note the #:. That will …

chipx86chipx86

Should be allow_local_sites.

chipx86chipx86

How about config_form?

chipx86chipx86

Instead of using join, we should just use a format string.

chipx86chipx86

"integration"

chipx86chipx86

"toggled"

chipx86chipx86

"Subclasses" ".. to shut down the integration and ..."

chipx86chipx86

"... to be toggled ..."

chipx86chipx86

No need for the None parameter. That's the default.

chipx86chipx86

I don't think we should have a load method, and we certainly shouldn't call it when initializing Review Board. It …

chipx86chipx86

Combine these conditionals.

chipx86chipx86

This is pretty redundant. We should have one or the other.

chipx86chipx86

"integration"

chipx86chipx86

This can be one statement.

chipx86chipx86

Swap these (alphabetical order).

chipx86chipx86

""" on the next line.

chipx86chipx86

Here too. Same below.

chipx86chipx86

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

"Create"

chipx86chipx86

'reviewboard' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'get_integration_manager' imported but unused

reviewbotreviewbot

'reviewboard' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

Imports should come in alphabetical order.

daviddavid

Should be in the imperative mood ("Wrap a callback ...")

daviddavid

exc_info should be True, rather than 1 (we have 1 a bunch of other places, but I'm slowly cleaning those …

daviddavid

This should use the same terminology as you changed above ("registration" rather than "initializing")

daviddavid

This should use the same terminology as you changed above ("registration" rather than "initializing")

daviddavid

Should be in the imperative mood ("Shut down...")

daviddavid

Should be in the imperative mood ("Return the...")

daviddavid

Should be in the imperative mood ("Return the...")

daviddavid

Should be in the imperative mood ("Return the...")

daviddavid

The logging methods will do a format operation internally, so you can pass in config_id as a parameter rather than …

daviddavid

Should be in the imperative mood ("Return all...")

daviddavid

Should be in the imperative mood ("Return the...")

daviddavid

Should be in the imperative mood ("Return the...")

daviddavid

Should be in the imperative mood ("Return...")

daviddavid

Should be in the imperative mood ("Return...")

daviddavid

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/integrations/models.py
        reviewboard/integrations/integration.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/manager.py
        reviewboard/settings.py
    
    Ignored Files:
        reviewboard/integrations/tests.py
        reviewboard/integrations/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/integrations/models.py
        reviewboard/integrations/integration.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/manager.py
        reviewboard/settings.py
    
    Ignored Files:
        reviewboard/integrations/tests.py
        reviewboard/integrations/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 1)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 1)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 1)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 1)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
XU
XU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/models.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/fixtures/test_configs.json
        reviewboard/integrations/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/models.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/fixtures/test_configs.json
        reviewboard/integrations/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 2)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 2)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 2)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 2)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
XU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/models.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/fixtures/test_configs.json
        reviewboard/integrations/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/models.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/fixtures/test_configs.json
        reviewboard/integrations/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 3)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 3)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 3)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 3)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
XU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/models.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/fixtures/test_configs.json
        reviewboard/integrations/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/models.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/fixtures/test_configs.json
        reviewboard/integrations/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 4)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 4)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 4)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 4)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
chipx86
  1. 
      
  2. reviewboard/extensions/hooks.py (Diff revision 4)
     
     

    "Allows extensions to register and unregister service integrations."

  3. reviewboard/extensions/hooks.py (Diff revision 4)
     
     
     
     

    Two blank lines are needed here.

  4. reviewboard/extensions/tests.py (Diff revision 4)
     
     

    Must be inserted in alphabetical order.

  5. reviewboard/extensions/tests.py (Diff revision 4)
     
     
     

    When the imports can fit on one line, they should be on one line without parens.

    However, instead of importing _integrations, we should import a function that can return an integration with the given ID.

  6. reviewboard/extensions/tests.py (Diff revision 4)
     
     

    "registration"

  7. reviewboard/extensions/tests.py (Diff revision 4)
     
     

    "unregistration"

  8. reviewboard/integrations/fixtures/test_configs.json (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    We don't want to introduce any new fixtures into the codebase. Instead, create the objects as you need them.

    See reviewboard/testing/testcase.py.

  9. reviewboard/integrations/integration.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     

    Each of these should be documented, using this form:

    #: Single line comment
    myvar1 = ...
    
    #: Single-line summary
    #:
    #: Description that spans
    #: multiple lines.
    myvar2 = ...
    
  10. reviewboard/integrations/integration.py (Diff revision 4)
     
     
     

    Blank line between these.

  11. reviewboard/integrations/integration.py (Diff revision 4)
     
     
     
     

    Two blank lines between these.

  12. reviewboard/integrations/integration.py (Diff revision 4)
     
     

    "Return" instead of "Get".

  13. reviewboard/integrations/integration.py (Diff revision 4)
     
     

    "Returns" instead of "Retrieves", "ID" instead of "id", and a period is needed.

  14. reviewboard/integrations/manager.py (Diff revision 4)
     
     
     
     

    No blank line here.

  15. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Blank line after class docstrings.

  16. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Private functions should always come after all the public functions.

  17. reviewboard/integrations/manager.py (Diff revision 4)
     
     
     
     

    This can be combined into one statement.

  18. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Should use pk instead of id for model instances.

  19. reviewboard/integrations/manager.py (Diff revision 4)
     
     
     

    How about just: "This configuration is already registered."

  20. reviewboard/integrations/manager.py (Diff revision 4)
     
     
     
     
     
     

    This needs to not break loading the rest of the integrations. If an extension has been disabled, the ConfiguredIntegrations will still be in the database, which is completely valid.

    What I'd suggest is not erroring out if the integration is not registered. Instead, silently skip it.

  21. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Missing a period.

  22. reviewboard/integrations/manager.py (Diff revision 4)
     
     
     
     
     
     
     

    This would be better as:

    try:
        del self._config_instances[config_instance.pl]
    except KeyError:
        raise ...
    

    Also, note pk instead of id for model instances.

    I'd change the test like I suggested above.

  23. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Missing a trailing period.

  24. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Trailing period.

  25. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Change the get to a filter, to save an SQL query. This way, we don't have to fetch the entire object before deleting it.

  26. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Trailing period.

  27. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Trailing period.

  28. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Trailing period.

  29. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Trailing period.

  30. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Trailing period.

  31. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Trailing period.

  32. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Should use six.itervalues(...) instead of calling values() on the dictionary.

  33. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Trailing period.

  34. reviewboard/integrations/manager.py (Diff revision 4)
     
     

    Needs a docstring.

  35. reviewboard/integrations/models.py (Diff revision 4)
     
     
     

    Must be in alphabetical order.

  36. reviewboard/integrations/models.py (Diff revision 4)
     
     

    Needs a docstring.

  37. reviewboard/integrations/models.py (Diff revision 4)
     
     

    Needs a docstring.

    This should use @cached_property instead of doing the hasattr trick. Search the codebase for the proper import statement.

  38. reviewboard/integrations/models.py (Diff revision 4)
     
     

    "has" access.

  39. reviewboard/integrations/models.py (Diff revision 4)
     
     
     

    Must be a single line. How about:

    "Return whether the user can modify this configuration."

  40. reviewboard/integrations/models.py (Diff revision 4)
     
     

    This needs to return something.

    1. Hi Christian,

      Thank you for putting in so much effort in reviewing my code. I really appreciate it.

      With regards to this issue, I am still confuse about how I should go about setting the permission. For the other models which dealt with LocalSite, the is_mutable_by method will be returning the permission base on _VALID_LOCAL_SITE_PERMISSIONS in backend.py. However, since integration may not solely deal with LocalSite, I was wondering whether I could make use of this to achieve the same purpose. Also, since the views depends on these permission settings, I was wondering how it will affect the admin access control in managing the integrations.

  41. reviewboard/integrations/tests.py (Diff revision 4)
     
     
     
     
     
     

    Must be in alphabetical order.

  42. reviewboard/integrations/tests.py (Diff revision 4)
     
     

    Trailing period.

  43. reviewboard/integrations/tests.py (Diff revision 4)
     
     
     
     

    Multi-line unit test docstrings must be in this form:

    """First line second line. """

  44. reviewboard/integrations/tests.py (Diff revision 4)
     
     
     
     

    No blank line here.

  45. 
      
brennie
  1. 
      
  2. reviewboard/integrations/integration.py (Diff revision 4)
     
     
     
     
     
     
     

    Why is there a separate initialize method? Why can't subclass initialization be done in __init__?

    1. Hi Barret,

      Thank you so much for reviewing my code!

      Regarding this issue, I have abstract out the intialize component because of the following two reasons:
      1) In the case of toggling, the integration will have to be re-initialize after shutting down. With the initialize method, the toggle method could call it to re-initialize the integration.
      2) The alternative will be to recreate the object after it has been shutdown. However, as each configured integration is tied to an integration instance, I am wondering whether it will be easier to reuse that instance to restart the service instead of recreating a new instance.

      Sorry about the confusion I have over this part. I will make appropriate change to it if it is not right.

    2. OK! This seems like a good approach to me. However, this information would be really good for the docstring so that others know that initialize/shutdown is for the toggling of the instances after they have been created.

  3. reviewboard/integrations/integration.py (Diff revision 4)
     
     

    No blank line here.

  4. 
      
XU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/models.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/models.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
  2. reviewboard/integrations/tests.py (Diff revision 5)
     
     
    Col: 13
     E123 closing bracket does not match indentation of opening bracket's line
    
  3. reviewboard/settings.py (Diff revision 5)
     
     
     'django_reset' imported but unused
    
  4. reviewboard/settings.py (Diff revision 5)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  5. reviewboard/settings.py (Diff revision 5)
     
     
     'PIPELINE_JS' imported but unused
    
  6. reviewboard/settings.py (Diff revision 5)
     
     
     'PIPELINE_CSS' imported but unused
    
  7. 
      
XU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/models.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/models.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 6)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 6)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 6)
     
     
     'PIPELINE_JS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 6)
     
     
     'PIPELINE_CSS' imported but unused
    
  6. 
      
XU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/__init__.py
        reviewboard/accounts/backends.py
        reviewboard/integrations/models.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/__init__.py
        reviewboard/accounts/backends.py
        reviewboard/integrations/models.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
  2. reviewboard/__init__.py (Diff revision 7)
     
     
     'reviewboard' imported but unused
    
  3. reviewboard/integrations/manager.py (Diff revision 7)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. reviewboard/integrations/manager.py (Diff revision 7)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  5. reviewboard/settings.py (Diff revision 7)
     
     
     'django_reset' imported but unused
    
  6. reviewboard/settings.py (Diff revision 7)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  7. reviewboard/settings.py (Diff revision 7)
     
     
     'PIPELINE_CSS' imported but unused
    
  8. reviewboard/settings.py (Diff revision 7)
     
     
     'PIPELINE_JS' imported but unused
    
  9. 
      
XU
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/__init__.py
        reviewboard/accounts/backends.py
        reviewboard/integrations/models.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/__init__.py
        reviewboard/accounts/backends.py
        reviewboard/integrations/models.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
  2. reviewboard/__init__.py (Diff revision 8)
     
     
     'reviewboard' imported but unused
    
  3. reviewboard/settings.py (Diff revision 8)
     
     
     'django_reset' imported but unused
    
  4. reviewboard/settings.py (Diff revision 8)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  5. reviewboard/settings.py (Diff revision 8)
     
     
     'PIPELINE_CSS' imported but unused
    
  6. reviewboard/settings.py (Diff revision 8)
     
     
     'PIPELINE_JS' imported but unused
    
  7. 
      
XU
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/__init__.py
        reviewboard/accounts/backends.py
        reviewboard/integrations/models.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/__init__.py
        reviewboard/accounts/backends.py
        reviewboard/integrations/models.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
  2. reviewboard/__init__.py (Diff revision 9)
     
     
     'reviewboard' imported but unused
    
  3. reviewboard/settings.py (Diff revision 9)
     
     
     'django_reset' imported but unused
    
  4. reviewboard/settings.py (Diff revision 9)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  5. reviewboard/settings.py (Diff revision 9)
     
     
     'PIPELINE_JS' imported but unused
    
  6. reviewboard/settings.py (Diff revision 9)
     
     
     'PIPELINE_CSS' imported but unused
    
  7. 
      
chipx86
  1. General note: The Description is the wrong place for the updates. Those belong in the draft banner at the top when posting a new change. The Description should be a commit-ready, full description of this change, useful for anyone to understand what this commit provides without knowing the rest of the work going on.

  2. 
      
chipx86
  1. 
      
  2. reviewboard/extensions/hooks.py (Diff revision 9)
     
     
     

    This shouldn't be set here. Rather, Integrations should accept an extension in the constructor, but only if that specific integration needs to.

    1. I was making use of the extension's staticfiles finder to locate files(e.g. icon image) for the integration, which requires the "extension_id" to locate the file properly. However, the only place which the integration could know about the extension is through the initilization of the hook or in the initialize method of the extension. What will be a good way to resolve this dependency?

  3. reviewboard/integrations/integration.py (Diff revision 9)
     
     
     
     
     
     

    Each comment before an attribute should be in this form:

    #: Summary
    #:
    #: ...
    

    Note the #:. That will let Sphinx (our docs generator) use the comments as documentation.

  4. reviewboard/integrations/integration.py (Diff revision 9)
     
     

    Should be allow_local_sites.

  5. reviewboard/integrations/integration.py (Diff revision 9)
     
     

    How about config_form?

  6. reviewboard/integrations/integration.py (Diff revision 9)
     
     
     

    Instead of using join, we should just use a format string.

  7. reviewboard/integrations/integration.py (Diff revision 9)
     
     

    "integration"

  8. reviewboard/integrations/integration.py (Diff revision 9)
     
     

    "toggled"

  9. reviewboard/integrations/integration.py (Diff revision 9)
     
     

    "Subclasses"

    ".. to shut down the integration and ..."

  10. reviewboard/integrations/integration.py (Diff revision 9)
     
     

    "... to be toggled ..."

  11. reviewboard/integrations/integration.py (Diff revision 9)
     
     

    No need for the None parameter. That's the default.

  12. reviewboard/integrations/manager.py (Diff revision 9)
     
     
     

    I don't think we should have a load method, and we certainly shouldn't call it when initializing Review Board. It means an extra database query and setup work every time initialize is called, which happens often enough.

    Instead, how about fetching integrations the first time they're needed for an Integration. That way, we don't have a database query unconditionally for all installs, but rather, only if there are integrations configured.

    1. I could make the integration to be fetch the first time they are needed for the initialization. However, currently the configured integration only initialize their services after it has been retrieve for the first time. What will then be a good way to ensure that all the configured integration's services are initialize when the server is starting up?

  13. reviewboard/integrations/manager.py (Diff revision 9)
     
     
     

    Combine these conditionals.

  14. reviewboard/integrations/manager.py (Diff revision 9)
     
     
     
     
     
     
     
     

    This is pretty redundant. We should have one or the other.

  15. reviewboard/integrations/models.py (Diff revision 9)
     
     

    "integration"

  16. reviewboard/integrations/models.py (Diff revision 9)
     
     
     
     
     

    This can be one statement.

    1. Most of the reviewboard models which requires local_site used this instead of a single statement. Should I still keep this to be consistent with them?

  17. reviewboard/integrations/tests.py (Diff revision 9)
     
     
     

    Swap these (alphabetical order).

  18. reviewboard/integrations/tests.py (Diff revision 9)
     
     

    """ on the next line.

  19. reviewboard/integrations/tests.py (Diff revision 9)
     
     

    Here too.

    Same below.

  20. reviewboard/testing/testcase.py (Diff revision 9)
     
     

    "Create"

  21. 
      
XU
XU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/__init__.py
        reviewboard/accounts/backends.py
        reviewboard/integrations/models.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
  2. reviewboard/__init__.py (Diff revision 10)
     
     
     'reviewboard' imported but unused
    
  3. reviewboard/settings.py (Diff revision 10)
     
     
     'django_reset' imported but unused
    
  4. reviewboard/settings.py (Diff revision 10)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  5. reviewboard/settings.py (Diff revision 10)
     
     
     'PIPELINE_JS' imported but unused
    
  6. reviewboard/settings.py (Diff revision 10)
     
     
     'PIPELINE_CSS' imported but unused
    
  7. 
      
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/__init__.py
        reviewboard/accounts/backends.py
        reviewboard/integrations/models.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
  2. 
      
XU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/__init__.py
        reviewboard/accounts/backends.py
        reviewboard/integrations/models.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/__init__.py
        reviewboard/accounts/backends.py
        reviewboard/integrations/models.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
  2. reviewboard/__init__.py (Diff revision 11)
     
     
     'get_integration_manager' imported but unused
    
  3. reviewboard/__init__.py (Diff revision 11)
     
     
     'reviewboard' imported but unused
    
  4. reviewboard/settings.py (Diff revision 11)
     
     
     'django_reset' imported but unused
    
  5. reviewboard/settings.py (Diff revision 11)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  6. reviewboard/settings.py (Diff revision 11)
     
     
     'PIPELINE_CSS' imported but unused
    
  7. reviewboard/settings.py (Diff revision 11)
     
     
     'PIPELINE_JS' imported but unused
    
  8. 
      
XU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/backends.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/models.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/backends.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/models.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 12)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 12)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 12)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 12)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
XU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/backends.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/models.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/settings.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/backends.py
        reviewboard/extensions/hooks.py
        reviewboard/integrations/models.py
        reviewboard/integrations/manager.py
        reviewboard/integrations/tests.py
        reviewboard/integrations/integration.py
    
    Ignored Files:
        reviewboard/integrations/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 13)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 13)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 13)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 13)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
david
  1. 
      
  2. reviewboard/extensions/hooks.py (Diff revision 13)
     
     
     

    Imports should come in alphabetical order.

  3. reviewboard/extensions/hooks.py (Diff revision 13)
     
     

    Should be in the imperative mood ("Wrap a callback ...")

  4. reviewboard/extensions/hooks.py (Diff revision 13)
     
     

    exc_info should be True, rather than 1 (we have 1 a bunch of other places, but I'm slowly cleaning those up).

  5. reviewboard/extensions/tests.py (Diff revision 13)
     
     

    This should use the same terminology as you changed above ("registration" rather than "initializing")

  6. reviewboard/extensions/tests.py (Diff revision 13)
     
     

    This should use the same terminology as you changed above ("registration" rather than "initializing")

  7. reviewboard/integrations/integration.py (Diff revision 13)
     
     

    Should be in the imperative mood ("Shut down...")

  8. reviewboard/integrations/integration.py (Diff revision 13)
     
     

    Should be in the imperative mood ("Return the...")

  9. reviewboard/integrations/integration.py (Diff revision 13)
     
     

    Should be in the imperative mood ("Return the...")

  10. reviewboard/integrations/integration.py (Diff revision 13)
     
     

    Should be in the imperative mood ("Return the...")

  11. reviewboard/integrations/manager.py (Diff revision 13)
     
     

    The logging methods will do a format operation internally, so you can pass in config_id as a parameter rather than formatting it yourself.

  12. reviewboard/integrations/manager.py (Diff revision 13)
     
     

    Should be in the imperative mood ("Return all...")

  13. reviewboard/integrations/manager.py (Diff revision 13)
     
     

    Should be in the imperative mood ("Return the...")

  14. reviewboard/integrations/manager.py (Diff revision 13)
     
     

    Should be in the imperative mood ("Return the...")

  15. reviewboard/integrations/models.py (Diff revision 13)
     
     

    Should be in the imperative mood ("Return...")

  16. reviewboard/integrations/models.py (Diff revision 13)
     
     

    Should be in the imperative mood ("Return...")

  17. 
      
XU
Review request changed

Status: Discarded

Change Summary:

Closed in favor of a revised version of Integrations that landed for 3.0.

Loading...