• 
      

    Add support for JIRA bug tracker

    Review Request #5745 — Created April 28, 2014 and discarded

    Information

    Review Board
    master
    bfc54a9...

    Reviewers

    Adds support for JIRA bug tracker.

    Tested against a local JIRA instance and Atlassian demo JIRA at https://jira.atlassian.com/

    Description From Last Updated

    These should be rearranged a bit to follow the PEP-8 format of standard lib, third-party, this package. You can also …

    daviddavid

    Indentation in here has an extra 4 spaces.

    daviddavid

    This could use self._get_issue()

    daviddavid

    When logging things from within an exception handler, it's usually nice to add exc_info=1 to the logging.*() parameters.

    daviddavid

    This could use self._get_issue()

    daviddavid

    Should add exc_info=1

    daviddavid

    Should add exc_info=1

    daviddavid

    Is it worth caching the result of this call anywhere? It seems likely that if you want one part of …

    daviddavid

    You may want to also trim whitespace.

    daviddavid

    Please add a docstring.

    daviddavid

    Please add a blank line between these two.

    daviddavid

    These could be combined into one statement.

    daviddavid

    Please add a docstring.

    daviddavid

    Please add a blank line between these two.

    daviddavid

    These can be combined.

    daviddavid

    Please add a docstring.

    daviddavid

    Please add a blank line.

    daviddavid

    These can be combined.

    daviddavid

    Can you reformat this to put all the keys one indentation level down? issue = { 'description': ..., 'summary': ... …

    daviddavid

    This should probably also include the URL of the JIRA server, in case there are multiple JIRA servers present.

    daviddavid

    These could be combined into a single statement.

    daviddavid

    If we get an exception, this will return None, which your other change will then just insert into the template. …

    daviddavid

    Same question about None vs ''.

    daviddavid

    Same question about None vs ''.

    daviddavid
    david
    1. 
        
    2. reviewboard/hostingsvcs/jirasvc.py (Diff revision 1)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      These should be rearranged a bit to follow the PEP-8 format of standard lib, third-party, this package. You can also catch ImportError without assigning it to a variable because you don't use the exception.

      import logging
      
      from django import forms
      from django.utils.translation import ugettext_lazy as _
      try:
          from jira.client import JRIA as JIRAClient
          from jira.exceptions import JIRAError
          has_jira = True
      except ImportError:
          has_jira = False
      
      from reviewboard.hostingsvcs.bugtracker import BugTracker
      ...
      
    3. reviewboard/hostingsvcs/jirasvc.py (Diff revision 1)
       
       
       
       
       
       
       
       
       
      Show all issues

      Indentation in here has an extra 4 spaces.

    4. reviewboard/hostingsvcs/jirasvc.py (Diff revision 1)
       
       
      Show all issues

      This could use self._get_issue()

    5. reviewboard/hostingsvcs/jirasvc.py (Diff revision 1)
       
       
       
      Show all issues

      When logging things from within an exception handler, it's usually nice to add exc_info=1 to the logging.*() parameters.

    6. reviewboard/hostingsvcs/jirasvc.py (Diff revision 1)
       
       
      Show all issues

      This could use self._get_issue()

    7. reviewboard/hostingsvcs/jirasvc.py (Diff revision 1)
       
       
       
      Show all issues

      Should add exc_info=1

    8. reviewboard/hostingsvcs/jirasvc.py (Diff revision 1)
       
       
       
      Show all issues

      Should add exc_info=1

    9. reviewboard/hostingsvcs/jirasvc.py (Diff revision 1)
       
       
       
      Show all issues

      Is it worth caching the result of this call anywhere? It seems likely that if you want one part of the issue data, you'll want the others.

      1. It easily could be, but how should this cache be expired?

        On the other hand, it might be the JIRA library that should take care of caching requests.

      2. Can you do some investigation to see if the JIRA library does this? (I'm assuming it's a third-party library? where is it from?)

        If not, you could easy use django's caching primitives to store this in memcached with a reasonably quick timeout. It just seems silly to fetch three times from the server in quick succession.

      3. It seems that no caching is performed by the library. It is indeed a third-party library that wraps the official JIRA REST API.

        The source is available at https://bitbucket.org/bspeakmon/jira-python and the libary can be installed from PyPI.

        I will look into the Django caching primitives (django.core.cache I assume).

    10. 
        
    TO
    TO
    TO
    david
    1. 
        
    2. reviewboard/hostingsvcs/jirasvc.py (Diff revision 4)
       
       
      Show all issues

      You may want to also trim whitespace.

    3. reviewboard/hostingsvcs/jirasvc.py (Diff revision 4)
       
       
      Show all issues

      Please add a docstring.

    4. reviewboard/hostingsvcs/jirasvc.py (Diff revision 4)
       
       
       
      Show all issues

      Please add a blank line between these two.

    5. reviewboard/hostingsvcs/jirasvc.py (Diff revision 4)
       
       
       
       
      Show all issues

      These could be combined into one statement.

    6. reviewboard/hostingsvcs/jirasvc.py (Diff revision 4)
       
       
      Show all issues

      Please add a docstring.

    7. reviewboard/hostingsvcs/jirasvc.py (Diff revision 4)
       
       
       
      Show all issues

      Please add a blank line between these two.

    8. reviewboard/hostingsvcs/jirasvc.py (Diff revision 4)
       
       
       
       
      Show all issues

      These can be combined.

    9. reviewboard/hostingsvcs/jirasvc.py (Diff revision 4)
       
       
      Show all issues

      Please add a docstring.

    10. reviewboard/hostingsvcs/jirasvc.py (Diff revision 4)
       
       
       
      Show all issues

      Please add a blank line.

    11. reviewboard/hostingsvcs/jirasvc.py (Diff revision 4)
       
       
       
       
      Show all issues

      These can be combined.

    12. reviewboard/hostingsvcs/jirasvc.py (Diff revision 4)
       
       
       
       
      Show all issues

      Can you reformat this to put all the keys one indentation level down?

      issue = {
         'description': ...,
         'summary': ...
         ...
      }
      
    13. reviewboard/hostingsvcs/jirasvc.py (Diff revision 4)
       
       
      Show all issues

      This should probably also include the URL of the JIRA server, in case there are multiple JIRA servers present.

    14. reviewboard/hostingsvcs/jirasvc.py (Diff revision 4)
       
       
       
      Show all issues

      These could be combined into a single statement.

    15. 
        
    TO
    TO
    david
    1. 
        
    2. reviewboard/hostingsvcs/jirasvc.py (Diff revision 6)
       
       
       
       
       
       
      Show all issues

      If we get an exception, this will return None, which your other change will then just insert into the template. Should this return ''?

    3. reviewboard/hostingsvcs/jirasvc.py (Diff revision 6)
       
       
       
       
       
       
      Show all issues

      Same question about None vs ''.

    4. reviewboard/hostingsvcs/jirasvc.py (Diff revision 6)
       
       
       
       
       
       
      Show all issues

      Same question about None vs ''.

    5. 
        
    TO
    TO
    TO
    TO
    Review request changed
    Status:
    Discarded
    Change Summary:
    Obsoleted by https://reviews.reviewboard.org/r/6047/