Review Board and Bugzilla reviews, take 2.

review-board-diff

Last time I played with Review Board and bugzilla request queues things were great for me, but no one else.  I had to create an account for you on the instance, add you to my script that synchronizes request queues, run the script, and then keep running the script periodically.  Not to mention there wasn’t really a way to get your review out and into bugzilla.  No one actually tried to use it, so they probably also didn’t notice there were caching issues related to the ever-changing definition of “HEAD” (“tip”).  It sucked, and when I upgraded and everything broke, no one cared, not even me.

But now it’s back and better than ever:

review-board-bugzilla-queue-sync

  • People don’t need to login at all to see review requests and reviews!  Just point them at the URL and away they go.  (Actually, this was the case before, but it was not obvious.)
  • You can/must sign in with Open ID!  If you have Weave and are reading this in the future, Weave can be your Open ID friend!  If you are like me and live in the present (Weave 0.3.2), something is wrong and it doesn’t work, not to mention that Weave takes over the Open ID box so you can’t use credentials that work.
  • There’s a button that updates your request queue for the 16 most recent requests made of you.  Just make sure that you have entered your bugzilla e-mail address on the “my account” page.  This may have happened automatically depending on what your Open ID provider provided/you told them to provide.
  • There’s export functionality so you can take your review and cram it in bugzilla.
  • The definition of “tip” gets nailed down when the review request gets created, so no more ugly caching issues.  Patches can still fail to apply, though, if “tip” has drifted from when the patch was first created.
  • It has friendlier assumptions about what repo you are dealing with.  Thunderbird/MailNews Core/Calendar/SeaMonkey are all assumed to be in comm-central, everyone else is assumed to be in mozilla-central.  Patches against other repos (including mozilla 1.9.1) clearly will not work without additional logic (and some kind of extra info, like people putting “1.9.1” in their attachment descriptions.)

Here is a (fake) example of the “pretty” review output that is possible if you tell people about your reviewboard review (see it live here).  Although it says Bienvenu, it’s just me pretending to be him because his review queue is more interesting than mine.  The comments are accordingly mine.

review-board-review-pretty

Now, what does the export look like (see it live here)?

on file: mailnews/base/src/nsMessenger.cpp line 635
>     // if we have a mailbox:// url that points to an .eml file, we have to read
>     // the file size as well

what a pretty comment

on file: mailnews/base/src/nsMessenger.cpp line 642
>     NS_ENSURE_SUCCESS(rv, rv);

please rename rv ARRRRRR-VEEEEEE

Yeah, it looks like that.

A quick feature summary that explains why this is better than just looking at diffs in bugzilla:

  • Syntax highlighting!
  • It actually has the full-context of the rest of the file!  No more being limited to the 3 or 8 lines of context in the diff you are provided.  I know I have done a lazy review and let a bug through that would not have happened if I had more context at my fingertips (or was not sometimes lazy).
  • People just trimming down your patch to what they are commenting on leaves you with no context of what changed at all!

Useful links:

  • A more interesting live diff to check out.
  • The root of the review board that will prompt you to login via an Open ID account.  When syncing your review queue, please keep in mind that it can take a bit to do all the legwork and you won’t see any feedback until it is actually done doing everything else.  You should get some form of feedback no matter what happens, so don’t keep hitting refresh.
  • The hg repo for my modified version of review-board.  It’s based on an extremely shallow hgsvn checkout.  My questionable development strategy was to make changes with emacs locally, commit, then push to my VM, so the changesets are sometimes a bit excessive.

Caveat usor:

  • This is running on a linode VM right now.  This is better than my local box or what not, but it’s not Mo[MC]o IT or anything.
  • My changes, at least the export functionality, may be buggy.  You may need to rely on me to fix the export functionality to get your stuff out that way.  (If the hg diff doesn’t apply cleanly, you can’t enter data to lose it, so I’m less concerned about that.)
  • I have no plans to blow away the database, but at the same time, please be prepared for the possibility that space ninjas destroy your data.  Use the export functionality and save it to a text file or something periodically if you’re doing a major review.  (In case it’s not obvious, the export functionality is the text labeled “bugzilla-style export” to the right of the reviewer’s name at the top-left of each review.)  You can do your review in multiple passes, exporting each review pass individually.
  • I am confident something will go wrong.  Feel free to post comments here or ping me on IRC (:asuth).
  • If people actually try using this, I’ll stop developing on the live server, but do be aware that apache restarts (lasting a few seconds) may periodically happen, but this should not really impact anything.

Props:

Using BugXhibit to find that bug you know you saw recently but can’t find

bugxhibit-cc-search

