Summary

Webapi Resource for integrations

Review Request #7105 — Created March 23, 2015 and discarded April 28, 2016, 9:20 a.m.

Information

Owner

xuanyi

Repository

Review Board

Branch

master

Bugs

Depends On

~~6918~~

Blocks

7221, 7136, 7106

Reviewers

Groups

reviewboard, students

People

Description

This provides the webapi resource and the views for the integration admin page.
The webapi resource for both Integration and ConfiguredIntegration are provided here. The Integration resource allows for getting the list of Integrations that implements the class and hook defined in review 6918.
The ConfiguredIntegration resource allows GET, PUT and DELETE call to manage individual configured integration.

Testing Done

Unit test done.
Tested with WebapiResource test for both integrations and configuredIntegration resource.

Issues

Description	From	Last Updated
'PERMISSION_DENIED' imported but unused	reviewbot	March 30, 2015, 4:14 p.m.
'NOT_LOGGED_IN' imported but unused	reviewbot	March 30, 2015, 4:14 p.m.
'webapi_response_errors' imported but unused	reviewbot	March 30, 2015, 4:14 p.m.
'webapi_login_required' imported but unused	reviewbot	March 30, 2015, 4:14 p.m.
'webapi_check_local_site' imported but unused	reviewbot	March 30, 2015, 4:14 p.m.
local variable 'manager' is assigned to but never used	reviewbot	April 13, 2015, 2:53 p.m.
'IntegrationManager' imported but unused	reviewbot	April 13, 2015, 3 p.m.
This function needs a docstring.	brennie	April 17, 2015, 6:59 p.m.
This function needs a docstring.	brennie	April 17, 2015, 6:59 p.m.
Please alphabetize these.	brennie	April 17, 2015, 6:59 p.m.
Undo this change.	brennie	April 17, 2015, 6:59 p.m.
This method isn't called from anywhere.	brennie	April 18, 2015, 12:01 a.m.
This will raise an exception if integration_class is None. You should define integration as None before the if integration_class block.	brennie	April 26, 2015, 5:05 a.m.
ConfiguredIntegration.	brennie	April 26, 2015, 5:05 a.m.
This docstring will end up in public facing documentation. It should not mention class names of RB-specific things (e.g., ConfiguredIntegration). …	brennie	April 26, 2015, 5:04 a.m.
ConfiguredIntegration	brennie	April 26, 2015, 5:05 a.m.
integration-id	brennie	April 26, 2015, 5:04 a.m.
When is_list is false, this will return a list. Shouldn't it return a single instance?	brennie	April 26, 2015, 5:15 a.m.
Can we change this to use integration-id ?	brennie	April 26, 2015, 5:04 a.m.
See previous comment about docstrings.	brennie	April 26, 2015, 5:04 a.m.
You can use enabled as a key-word argument to the function to not have to look into kwargs.	brennie	April 26, 2015, 5:04 a.m.
"Delete a configured integration."	brennie	April 26, 2015, 5:04 a.m.
Can we just do the call to get_integration_manager inside the class's __init__ method? Are we ever expecting to instantiate this …	brennie	April 26, 2015, 5:04 a.m.
Can we pull the query set out into a variable?	brennie	April 26, 2015, 5:04 a.m.
Same comment here re: integration managers as in the previous resource.	brennie	April 26, 2015, 5:04 a.m.
No blank line here.	david	May 20, 2015, 12:53 a.m.
All str in here should be six.text_type for future Python 3 compatibility. Here and below.	david	May 20, 2015, 12:53 a.m.
I think I'd prefer if these conditionals were switched: if config and config.integration: return config.integration.name else: return None	david	May 20, 2015, 12:53 a.m.
Same as above.	david	May 20, 2015, 12:53 a.m.
Same as above.	david	May 20, 2015, 12:53 a.m.
Same as above.	david	May 20, 2015, 12:53 a.m.
This should have a docstring.	david	May 20, 2015, 12:53 a.m.
This should have a docstring.	david	May 20, 2015, 12:53 a.m.
No blank line here.	david	May 20, 2015, 12:53 a.m.
All str items in here should be six.text_type	david	May 20, 2015, 12:53 a.m.
Please switch the conditional to test if integration: and return None in the else clause.	david	May 20, 2015, 12:53 a.m.
This should have a docstring.	david	May 20, 2015, 12:53 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/integrations/admin.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/integrations/admin.py

Summary:

[WIP] Webapi Resource for integrations

Webapi Resource for integrations

Testing Done:

~		Unit test done.
	~	Unit test done.
	+	Tested with WebapiResource test for both integrations and configuredIntegration resource.

Diff:

Revision 2 (+594 -2)

