Summary:

Add integration model and manager for third-party services framework

Review Request #6918 — Created Feb. 7, 2015 and discarded April 28, 2016, 3:21 a.m.

Information
Owner:	xuanyi
Repository:	Review Board
Branch:	master
Bugs:
Depends On:
Blocks:	7221, 7105, 7136, 7106, 7034, 7033
Reviewers
Groups:	reviewboard, students
People:

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
'django_reset' imported but unused	reviewbot	Feb. 7, 2015, 10:17 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Feb. 7, 2015, 10:17 p.m.
'PIPELINE_CSS' imported but unused	reviewbot	Feb. 7, 2015, 10:17 p.m.
'PIPELINE_JS' imported but unused	reviewbot	Feb. 7, 2015, 10:17 p.m.
'django_reset' imported but unused	reviewbot	Feb. 14, 2015, 9:31 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Feb. 14, 2015, 9:31 p.m.
'PIPELINE_CSS' imported but unused	reviewbot	Feb. 14, 2015, 9:31 p.m.
'PIPELINE_JS' imported but unused	reviewbot	Feb. 14, 2015, 9:31 p.m.
'django_reset' imported but unused	reviewbot	Feb. 14, 2015, 9:37 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Feb. 14, 2015, 9:37 p.m.
'PIPELINE_CSS' imported but unused	reviewbot	Feb. 14, 2015, 9:37 p.m.
'PIPELINE_JS' imported but unused	reviewbot	Feb. 14, 2015, 9:37 p.m.
"Allows extensions to register and unregister service integrations."	chipx86	March 13, 2015, 8:50 a.m.
Two blank lines are needed here.	chipx86	March 13, 2015, 8:50 a.m.
Must be inserted in alphabetical order.	chipx86	March 13, 2015, 8:50 a.m.
When the imports can fit on one line, they should be on one line without parens. However, instead of importing …	chipx86	March 13, 2015, 8:50 a.m.
"registration"	chipx86	March 13, 2015, 8:50 a.m.
"unregistration"	chipx86	March 13, 2015, 8:50 a.m.
We don't want to introduce any new fixtures into the codebase. Instead, create the objects as you need them. See …	chipx86	March 13, 2015, 8:50 a.m.
Each of these should be documented, using this form: #: Single line comment myvar1 = ... #: Single-line summary #: …	chipx86	March 13, 2015, 8:50 a.m.
Blank line between these.	chipx86	March 13, 2015, 8:50 a.m.
Why is there a separate initialize method? Why can't subclass initialization be done in __init__?	brennie	March 21, 2015, 10:49 a.m.
"URL"	chipx86	March 13, 2015, 8:50 a.m.
Two blank lines between these.	chipx86	March 13, 2015, 7:22 a.m.
No blank line here.	brennie	March 13, 2015, 8:53 a.m.
"Return" instead of "Get".	chipx86	March 13, 2015, 8:50 a.m.
"Returns" instead of "Retrieves", "ID" instead of "id", and a period is needed.	chipx86	March 13, 2015, 8:50 a.m.
No blank line here.	chipx86	March 13, 2015, 8:51 a.m.
Blank line after class docstrings.	chipx86	March 13, 2015, 8:51 a.m.
Private functions should always come after all the public functions.	chipx86	March 13, 2015, 8:51 a.m.
This can be combined into one statement.	chipx86	March 13, 2015, 8:51 a.m.
Should use pk instead of id for model instances.	chipx86	March 13, 2015, 8:51 a.m.
How about just: "This configuration is already registered."	chipx86	March 13, 2015, 8:51 a.m.
"ID"	chipx86	March 13, 2015, 8:52 a.m.
This needs to not break loading the rest of the integrations. If an extension has been disabled, the ConfiguredIntegrations will …	chipx86	March 13, 2015, 8:52 a.m.
Missing a period.	chipx86	March 13, 2015, 8:52 a.m.
This would be better as: try: del self._config_instances[config_instance.pl] except KeyError: raise ... Also, note pk instead of id for model …	chipx86	March 13, 2015, 8:52 a.m.
Missing a trailing period.	chipx86	March 13, 2015, 8:52 a.m.
Trailing period.	chipx86	March 13, 2015, 8:52 a.m.
Change the get to a filter, to save an SQL query. This way, we don't have to fetch the entire …	chipx86	March 13, 2015, 8:52 a.m.
Trailing period.	chipx86	March 13, 2015, 8:52 a.m.
Trailing period.	chipx86	March 13, 2015, 8:52 a.m.
Trailing period.	chipx86	March 13, 2015, 8:52 a.m.
Trailing period.	chipx86	March 13, 2015, 8:52 a.m.
Trailing period.	chipx86	March 13, 2015, 8:52 a.m.
Trailing period.	chipx86	March 13, 2015, 8:52 a.m.
Should use six.itervalues(...) instead of calling values() on the dictionary.	chipx86	March 13, 2015, 8:52 a.m.
Trailing period.	chipx86	March 13, 2015, 8:52 a.m.
Needs a docstring.	chipx86	March 13, 2015, 8:52 a.m.
Must be in alphabetical order.	chipx86	March 13, 2015, 8:52 a.m.
Needs a docstring.	chipx86	March 13, 2015, 8:52 a.m.
Needs a docstring. This should use @cached_property instead of doing the hasattr trick. Search the codebase for the proper import …	chipx86	March 13, 2015, 8:53 a.m.
"has" access.	chipx86	March 13, 2015, 8:53 a.m.
Must be a single line. How about: "Return whether the user can modify this configuration."	chipx86	March 13, 2015, 8:53 a.m.
This needs to return something.	chipx86	March 21, 2015, 10:49 a.m.
Must be in alphabetical order.	chipx86	March 13, 2015, 8:53 a.m.
Trailing period.	chipx86	March 13, 2015, 8:53 a.m.
Multi-line unit test docstrings must be in this form: """First line second line. """	chipx86	March 13, 2015, 8:53 a.m.
No blank line here.	chipx86	March 13, 2015, 8:53 a.m.
'django_reset' imported but unused	reviewbot	March 2, 2015, 9:09 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	March 2, 2015, 9:09 p.m.
'PIPELINE_CSS' imported but unused	reviewbot	March 2, 2015, 9:09 p.m.
'PIPELINE_JS' imported but unused	reviewbot	March 5, 2015, 1:20 a.m.
Col: 13 E123 closing bracket does not match indentation of opening bracket's line	reviewbot	March 13, 2015, 9:05 a.m.
'django_reset' imported but unused	reviewbot	March 13, 2015, 9:05 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	March 13, 2015, 9:05 a.m.
'PIPELINE_JS' imported but unused	reviewbot	March 13, 2015, 9:05 a.m.
'PIPELINE_CSS' imported but unused	reviewbot	March 13, 2015, 9:05 a.m.
'django_reset' imported but unused	reviewbot	March 13, 2015, 9:05 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	March 13, 2015, 9:05 a.m.
'PIPELINE_JS' imported but unused	reviewbot	March 13, 2015, 9:05 a.m.
'PIPELINE_CSS' imported but unused	reviewbot	March 13, 2015, 9:05 a.m.
'reviewboard' imported but unused	reviewbot	March 21, 2015, 9:46 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	March 21, 2015, 9:46 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	March 21, 2015, 9:46 p.m.
'django_reset' imported but unused	reviewbot	March 21, 2015, 9:46 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	March 21, 2015, 9:46 p.m.
'PIPELINE_CSS' imported but unused	reviewbot	March 21, 2015, 9:46 p.m.
'PIPELINE_JS' imported but unused	reviewbot	March 21, 2015, 9:46 p.m.
'reviewboard' imported but unused	reviewbot	March 21, 2015, 9:46 p.m.
'django_reset' imported but unused	reviewbot	March 21, 2015, 9:46 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	March 21, 2015, 9:46 p.m.
'PIPELINE_CSS' imported but unused	reviewbot	March 21, 2015, 9:46 p.m.
'PIPELINE_JS' imported but unused	reviewbot	March 21, 2015, 9:46 p.m.
'reviewboard' imported but unused	reviewbot	March 29, 2015, 3:32 a.m.
This shouldn't be set here. Rather, Integrations should accept an extension in the constructor, but only if that specific integration …	chipx86	April 25, 2015, 10:52 p.m.
Each comment before an attribute should be in this form: #: Summary #: #: ... Note the #:. That will …	chipx86	April 5, 2015, 1:55 a.m.
Should be allow_local_sites.	chipx86	April 5, 2015, 1:55 a.m.
How about config_form?	chipx86	April 5, 2015, 1:55 a.m.
Instead of using join, we should just use a format string.	chipx86	April 5, 2015, 1:56 a.m.
"integration"	chipx86	April 5, 2015, 1:56 a.m.
"toggled"	chipx86	April 5, 2015, 1:56 a.m.
"Subclasses" ".. to shut down the integration and ..."	chipx86	April 5, 2015, 1:56 a.m.
"... to be toggled ..."	chipx86	April 5, 2015, 1:56 a.m.
No need for the None parameter. That's the default.	chipx86	April 5, 2015, 1:56 a.m.
I don't think we should have a load method, and we certainly shouldn't call it when initializing Review Board. It …	chipx86	April 25, 2015, 10:52 p.m.
Combine these conditionals.	chipx86	April 5, 2015, 1:56 a.m.
This is pretty redundant. We should have one or the other.	chipx86	April 5, 2015, 1:56 a.m.
"integration"	chipx86	April 5, 2015, 1:56 a.m.
This can be one statement.	chipx86	April 14, 2015, 7:33 p.m.
Swap these (alphabetical order).	chipx86	April 5, 2015, 1:56 a.m.
""" on the next line.	chipx86	April 5, 2015, 1:56 a.m.
Here too. Same below.	chipx86	April 5, 2015, 1:56 a.m.
'django_reset' imported but unused	reviewbot	March 29, 2015, 3:32 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	March 29, 2015, 3:32 a.m.
'PIPELINE_JS' imported but unused	reviewbot	March 29, 2015, 3:32 a.m.
'PIPELINE_CSS' imported but unused	reviewbot	March 29, 2015, 3:32 a.m.
"Create"	chipx86	April 5, 2015, 1:56 a.m.
'reviewboard' imported but unused	reviewbot	April 25, 2015, 10:53 p.m.
'django_reset' imported but unused	reviewbot	April 25, 2015, 10:53 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	April 25, 2015, 10:53 p.m.
'PIPELINE_JS' imported but unused	reviewbot	April 25, 2015, 10:53 p.m.
'PIPELINE_CSS' imported but unused	reviewbot	April 25, 2015, 10:53 p.m.
'get_integration_manager' imported but unused	reviewbot	April 25, 2015, 10:59 p.m.
'reviewboard' imported but unused	reviewbot	April 25, 2015, 10:59 p.m.
'django_reset' imported but unused	reviewbot	April 25, 2015, 10:59 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	April 25, 2015, 10:59 p.m.
'PIPELINE_CSS' imported but unused	reviewbot	April 25, 2015, 10:59 p.m.
'PIPELINE_JS' imported but unused	reviewbot	April 25, 2015, 10:59 p.m.
'django_reset' imported but unused	reviewbot	April 25, 2015, 11:03 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	April 25, 2015, 11:03 p.m.
'PIPELINE_CSS' imported but unused	reviewbot	April 25, 2015, 11:03 p.m.
'PIPELINE_JS' imported but unused	reviewbot	April 25, 2015, 11:03 p.m.
Imports should come in alphabetical order.	david	May 19, 2015, 6:39 p.m.
Should be in the imperative mood ("Wrap a callback ...")	david	May 19, 2015, 6:39 p.m.
exc_info should be True, rather than 1 (we have 1 a bunch of other places, but I'm slowly cleaning those …	david	May 19, 2015, 6:39 p.m.
This should use the same terminology as you changed above ("registration" rather than "initializing")	david	May 19, 2015, 6:39 p.m.
This should use the same terminology as you changed above ("registration" rather than "initializing")	david	May 19, 2015, 6:39 p.m.
Should be in the imperative mood ("Shut down...")	david	May 19, 2015, 6:39 p.m.
Should be in the imperative mood ("Return the...")	david	May 19, 2015, 6:39 p.m.
Should be in the imperative mood ("Return the...")	david	May 19, 2015, 6:39 p.m.
Should be in the imperative mood ("Return the...")	david	May 19, 2015, 6:39 p.m.
The logging methods will do a format operation internally, so you can pass in config_id as a parameter rather than …	david	May 19, 2015, 6:39 p.m.
Should be in the imperative mood ("Return all...")	david	May 19, 2015, 6:39 p.m.
Should be in the imperative mood ("Return the...")	david	May 19, 2015, 6:39 p.m.
Should be in the imperative mood ("Return the...")	david	May 19, 2015, 6:39 p.m.
Should be in the imperative mood ("Return...")	david	May 19, 2015, 6:39 p.m.
Should be in the imperative mood ("Return...")	david	May 19, 2015, 6:39 p.m.
'django_reset' imported but unused	reviewbot	May 5, 2015, 9:01 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	May 5, 2015, 9:01 p.m.
'PIPELINE_CSS' imported but unused	reviewbot	May 5, 2015, 9:01 p.m.
'PIPELINE_JS' imported but unused	reviewbot	May 5, 2015, 9:01 p.m.

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

reviewboard/settings.py (Diff revision 1)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 1)

Show all issues

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

reviewboard/settings.py (Diff revision 1)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 1)
Show all issues
```
 'PIPELINE_JS' imported but unused
```

Description:

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

		Initial: February 8, 2015

		Added IntegrationHook in hooks.py
		Added integration class to be use as the base class for all integration
		Added manager class to manage all integrations
		Added ConfiguredIntegration model that stores the configuration of an integration
~		Register integration in settings.py
	~	Registered integration in settings.py

Summary:

-	[WIP] Add integration model and manager for third-party services framework
+	Add integration model and manager for third-party services framework

Description:

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

		Initial: February 8, 2015

		Added IntegrationHook in hooks.py
		Added integration class to be use as the base class for all integration
		Added manager class to manage all integrations
		Added ConfiguredIntegration model that stores the configuration of an integration
		Registered integration in settings.py
	+
	+	Update: February 15, 2015
	+
	+	Added unit tests for IntegrationManager
	+	Added unit tests to extensions.test for IntegrationHook
	+	Added fixtures

Testing Done:

~		Manual testing with a dummy integration
	~	Manual testing with a dummy integration.
	+
	+	Unit tests pass.

Commit:

-	1299320179c17866161267d203cb93cd21a58776
+	4d035decd1e3a58d078af14f830c0ba759fe9e73

Diff:

Revision 2 (+428 -1)

Show changes

	reviewboard/settings.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/integrations/__init__.py
	reviewboard/integrations/integration.py
	reviewboard/integrations/manager.py
	reviewboard/integrations/models.py
	reviewboard/integrations/tests.py
	1 more

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

reviewboard/settings.py (Diff revision 2)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 2)

Show all issues

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

reviewboard/settings.py (Diff revision 2)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 2)
Show all issues
```
 'PIPELINE_JS' imported but unused
```

Commit:

-	4d035decd1e3a58d078af14f830c0ba759fe9e73
+	d52d21a7e6ef28405240d07e4b1c722b5abed591

Diff:

Revision 3 (+428 -1)

Show changes

	reviewboard/settings.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/integrations/__init__.py
	reviewboard/integrations/integration.py
	reviewboard/integrations/manager.py
	reviewboard/integrations/models.py
	reviewboard/integrations/tests.py
	1 more

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

reviewboard/settings.py (Diff revision 3)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 3)

Show all issues

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

reviewboard/settings.py (Diff revision 3)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 3)
Show all issues
```
 'PIPELINE_JS' imported but unused
```

Commit:

-	d52d21a7e6ef28405240d07e4b1c722b5abed591
+	791665e60fb1f6ef70731ce9fed0cb53b3b6f36f

Diff:

Revision 4 (+427 -1)

Show changes

	reviewboard/settings.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/integrations/__init__.py
	reviewboard/integrations/integration.py
	reviewboard/integrations/manager.py
	reviewboard/integrations/models.py
	reviewboard/integrations/tests.py
	1 more

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

reviewboard/settings.py (Diff revision 4)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 4)

Show all issues

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

reviewboard/settings.py (Diff revision 4)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 4)
Show all issues
```
 'PIPELINE_JS' imported but unused
```

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

Show all issues

"Allows extensions to register and unregister service integrations."

reviewboard/extensions/hooks.py (Diff revision 4)
Show all issues
```
Two blank lines are needed here.
```
reviewboard/extensions/tests.py (Diff revision 4)
Show all issues
```
Must be inserted in alphabetical order.
```

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

Show all issues

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.

reviewboard/extensions/tests.py (Diff revision 4)
Show all issues
```
"registration"
```
reviewboard/extensions/tests.py (Diff revision 4)
Show all issues
```
"unregistration"
```

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

Show all issues

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.

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

Show all issues

Each of these should be documented, using this form:

#: Single line comment
myvar1 = ...

#: Single-line summary
#:
#: Description that spans
#: multiple lines.
myvar2 = ...

reviewboard/integrations/integration.py (Diff revision 4)
Show all issues
```
Blank line between these.
```
reviewboard/integrations/integration.py (Diff revision 4)
Show all issues
```
"URL"
```
reviewboard/integrations/integration.py (Diff revision 4)
Show all issues
```
Two blank lines between these.
```
reviewboard/integrations/integration.py (Diff revision 4)
Show all issues
```
"Return" instead of "Get".
```

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

Show all issues

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

reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
No blank line here.
```
reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
Blank line after class docstrings.
```

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

Show all issues

Private functions should always come after all the public functions.

reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
This can be combined into one statement.
```
reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
Should use pk instead of id for model instances.
```

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

Show all issues

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

reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
"ID"
```

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

Show all issues

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.

reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
Missing a period.
```

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

Show all issues

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.

reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
Missing a trailing period.
```
reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
Trailing period.
```

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

Show all issues

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.

reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
Trailing period.
```
reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
Trailing period.
```
reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
Trailing period.
```
reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
Trailing period.
```
reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
Trailing period.
```
reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
Trailing period.
```

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

Show all issues

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

reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
Trailing period.
```
reviewboard/integrations/manager.py (Diff revision 4)
Show all issues
```
Needs a docstring.
```
reviewboard/integrations/models.py (Diff revision 4)
Show all issues
```
Must be in alphabetical order.
```
reviewboard/integrations/models.py (Diff revision 4)
Show all issues
```
Needs a docstring.
```

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

Show all issues

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

reviewboard/integrations/models.py (Diff revision 4)
Show all issues
```
"has" access.
```

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

Show all issues

Must be a single line. How about:

"Return whether the user can modify this configuration."

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

Show all issues

This needs to return something.

xuanyi March 13, 2015, 9:38 a.m.

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.

reviewboard/integrations/tests.py (Diff revision 4)
Show all issues
```
Must be in alphabetical order.
```
reviewboard/integrations/tests.py (Diff revision 4)
Show all issues
```
Trailing period.
```

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

Show all issues

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

"""First line second line. """

reviewboard/integrations/tests.py (Diff revision 4)
Show all issues
```
No blank line here.
```

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

Show all issues

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

xuanyi March 13, 2015, 9:38 a.m.

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.

brennie March 13, 2015, 10:12 a.m.

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.

reviewboard/integrations/integration.py (Diff revision 4)
Show all issues
```
No blank line here.
```

Commit:

-	791665e60fb1f6ef70731ce9fed0cb53b3b6f36f
+	bb05d3e087daed1eb9c8b08905da7c713d562a51

Diff:

Revision 5 (+461 -2)

Show changes

	reviewboard/settings.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/integrations/__init__.py
	reviewboard/integrations/integration.py
	reviewboard/integrations/manager.py
	reviewboard/integrations/models.py
	reviewboard/integrations/tests.py

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

reviewboard/integrations/tests.py (Diff revision 5)

Show all issues

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

reviewboard/settings.py (Diff revision 5)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 5)

Show all issues

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

reviewboard/settings.py (Diff revision 5)
Show all issues
```
 'PIPELINE_JS' imported but unused
```
reviewboard/settings.py (Diff revision 5)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```

Commit:

-	bb05d3e087daed1eb9c8b08905da7c713d562a51
+	8955efcaad9674e4b65da6e164cc92ecfad9c963

Diff:

Revision 6 (+460 -2)

Show changes

	reviewboard/settings.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/integrations/__init__.py
	reviewboard/integrations/integration.py
	reviewboard/integrations/manager.py
	reviewboard/integrations/models.py
	reviewboard/integrations/tests.py

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

reviewboard/settings.py (Diff revision 6)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 6)

Show all issues

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

reviewboard/settings.py (Diff revision 6)
Show all issues
```
 'PIPELINE_JS' imported but unused
```
reviewboard/settings.py (Diff revision 6)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```

Commit:

-	8955efcaad9674e4b65da6e164cc92ecfad9c963
+	2d76fd290f9026538955d85ff2702561574c32ec

Diff:

Revision 7 (+478 -2)

Show changes

	reviewboard/__init__.py
	reviewboard/settings.py
	reviewboard/accounts/backends.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/integrations/__init__.py
	reviewboard/integrations/integration.py
	reviewboard/integrations/manager.py
	2 more

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

reviewboard/__init__.py (Diff revision 7)
Show all issues
```
 'reviewboard' imported but unused
```
reviewboard/integrations/manager.py (Diff revision 7)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/integrations/manager.py (Diff revision 7)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/settings.py (Diff revision 7)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 7)

Show all issues

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

reviewboard/settings.py (Diff revision 7)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 7)
Show all issues
```
 'PIPELINE_JS' imported but unused
```

Commit:

-	2d76fd290f9026538955d85ff2702561574c32ec
+	1f5a611c3101001debe91b0e3063613bd76416a3

Diff:

Revision 8 (+478 -2)

Show changes

	reviewboard/__init__.py
	reviewboard/settings.py
	reviewboard/accounts/backends.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/integrations/__init__.py
	reviewboard/integrations/integration.py
	reviewboard/integrations/manager.py
	2 more

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

reviewboard/__init__.py (Diff revision 8)
Show all issues
```
 'reviewboard' imported but unused
```
reviewboard/settings.py (Diff revision 8)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 8)

Show all issues

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

reviewboard/settings.py (Diff revision 8)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 8)
Show all issues
```
 'PIPELINE_JS' imported but unused
```

Description:

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

		Initial: February 8, 2015

		Added IntegrationHook in hooks.py
		Added integration class to be use as the base class for all integration
		Added manager class to manage all integrations
		Added ConfiguredIntegration model that stores the configuration of an integration
		Registered integration in settings.py

		Update: February 15, 2015

		Added unit tests for IntegrationManager
		Added unit tests to extensions.test for IntegrationHook
		Added fixtures
	+
	+	Update: March 29, 2015
	+
	+	Implemented common test method in testcase
	+	Modified permission for models

Commit:

-	1f5a611c3101001debe91b0e3063613bd76416a3
+	709a7e7548cddf0a611b3e01103c693fd118c992

Diff:

Revision 9 (+507 -2)

Show changes

	reviewboard/__init__.py
	reviewboard/settings.py
	reviewboard/accounts/backends.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/integrations/__init__.py
	reviewboard/integrations/integration.py
	reviewboard/integrations/manager.py
	3 more

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

reviewboard/__init__.py (Diff revision 9)
Show all issues
```
 'reviewboard' imported but unused
```
reviewboard/settings.py (Diff revision 9)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 9)

Show all issues

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

reviewboard/settings.py (Diff revision 9)
Show all issues
```
 'PIPELINE_JS' imported but unused
```
reviewboard/settings.py (Diff revision 9)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```

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.

reviewboard/extensions/hooks.py (Diff revision 9)

Show all issues

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

xuanyi April 5, 2015, 1:55 a.m.

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?

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

Show all issues

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.

reviewboard/integrations/integration.py (Diff revision 9)
Show all issues
```
Should be allow_local_sites.
```
reviewboard/integrations/integration.py (Diff revision 9)
Show all issues
```
How about config_form?
```

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

Show all issues

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

reviewboard/integrations/integration.py (Diff revision 9)
Show all issues
```
"integration"
```
reviewboard/integrations/integration.py (Diff revision 9)
Show all issues
```
"toggled"
```
reviewboard/integrations/integration.py (Diff revision 9)
Show all issues
```
"Subclasses"
".. to shut down the integration and ..."
```
reviewboard/integrations/integration.py (Diff revision 9)
Show all issues
```
"... to be toggled ..."
```
reviewboard/integrations/integration.py (Diff revision 9)
Show all issues
```
No need for the None parameter. That's the default.
```

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

Show all issues

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.

xuanyi April 5, 2015, 1:55 a.m.

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?

reviewboard/integrations/manager.py (Diff revision 9)
Show all issues
```
Combine these conditionals.
```

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

Show all issues

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

reviewboard/integrations/models.py (Diff revision 9)
Show all issues
```
"integration"
```

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

Show all issues

This can be one statement.

xuanyi April 5, 2015, 1:55 a.m.

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?

reviewboard/integrations/tests.py (Diff revision 9)
Show all issues
```
Swap these (alphabetical order).
```
reviewboard/integrations/tests.py (Diff revision 9)
Show all issues
```
""" on the next line.
```
reviewboard/integrations/tests.py (Diff revision 9)
Show all issues
```
Here too.
Same below.
```
reviewboard/testing/testcase.py (Diff revision 9)
Show all issues
```
"Create"
```

Description:

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

~		Initial: February 8, 2015
	~	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.

~		Added IntegrationHook in hooks.py
	~	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.
-		Added integration class to be use as the base class for all integration
-		Added manager class to manage all integrations
-		Added ConfiguredIntegration model that stores the configuration of an integration
-		Registered integration in settings.py

~		Update: February 15, 2015
	~	All these configured integration will then be managed through the integration manager.
-
-		Added unit tests for IntegrationManager
-		Added unit tests to extensions.test for IntegrationHook
-		Added fixtures
-
-		Update: March 29, 2015
-
-		Implemented common test method in testcase
-		Modified permission for models

Change Summary:

Fix review issues

Commit:

-	709a7e7548cddf0a611b3e01103c693fd118c992
+	90f5d8a4e3a602bfd4a77d1f4aa3304badadcb75

Diff:

Revision 10 (+562 -2)

Show changes

	reviewboard/__init__.py
	reviewboard/settings.py
	reviewboard/accounts/backends.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/integrations/__init__.py
	reviewboard/integrations/integration.py
	reviewboard/integrations/manager.py
	3 more

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

reviewboard/__init__.py (Diff revision 10)
Show all issues
```
 'reviewboard' imported but unused
