Summary

Add the BugTracker framework and Bugzilla/JIRA bug infoboxes.

Review Request #6047 — Created July 2, 2014 and submitted July 3, 2014, 11:31 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

193

Depends On

Commit

138ff8c...

Reviewers

Groups

reviewboard

People

Description

This change adds a new interface to the hosting service app for deeper
integration with bug trackers. At the moment, this interface defines three
methods to fetch information about a given bug, which are used to provide a
hover infobox that shows the summary, description, and status.

This interface is currently implemented by the existing Bugzilla backend, and
by a new JIRA backend.

The next steps for this is to implement the interface for more bug-tracker
backends, and to start adding capabilities such that Review Board can do things
like add a comment to a bug when a review request linking to it is published.

This change is based on these changes by Tomi Äijö: * https://reviews.reviewboard.org/r/5531/ * https://reviews.reviewboard.org/r/5769/ * https://reviews.reviewboard.org/r/5745/

I've made several changes to Tomi's original implementation. The bug infobox
formatting has been cleaned up a bit, especially with regards to the formatting
of the description text. The caching steps are now done in a helper inside the
BugTracker class so we don't have to re-implement it everywhere. I've also made
it so that failures to fetch the bug result in no infobox, rather than an
infobox which just has the ID and nothing else.

Testing Done


Tested the bug infobox with a bug tracker configured to point to

  https://bugzilla.mozilla.org, and https://jira.atlassian.com. Saw that bug

  information was fetched correctly and that the infobox looked good.
Tested invalid bug numbers and saw that the infobox request returned 404 and

  no infobox was displayed.
Tested that links to existing bug trackers (Google Code, etc) worked well.
Tested that newly-added bugs linked correctly.
Ran unit tests.

Issues

Description	From	Last Updated
'logging' imported but unused	reviewbot	July 2, 2014, 5:40 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	July 2, 2014, 5:40 p.m.
'logging' imported but unused	reviewbot	July 2, 2014, 5:40 p.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	July 2, 2014, 5:41 p.m.
list comprehension redefines 'file_attachment' from line 584	reviewbot	July 2, 2014, 5:41 p.m.
Is there ever a time when bug_url could be None? I would expect it to fail in that case. Also, …	chipx86	July 3, 2014, 12:46 a.m.
This _bug_id thing seems a bit iffy. If the bug tracker happens to have that text, we'll replace it. Can …	chipx86	July 3, 2014, 12:46 a.m.
Instead of all these methods, I think we should have a get_bug_info function that returns any and all detail that …	chipx86	July 3, 2014, 1:13 a.m.
Why not cache_memoize?	chipx86	July 3, 2014, 1:13 a.m.
In the past, people have complained that URLField didn't validate non-FQDN URLs. I don't know if this is still the …	chipx86	July 3, 2014, 12:17 a.m.
Blank line before this.	chipx86	July 3, 2014, 12:20 a.m.
And here.	chipx86	July 3, 2014, 12:20 a.m.
And here.	chipx86	July 3, 2014, 12:20 a.m.
We should catch exceptions here. That URL may not exist, or it may not contain what we expect.	chipx86	July 3, 2014, 1:14 a.m.
Same.	chipx86	July 3, 2014, 1:15 a.m.
No need for the variable. PyFlakes will just yell.	chipx86	July 3, 2014, 12:17 a.m.
Should there be a space after the '/'?	chipx86	July 3, 2014, 12:17 a.m.
I really don't like defining instance defaults as class members. It complicates their purpose.	chipx86	July 3, 2014, 12:19 a.m.
Blank line above. Same with the ones below.	chipx86	July 3, 2014, 12:19 a.m.
Can we move this to format_html while here?	chipx86	July 3, 2014, 12:28 a.m.
Blank line before.	chipx86	July 3, 2014, 12:30 a.m.
We're no longer explicitly returning a value if a bug tracker is not configured. We should still have a return …	chipx86	July 3, 2014, 12:29 a.m.
Can we pull this out into another urlpatterns in this file, so that we only have to have the bugs/... …	chipx86	July 3, 2014, 12:32 a.m.
list comprehension redefines 'file_attachment' from line 584	reviewbot	July 2, 2014, 5:44 p.m.
You can return this directly.	chipx86	July 3, 2014, 12:21 a.m.
Should probably be a @cached_property	chipx86	July 3, 2014, 12:26 a.m.
No need for , None)	chipx86	July 3, 2014, 12:22 a.m.
Should be one statement.	chipx86	July 3, 2014, 12:22 a.m.
Should be #bug_infobox (here and where the actual element is created).	chipx86	July 3, 2014, 12:33 a.m.
This can be folded up into the rule above it.	chipx86	July 3, 2014, 12:34 a.m.
Swap these. Alphabetical order.	chipx86	July 3, 2014, 12:34 a.m.
I never got around to moving the old infobox into a view, but I think if we're going to introduce …	chipx86	July 3, 2014, 1:27 a.m.
#bug_infobox	chipx86	July 3, 2014, 12:33 a.m.
What's the migration plans for bug trackers containing %s? Again, I don't think we should use _bug_id as the placeholder. …	chipx86	July 3, 2014, 12:37 a.m.
list comprehension redefines 'file_attachment' from line 584	reviewbot	July 3, 2014, 1:21 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/hostingsvcs/jira.py
    reviewboard/datagrids/columns.py
    reviewboard/scmtools/models.py
    reviewboard/reviews/urls.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/hostingsvcs/bugzilla.py
    setup.py
    reviewboard/hostingsvcs/bugtracker.py

