Rbslack with Integration framework
Review Request #7221 — Created April 17, 2015 and discarded
| Information | |
|---|---|
| xuanyi | |
| rb-extension-pack | |
| master | |
|
|
|
| Reviewers | |
| rb-extension-pack, students | |
This is a simple reimplementation of rbslack extension with the integration framework. This allow multiple instance of the integration, with different configuration for different channels. A custom form and template is also added for the configure integration page.
Additionally, it also allow the user to configure the integration such that the channel will only receive notification if the review request is intended for the selected groups.
Lastly, the integration can be configure to target a local site. Notification will only be send to Slack if the review belongs to the particular local site.
Manual testing with different configuration and different review's update.
- No local site and group
- Target a single group
- Target multiple groups
- Local site
- Local site with groups
- Multiple Slack integration.
| Description | From | Last Updated |
|---|---|---|
|
Col: 10 W292 no newline at end of file |
 reviewbot
|
|
|
Shouldn't all the behaviour that was moved into the SlackIntegration be ripped out of this file? |
 brennie
|
|
|
Won't this all end up with everything being notified twice, once through the new integration stuff and once through the … |
brennie
|
|
|
I feel like this shouldn't be hardcoded to reference reviews.reviewboard.org. |
brennie
|
|
|
No blank line here. |
brennie
|
|
|
Can this be formatted as: return ('<%s://%s%s|%s>' % (siteconfig.get('site_domain_method'), site.domain, path, text)) |
brennie
|
|
|
Can we format this as: username = user.get_full_name() or user.username return format_link(integration, user_url, user.get_full_name, username) |
brennie
|
|
|
Can we format this as: return format_link(integration, review_request.get_absolute_url(), review_request.summary) |
brennie
|
|
|
Blank line between class docstring and its first member. |
brennie
|
|
|
Needs a docstring. |
brennie
|
|
|
Docstring summaries should be written in the imperative mood: """Handle the review_published signal.""" |
brennie
|
|
|
"""Handle the reply_published signal.""" |
brennie
|
|
|
This method needs a docstring. |
brennie
|
|
|
Leftover from debugging? |
brennie
|
|
|
Needs a docstring. |
brennie
|
|
|
Trailing whitespace. |
brennie
|
|
|
All functions that take integration as the first parameter should probably be member functions of the integration class if they're … |
brennie
|
|
|
No blank line here. |
brennie
|
|
|
Can you rewrite this to mention what it does, not what it is used for? e.g., "Send a notification..." |
brennie
|
|
|
This comment should be just above the local_site_reverse('user', ...) call. |
 david