```
reviewboard/settings.py (Diff revision 10)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 10)

Show all issues

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

reviewboard/settings.py (Diff revision 10)
Show all issues
```
 'PIPELINE_JS' imported but unused
```
reviewboard/settings.py (Diff revision 10)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```

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

Testing Done:

~		Manual testing with a dummy integration.
	~	Tested with a dummy integration.

		Unit tests pass.

Commit:

-	90f5d8a4e3a602bfd4a77d1f4aa3304badadcb75
+	145dd6db6d769a0728b5b34f9d185b8e0467f013

Diff:

Revision 11 (+554 -2)

Show changes

	reviewboard/__init__.py
	reviewboard/settings.py
	reviewboard/accounts/backends.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/integrations/__init__.py
	reviewboard/integrations/integration.py
	reviewboard/integrations/manager.py
	3 more

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

reviewboard/__init__.py (Diff revision 11)

Show all issues

 'get_integration_manager' imported but unused

reviewboard/__init__.py (Diff revision 11)
Show all issues
```
 'reviewboard' imported but unused
```
reviewboard/settings.py (Diff revision 11)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 11)

Show all issues

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

reviewboard/settings.py (Diff revision 11)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 11)
Show all issues
```
 'PIPELINE_JS' imported but unused
```

Testing Done:

~		Tested with a dummy integration.
	~	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.

Commit:

-	145dd6db6d769a0728b5b34f9d185b8e0467f013
+	bdfd08f950d23f696b8776d8e033dc2b47fd965f

Diff:

Revision 12 (+553 -2)

Show changes

	reviewboard/settings.py
	reviewboard/accounts/backends.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/integrations/__init__.py
	reviewboard/integrations/integration.py
	reviewboard/integrations/manager.py
	reviewboard/integrations/models.py
	2 more

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

reviewboard/settings.py (Diff revision 12)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 12)

Show all issues

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

reviewboard/settings.py (Diff revision 12)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 12)
Show all issues
```
 'PIPELINE_JS' imported but unused
```

Commit:

-	bdfd08f950d23f696b8776d8e033dc2b47fd965f
+	d0bd6c60c3d6fe5226dfc01dd62c64c43fd3da05

Diff:

Revision 13 (+556 -2)

Show changes

	reviewboard/settings.py
	reviewboard/accounts/backends.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/integrations/__init__.py
	reviewboard/integrations/integration.py
	reviewboard/integrations/manager.py
	reviewboard/integrations/models.py
	2 more

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

reviewboard/settings.py (Diff revision 13)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 13)

Show all issues

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

reviewboard/settings.py (Diff revision 13)
Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 13)
Show all issues
```
 'PIPELINE_JS' imported but unused
```