Ignored Files:
    reviewboard/static/rb/js/dashboard/views/dashboardView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/bug_infobox.html
    reviewboard/static/rb/js/common.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/hostingsvcs/jira.py
    reviewboard/datagrids/columns.py
    reviewboard/scmtools/models.py
    reviewboard/reviews/urls.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/hostingsvcs/bugzilla.py
    setup.py
    reviewboard/hostingsvcs/bugtracker.py

Ignored Files:
    reviewboard/static/rb/js/dashboard/views/dashboardView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/bug_infobox.html
    reviewboard/static/rb/js/common.js

reviewboard/datagrids/columns.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'logging' imported but unused
```
reviewboard/hostingsvcs/bugzilla.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```
reviewboard/reviews/builtin_fields.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'logging' imported but unused
```
reviewboard/reviews/urls.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (92 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 584
```

Change Summary:

Fix Review Bot issues.

Commit:

1a4a8bde8835509a45c26a94a53731b566a8e1d9

e942e342feb72aadb865f1c8dad88c012ac17a5b

Diff:

Revision 2 (+448 -55)

Show changes

	setup.py
	reviewboard/datagrids/columns.py
	reviewboard/hostingsvcs/bugtracker.py
	reviewboard/hostingsvcs/bugzilla.py
	reviewboard/hostingsvcs/jira.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	7 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/hostingsvcs/jira.py
    reviewboard/datagrids/columns.py
    reviewboard/scmtools/models.py
    reviewboard/reviews/urls.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/hostingsvcs/bugzilla.py
    setup.py
    reviewboard/hostingsvcs/bugtracker.py

Ignored Files:
    reviewboard/static/rb/js/dashboard/views/dashboardView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/bug_infobox.html
    reviewboard/static/rb/js/common.js



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/hostingsvcs/jira.py
    reviewboard/datagrids/columns.py
    reviewboard/scmtools/models.py
    reviewboard/reviews/urls.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/hostingsvcs/bugzilla.py
    setup.py
    reviewboard/hostingsvcs/bugtracker.py

Ignored Files:
    reviewboard/static/rb/js/dashboard/views/dashboardView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/bug_infobox.html
    reviewboard/static/rb/js/common.js

reviewboard/reviews/views.py (Diff revision 2)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 584
```

Much of this is probably the original code, but nonetheless, some comments.

Really excited to finally get this in!

reviewboard/datagrids/columns.py (Diff revision 2)

The issue has been resolved. Show all issues

Is there ever a time when bug_url could be None? I would expect it to fail in that case.

Also, should be local_site_reverse.

reviewboard/datagrids/columns.py (Diff revision 2)

The issue has been resolved. Show all issues

This _bug_id thing seems a bit iffy. If the bug tracker happens to have that text, we'll replace it. Can we instead just reverse on demand?

IIRC, Django caches found URL regexes based on names, so I don't expect it'll add much overhead.

reviewboard/hostingsvcs/bugtracker.py (Diff revision 2)

The issue has been resolved. Show all issues

Instead of all these methods, I think we should have a get_bug_info function that returns any and all detail that the tracker can provide. That way there are fewer calls and fewer functions that must be implemented. It'll also be a lot easier to extend the capabilities in the future.
We're basically doing this anyway in _get_bug. So, I'd suggest simplifing all this down to get_bug_info() and _get_bug_info_uncached().

reviewboard/hostingsvcs/bugtracker.py (Diff revision 2)

The issue has been resolved. Show all issues

Why not cache_memoize?

reviewboard/hostingsvcs/bugzilla.py (Diff revision 2)

The issue has been resolved. Show all issues

In the past, people have complained that URLField didn't validate non-FQDN URLs. I don't know if this is still the case, but if so, we may want to use CharField.

reviewboard/hostingsvcs/bugzilla.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line before this.
```
reviewboard/hostingsvcs/bugzilla.py (Diff revision 2)
The issue has been resolved. Show all issues
```
And here.
```
reviewboard/hostingsvcs/bugzilla.py (Diff revision 2)
The issue has been resolved. Show all issues
```
And here.
```