Show changes

	reviewboard/urls.py
	reviewboard/integrations/admin.py
	reviewboard/integrations/urls.py
	reviewboard/integrations/views.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/configured_integration.py
	reviewboard/webapi/resources/integration.py
	reviewboard/webapi/resources/root.py
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/tests/test_configured_integration.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/webapi/tests/test_integration.py
    reviewboard/integrations/admin.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/tests/test_configured_integration.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/webapi/tests/test_integration.py
    reviewboard/integrations/admin.py

reviewboard/webapi/resources/integration.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 'PERMISSION_DENIED' imported but unused
```
reviewboard/webapi/resources/integration.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 'NOT_LOGGED_IN' imported but unused
```
reviewboard/webapi/resources/integration.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 'webapi_response_errors' imported but unused
```
reviewboard/webapi/resources/integration.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 'webapi_login_required' imported but unused
```
reviewboard/webapi/resources/integration.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 'webapi_check_local_site' imported but unused
```

Change Summary:

Remove redundant code.

Diff:

Revision 3 (+572 -2)

Show changes

	reviewboard/urls.py
	reviewboard/integrations/admin.py
	reviewboard/integrations/urls.py
	reviewboard/integrations/views.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/configured_integration.py
	reviewboard/webapi/resources/integration.py
	reviewboard/webapi/resources/root.py
	2 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/tests/test_configured_integration.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/webapi/tests/test_integration.py
    reviewboard/integrations/admin.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/tests/test_configured_integration.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/webapi/tests/test_integration.py
    reviewboard/integrations/admin.py

Diff:

Revision 4 (+577 -2)

Show changes

	reviewboard/urls.py
	reviewboard/integrations/admin.py
	reviewboard/integrations/urls.py
	reviewboard/integrations/views.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/configured_integration.py
	reviewboard/webapi/resources/integration.py
	reviewboard/webapi/resources/root.py
	2 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/tests/test_configured_integration.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/webapi/tests/test_integration.py
    reviewboard/integrations/admin.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/tests/test_configured_integration.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/webapi/tests/test_integration.py
    reviewboard/integrations/admin.py

reviewboard/webapi/tests/test_integration.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 local variable 'manager' is assigned to but never used
```

Diff:

Revision 5 (+576 -2)

Show changes

	reviewboard/urls.py
	reviewboard/integrations/admin.py
	reviewboard/integrations/urls.py
	reviewboard/integrations/views.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/configured_integration.py
	reviewboard/webapi/resources/integration.py
	reviewboard/webapi/resources/root.py
	2 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/tests/test_configured_integration.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/webapi/tests/test_integration.py
    reviewboard/integrations/admin.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/tests/test_configured_integration.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/webapi/tests/test_integration.py
    reviewboard/integrations/admin.py

reviewboard/webapi/tests/test_integration.py (Diff revision 5)
The issue has been resolved. Show all issues
```
 'IntegrationManager' imported but unused
```

Diff:

Revision 6 (+575 -2)

Show changes

	reviewboard/urls.py
	reviewboard/integrations/admin.py
	reviewboard/integrations/urls.py
	reviewboard/integrations/views.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/configured_integration.py
	reviewboard/webapi/resources/integration.py
	reviewboard/webapi/resources/root.py
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/tests/test_configured_integration.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/webapi/tests/test_integration.py
    reviewboard/integrations/admin.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/tests/test_configured_integration.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/webapi/tests/test_integration.py
    reviewboard/integrations/admin.py

Can you change references to "webapi" in your summary and description to "WebAPI" ?

reviewboard/integrations/views.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This function needs a docstring.
```
reviewboard/integrations/views.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This function needs a docstring.
```
reviewboard/webapi/resources/__init__.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Please alphabetize these.
```
reviewboard/webapi/resources/__init__.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Undo this change.
```
reviewboard/webapi/resources/integration.py (Diff revision 6)
The issue has been dropped. Show all issues
```
This method isn't called from anywhere.
```
1. brennie
  
  April 18, 2015, 12:01 a.m.
  I understand how this works now, dropping.

Change Summary:

Fix review issues

Diff:

Revision 7 (+583 -1)

Show changes

	reviewboard/urls.py
	reviewboard/integrations/admin.py
	reviewboard/integrations/urls.py
	reviewboard/integrations/views.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/configured_integration.py
	reviewboard/webapi/resources/integration.py
	reviewboard/webapi/resources/root.py
	2 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/tests/test_configured_integration.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/webapi/tests/test_integration.py
    reviewboard/integrations/admin.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/tests/test_configured_integration.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/webapi/tests/test_integration.py
    reviewboard/integrations/admin.py

reviewboard/integrations/views.py (Diff revision 7)

The issue has been resolved. Show all issues

This will raise an exception if integration_class is None. You should define integration as None before the if integration_class block.

reviewboard/webapi/resources/configured_integration.py (Diff revision 7)
The issue has been resolved. Show all issues
```
ConfiguredIntegration.
```

