Add a new integration for I Done This.

Review Request #8776 - Created Feb. 23, 2017 and submitted

Michael Udaltsov
rbintegrations
master
ebbfd8e...
rbintegrations

I Done This is a service that provides daily status reports with a list of
tasks which are done, planned, or blocked. This integration allows posting
'done' entries to I Done This teams on behalf of users when they publish,
change, or close a review request, and when they publish reviews or replies.

Each posted message includes a description of what the user did, along with
details such as the review request id, summary, and url. Reviewer groups are
also added as #tags for tracking on the I Done This website. The messages are
currently hard-coded, but could become configurable in another change.

The I Done This API gives control over individual user accounts, so each user
has to provide their personal API token on their Review Board account page.
The token is validated via the API before getting encrypted and saved in the
user settings. The list of the user's teams is retrieved from the API the first
time the user needs to post, and is cached to avoid duplicate requests. The
cache expires in 24 hours, or can be deleted manually by re-saving the token.

The administrator has to add an integration configuration for each I Done This
team that posts are allowed to, along with conditions to filter which review
request activity gets posted. Multiple configurations can specify the same team
to allow alternative sets of conditions, but only one message will be posted
to each unique team.

The configuration method is not ideal, since the admin can't verify that the
team is correctly set up without using an API token of a user on that team.
Users also don't have access to the team configurations to choose or even see
which teams or review requests get posted. However, the overall goal is to keep
user settings to a minimum, and let he admin control the configuration. There
may be ways in the future to improve the user experience, for example by adding
UI notifications from the integration when posting to I Done This.

Unit tests have been added to verify all the posted message types, various
reasons for not posting, posts with multiple configurations and team ids,
functionality of the user and admin forms, and team id requests with caching.

Unit tests pass, with full coverage except icon_static_urls().

Manually tested adding integration configurations, setting the user API token,
form validation errors, and posting all message types to I Done This.

Loading file attachments...

  • 0
  • 0
  • 47
  • 3
  • 50
Description From Last Updated
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbintegrations/idonethis/forms.py
        rbintegrations/extension.py
        rbintegrations/idonethis/integration.py
        rbintegrations/idonethis/utils.py
        rbintegrations/idonethis/tests.py
        rbintegrations/idonethis/pages.py
    
    Ignored Files:
        rbintegrations/static/images/idonethis/icon@2x.png
        rbintegrations/idonethis/__init__.py
        rbintegrations/static/images/idonethis/icon.png
        rbintegrations/templates/rbintegrations/idonethis/account_page_form.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbintegrations/idonethis/forms.py
        rbintegrations/extension.py
        rbintegrations/idonethis/integration.py
        rbintegrations/idonethis/utils.py
        rbintegrations/idonethis/tests.py
        rbintegrations/idonethis/pages.py
    
    Ignored Files:
        rbintegrations/static/images/idonethis/icon@2x.png
        rbintegrations/idonethis/__init__.py
        rbintegrations/static/images/idonethis/icon.png
        rbintegrations/templates/rbintegrations/idonethis/account_page_form.html
    
    
  2. rbintegrations/idonethis/integration.py (Diff revision 1)
     
     
    Col: 19
     E127 continuation line over-indented for visual indent
    
  3. 
      
David Trowbridge
  1. 
      
  2. rbintegrations/idonethis/forms.py (Diff revision 1)
     
     

    This doc comment should include a "Returns" section.

  3. rbintegrations/idonethis/forms.py (Diff revision 1)
     
     

    This doc comment should include a "Returns" section.

  4. rbintegrations/idonethis/integration.py (Diff revision 1)
     
     
     
     
     
     
     

    Because this is a property, the docstring should look more like it is for attributes (such as just "The icons used for the integration") instead of the docstring for a method.

  5. rbintegrations/idonethis/tests.py (Diff revision 1)
     
     

    Add a trailing comma.

  6. rbintegrations/idonethis/tests.py (Diff revision 1)
     
     

    Unlike all our other docstrings, test case descriptions shouldn't include a period, because the test runner adds an ellipsis when printing. Here and throughout this file.

  7. rbintegrations/idonethis/tests.py (Diff revision 1)
     
     
     

    Would you mind changing these to use the format strings instead of a hard-coded result? These tests are fragile to text changes.

    1. My intention was to verify the content of the final message strings, with expected data, eliminated whitespace, etc. Using the format strings will make the expected results less obvious, and essentially duplicate the string formatting code that I'm trying to test.

      I'd prefer to keep it as-is. It's not too difficult to change if the format strings are ever modified.

    2. Fair enough.

  8. rbintegrations/idonethis/tests.py (Diff revision 1)
     
     

    Trailing comma.

  9. rbintegrations/idonethis/tests.py (Diff revision 1)
     
     

    Trailing comma.

  10. rbintegrations/idonethis/tests.py (Diff revision 1)
     
     

    Trailing comma. I assume the same throughout this file.

  11. rbintegrations/idonethis/tests.py (Diff revision 1)
     
     

    Trailing comma.

  12. rbintegrations/idonethis/utils.py (Diff revision 1)
     
     

    Because this is imported into other code as just create_request, it was initially a bit confusing when I read that code. Can we rename this as create_idt_request so it's clear what's going on?

    1. Renamed to create_idonethis_request and changed all call sites to use named arguments for clarity.

  13. 
      