|
|
|
This should all fit on one line. |
david
|
|
|
We should use something other than type, which is a reserved word. |
david
|
|
|
Mark for translation. |
david
|
|
|
Mark for translation. |
david
|
|
|
This should probably just be a couple paragraphs rather than a list. |
david
|
|
|
Mark for translation. |
david
|
|
|
Mark for translation. |
david
|
|
|
Mark for translation. |
david
|
|
|
Mark for translation. |
david
|
|
|
Mark for translation. |
david
|
XU
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+412 -1) |
-
Tool: Pyflakes Processed Files: rbslack/rbslack/extension.py rbslack/rbslack/integration_forms.py rbslack/rbslack/integration.py Ignored Files: rbslack/rbslack/templates/slack_integration_config.html rbslack/rbslack/static/slack.png Tool: PEP8 Style Checker Processed Files: rbslack/rbslack/extension.py rbslack/rbslack/integration_forms.py rbslack/rbslack/integration.py Ignored Files: rbslack/rbslack/templates/slack_integration_config.html rbslack/rbslack/static/slack.png
-
-
rbslack/rbslack/extension.py (Diff revision 2) Shouldn't all the behaviour that was moved into the
SlackIntegrationbe ripped out of this file? -
rbslack/rbslack/extension.py (Diff revision 2) Won't this all end up with everything being notified twice, once through the new integration stuff and once through the old extension stuff?
-
rbslack/rbslack/integration.py (Diff revision 2) I feel like this shouldn't be hardcoded to reference reviews.reviewboard.org.
-
-
-
rbslack/rbslack/integration.py (Diff revision 2) Can this be formatted as:
return ('<%s://%s%s|%s>' % (siteconfig.get('site_domain_method'), site.domain, path, text)) -
rbslack/rbslack/integration.py (Diff revision 2) Can we format this as:
username = user.get_full_name() or user.username return format_link(integration, user_url, user.get_full_name, username) -
rbslack/rbslack/integration.py (Diff revision 2) Can we format this as:
return format_link(integration, review_request.get_absolute_url(), review_request.summary) -
rbslack/rbslack/integration.py (Diff revision 2) Blank line between class docstring and its first member.
-
-
rbslack/rbslack/integration.py (Diff revision 2) Docstring summaries should be written in the imperative mood:
"""Handle the review_published signal.""" -
-
-
-
-
XU
Summary: |
|
|||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||
Commit: |
|
|||||||||||||||
Diff: |
Revision 3 (+414 -273) |
-
Tool: Pyflakes Processed Files: rbslack/rbslack/extension.py rbslack/rbslack/integration_forms.py rbslack/rbslack/integration.py Ignored Files: rbslack/rbslack/templates/slack_integration_config.html rbslack/rbslack/static/slack.png
-
Tool: PEP8 Style Checker Processed Files: rbslack/rbslack/extension.py rbslack/rbslack/integration_forms.py rbslack/rbslack/integration.py Ignored Files: rbslack/rbslack/templates/slack_integration_config.html rbslack/rbslack/static/slack.png
-
-
rbslack/rbslack/integration.py (Diff revision 3) All functions that take
integrationas the first parameter should probably be member functions of the integration class if they're not used elsewhere. -
-
rbslack/rbslack/integration.py (Diff revision 3) Can you rewrite this to mention what it does, not what it is used for?
e.g., "Send a notification..."
XU
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+418 -273) |
-
Tool: PEP8 Style Checker Processed Files: rbslack/rbslack/extension.py rbslack/rbslack/integration_forms.py rbslack/rbslack/integration.py Ignored Files: rbslack/rbslack/templates/slack_integration_config.html rbslack/rbslack/static/slack.png Tool: Pyflakes Processed Files: rbslack/rbslack/extension.py rbslack/rbslack/integration_forms.py rbslack/rbslack/integration.py Ignored Files: rbslack/rbslack/templates/slack_integration_config.html rbslack/rbslack/static/slack.png
XU
Change Summary:
Added local site and update setup
Description: |
|
|||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||
Diff: |
Revision 5 (+445 -273) |
-
Tool: Pyflakes Processed Files: rbslack/rbslack/extension.py rbslack/rbslack/integration_forms.py rbslack/setup.py rbslack/rbslack/integration.py Ignored Files: rbslack/rbslack/templates/slack_integration_config.html rbslack/rbslack/static/slack.png Tool: PEP8 Style Checker Processed Files: rbslack/rbslack/extension.py rbslack/rbslack/integration_forms.py rbslack/setup.py rbslack/rbslack/integration.py Ignored Files: rbslack/rbslack/templates/slack_integration_config.html rbslack/rbslack/static/slack.png
-
-
rbslack/rbslack/integration.py (Diff revision 5) This comment should be just above the
local_site_reverse('user', ...)call. -
-
rbslack/rbslack/integration.py (Diff revision 5) We should use something other than
type, which is a reserved word. -
-
-
rbslack/rbslack/templates/slack_integration_config.html (Diff revision 5) This should probably just be a couple paragraphs rather than a list.
-
-
-
-
-
Tool: PEP8 Style Checker Processed Files: rbslack/rbslack/extension.py rbslack/rbslack/integration_forms.py rbslack/rbslack/integration.py Ignored Files: rbslack/rbslack/templates/slack_integration_config.html rbslack/rbslack/static/slack.png Tool: Pyflakes Processed Files: rbslack/rbslack/extension.py rbslack/rbslack/integration_forms.py rbslack/rbslack/integration.py Ignored Files: rbslack/rbslack/templates/slack_integration_config.html rbslack/rbslack/static/slack.png