Fix it!

reviewboard/extensions/hooks.py (Diff revision 13)
Show all issues
```
Imports should come in alphabetical order.
```

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

reviewboard/integrations/integration.py (Diff revision 13)
Show all issues
```
Should be in the imperative mood ("Shut down...")
```
reviewboard/integrations/integration.py (Diff revision 13)
Show all issues
```
Should be in the imperative mood ("Return the...")
```
reviewboard/integrations/integration.py (Diff revision 13)
Show all issues
```
Should be in the imperative mood ("Return the...")
```
reviewboard/integrations/integration.py (Diff revision 13)
Show all issues
```
Should be in the imperative mood ("Return the...")
```

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

Show all issues

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

reviewboard/integrations/manager.py (Diff revision 13)
Show all issues
```
Should be in the imperative mood ("Return all...")
```
reviewboard/integrations/manager.py (Diff revision 13)
Show all issues
```
Should be in the imperative mood ("Return the...")
```
reviewboard/integrations/manager.py (Diff revision 13)
Show all issues
```
Should be in the imperative mood ("Return the...")
```
reviewboard/integrations/models.py (Diff revision 13)
Show all issues
```
Should be in the imperative mood ("Return...")
```
reviewboard/integrations/models.py (Diff revision 13)
Show all issues
```
Should be in the imperative mood ("Return...")
```

Status: Discarded

Change Summary:

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