Change Summary:
Added more signals to bug tracker integration
Branch: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 2 (+192 -2) |
Review Request #5531 — Created Feb. 23, 2014 and submitted
Information | |
---|---|
tomiaijo | |
Review Board | |
master | |
193 | |
5769, 5745 | |
af01285... | |
Reviewers | |
reviewboard, students | |
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. |
|
|
Missing trailing period. |
|
|
Each of these should be documented. Also, the names are very vague, given that this will be an interface that … |
|
|
Imports must always be in the following groupings, each separated by a blank line: Official Python modules. Third-party Python modules … |
|
|
Imports should always be absolute. This must be reviewboard.hostingsvcs.bugtracker. |
|
|
I'm concerned about requiring jira, since not all installs will need it, and it's just one more thing to pull … |
|
|
Missing a docstring. |
|
|
No need for the u prefixes, since we're importing unicode_literals above. |
|
|
Two blank lines was correct. |
|
|
This can be one import statement. |
|
|
Here too. Two blank lines between things in the top-level of a file. |
|
|
No blank line between these. |
|
|
Make sure all docstrings follow one of these two formats: """Single-line summary.""" or: """Single-line summary. Multi-line description. """ The summary … |
|
|
Two blank lines between all top-level functions. |
|
|
Needs a docstring. |
|
|
This may be None, and 'bug_tracker_type' my not even be in extra_data. Need to account for these possibilities. |
|
|
Even though you don't have any string literals in here, we want all files to start with this line: from … |
|
|
Please wrap this to 80 columns. |
|
|
I think a more common way to do this would be to define the fields in the class like all … |
|
|
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 … |
|
|
For single lines like this, you don't need the outer parentheses. |
|
|
This should be wrapped for localization. |
|
|
This should be wrapped for localization. |
|
|
This should be wrapped for localization. |
|
|
Leftover debug output? |
|
|
Can you wrap this to 80 columns? |
|
|
Should have two blank lines between here. |
|
|
Wrap to 80 columns. |
|
|
Generally we use '' instead of str() |
|
|
Remove the spcase before the :s. |
|
|
Should have two blank lines here. |
|
|
The id here should probably be bug-infobox. |
|
|
The id here should probably be bug-infobox. |
|
|
Looks like this doesn't yet include the bug ID from the page? |
|
|
This should probably either have its own cache, or change the name of this to be more general. |
|
|
This doesn't look right. You've added .bug_infobox() in the middle of a function definition. I think it probably belongs at … |
|
|
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 … |
|
|
Alignment isn't quite right here. |
|
|
Can you revert this part of the change? I prefer it as three lines. |
|
|
Same question here about the replace. |
|
|
Mind switching the name attributes to use single-quotes? |
|
|
Can you reformat this to look more like the one in the method below it? |
|
|
Can you wrap this string in _()? |
|
|
Single quotes? |
|
|
Can you switch the single quotes for double and vice-versa? |
|
|
Can you switch the "a" to use single quotes? |
|
|
Single quotes? |
|
|
Is it really possible for this to fail? |
|
|
Please remove this line. |
|
|
Please remove this line. |
|
|
Perhaps we should just have the default implementations return empty strings instead of raising NotImplementedError? Then this could just be … |
|
|
Please remove this line. |
|
|
Can you call this $infobox so it's clear it's a jquery object? |
|
|
Please put a space before the />. Also, you could combine these two lines: infobox = $("<div id='bug-infobox' />") .hide() … |
|
|
It's a little outside our javascript coding style to assign functions to vars. Can you just define it as a … |
|
|
A lot of this definition seems to be shared with the user infobox. Can you see what you can do … |
|
|
Now that we use this in two places, can we pull it out into a variable? |
|
|
This isn't used anymore, right? |
|
|
I don't see an ID of "bug-infobox-text", only a class. Should this be .bug-infobox-text ? |
|
|
This isn't part of your bug infobox, and is now defined twice. |
|
|
Can you pull out this function and name it? (put it alongside showInfobox). Right now it gets re-defined for every … |
|
|
Can you pull out this function and name it? (put it alongside showInfobox). Right now it gets re-defined for every … |
|
|
Can you pull out this function and name it? (put it alongside showInfobox). Right now it gets re-defined for every … |
|
|
This should be called $bugList |
|
|
Can you attach these to the $bug_list definition, instead of in here? |
|
|
Can you make showInfobox and fetchInfobox take the arguments in the same order? |
|
|
This might read better if we break it up into two statements: var bugList = view.urlizeList(data, function(item) { return bugTrackerURL.replace('_bug_id', … |
|
Added more signals to bug tracker integration
Branch: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 2 (+192 -2) |
reviewboard/hostingsvcs/bugtracker.py (Diff revision 2) |
---|
There's a lot of excessive blank lines here. Make sure to follow the style used in other files.
reviewboard/hostingsvcs/bugtracker.py (Diff revision 2) |
---|
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.
reviewboard/hostingsvcs/jira.py (Diff revision 2) |
---|
Imports must always be in the following groupings, each separated by a blank line:
- Official Python modules.
- Third-party Python modules (django, djblets, jira)
- Project's Python modules.
They also need to be in alphabetical order.
reviewboard/hostingsvcs/jira.py (Diff revision 2) |
---|
Imports should always be absolute. This must be
reviewboard.hostingsvcs.bugtracker
.
reviewboard/hostingsvcs/jira.py (Diff revision 2) |
---|
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.)
reviewboard/hostingsvcs/jira.py (Diff revision 2) |
---|
No need for the
u
prefixes, since we're importing unicode_literals above.
reviewboard/notifications/__init__.py (Diff revision 2) |
---|
Here too.
Two blank lines between things in the top-level of a file.
reviewboard/notifications/bugtracker.py (Diff revision 2) |
---|
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.)
reviewboard/notifications/bugtracker.py (Diff revision 2) |
---|
Two blank lines between all top-level functions.
reviewboard/scmtools/models.py (Diff revision 2) |
---|
This may be None, and
'bug_tracker_type'
my not even be inextra_data
. Need to account for these possibilities.
Initial and partly hard coded implementation of bug infobox
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+314 -5) |
Start implementing configuration UI
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+374 -6) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+286 -6) |
||||
Removed Files: |
|||||
Added Files: |
reviewboard/hostingsvcs/bugtracker.py (Diff revision 5) |
---|
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
reviewboard/hostingsvcs/forms.py (Diff revision 5) |
---|
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]
reviewboard/hostingsvcs/googlecode.py (Diff revision 5) |
---|
Would you mind adding the parameter name here? (
supports_posting_bug_comments=True
). That way one doesn't have to open up theBugTrackerForm
definition to see what that parameter is.
reviewboard/notifications/__init__.py (Diff revision 5) |
---|
For single lines like this, you don't need the outer parentheses.
reviewboard/static/rb/js/common.js (Diff revision 5) |
---|
Looks like this doesn't yet include the bug ID from the page?
reviewboard/static/rb/js/common.js (Diff revision 5) |
---|
This should probably either have its own cache, or change the name of this to be more general.
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 5) |
---|
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()
).
Fixed most of the issues
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+380 -2) |
Fix few issues and attemp to finalize bug infobox
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+425 -20) |
Changed this review request to concern inbound information
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 8 (+264 -22) |
|||||||||||||||||||||||||||||||||||||||||||||
Removed Files: |
reviewboard/static/rb/js/common.js (Diff revision 8) |
---|
This should only be called when hosting service acting as a bug tracker supports any metadata (description, status or summary)
Clean up some accidental changes
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+265 -19) |
Show infobox only if supported by the chosen bug tracker
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+268 -19) |
reviewboard/datagrids/columns.py (Diff revision 10) |
---|
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
andreplace
instead of just passing inbug
in theargs
.
reviewboard/reviews/builtin_fields.py (Diff revision 10) |
---|
Can you revert this part of the change? I prefer it as three lines.
reviewboard/reviews/urls.py (Diff revision 10) |
---|
Mind switching the name attributes to use single-quotes?
reviewboard/reviews/views.py (Diff revision 10) |
---|
Can you reformat this to look more like the one in the method below it?
reviewboard/static/rb/js/common.js (Diff revision 10) |
---|
Can you switch the single quotes for double and vice-versa?
reviewboard/static/rb/js/dashboard/views/dashboardView.js (Diff revision 10) |
---|
Can you switch the
"a"
to use single quotes?
Fix issues and add more exception handling
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+278 -24) |
reviewboard/reviews/views.py (Diff revision 11) |
---|
Perhaps we should just have the default implementations return empty strings instead of raising NotImplementedError? Then this could just be the three assignment lines.
reviewboard/static/rb/js/common.js (Diff revision 11) |
---|
Can you call this $infobox so it's clear it's a jquery object?
reviewboard/static/rb/js/common.js (Diff revision 11) |
---|
Please put a space before the
/>
. Also, you could combine these two lines:infobox = $("<div id='bug-infobox' />") .hide() .appendTo(document.body);
reviewboard/static/rb/js/common.js (Diff revision 11) |
---|
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() { ... } }
Fix issues
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+258 -24) |
reviewboard/static/rb/css/common.less (Diff revision 12) |
---|
A lot of this definition seems to be shared with the user infobox. Can you see what you can do to share rules?
reviewboard/static/rb/css/common.less (Diff revision 12) |
---|
Now that we use this in two places, can we pull it out into a variable?
reviewboard/static/rb/css/common.less (Diff revision 12) |
---|
I don't see an ID of "bug-infobox-text", only a class. Should this be
.bug-infobox-text
?
reviewboard/static/rb/css/common.less (Diff revision 12) |
---|
This isn't part of your bug infobox, and is now defined twice.
reviewboard/static/rb/js/common.js (Diff revision 12) |
---|
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.
reviewboard/static/rb/js/common.js (Diff revision 12) |
---|
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.
reviewboard/static/rb/js/common.js (Diff revision 12) |
---|
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.
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 12) |
---|
This should be called $bugList
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 12) |
---|
Can you attach these to the $bug_list definition, instead of in here?
Add parameter repository to get_bug_ -methods so that bug trackers can resolve their url
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+259 -24) |
Clean up common.[less|js]
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 14 (+250 -25) |
Fix indentation
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 15 (+250 -25) |
Make common.less more compact
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 16 (+252 -46) |
reviewboard/static/rb/js/common.js (Diff revision 16) |
---|
Can you make
showInfobox
andfetchInfobox
take the arguments in the same order?
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 16) |
---|
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();
Fix issues
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 17 (+253 -46) |