reviewboard/hostingsvcs/bugzilla.py (Diff revision 2)

The issue has been resolved. Show all issues

We should catch exceptions here. That URL may not exist, or it may not contain what we expect.

reviewboard/hostingsvcs/bugzilla.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same.
```
reviewboard/hostingsvcs/jira.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No need for the variable. PyFlakes will just yell.
```
1. david July 3, 2014, 1:17 a.m.
  Leftover from prior debugging.
reviewboard/hostingsvcs/jira.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Should there be a space after the '/'?
```
1. david July 3, 2014, 1:17 a.m.
  rstrip treats the string as a set of characters to strip, so yes.

reviewboard/hostingsvcs/jira.py (Diff revision 2)

The issue has been resolved. Show all issues

I really don't like defining instance defaults as class members. It complicates their purpose.

reviewboard/hostingsvcs/jira.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line above. Same with the ones below.
```
reviewboard/reviews/builtin_fields.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we move this to format_html while here?
```
reviewboard/reviews/builtin_fields.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line before.
```

reviewboard/reviews/builtin_fields.py (Diff revision 2)

The issue has been resolved. Show all issues

We're no longer explicitly returning a value if a bug tracker is not configured. We should still have a return in all cases.
Maybe just define a bug_url = None above, set it only when we call reverse, and return it at the end? That way, only one return statement.

reviewboard/reviews/urls.py (Diff revision 2)

The issue has been resolved. Show all issues

Can we pull this out into another urlpatterns in this file, so that we only have to have the bugs/... prefix defined once? We can then just include(bugs_urlpatterns) here.

reviewboard/reviews/views.py (Diff revision 2)

The issue has been resolved. Show all issues

You can return this directly.

reviewboard/scmtools/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should probably be a @cached_property
```
reviewboard/scmtools/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No need for , None)
```
reviewboard/scmtools/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should be one statement.
```
reviewboard/static/rb/css/common.less (Diff revision 2)
The issue has been resolved. Show all issues
```
Should be #bug_infobox (here and where the actual element is created).
```
reviewboard/static/rb/css/common.less (Diff revision 2)
The issue has been resolved. Show all issues
```
This can be folded up into the rule above it.
```

reviewboard/static/rb/css/common.less (Diff revision 2)

The issue has been resolved. Show all issues

Swap these. Alphabetical order.

reviewboard/static/rb/js/common.js (Diff revision 2)

The issue has been dropped. Show all issues

I never got around to moving the old infobox into a view, but I think if we're going to introduce a new one, we should start by making it a proper view.

david July 3, 2014, 1:17 a.m.

I think I'd like to do that as a subsequent cleanup.

chipx86 July 3, 2014, 1:26 a.m.
```
Works for me.
```

reviewboard/static/rb/js/common.js (Diff revision 2)
The issue has been resolved. Show all issues
```
#bug_infobox
```

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2)

The issue has been resolved. Show all issues

What's the migration plans for bug trackers containing %s?

Again, I don't think we should use _bug_id as the placeholder. If we really must, maybe __bug_id__, since that's far less likely to conflict.

david July 3, 2014, 1:17 a.m.

This no longer passes the actual bug URL to the page. Instead, it passes a URL that goes to the bug_url view, which does the actual substitution and redirects. It's pretty unlikely that _bug_id is going to be part of the reviewboard URL, but just in case, I'll make it a crazier substitution.

Commit:

e942e342feb72aadb865f1c8dad88c012ac17a5b

138ff8c1a672e34cda260ef4d9e6f90a8cd4b783

Diff:

Revision 3 (+416 -57)

Show changes

	setup.py
	reviewboard/datagrids/columns.py
	reviewboard/hostingsvcs/bugtracker.py
	reviewboard/hostingsvcs/bugzilla.py
	reviewboard/hostingsvcs/jira.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	7 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/hostingsvcs/jira.py
    reviewboard/datagrids/columns.py
    reviewboard/scmtools/models.py
    reviewboard/reviews/urls.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/hostingsvcs/bugzilla.py
    setup.py
    reviewboard/hostingsvcs/bugtracker.py

Ignored Files:
    reviewboard/static/rb/js/dashboard/views/dashboardView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/bug_infobox.html
    reviewboard/static/rb/js/common.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/hostingsvcs/jira.py
    reviewboard/datagrids/columns.py
    reviewboard/scmtools/models.py
    reviewboard/reviews/urls.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/hostingsvcs/bugzilla.py
    setup.py
    reviewboard/hostingsvcs/bugtracker.py

Ignored Files:
    reviewboard/static/rb/js/dashboard/views/dashboardView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/bug_infobox.html
    reviewboard/static/rb/js/common.js

reviewboard/reviews/views.py (Diff revision 3)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 584
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (aecb3a0)