Tight bug tracker integration

Review Request #5531 — Created Feb. 23, 2014 and submitted

Information

Review Board
master
193
af01285...

Reviewers

This is work in progress to make RB integrate better with different bug trackers. This review request lays foundation for tighter bug tracker support by providing an interface that hosting providers can implement to enable more advanced functionality with bug trackers. Currently the interface offers ways to query information about given bug. This information is shown on an infobox shown when a bug is mouse hovered.

There is a discussion on this at review board mailing list: https://groups.google.com/d/msg/reviewboard/DymDgBu_9_Q/HK91wXJ4PhcJ.

I have been testing things with JIRA bug tracker.


Description From Last Updated

There's a lot of excessive blank lines here. Make sure to follow the style used in other files.

chipx86chipx86

Missing trailing period.

chipx86chipx86

Each of these should be documented. Also, the names are very vague, given that this will be an interface that …

chipx86chipx86

Imports must always be in the following groupings, each separated by a blank line: Official Python modules. Third-party Python modules …

chipx86chipx86

Imports should always be absolute. This must be reviewboard.hostingsvcs.bugtracker.

chipx86chipx86

I'm concerned about requiring jira, since not all installs will need it, and it's just one more thing to pull …

chipx86chipx86

Missing a docstring.

chipx86chipx86

No need for the u prefixes, since we're importing unicode_literals above.

chipx86chipx86

Two blank lines was correct.

chipx86chipx86

This can be one import statement.

chipx86chipx86

Here too. Two blank lines between things in the top-level of a file.

chipx86chipx86

No blank line between these.

chipx86chipx86

Make sure all docstrings follow one of these two formats: """Single-line summary.""" or: """Single-line summary. Multi-line description. """ The summary …

chipx86chipx86

Two blank lines between all top-level functions.

chipx86chipx86

Needs a docstring.

chipx86chipx86

This may be None, and 'bug_tracker_type' my not even be in extra_data. Need to account for these possibilities.

chipx86chipx86

Even though you don't have any string literals in here, we want all files to start with this line: from …

daviddavid

Please wrap this to 80 columns.

daviddavid

I think a more common way to do this would be to define the fields in the class like all …

daviddavid

Would you mind adding the parameter name here? (supports_posting_bug_comments=True). That way one doesn't have to open up the BugTrackerForm definition …

daviddavid

For single lines like this, you don't need the outer parentheses.

daviddavid

This should be wrapped for localization.

daviddavid

This should be wrapped for localization.

daviddavid

This should be wrapped for localization.

daviddavid

Leftover debug output?

daviddavid

Can you wrap this to 80 columns?

daviddavid

Should have two blank lines between here.

daviddavid

Wrap to 80 columns.

daviddavid

Generally we use '' instead of str()

daviddavid

Remove the spcase before the :s.

daviddavid

Should have two blank lines here.

daviddavid

The id here should probably be bug-infobox.

daviddavid

The id here should probably be bug-infobox.

daviddavid

Looks like this doesn't yet include the bug ID from the page?

daviddavid

This should probably either have its own cache, or change the name of this to be more general.

daviddavid

This doesn't look right. You've added .bug_infobox() in the middle of a function definition. I think it probably belongs at …

daviddavid

This should only be called when hosting service acting as a bug tracker supports any metadata (description, status or summary)

TO tomiaijo

I think this is getting a little unweildy for a list comprehension. We might want to split it up into …

daviddavid

Alignment isn't quite right here.

daviddavid

Can you revert this part of the change? I prefer it as three lines.

daviddavid

Same question here about the replace.

daviddavid

Mind switching the name attributes to use single-quotes?

daviddavid

Can you reformat this to look more like the one in the method below it?

daviddavid

Can you wrap this string in _()?

daviddavid

Single quotes?

daviddavid

Can you switch the single quotes for double and vice-versa?

daviddavid

Can you switch the "a" to use single quotes?

daviddavid

Single quotes?

daviddavid

Is it really possible for this to fail?

daviddavid

Please remove this line.

daviddavid

Please remove this line.

daviddavid

Perhaps we should just have the default implementations return empty strings instead of raising NotImplementedError? Then this could just be …

daviddavid

Please remove this line.

daviddavid

Can you call this $infobox so it's clear it's a jquery object?

daviddavid

Please put a space before the />. Also, you could combine these two lines: infobox = $("<div id='bug-infobox' />") .hide() …

daviddavid

