• 
      

    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:
    Completed
    Change Summary:
    Obsoleted by https://reviews.reviewboard.org/r/6047/