Add integration model and manager for third-party services framework

Review Request #6918 - Created Feb. 8, 2015 and discarded

Xuanyi Lin
Review Board
master
7221, 7105, 7136, 7106, 7034, 7033
reviewboard, students

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.

  • 15
  • 0
  • 74
  • 53
  • 142
Description From Last Updated
Imports should come in alphabetical order. David Trowbridge David Trowbridge
Should be in the imperative mood ("Wrap a callback ...") David Trowbridge David Trowbridge
exc_info should be True, rather than 1 (we have 1 a bunch of other places, but I'm slowly cleaning those ... David Trowbridge David Trowbridge
This should use the same terminology as you changed above ("registration" rather than "initializing") David Trowbridge David Trowbridge
This should use the same terminology as you changed above ("registration" rather than "initializing") David Trowbridge David Trowbridge
Should be in the imperative mood ("Shut down...") David Trowbridge David Trowbridge
Should be in the imperative mood ("Return the...") David Trowbridge David Trowbridge
Should be in the imperative mood ("Return the...") David Trowbridge David Trowbridge
Should be in the imperative mood ("Return the...") David Trowbridge David Trowbridge
The logging methods will do a format operation internally, so you can pass in config_id as a parameter rather than ... David Trowbridge David Trowbridge
Should be in the imperative mood ("Return all...") David Trowbridge David Trowbridge
Should be in the imperative mood ("Return the...") David Trowbridge David Trowbridge
Should be in the imperative mood ("Return the...") David Trowbridge David Trowbridge
Should be in the imperative mood ("Return...") David Trowbridge David Trowbridge
Should be in the imperative mood ("Return...") David Trowbridge David Trowbridge
Review Bot
  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. 
      
Xuanyi Lin
Xuanyi Lin
Review Bot
  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. 
      
Xuanyi Lin
Review Bot
  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. 
      
Xuanyi Lin
Review Bot
  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. 
      
Christian Hammond
  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. 
      
Barret Rennie
  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. 
      
Xuanyi Lin
Review Bot
  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. 
      
Xuanyi Lin
Review Bot
  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. 
      
Xuanyi Lin
Review Bot
  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. 
      
Xuanyi Lin
Review Bot
  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. 
      
Xuanyi Lin
Review Bot
  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. 
      
Christian Hammond
  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. 
      
Christian Hammond
  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. 
      
Xuanyi Lin
Xuanyi Lin
Review Bot
  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. 
      
Review Bot
  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. 
      
Xuanyi Lin
Review Bot
  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. 
      
Xuanyi Lin
Review Bot
  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. 
      
Xuanyi Lin
Review Bot
  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 Trowbridge
  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. 
      
Xuanyi Lin
Review request changed

Status: Discarded

Change Summary:

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

Loading...