It's a little outside our javascript coding style to assign functions to vars. Can you just define it as a …

daviddavid

A lot of this definition seems to be shared with the user infobox. Can you see what you can do …

daviddavid

Now that we use this in two places, can we pull it out into a variable?

daviddavid

This isn't used anymore, right?

daviddavid

I don't see an ID of "bug-infobox-text", only a class. Should this be .bug-infobox-text ?

daviddavid

This isn't part of your bug infobox, and is now defined twice.

daviddavid

Can you pull out this function and name it? (put it alongside showInfobox). Right now it gets re-defined for every …

daviddavid

Can you pull out this function and name it? (put it alongside showInfobox). Right now it gets re-defined for every …

daviddavid

Can you pull out this function and name it? (put it alongside showInfobox). Right now it gets re-defined for every …

daviddavid

This should be called $bugList

daviddavid

Can you attach these to the $bug_list definition, instead of in here?

daviddavid

Can you make showInfobox and fetchInfobox take the arguments in the same order?

daviddavid

This might read better if we break it up into two statements: var bugList = view.urlizeList(data, function(item) { return bugTrackerURL.replace('_bug_id', …

daviddavid
TO
TO
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/bugtracker.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    There's a lot of excessive blank lines here. Make sure to follow the style used in other files.

  3. reviewboard/hostingsvcs/bugtracker.py (Diff revision 2)
     
     
    Show all issues

    Missing trailing period.

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

    Each of these should be documented.

    Also, the names are very vague, given that this will be an interface that a HostingService will inherit from. Should change the names to things like get_bug_description, get_bug_status, post_bug_comment, etc.

  5. reviewboard/hostingsvcs/jira.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Imports must always be in the following groupings, each separated by a blank line:

    1. Official Python modules.
    2. Third-party Python modules (django, djblets, jira)
    3. Project's Python modules.

    They also need to be in alphabetical order.

  6. reviewboard/hostingsvcs/jira.py (Diff revision 2)
     
     
    Show all issues

    Imports should always be absolute. This must be reviewboard.hostingsvcs.bugtracker.

  7. reviewboard/hostingsvcs/jira.py (Diff revision 2)
     
     
     
    Show all issues

    I'm concerned about requiring jira, since not all installs will need it, and it's just one more thing to pull down and install.

    We should make this an optional dependency instead. We do this elsewhere. You can do this with:

    try:
        from jira blah blah
        has_jira = True
    except ImportError:
        has_jira = False
    

    Then you can check has_jira before doing any operations, and failing gracefully.

    (Note that we'll want to figure out a way of communicating to the user that some support is missing, but we can tackle that later.)

  8. reviewboard/hostingsvcs/jira.py (Diff revision 2)
     
     
    Show all issues

    Missing a docstring.

  9. reviewboard/hostingsvcs/jira.py (Diff revision 2)
     
     
    Show all issues

    No need for the u prefixes, since we're importing unicode_literals above.

  10. reviewboard/notifications/__init__.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    Two blank lines was correct.

  11. reviewboard/notifications/__init__.py (Diff revision 2)
     
     
     
    Show all issues

    This can be one import statement.

  12. reviewboard/notifications/__init__.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    Here too.

    Two blank lines between things in the top-level of a file.

  13. reviewboard/notifications/bugtracker.py (Diff revision 2)
     
     
     
     
    Show all issues

    No blank line between these.

  14. reviewboard/notifications/bugtracker.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues

    Make sure all docstrings follow one of these two formats:

    """Single-line summary."""
    

    or:

    """Single-line summary.
    
    Multi-line description.
    """
    

    The summary cannot ever wrap or exceed the 79 character line limit.

    This applies throughout the file.

    (I know we have some older files that violate this, but we're slowly fixing those up.)

  15. reviewboard/notifications/bugtracker.py (Diff revision 2)
     
     
     
     
    Show all issues

    Two blank lines between all top-level functions.

  16. reviewboard/scmtools/models.py (Diff revision 2)
     
     
    Show all issues

    Needs a docstring.

  17. reviewboard/scmtools/models.py (Diff revision 2)
     
     
    Show all issues

    This may be None, and 'bug_tracker_type' my not even be in extra_data. Need to account for these possibilities.

  18. 
      
TO
TO
TO
TO
TO
david
  1. 
      
  2. reviewboard/hostingsvcs/bugtracker.py (Diff revision 5)
     
     
    Show all issues

    Even though you don't have any string literals in here, we want all files to start with this line:

    from __future__ import unicode_literals
    
  3. reviewboard/hostingsvcs/forms.py (Diff revision 5)
     
     
    Show all issues

    Please wrap this to 80 columns.

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

    I think a more common way to do this would be to define the fields in the class like all the other forms, and then here you could do:

    if not supports_posting_bug_comments:
        del self.fields['post_comment_on_publish']
        del self.fields['post_comment_on_closed']
        del self.fields['post_comment_on_reviews]
    
  5. reviewboard/hostingsvcs/googlecode.py (Diff revision 5)
     
     
    Show all issues

    Would you mind adding the parameter name here? (supports_posting_bug_comments=True). That way one doesn't have to open up the BugTrackerForm definition to see what that parameter is.

  6. reviewboard/notifications/__init__.py (Diff revision 5)
     
     
    Show all issues

    For single lines like this, you don't need the outer parentheses.

  7. reviewboard/notifications/bugtracker.py (Diff revision 5)
     
     
    Show all issues

    This should be wrapped for localization.

  8. reviewboard/notifications/bugtracker.py (Diff revision 5)
     
     
    Show all issues

    This should be wrapped for localization.

  9. reviewboard/notifications/bugtracker.py (Diff revision 5)
     
     
    Show all issues

    This should be wrapped for localization.

  10. reviewboard/notifications/bugtracker.py (Diff revision 5)
     
     
    Show all issues

    Leftover debug output?

  11. reviewboard/reviews/builtin_fields.py (Diff revision 5)
     
     
    Show all issues

    Can you wrap this to 80 columns?

  12. reviewboard/reviews/views.py (Diff revision 5)
     
     
     
     
    Show all issues

    Should have two blank lines between here.

  13. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues

    Wrap to 80 columns.

  14. reviewboard/reviews/views.py (Diff revision 5)
     
     
     
     
    Show all issues

    Generally we use '' instead of str()

  15. reviewboard/reviews/views.py (Diff revision 5)
     
     
     
    Show all issues

    Remove the spcase before the :s.

  16. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues

    Should have two blank lines here.

  17. reviewboard/static/rb/js/common.js (Diff revision 5)
     
     
    Show all issues

    The id here should probably be bug-infobox.

  18. reviewboard/static/rb/js/common.js (Diff revision 5)
     
     
    Show all issues

    The id here should probably be bug-infobox.

  19. reviewboard/static/rb/js/common.js (Diff revision 5)
     
     
    Show all issues

    Looks like this doesn't yet include the bug ID from the page?

  20. reviewboard/static/rb/js/common.js (Diff revision 5)
     
     
    Show all issues

    This should probably either have its own cache, or change the name of this to be more general.

  21. Show all issues

    This doesn't look right. You've added .bug_infobox() in the middle of a function definition. I think it probably belongs at the very end of this line (so $el.html(...).bug_infobox()).

  22. 
      
TO
TO
TO
TO
TO
  1. 
      
  2. reviewboard/static/rb/js/common.js (Diff revision 8)
     
     
    Show all issues

    This should only be called when hosting service acting as a bug tracker supports any metadata (description, status or summary)

  3. 
      
TO
TO
TO
david
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 10)
     
     
     
     
     
     
     
    Show all issues

    I think this is getting a little unweildy for a list comprehension. We might want to split it up into multiple statements.

    Also, I'm curious why you use _bug_id and replace instead of just passing in bug in the args.

  3. reviewboard/reviews/builtin_fields.py (Diff revision 10)
     
     
     
    Show all issues

    Alignment isn't quite right here.

  4. reviewboard/reviews/builtin_fields.py (Diff revision 10)
     
     
     
     
    Show all issues

    Can you revert this part of the change? I prefer it as three lines.

  5. reviewboard/reviews/builtin_fields.py (Diff revision 10)
     
     
     
     
    Show all issues

    Same question here about the replace.

  6. reviewboard/reviews/urls.py (Diff revision 10)
     
     
     
    Show all issues

    Mind switching the name attributes to use single-quotes?

  7. reviewboard/reviews/views.py (Diff revision 10)
     
     
     
     
    Show all issues

    Can you reformat this to look more like the one in the method below it?

  8. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues

    Can you wrap this string in _()?

  9. reviewboard/static/rb/js/common.js (Diff revision 10)
     
     
    Show all issues

    Single quotes?

  10. reviewboard/static/rb/js/common.js (Diff revision 10)
     
     
    Show all issues

    Can you switch the single quotes for double and vice-versa?

  11. Show all issues

    Can you switch the "a" to use single quotes?

  12. Show all issues

    Single quotes?

  13. 
      
TO
david
  1. 
      
  2. reviewboard/datagrids/columns.py (Diff revision 11)
     
     
     
     
     
     
    Show all issues

    Is it really possible for this to fail?

  3. reviewboard/reviews/views.py (Diff revision 11)
     
     
    Show all issues

    Please remove this line.

  4. reviewboard/reviews/views.py (Diff revision 11)
     
     
    Show all issues

    Please remove this line.

  5. reviewboard/reviews/views.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Perhaps we should just have the default implementations return empty strings instead of raising NotImplementedError? Then this could just be the three assignment lines.

  6. reviewboard/scmtools/models.py (Diff revision 11)
     
     
    Show all issues

    Please remove this line.

  7. reviewboard/static/rb/js/common.js (Diff revision 11)
     
     
    Show all issues

    Can you call this $infobox so it's clear it's a jquery object?

  8. reviewboard/static/rb/js/common.js (Diff revision 11)
     
     
     
    Show all issues

    Please put a space before the />. Also, you could combine these two lines:

    infobox = $("<div id='bug-infobox' />")
        .hide()
        .appendTo(document.body);
    
  9. reviewboard/static/rb/js/common.js (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    It's a little outside our javascript coding style to assign functions to vars. Can you just define it as a function inside the top-level bug_infobox scope?

    Something like this:

    $.fn.bug_infobox = function() {
        var POPUP_DELAY_MS = 500,
            ...
    
        function showInfobox() {
            ...
        }
    
        return this.each(function() {
            ...
        }
    }
    
  10. 
      
TO
david
  1. 
      
  2. reviewboard/static/rb/css/common.less (Diff revision 12)
     
     
    Show all issues

    A lot of this definition seems to be shared with the user infobox. Can you see what you can do to share rules?

  3. reviewboard/static/rb/css/common.less (Diff revision 12)
     
     
    Show all issues

    Now that we use this in two places, can we pull it out into a variable?

  4. reviewboard/static/rb/css/common.less (Diff revision 12)
     
     
     
     
     
     
     
    Show all issues

    This isn't used anymore, right?

  5. reviewboard/static/rb/css/common.less (Diff revision 12)
     
     
    Show all issues

    I don't see an ID of "bug-infobox-text", only a class. Should this be .bug-infobox-text ?

  6. reviewboard/static/rb/css/common.less (Diff revision 12)
     
     
     
     
    Show all issues

    This isn't part of your bug infobox, and is now defined twice.

  7. reviewboard/static/rb/js/common.js (Diff revision 12)
     
     
    Show all issues

    Can you pull out this function and name it? (put it alongside showInfobox).

    Right now it gets re-defined for every matching element in the selector.

  8. reviewboard/static/rb/js/common.js (Diff revision 12)
     
     
    Show all issues

    Can you pull out this function and name it? (put it alongside showInfobox).

    Right now it gets re-defined for every matching element in the selector.

    1. I tried to pull out this and the one above it. It seems that is causes issues with the timeout variable, it is somehow not passed by reference.

      Does it make sense to pull out these two anyways as you still have to define a new anonymous function that calls the function pulled out as it has two parameters ($infobox and timeout)?

  9. reviewboard/static/rb/js/common.js (Diff revision 12)
     
     
    Show all issues

    Can you pull out this function and name it? (put it alongside showInfobox).

    Right now it gets re-defined for every matching element in the selector.

  10. Show all issues

    This should be called $bugList

  11. Show all issues

    Can you attach these to the $bug_list definition, instead of in here?

  12. 
      
TO
TO
TO
TO
david
  1. 
      
  2. reviewboard/static/rb/js/common.js (Diff revision 16)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Can you make showInfobox and fetchInfobox take the arguments in the same order?

  3. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 16)
     
     
     
     
     
     
    Show all issues

    This might read better if we break it up into two statements:

    var bugList = view.urlizeList(data, function(item) {
            return bugTrackerURL.replace('_bug_id', item);
        }),
        $bugList = $(bugList)
            .addClass('bug')
            .bug_infobox();
    
  4. 
      
TO
TO
Review request changed

Status: Closed (submitted)

Change Summary:

Obsoleted by https://reviews.reviewboard.org/r/6047/
Loading...