• 
      

    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

    reviewbot reviewbot

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

    brennie brennie

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

    brennie brennie

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

    brennie brennie

    No blank line here.

    brennie brennie

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

    brennie 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 brennie

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

    brennie brennie

    Blank line between class docstring and its first member.

    brennie brennie

    Needs a docstring.

    brennie brennie

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

    brennie brennie

    """Handle the reply_published signal."""

    brennie brennie

    This method needs a docstring.

    brennie brennie

    Leftover from debugging?

    brennie brennie

    Needs a docstring.

    brennie brennie

    Trailing whitespace.

    brennie brennie

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

    brennie brennie

    No blank line here.

    brennie brennie

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

    brennie brennie

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

    david david

    This should all fit on one line.

    david david

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

    david david

    Mark for translation.

    david david

    Mark for translation.

    david david

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

    david david

    Mark for translation.

    david david

    Mark for translation.

    david david

    Mark for translation.

    david david

    Mark for translation.

    david david

    Mark for translation.

    david david
    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)
       
       
      Show all issues
      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)
       
       
      Show all issues

      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)
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      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)
       
       
      Show all issues

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

    5. rbslack/rbslack/integration.py (Diff revision 2)
       
       
      Show all issues

      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)
       
       
       
       
       
       
      Show all issues

      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)
       
       
       
       
       
      Show all issues

      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)
       
       
       
       
       
      Show all issues

      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)
       
       
       
      Show all issues

      Blank line between class docstring and its first member.

    11. rbslack/rbslack/integration.py (Diff revision 2)
       
       
      Show all issues

      Needs a docstring.

    12. rbslack/rbslack/integration.py (Diff revision 2)
       
       
      Show all issues

      Docstring summaries should be written in the imperative mood:

      """Handle the review_published signal."""

    13. rbslack/rbslack/integration.py (Diff revision 2)
       
       
      Show all issues

      """Handle the reply_published signal."""

    14. rbslack/rbslack/integration.py (Diff revision 2)
       
       
      Show all issues

      This method needs a docstring.

    15. rbslack/rbslack/integration.py (Diff revision 2)
       
       
       
      Show all issues

      Leftover from debugging?

    16. rbslack/rbslack/integration_forms.py (Diff revision 2)
       
       
      Show all issues

      Needs a docstring.

    17. Show all issues

      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)
       
       
      Show all issues

      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)
       
       
      Show all issues

      No blank line here.

    4. rbslack/rbslack/integration.py (Diff revision 3)
       
       
      Show all issues

      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)
       
       
       
      Show all issues

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

    3. rbslack/rbslack/integration.py (Diff revision 5)
       
       
       
       
      Show all issues

      This should all fit on one line.

    4. rbslack/rbslack/integration.py (Diff revision 5)
       
       
      Show all issues

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

    5. Show all issues

      Mark for translation.

    6. Show all issues

      Mark for translation.

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

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

    8. Show all issues

      Mark for translation.

    9. Show all issues

      Mark for translation.

    10. Show all issues

      Mark for translation.

    11. Show all issues

      Mark for translation.

    12. Show all issues

      Mark for translation.

    13. 
        
    chipx86
    Review request changed
    Status:
    Discarded
    Change Summary:

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