286822 - Process Problem: Review From the Wind Never Happens

Status: RESOLVED FIXED

(Bugzilla :: bugzilla.org, defect)

Description

[Max Kanat-Alexander]

I think that one of the difficulties of getting into Bugzilla development is
that unless you've been working on the project for a while, it's difficult to
know who to request review from.

The Reviewers List is good, but it's still difficult to figure out who should
review certain changes to certain files.

Basically, I think that we should restructure the Reviewers List -- instead of
being a list of names, it should be like a MAINTAINERS file, where we have a
list of areas-of-Bugzilla/files, and then email addresses next to them.

I'll attach a proposed mapping.

Comment 1

[Max Kanat-Alexander]

Oh, and to address the actual summary line, the problem is that people can't
request review from the wind, because then it never happens unless some
concerned reviewer is already on the CC list.

Comment 2

[Max Kanat-Alexander]

Attachment: Rough Draft

Here's a rough draft, in plain text. This is just what I could come up with off
the top of my head.

The final version will, of course, be in HTML.

If I left you out of a certain area, it's not because I don't think you have
that skill, it's probably just because I didn't think to add you. :-)

In particular, if you'd like to do reviews on a particular file that doesn't
currently have a name listed to it, PLEASE let me know. :-)

Comment 3

[Max Kanat-Alexander]

I'm thinking that I perhaps also ought to add a "for trivial or small changes"
section that lists a lot of possible reviewers, or perhaps randomly generates a
reviewer. :-)

It would be nice to have a list somewhere that said how many patches each person
currently had in their review queue, also, so that we could pick people who were
less busy. :-)

Comment 4

[Dave Miller [:justdave]]

Matty attempted this once a couple years ago and didn't get people to fill in
their info about what they knew.  It used to be on the bugzilla.org website, and
got nuked during the site redesign last year.  I just resurrected it from CVS
and posted it at http://www.justdave.net/skills.html for reference.  It's not
guaranteed to stay there for any period of time more than about a week or so :)

Comment 5

[Shane H. W. Travis]

You could put me down for enter_bug.cgi as well; I had to re-write large 
portions of that for bug cloning, and have done several other bugs associated 
with it as well. (I'll be changing my e-mail address sometime in the next few 
days, though, given that today was my last day at SED Systems. :)

You could also put down that any documentation fixes should be have a review 
request of [e-mail], and someone from the docs team will get 
to it.

Comment 6

[Jouni Heikniemi]

Even though Matty tried it (and failed) already, I still think this is a worthy
effort. However, the key thing here is getting it to be much simpler and more
readable. Here, the difference between plaintext and a decently readable HTML
table (nothing fancy) is enormous! Please spend the hour it takes to make this
look good, as it will undoubtedly increase the usefulness of this list greatly.