Michael Udaltsov
Barret Rennie
  1. 
      
  2. rbintegrations/idonethis/forms.py (Diff revision 2)
     
     

    File-level docstring.

  3. rbintegrations/idonethis/forms.py (Diff revision 2)
     
     

    <b> is deprecated. Use <strong> instead.

  4. rbintegrations/idonethis/forms.py (Diff revision 2)
     
     

    The fieldset label should be marked for translation.

  5. rbintegrations/idonethis/forms.py (Diff revision 2)
     
     

    Here too.

  6. rbintegrations/idonethis/forms.py (Diff revision 2)
     
     

    Maybe use <p> over linebreaks? Also, IDK if we use self-closing tags (they are deprecated in the HTML spec)

    1. Turns out I can just use \n and the fieldset template will turn them into paragraphs. I think I originally only tried it on the field help text, where the template doesn't split paragraphs.

      I've changed the self-closing <br /> to regular <br> on the user account page.

  7. rbintegrations/idonethis/integration.py (Diff revision 2)
     
     

    Missing file-level docstring.

  8. rbintegrations/idonethis/integration.py (Diff revision 2)
     
     

    Must inherit from object.

    1. Moved to a separate module instead of a class.

  9. rbintegrations/idonethis/integration.py (Diff revision 2)
     
     
     
     

    unicode.format is slow compared to using %-based interpolation (and we also don't really use the former elsewhere). You can change these to use:

    format_strings = {
        REPLY_PUBLISHED:
            'Replied to review request %(request_id)s: '
            '%(summary)s %(url)s %(group_tags)s',
    
        #...
    }
    

    and then interpolate with:

    format_strings[entry] % {
        'group_tags': group_tags,
        'num_issues': num_issues,
        'request_id': review_request.display_id,
        'summary': review_request.summary,
        'url': url,
    }
    
    1. After looking more into possible string formatting, both .format() and % have issues with being able to properly validate custom format strings. I've decided to use string.Template instead, which provides very limited named placeholder replacement. It works better for admin-specified format strings.

  10. rbintegrations/idonethis/integration.py (Diff revision 2)
     
     

    You may want to replace - with _ in group.name because idt doesn't like tags with dashes (but underscores work for some reason :/)

    1. Good catch!
      Are there any other special characters allowed in group names that I may need to worry about?

  11. rbintegrations/idonethis/integration.py (Diff revision 2)
     
     

    Why not just use, e.g.:

    import re
    #...
    
    re.sub(r'\s{2,}', ' ', entry)
    

    You can compile the regex with re.compile so we only have to parse the regex once.

    I'm not sure which is more expensive

    1. I was mostly going for simplicity and avoiding additional imports, but I could change to re. The preformance probably doesn't matter since urlopen will be slower than any string operations.

  12. rbintegrations/idonethis/integration.py (Diff revision 2)
     
     
     

    Coalesce into elif

  13. rbintegrations/idonethis/pages.py (Diff revision 2)
     
     

    File-level docstring.

  14. rbintegrations/idonethis/tests.py (Diff revision 2)
     
     

    File-level docstring.

  15. rbintegrations/idonethis/utils.py (Diff revision 2)
     
     

    File-level docstring.

  16. 
      
Michael Udaltsov
Review request changed

Change Summary:

Addressed Barret's review comments.
Moved entry types, template strings, and formatting to entries.py.
Changed template string formatting to use string.Template.
Renamed event_name to signal_name for clarity, and to avoid confusion with custom event filtering (separate change).
Switched to explicit keyword arguments when calling IDoneThisIntegration.post_entry().

Commit:

-12e3ce01ac26185b641a21046d9f69c09b00ad36
+ac51636aa0952f19162af89e9145298fe22af258

Diff:

Revision 3 (+2859)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Christian Hammond
  1. Sorry this took so long :( Change looks great, and I love the test coverage. Most of my stuff are small doc tweaks and other tiny suggestions.

  2. rbintegrations/idonethis/entries.py (Diff revision 3)
     
     

    This should be "Format".

  3. rbintegrations/idonethis/entries.py (Diff revision 3)
     
     

    Text like ${num_issues} should be surrounded with double backticks so it'll appear as code/literal text in docs (not that we generate them yet, but still).

    Same with others in this function.

  4. rbintegrations/idonethis/entries.py (Diff revision 3)
     
     

    We should use review_request_id instead of request_id in places. In our terminology, "Request" is the HTTP request.

  5. rbintegrations/idonethis/entries.py (Diff revision 3)
     
     
     

    I couple suggestions for formatting and optimizations:

    group_names = review_request.target_groups.values_list('name', flat=True)
    
    group_tags = ' '.join(
        '#%s' % re.sub(r'\W', '_', group_name)
        for group_name in group_names
    )
    

    We should also ideally precompile the regex in the module.

  6. rbintegrations/idonethis/entries.py (Diff revision 3)
     
     

    Let's precompile this one too.

  7. rbintegrations/idonethis/forms.py (Diff revision 3)
     
     
  8. rbintegrations/idonethis/forms.py (Diff revision 3)
     
     
     

    Let's put the URL in <code>...</code>.

  9. rbintegrations/idonethis/forms.py (Diff revision 3)
     
     
  10. rbintegrations/idonethis/forms.py (Diff revision 3)
     
     
     

    We should maybe expand this to say why this might be the case (the API token may be incorrect, please check the token in your account, etc.).

  11. rbintegrations/idonethis/integration.py (Diff revision 3)
     
     

    You can reference by doing:

    :py:mod:`rbintegrations.idonethis.entries`
    
  12. rbintegrations/idonethis/integration.py (Diff revision 3)
     
     
     
     

    Might want to explicitly disregard None entries in the conditional instead of doing the lookup.

  13. rbintegrations/idonethis/integration.py (Diff revision 3)
     
     
     
     
     
     
     

    I'm generally a fan of collapsing down if statements, but given that we have these same checks within the if review.ship_it, maybe these ones should go in an else.

    1. You win over Barret, reverting :)

  14. rbintegrations/idonethis/integration.py (Diff revision 3)
     
     

    Wonder if we should just do the build_server_url in post_entry, so each call site doesn't have to reepeat it.

    1. The default full review request url is already built inside format_template_string. There are just two special cases for reviews and replies. It would be uglier to check for a value and call build_server_url in post_entry.

  15. rbintegrations/idonethis/integration.py (Diff revision 3)
     
     

    Should be "reply".

  16. rbintegrations/idonethis/utils.py (Diff revision 3)
     
     

    "Return ..."

    "Get" implies that it's getting it from somewhere else, and doesn't necessarily imply that it's returning it.

  17. rbintegrations/idonethis/utils.py (Diff revision 3)
     
     
     
     
     

    No need for the settings.get if it's known to be in there.

    Since it's most likely in there, you can actually do:

    try:
        return decrypt_password(settings['api_token'])
    except KeyError:
        return None
    
  18. rbintegrations/idonethis/utils.py (Diff revision 3)
     
     

    Same as above.

  19. rbintegrations/idonethis/utils.py (Diff revision 3)
     
     
  20. We probably should turn this into two different paragraphs, or just let it wrap, rather than forcing a line break.

  21. 
      
Michael Udaltsov
David Trowbridge
  1. This is looking pretty solid!

  2. rbintegrations/idonethis/entries.py (Diff revision 4)
     
     

    This should be models.review_request.ReviewRequest. You can wrap it after any period.

  3. rbintegrations/idonethis/forms.py (Diff revision 4)
     
     
     

    Maybe wrap this as:

    raise forms.ValidationError(
        ugettext('Team ID cannot contain slashes.'))
    
  4. 
      
Michael Udaltsov
Michael Udaltsov
David Trowbridge
  1. This looks great!

    Was there anything else you were planning for this change? If not, can you add the binary files (icons) as file attachments to this review request?

    1. Thanks!
      This change should be good to go. Additional customization will be in a separate change: https://reviews.reviewboard.org/r/8961/ (just need to finish up the unit tests for it)

  2. 
      
Michael Udaltsov
Michael Udaltsov
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (5db5cd3)
Loading...