- Change Summary:
-
Added more signals to bug tracker integration
- Branch:
-
feature/TighterBugTrackerIntegrationmaster
- Commit:
-
506a5f87f2c35ac7d639737a8b1d254682d5083764398a915a7cb02a88d649d67d5ae38b0d562325
Tight bug tracker integration
Review Request #5531 — Created Feb. 23, 2014 and submitted
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. |
chipx86 | |
Missing trailing period. |
chipx86 | |
Each of these should be documented. Also, the names are very vague, given that this will be an interface that … |
chipx86 | |
Imports must always be in the following groupings, each separated by a blank line: Official Python modules. Third-party Python modules … |
chipx86 | |
Imports should always be absolute. This must be reviewboard.hostingsvcs.bugtracker. |
chipx86 | |
I'm concerned about requiring jira, since not all installs will need it, and it's just one more thing to pull … |
chipx86 | |
Missing a docstring. |
chipx86 | |
No need for the u prefixes, since we're importing unicode_literals above. |
chipx86 | |
Two blank lines was correct. |
chipx86 | |
This can be one import statement. |
chipx86 | |
Here too. Two blank lines between things in the top-level of a file. |
chipx86 | |
No blank line between these. |
chipx86 | |
Make sure all docstrings follow one of these two formats: """Single-line summary.""" or: """Single-line summary. Multi-line description. """ The summary … |
chipx86 | |
Two blank lines between all top-level functions. |
chipx86 | |
Needs a docstring. |
chipx86 | |
This may be None, and 'bug_tracker_type' my not even be in extra_data. Need to account for these possibilities. |
chipx86 | |
Even though you don't have any string literals in here, we want all files to start with this line: from … |
david | |
Please wrap this to 80 columns. |
david | |
I think a more common way to do this would be to define the fields in the class like all … |
david | |
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 … |
david | |
For single lines like this, you don't need the outer parentheses. |
david | |
This should be wrapped for localization. |
david | |
This should be wrapped for localization. |
david | |
This should be wrapped for localization. |
david | |
Leftover debug output? |
david | |
Can you wrap this to 80 columns? |
david | |
Should have two blank lines between here. |
david | |
Wrap to 80 columns. |
david | |
Generally we use '' instead of str() |
david | |
Remove the spcase before the :s. |
david | |
Should have two blank lines here. |
david | |
The id here should probably be bug-infobox. |
david | |
The id here should probably be bug-infobox. |
david | |
Looks like this doesn't yet include the bug ID from the page? |
david | |
This should probably either have its own cache, or change the name of this to be more general. |
david | |
This doesn't look right. You've added .bug_infobox() in the middle of a function definition. I think it probably belongs at … |
david | |
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 … |
david | |
Alignment isn't quite right here. |
david | |
Can you revert this part of the change? I prefer it as three lines. |
david | |
Same question here about the replace. |
david | |
Mind switching the name attributes to use single-quotes? |
david | |
Can you reformat this to look more like the one in the method below it? |
david | |
Can you wrap this string in _()? |
david | |
Single quotes? |
david | |
Can you switch the single quotes for double and vice-versa? |
david | |
Can you switch the "a" to use single quotes? |
david | |
Single quotes? |
david | |
Is it really possible for this to fail? |
david | |
Please remove this line. |
david | |
Please remove this line. |
david | |
Perhaps we should just have the default implementations return empty strings instead of raising NotImplementedError? Then this could just be … |
david | |
Please remove this line. |
david | |
Can you call this $infobox so it's clear it's a jquery object? |
david | |
Please put a space before the />. Also, you could combine these two lines: infobox = $("<div id='bug-infobox' />") .hide() … |
david | |
It's a little outside our javascript coding style to assign functions to vars. Can you just define it as a … |
david | |
A lot of this definition seems to be shared with the user infobox. Can you see what you can do … |
david | |
Now that we use this in two places, can we pull it out into a variable? |
david | |
This isn't used anymore, right? |
david | |
I don't see an ID of "bug-infobox-text", only a class. Should this be .bug-infobox-text ? |
david | |
This isn't part of your bug infobox, and is now defined twice. |
david | |
Can you pull out this function and name it? (put it alongside showInfobox). Right now it gets re-defined for every … |
david | |
Can you pull out this function and name it? (put it alongside showInfobox). Right now it gets re-defined for every … |
david | |
Can you pull out this function and name it? (put it alongside showInfobox). Right now it gets re-defined for every … |
david | |
This should be called $bugList |
david | |
Can you attach these to the $bug_list definition, instead of in here? |
david | |
Can you make showInfobox and fetchInfobox take the arguments in the same order? |
david | |
This might read better if we break it up into two statements: var bugList = view.urlizeList(data, function(item) { return bugTrackerURL.replace('_bug_id', … |
david |
- Groups:
-
-
-
-
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. -
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.
-
-
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.)
-
-
-
-
-
-
-
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.)
-
-
-
This may be None, and
'bug_tracker_type'
my not even be inextra_data
. Need to account for these possibilities.
- Change Summary:
-
Initial and partly hard coded implementation of bug infobox
- Description:
-
This is work in progress to make RB integrate better with different bug trackers.
Current plan is to provide user with bug tracker related actions that can be tied to review board events (RR created / updated / submitted / bug linked to RR). Based on discussion on issue 193 and its duplicates, and mailing list the following actions are the most desired:
- Posting comments
- Creating new (sub)issues
- Change the status of a bug
~ I have also looked into showing information about the bug in review board review request.
~ I have also looked into showing information about the bug in review board review request. This information is now shown in a bug infobox similar to the one shown for users.
~ At the Facebook code sprint, I played around with JIRA bug tracker and hacked together ugly and mostly hard coded integration that posted comments and changed JIRA issue status based on RR status. I have realized that end users have quite different workflows so the action - event -based configuration seems to be the most approriate approach.
~ Currently the implementation is quite messy as I have been mostly creating mvp:s of different features and testing how well they work in practice. Once all the features are frozen I plan on refactoring and cleaning up the code (and fixing the issues raised on this review request).
There is a discussion on this at review board mailing list: https://groups.google.com/d/msg/reviewboard/DymDgBu_9_Q/HK91wXJ4PhcJ.
- Commit:
-
64398a915a7cb02a88d649d67d5ae38b0d5623251286eb97aaad1df982368e1eb5860225ea574723
- Change Summary:
-
Start implementing configuration UI
- Commit:
-
1286eb97aaad1df982368e1eb5860225ea5747233cf36b0b217dedcb8457519344b707e3def10945
- Diff:
-
Revision 4 (+374 -6)
- Commit:
-
3cf36b0b217dedcb8457519344b707e3def1094557e4455ea1dfce870de68ac17b3b10f23b5a1d61
- Diff:
-
Revision 5 (+286 -6)
- Removed Files:
- Added Files:
-
-
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
-
-
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]
-
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. -
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
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()
).
- Change Summary:
-
Fixed most of the issues
- Commit:
-
57e4455ea1dfce870de68ac17b3b10f23b5a1d6124357883d974c2879eb8c40946318ac8690ace79
- Diff:
-
Revision 6 (+380 -2)
- Change Summary:
-
Fix few issues and attemp to finalize bug infobox
- Commit:
-
24357883d974c2879eb8c40946318ac8690ace79d4fde96c3fb7f55244de2189e91d7373c64dc6b8
- Diff:
-
Revision 7 (+425 -20)
- Change Summary:
-
Changed this review request to concern inbound information
- Description:
-
~ This is work in progress to make RB integrate better with different bug trackers.
~ 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.
~ Current plan is to provide user with bug tracker related actions that can be tied to review board events (RR created / updated / submitted / bug linked to RR). Based on discussion on issue 193 and its duplicates, and mailing list the following actions are the most desired:
~ - - - Posting comments
- - Creating new (sub)issues
- - Change the status of a bug
- - I have also looked into showing information about the bug in review board review request. This information is now shown in a bug infobox similar to the one shown for users.
- - Currently the implementation is quite messy as I have been mostly creating mvp:s of different features and testing how well they work in practice. Once all the features are frozen I plan on refactoring and cleaning up the code (and fixing the issues raised on this review request).
There is a discussion on this at review board mailing list: https://groups.google.com/d/msg/reviewboard/DymDgBu_9_Q/HK91wXJ4PhcJ.
- Testing Done:
-
~ I have been testing different work flows with JIRA bug tracker.
~ I have been testing things with JIRA bug tracker.
- Commit:
-
d4fde96c3fb7f55244de2189e91d7373c64dc6b8e8af24445910d5ff0256243a88f72757a6bce2df
- Diff:
-
Revision 8 (+264 -22)
- Removed Files:
- Change Summary:
-
Clean up some accidental changes
- Commit:
-
e8af24445910d5ff0256243a88f72757a6bce2df1cc85f5da209dfaffbf8ab7181a43ff3f54b8b33
- Diff:
-
Revision 9 (+265 -19)
- Change Summary:
-
Show infobox only if supported by the chosen bug tracker
- Commit:
-
1cc85f5da209dfaffbf8ab7181a43ff3f54b8b330811bedcf9ca0faf4eb7c5959e0e1764a4effa2d
- Diff:
-
Revision 10 (+268 -19)
- Change Summary:
-
Fix issues and add more exception handling
- Commit:
-
0811bedcf9ca0faf4eb7c5959e0e1764a4effa2de0c26c05f76503d0b0fedea983ab664c244f6947
- Diff:
-
Revision 11 (+278 -24)
-
-
-
-
-
Perhaps we should just have the default implementations return empty strings instead of raising NotImplementedError? Then this could just be the three assignment lines.
-
-
-
Please put a space before the
/>
. Also, you could combine these two lines:infobox = $("<div id='bug-infobox' />") .hide() .appendTo(document.body);
-
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() { ... } }
- Change Summary:
-
Fix issues
- Commit:
-
e0c26c05f76503d0b0fedea983ab664c244f69472918762613c9b93fb92b81c551a3bd7d787b9ac6
- Diff:
-
Revision 12 (+258 -24)
-
-
A lot of this definition seems to be shared with the user infobox. Can you see what you can do to share rules?
-
-
-
-
-
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.
-
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.
-
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.
-
-
- Change Summary:
-
Add parameter repository to get_bug_ -methods so that bug trackers can resolve their url
- Commit:
-
2918762613c9b93fb92b81c551a3bd7d787b9ac67a6711ac3a41a5469410fa9a84ad62bcbff4669d
- Diff:
-
Revision 13 (+259 -24)
- Change Summary:
-
Clean up common.[less|js]
- Commit:
-
7a6711ac3a41a5469410fa9a84ad62bcbff4669d7bc7535025c226b5b05a49af2142c6f82d3e4c66
- Diff:
-
Revision 14 (+250 -25)
- Change Summary:
-
Fix indentation
- Commit:
-
7bc7535025c226b5b05a49af2142c6f82d3e4c66715bac69da9e6acede10f4ed1c55d8359aa47e02
- Diff:
-
Revision 15 (+250 -25)
- Change Summary:
-
Make common.less more compact
- Commit:
-
715bac69da9e6acede10f4ed1c55d8359aa47e0290395d4f8e3cd1875c508c808dece2933816964d
- Diff:
-
Revision 16 (+252 -46)
- Change Summary:
-
Fix issues
- Commit:
-
90395d4f8e3cd1875c508c808dece2933816964daf01285a75d9790abbd97d0a421cfe75ac57fa95
- Diff:
-
Revision 17 (+253 -46)