Re the content, I have dabbled a bit with most of the testing suite (runtests.pl
& t/*), so you can add me up for those, too.

Related: <http://www.heikniemi.net/hc/archives/000203.html>

Comment 7

[Tomas Kopal]

Well, what about automating this a bit with bugzilla? ;-). The idea is that
every component would have someone assigned (as there is currently component
owner, it may even be the same person) and when you ask for review with no email
specified explicitely, it would send the request to the designated person.
He/she can reassign, or review?

Comment 8

[Frédéric Buclin]

(In reply to comment #7)
> Well, what about automating this a bit with bugzilla? ;-). The idea is that
> every component would have someone assigned (as there is currently component
> owner, it may even be the same person) and when you ask for review with no email
> specified explicitely, it would send the request to the designated person.
> He/she can reassign, or review?

I'm with Tomas! I was going to write the same thing :) I really like this idea
of a default requestee. Well, it's name should not be added to the request in
the attachments table, but an email with a X-Bugzilla-Reason: You are the
default requestee for this product/component should be sent. And if this
reviewer thinks another reviewer should/could review that patch, then it would
be his job to triage requests to an appropriate reviewer.

Moreover, if possible, Bugzilla should whine every xx weeks to the default
requestee that pending requests in this component/products are still here. Does
it sound reasonable?

Comment 9

[Frédéric Buclin]

mkanat, you can add me to the following files:

describecomponents.cgi
enter_bug.cgi
post_bug.cgi
votes.cgi

Moreover, I'm sure myk would also be the right person for attachment.cgi and
editflagtypes.cgi files. ;)

Comment 10

[:glob ✱]

put me down for windows specific issues please.

perhaps we should set up a whine to email the reviewers list with "from the
wind" requests?

Comment 11

[Max Kanat-Alexander]

Attachment: v2, HTML Version

OK, here it is in HTML. Let me know what you think. :-)

I think this is pretty close to the final version that could go in the
templates on bugzilla.org.

Comment 12

[Colin Ogilvie [:cso]]

It doesn't seem entirely clear what to do if a patch touches many files - there
is a one line explanation, but I missed it when I read over the HTML file.
Perhaps this needs to be clearer?

Comment 13

[Max Kanat-Alexander]

Attachment: v3

I addressed Colin's comment and also added <link>'s to the bugzilla.org CSS, so
you can get an idea of what it will look like when it's on the site.

Comment 14

[Dave Miller [:justdave]]

Comment on attachment 177962 [details]
v3

gorgeous

Comment 15

[Vlad Dascalu]

Comment on attachment 177962 [details]
v3

Tomas appears with both [e-mail] and Tomas.Kopal@altap dot cz.
Probably only one should be used.

>> The maintainer of this page is 

Since when do we have maintainers for individual website pages?

>> Basically, if you'd like to offer your services in any area, just let me know by email. Updates to this file will almost always be accepted, provided that you are already a Bugzilla reviewer.

Errhm, I thought that was Dave's job. Either he is the one that updates the
page, or all cvs-www commiters update the page, via the review process. Did you
overnight got some special status that allows you to use your discretion in
deciding when "almost always" applies and when it does not, better than the
rest of the core team?

Mkanat, I realize that you're a support "manager" at Kerio or whatever, but
please refrain yourself from pushing your personal goals and your personal
agenda inside this community. The Bugzilla development process has enough
problems as it is, without you pushing for your personal status and power
inside every move that you make as a developper. Thank you.

Comment 16

[Vlad Dascalu]

To reply to my own question, Gerv is a maintainer for a www page, but that's an
exception because it makes sense to update the list of all Bugzilla
installations in a central place and have Template Toolkit generate the HTML
content for that.

Comment 17

[Max Kanat-Alexander]

Hey Vladd. I have no special status, only a desire to help. :-)

Yes, you are absolutely free to update the page via a CVS commit when you add a
file.

Comment 18

[Jouni Heikniemi]

(In reply to comment #15)
> Tomas appears with both [e-mail] and Tomas.Kopal@altap dot cz.
> Probably only one should be used.

Also, Erik's domain is wrong (dabistro.net should be dasbistro.net).

> Mkanat, I realize that you're a support "manager" at Kerio or whatever, but
> please refrain yourself from pushing your personal goals and your personal
> agenda inside this community. The Bugzilla development process has enough
> problems as it is, without you pushing for your personal status and power
> inside every move that you make as a developper. Thank you.

Oh please. The project has enough friction as it is without you bashing people
who offer their help. Totally regardless of what each of us does during the
working hours, I'm happy about a competent person's offer to actually write and
maintain this sort of a page (that's Max) instead of just whining about the
issues on one's blog (that's me). Hard work tends to produce status and power here.

I think the page looks rather nice, now. Let's get it published - we can easily
revise it when issues pop up.

Comment 19

[Jouni Heikniemi]

> Also, Erik's domain is wrong (dabistro.net should be dasbistro.net).

.com even. Duh.

Comment 20

[Gervase Markham [:gerv]]

How does this interact with module ownership?
https://bugzilla.mozilla.org/describecomponents.cgi?product=Bugzilla

Gerv

Comment 21

[Max Kanat-Alexander]

(In reply to comment #20)
> How does this interact with module ownership?
> https://bugzilla.mozilla.org/describecomponents.cgi?product=Bugzilla

    My understanding was that module ownership is a lot about *writing* code.
That is, if somebody reports a bug against your module, you become the actual
assignee, and if nobody else takes up the bug, it becomes your responsibility to
fix it. 

  Module ownership is also about reviewing code, definitely (note that you are
the noted reviewer for all of the Chart stuff, of which you are the module owner).

  I think owning a module is a larger responsibility than what's listed on this
page. Just because somebody is listed as a reviewer for a certain file,
definitely doesn't mean that they have to personally fix any bugs reported
against it. :-)

 That's not me trying to change anything, that's just my understanding of how
things work now.

Comment 22

[Joel Peshkin]

(In reply to comment #21)

My impression (and hope) is exactly the opposite.  I view the module owner as
the guardian of a module.  A module owner may fix many bugs in a module while
leaving plenty of bugs and RFEs for others. (Every author should accept primiary
responsibility for fixing his own bugs)  However, I would certainly expect that
no reviewer would permit a very radical change to a module without checking with
the owner.  Should a reviewer let that slip, the approver acts as another line
of defense.  The module owner is making sure that too many hands don't make a
mess of a module.

Comment 23

[Tomas Kopal]

(In reply to comment #7)
> Well, what about automating this a bit with bugzilla? ;-). The idea is that
> every component would have someone assigned (as there is currently component
> owner, it may even be the same person) and when you ask for review with no email
> specified explicitely, it would send the request to the designated person.
> He/she can reassign, or review?

Expanding on this a bit more :-), we can even have a special admin page in
bugzilla, accessible to members of a 'reviewer' access group, where you would
tick boxes next to components where you feel you could do the review, with the
mapping stored in the DB.
Then when a review request without explicitely specified reviewer comes,
Bugzilla would just pick one of the reviewers which have the component selected
and assign the review to him/her. Who to pick from the list would be a fun place
to play with 'schedulers' :-), but anything from random to round-robin to
'currently shortest queue' would do, I guess.
And yeah, as Fred indicated, whining on outstanding reviews would be great too.
I would just change the interval to more often, at least bi-daily ;-).

The main idea is that, similarly as bugs, review request should never be left
'unassigned'. There should always be exactly one person responsible for doing
some action, one person to blame, if you want :-).

Anyone interested in implementing this? ;-)

Comment 24

[timeless]

it looks like i fell off the world... i own a small section of the enter bug
process relating to browser detection, and i'm willing and generally available
to review small changes.

Comment 25

[Kevin Benton]

Guys - I don't know that this is necessarily the right way to go - to publish
email addresses of people who review particular parts of BZ as a way to get
reviews done.  What it doesn't do is allow the members of a particular reviewing
board to distribute the load among themselves without having reviews thrust upon
them.

On the other hand, I *REALLY* like the "[e-mail]" idea because
it immediately becomes a lot easier to maintain a list of people who get
notified in the event of a review request for docs bugs.  If we applied the same
idea to other parts of Bugzilla, I think we'd find that it would be very easy to
maintain, and more importantly, if someone dropped off the face of the Bugzilla
earth, it would be a near no-brainer to handle the changes.

Comment 26

[Max Kanat-Alexander]

(In reply to comment #25)

  Then we'd just have the equivalent of having Bugzilla modules for each file.
The idea here is to provide a list of reviewers that will help out newcomers and
current developers alike, but not to have a zillion Bugzilla modules that would
confused bug filers and make our process difficult.

Comment 27

[Greg Hendricks]

I would be willing to take on reviews of importxml.pl and possibly move.pl as
well. Of course I am still awaiting review on my patch to that system (*hint
hint) but I am now rather intimiately aware of the process there. 

Comment 28

[Greg Hendricks]

Also, as a new contributor I would like to voice my support for this idea. One
of the biggest headaches was trying to find out who had any previous knowledge
of the files my changes touched to request review from. After asking around in
the chat room I had a couple people volunteer others and finally Max came out
and did an initial review. After that it bounced around a couple times before
landing on Dave to review, but of course Dave is already swamped with other
reviews. Having a list of this sort is just the thing that a new contributor
needs to get a handle on who does what around here.

Comment 29

[Max Kanat-Alexander]

(In reply to comment #27)
> I would be willing to take on reviews of importxml.pl and possibly move.pl as
> well. Of course I am still awaiting review on my patch to that system (*hint
> hint) but I am now rather intimiately aware of the process there. 

  Yeah, in fact, you'll notice that your name is even already in there for
importxml.pl, it's just commented-out. :-D As soon as I get that patch review+,
I'll put you in there (unless justdave objects, of course, which I doubt he will).

  Oh, and thanks for your support! :-)

Comment 30

[Max Kanat-Alexander]

OK, before I checkin, I:

+ Fixed Erik's email.
+ Added a commented-out ghendricks to move.pl
+ Noted that timeless is in charge of OS Detection for enter_bug

There are probably some other little touch-ups to do, but I can do those after
it's checked-in. :-)

Also, I need to decide where to link it from, which I'll do once I have it
checked-in and I'm sure it looks OK on bugzilla.org itself.

Also, Vlad, did you want to be in the General Reviewers list for the CGIs? The
only reason that I didn't put you in there was because I haven't seen you around
much recently; I wasn't sure if you still wanted to do reviews.

Comment 31

[Max Kanat-Alexander]

Hit enter a bit too fast, there. :-) Here's the commit message:

RCS file: /cvsroot/mozilla-org/html/projects/bugzilla/docs/reviewer-list.html.tmpl,v
done
Checking in docs/reviewer-list.html.tmpl;
/cvsroot/mozilla-org/html/projects/bugzilla/docs/reviewer-list.html.tmpl,v  <--
 reviewer-list.html.tmpl
initial revision: 1.1
done

Comment 32

[Max Kanat-Alexander]

I realized I needed to fix up the CSS right after checkin:

Checking in css/bugzilla.css;
/cvsroot/mozilla-org/html/projects/bugzilla/css/bugzilla.css,v  <--  bugzilla.css
new revision: 1.9; previous revision: 1.8
done
Checking in docs/reviewer-list.html.tmpl;
/cvsroot/mozilla-org/html/projects/bugzilla/docs/reviewer-list.html.tmpl,v  <--
 reviewer-list.html.tmpl
new revision: 1.2; previous revision: 1.1
done