Rbslack with Integration framework

Review Request #7221 — Created April 17, 2015 and discarded

Information

rb-extension-pack
master

Reviewers

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

reviewbotreviewbot

Shouldn't all the behaviour that was moved into the SlackIntegration be ripped out of this file?

brenniebrennie

Won't this all end up with everything being notified twice, once through the new integration stuff and once through the …

brenniebrennie

I feel like this shouldn't be hardcoded to reference reviews.reviewboard.org.

brenniebrennie

No blank line here.

brenniebrennie

Can this be formatted as: return ('<%s://%s%s|%s>' % (siteconfig.get('site_domain_method'), site.domain, path, text))

brenniebrennie

Can we format this as: username = user.get_full_name() or user.username return format_link(integration, user_url, user.get_full_name, username)

brenniebrennie

Can we format this as: return format_link(integration, review_request.get_absolute_url(), review_request.summary)

brenniebrennie

Blank line between class docstring and its first member.

brenniebrennie

Needs a docstring.

brenniebrennie

Docstring summaries should be written in the imperative mood: """Handle the review_published signal."""

brenniebrennie

"""Handle the reply_published signal."""

brenniebrennie

This method needs a docstring.

brenniebrennie

Leftover from debugging?

brenniebrennie

Needs a docstring.

brenniebrennie

Trailing whitespace.

brenniebrennie

All functions that take integration as the first parameter should probably be member functions of the integration class if they're …

brenniebrennie

No blank line here.

brenniebrennie

Can you rewrite this to mention what it does, not what it is used for? e.g., "Send a notification..."

brenniebrennie

This comment should be just above the local_site_reverse('user', ...) call.

daviddavid

This should all fit on one line.

daviddavid

We should use something other than type, which is a reserved word.

daviddavid

Mark for translation.

daviddavid

Mark for translation.

daviddavid

This should probably just be a couple paragraphs rather than a list.

daviddavid

Mark for translation.

daviddavid

Mark for translation.

daviddavid

Mark for translation.

daviddavid

Mark for translation.

daviddavid

Mark for translation.

daviddavid
reviewbot
  1. 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
    
    
  2. rbslack/rbslack/integration_forms.py (Diff revision 1)
     
     
    Col: 10
     W292 no newline at end of file
    
  3. 
      
XU
reviewbot
  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
    
    
  2. 
      
brennie
  1. 
      
  2. rbslack/rbslack/extension.py (Diff revision 2)
     
     

    Shouldn't all the behaviour that was moved into the SlackIntegration be ripped out of this file?

  3. 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?

  4. rbslack/rbslack/integration.py (Diff revision 2)
     
     

    I feel like this shouldn't be hardcoded to reference reviews.reviewboard.org.

  5. rbslack/rbslack/integration.py (Diff revision 2)
     
     

    No blank line here.

  6. rbslack/rbslack/integration.py (Diff revision 2)
     
     
     
     
     
     

    What happens if we escape the other entities?

  7. 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))
    
  8. 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)
    
  9. rbslack/rbslack/integration.py (Diff revision 2)
     
     
     
     
     

    Can we format this as:

    return format_link(integration,
                       review_request.get_absolute_url(),
                       review_request.summary)
    
  10. rbslack/rbslack/integration.py (Diff revision 2)
     
     
     

    Blank line between class docstring and its first member.

  11. rbslack/rbslack/integration.py (Diff revision 2)
     
     

    Needs a docstring.

  12. rbslack/rbslack/integration.py (Diff revision 2)
     
     

    Docstring summaries should be written in the imperative mood:

    """Handle the review_published signal."""

  13. rbslack/rbslack/integration.py (Diff revision 2)
     
     

    """Handle the reply_published signal."""

  14. rbslack/rbslack/integration.py (Diff revision 2)
     
     

    This method needs a docstring.

  15. rbslack/rbslack/integration.py (Diff revision 2)
     
     
     

    Leftover from debugging?

  16. rbslack/rbslack/integration_forms.py (Diff revision 2)
     
     

    Needs a docstring.

  17. Trailing whitespace.

  18. 
      
XU
reviewbot
  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
    
    
  2. 
      
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. rbslack/rbslack/integration.py (Diff revision 3)
     
     

    All functions that take integration as the first parameter should probably be member functions of the integration class if they're not used elsewhere.

    1. These are common utility methods that can be used by any integrations to notify Slack. Will there be a need to abstract it out for any other kind of slack integration in addition to the current one?

  3. rbslack/rbslack/integration.py (Diff revision 3)
     
     

    No blank line here.

  4. 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..."

  5. 
      
XU
reviewbot
  1. 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
    
    
  2. 
      
XU
reviewbot
  1. 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
    
    
  2. 
      
XU
david
  1. 
      
  2. rbslack/rbslack/integration.py (Diff revision 5)
     
     
     

    This comment should be just above the local_site_reverse('user', ...) call.

  3. rbslack/rbslack/integration.py (Diff revision 5)
     
     
     
     

    This should all fit on one line.

  4. rbslack/rbslack/integration.py (Diff revision 5)
     
     

    We should use something other than type, which is a reserved word.

  5. Mark for translation.

  6. Mark for translation.

  7. rbslack/rbslack/templates/slack_integration_config.html (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This should probably just be a couple paragraphs rather than a list.

  8. Mark for translation.

  9. Mark for translation.

  10. Mark for translation.

  11. Mark for translation.

  12. Mark for translation.

  13. 
      
chipx86
Review request changed

Status: Discarded

Change Summary:

Discarded in favor of the new rbintegrations package, containing Slack.

Loading...