reviewboard/webapi/resources/configured_integration.py (Diff revision 7)

The issue has been resolved. Show all issues

This docstring will end up in public facing documentation. It should not mention class names of RB-specific things (e.g., ConfiguredIntegration). Instead it should just refer to, e.g., "configuration integration".

reviewboard/webapi/resources/configured_integration.py (Diff revision 7)
The issue has been resolved. Show all issues
```
ConfiguredIntegration
```
reviewboard/webapi/resources/configured_integration.py (Diff revision 7)
The issue has been resolved. Show all issues
```
integration-id
```

reviewboard/webapi/resources/configured_integration.py (Diff revision 7)

The issue has been dropped. Show all issues

When is_list is false, this will return a list. Shouldn't it return a single instance?

xuanyi

April 26, 2015, 5:11 a.m.

The superclass's get_object method in WebAPI resource will be calling get_queryset to get the queryset before reducing it down to a single item. From my understanding of the get_queryset method, it seems to be a pre-process optimzation stage to reduce/specialize the query depending on whether it is a list or not.

reviewboard/webapi/resources/configured_integration.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Can we change this to use integration-id ?
```

reviewboard/webapi/resources/configured_integration.py (Diff revision 7)

The issue has been resolved. Show all issues

See previous comment about docstrings.

reviewboard/webapi/resources/configured_integration.py (Diff revision 7)
The issue has been resolved. Show all issues
```
You can use enabled as a key-word argument to the function to not have to look into kwargs.
```
reviewboard/webapi/resources/configured_integration.py (Diff revision 7)
The issue has been resolved. Show all issues
```
"Delete a configured integration."
```

reviewboard/webapi/resources/configured_integration.py (Diff revision 7)

The issue has been resolved. Show all issues

Can we just do the call to get_integration_manager inside the class's __init__ method?

Are we ever expecting to instantiate this with a different integration manager?

reviewboard/webapi/resources/integration.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Can we pull the query set out into a variable?
```
reviewboard/webapi/resources/integration.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Same comment here re: integration managers as in the previous resource.
```

Diff:

Revision 8 (+583 -1)

Show changes

	reviewboard/urls.py
	reviewboard/integrations/admin.py
	reviewboard/integrations/urls.py
	reviewboard/integrations/views.py
	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/configured_integration.py
	reviewboard/webapi/resources/integration.py
	reviewboard/webapi/resources/root.py
	2 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/tests/test_configured_integration.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/webapi/tests/test_integration.py
    reviewboard/integrations/admin.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/root.py
    reviewboard/integrations/urls.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/webapi/tests/test_configured_integration.py
    reviewboard/integrations/views.py
    reviewboard/webapi/resources/configured_integration.py
    reviewboard/webapi/resources/integration.py
    reviewboard/urls.py
    reviewboard/webapi/tests/test_integration.py
    reviewboard/integrations/admin.py

Fix it!

Some of the parts of this change look like they'd be more appropriate for /r/7106 (like the admin.py, urls.py, and views.py changes)

reviewboard/webapi/resources/configured_integration.py (Diff revision 8)
An issue was opened. Show all issues
```
No blank line here.
```
reviewboard/webapi/resources/configured_integration.py (Diff revision 8)
An issue was opened. Show all issues
```
All str in here should be six.text_type for future Python 3 compatibility. Here and below.
```

reviewboard/webapi/resources/configured_integration.py (Diff revision 8)

An issue was opened. Show all issues

I think I'd prefer if these conditionals were switched:

if config and config.integration:
    return config.integration.name
else:
    return None

reviewboard/webapi/resources/configured_integration.py (Diff revision 8)

An issue was opened. Show all issues

Same as above.

reviewboard/webapi/resources/configured_integration.py (Diff revision 8)

An issue was opened. Show all issues

Same as above.

reviewboard/webapi/resources/configured_integration.py (Diff revision 8)

An issue was opened. Show all issues

Same as above.

reviewboard/webapi/resources/configured_integration.py (Diff revision 8)
An issue was opened. Show all issues
```
This should have a docstring.
```
reviewboard/webapi/resources/configured_integration.py (Diff revision 8)
An issue was opened. Show all issues
```
This should have a docstring.
```
reviewboard/webapi/resources/integration.py (Diff revision 8)
An issue was opened. Show all issues
```
No blank line here.
```
reviewboard/webapi/resources/integration.py (Diff revision 8)
An issue was opened. Show all issues
```
All str items in here should be six.text_type
```

reviewboard/webapi/resources/integration.py (Diff revision 8)

An issue was opened. Show all issues

Please switch the conditional to test if integration: and return None in the else clause.

reviewboard/webapi/resources/integration.py (Diff revision 8)
An issue was opened. Show all issues
```
This should have a docstring.
```

Status:: Discarded
Change Summary:: Closed in favor of a revised version of Integrations that landed for 3.0.