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/