BugXhibit, the Bugzilla search results viewer made with the SIMILE Exhibit widget, is now more fancy, and now addresses another one of my use cases.  I frequently find myself wanting to point someone at a bug, or go back to a bug that I know passed through my bugmail recently, and have trouble finding it.  So now BugXhibit can do easy searches based on reporter/assignee/cc/commenter with time ranges.

Examples by way of live links this time (noting that the default time interval is 7 days).  Uh, and if it gives you an error for reasons I don’t fully understand if you open it in a new tab (in the background) from here, just hitting enter in the address bar should fix it.  I’m going to lazyweb that problem for now.

Other changes:

  • It now is also self documenting, just click on “Show Docs” on the page.
  • You can now use arguments to specify the sort and whether grouping is active on the page.
  • The date parsing is better.  Bugzilla doesn’t provide the raw dates but attempts to change things based on how recent the date is.  BugXhibit does a good job of fixing up the date if you are in the same timezone as the bugzilla server, and a less good but acceptable job if you aren’t.
  • Upgraded to exhibit 2.1.0 and now the numeric sliders with histograms work for me.  Woo!

Other notes:

The hg repo is here, as always.

DevMoXhibit: Exhibit on DevMo (Deki Wiki) results

devmo-search-customize-toolbar

The above screenshot is of a normal search query on DevMo for “customize toolbar”.  I see 2.5 results, and I honestly have no interest in the first item at all.  (It’s a page that only advanced DevMo authors would care about, for those who refuse to squint or click on images to see bigger versions of images.)

devmoxhibit-search-customize-toolbar-corrected

The above screenshot is of the same query using DevMoXhibit.  You will note you can see more things, and the first result from the other page is completely elided because we filter by default so that only “Real” result pages are shown.  (In general, I am not looking for talk pages or user pages or meta-pages.)

But enough about my interpretations of pictures, why don’t you:

Neat things we do that may not be immediately obvious:

  • We flatten the score into deciles, and then within each decile range we sort based on the view count for the page.  The theory is that, given equally likely results, the one that more people have looked at is probably more interesting to you, roughly speaking.
  • We use a simple heuristic to figure out the page type, as mentioned above (“Real”, “Talk”, “User”, etc.)
  • We try and hide all things related to the language, as we explicitly query on a language which means it’s just noise.  Right now, that language is always english, but the code uses a variable if you want to write the code to hook that up and expose it in the UI.
  • We produce a “smart” snippet.  The snippets provided by the search results naively will include “chrome” that is part of the document, which makes for a nearly useless snippet.  For example, take a gander at XUL/toolbar:
    • Plain old snippet:
      • « XUL Reference home    [ Examples | Attributes | Properties | Methods | Related ] A container which typically contains a row of buttons. It is a type of box that defaults to horizontal orientation. …
    • Smart snippet:
      • A container which typically contains a row of buttons. It is a type of box that defaults to horizontal orientation. …
  • We produce a sometimes over-zealous smart snippet.  If you were to keep reading both of those snippets, you would notice that the smart snippet eats a bit that the non-smart-snippet does not.  That is because the smart snippet is based on looking at a version of the snippet which has HTML tags in it, and then it tries to nuke those HTML tags out of existence using simple regexps.

Implementation notes:

  • This probably should work on other deki wikis if so adapted, but I don’t use any others, so YMMV.
  • We actually issue two search queries because there are two result formats that can be produced.  “xml” is an inexplicable mixture of too much data and too little data.  Namely, it does not tell you the tags on a document, which is basically the most useful piece of info, but it does tell you every link to and from that page (which we expose, although I doubt it will be useful enough to justify it).  It does give you a link to be able to get the tags, but that’s a costly operation when you have to perform it for each search result.  In contrast, “search” gives you the tags; they are only space-delimited, but that’s fine.  (“Inexplicable” may be a bit harsh; looking at the source, it’s just dumping the page info without further processing/lookups, but arguably it would be very useful if they made the effort to fetch that data.)
  • Because of cross-site XHR issues, this is not quite as hackable as I would like.  My demo server above is using mod_proxy (with a very specific constraint) to proxy the search to DevMo.  When I develop locally, I have to do the same thing.  Presumably if you are using Firefox 3.5 and devmo is set up correctly, then this would not be a problem.  But, 1) for no good reason, I only use Firefox 3.0 and 2) have no clue whether devmo is emitting the headers that would enable that to work.  I strongly encourage someone to look into #2 and fix it if not.
  • As with BugXhibit, the sliders are totally broken for me and it’s sad, but I left them in there in the hopes that they work for someone, somewhere.  Alternately, I would not complain if someone, somewhere, fixed them.

The hg repo is here.