This page is dedicated to documenting solutions around code review issues. Please be realistic about what is achievable in an amount of time.
If you just want to complain about something, please file a complaint.
See zzolo's post for more context.
Short Term
What can be done in the next week or two?
- DONE! Improve drupal.org documentation (specifically the 'what to expect' post), to try and encourage applicants to perform certain 'pre-screening' steps before submitting their application.
- DONE! Add links to the 'Project Application: what to expect' and and 'Tips for ensuring a smooth review' to the 'Create new project applications' screen
- DONE! Encourage applicants to indicate the intended core Drupal version in their actual application post
- DONE! Add 'created' date to the Project Applications issue queue view, to allow reviewers to pull the 'oldest' applications, instead of the 'longest inactive'
- DONE! Leverage the 'priority' field to flag applications which have been waiting in 'needs review' state for greater than 2/3/4 weeks.
- DONE! Implement a means for reviewers and applicants to request a second opinion when review conflicts arise
- IN PROGRESS Identify 'quick hit' items that new and/or less technical reviewers can work on identifying, allowing them to contribute to the process without taking on accountability for a full security/technical review. Tasks identified, but process not formally agreed upon/documented
Medium Term
What can be done in the next 1 to 3 months?
-
Auto-tag incoming applications to promote process building and reviewers that are not capable of full review. If we auto-tag incoming applications with things like: License, README, Security, Coding Standards, etc. (things from the Goals), then people that don't feel like they can do a full review can more easily triage applications for lower level items. Also, if we can provide some support text, an applicant can see more easily what is expected. Overall, this could provide lots of focus.
-
Automated Coder Reviews of New Project Applications Leveraging qa.drupal.org into the code review process would help eliminate some of the simple coding standards and basic security issues which contribute to the 'nickel and dime' or 'reviewers focusing on unimportant issues' impressions surrounding the code review process
-
Add flags to the project application page, indicating which components of a review have been completed Easier to describe with a picture, which is located at http://groups.drupal.org/node/152304.
-
Discuss approaches and different treatments for the review of 'modules' vs. 'themes' vs. 'install profiles' vs. 'features' Some discussion has taken place, but no real resolutions or documentation exists yet.
Long Term
What can be done 6 months and more from now?
-
DONE! Move from CVS to Git and create Sandboxes. Migrate to Git and allow for any user to have a sandbox on Drupal.org. This should allow for more development of good modules because there are more resources available to the user.
-
Allow someone to review code without downloading it. A live code review system. Basically a code viewer that supports reviewing. No need to download code/module.
-
Transition away from a 'code review' process, and towards a 'code mentorship' process.
Comments
One thing I'd like to
One thing I'd like to consider is a set of standard tags. If we identify how often certain problems crop up we can work on automating them first and doing more education.
I propose a few tags:
Then after a few weeks we can query to see which are happening the most and work to automate reviews in those areas and do better education.
Similarly I noticed that TR has been doing a ton of reviews. It would probably be best to have him reviewing Ubercart applications. Likewise a theme person could review themes. I'm not sure if we should use components or tags for identifying these.
We have the technology to implement both of these ideas now. We just need agreement on them and we're set to go.
knaddison blog | Morris Animal Foundation
short or long term?
Hey @greggles, this is a good shorter term solution. Is this something you see happening on a regular basis, or just something to do for a couple weeks to get some basic metrics to move forward with? I think the latter is awesome, but the former would be harder to maintain.
--
zzolo
Yes? I think it provides
Yes?
I think it provides benefits in the short and long run since (hopefully) the most common tags would change over time.
knaddison blog | Morris Animal Foundation
I'm doing this
There's a separate conversation over here about how it would be useful to pick out themes in the review queue:
http://groups.drupal.org/node/145289
No one seems to have a problem with this, so I'm going to go ahead and start doing it, using the tagging system greggles outlined. If someone wants to add components for topics (Ubercart, Theme, Miscellaneous would be a good start), I think that would be more useful than tags, but I'm going to start by using tags, since that can happen now. Specifically, "PAReview: Theme" and "PAReview: Ubercart".
thank you.
Hi @sreynen. That's awesome. Can you document it here: http://groups.drupal.org/node/142479
This way people will know what to do, and it is a more definitive place to point people to. Many thanks.
--
zzolo
Application Date
It would be nice if the Application Date would be visible in the issue queue so that reviewers wouldn't have to view the issue to see how long it has been around. I have read that some reviewers prefer to do the oldest first but you can't tell at a glance by looking at the queue. Maybe there is a search filter that I don't know about.
Application Date
I've opened an issue to add 'created' dates to the project application issue queue on drupal.org ... http://drupal.org/node/1179586.
If I can get a 'snapshot' of the drupal.org code, I'll post the potential modifications to a git sandbox, to facilitate others involvement.
Simple Tests
The first project I looked at wasn't even functional yet. What do people think about a requirement that a project has to include some tests using the Simple Tests or Testing Module. I have to admit I haven't used this yet myself, but I will. The tools are there for us to use, why not use them?
too high of a barrier
I think this is too high of a barrier to creation of new projects. Very few projects on drupal.org have simpletests and we should make it a requirement right at the start.
We could certainly encourage simpletsts - perhaps we should have a bit of a carrot/stick approach. We require projects to follow licensing rules and Drupal API/security rules and we give preferential treatment to projects that have good documentation and simpletests.
knaddison blog | Morris Animal Foundation
agreed, high barrier
Yeah, I agree that its too high of a barrier, but do want to promote it.
Personally, I have yet to actually install any module I have reviewed. I don't personally find it necessary.
--
zzolo
???
You're not interested to know if the module actually works?
Yes and no
That's an interesting question. I obviously can't answer for zzolo, but I can answer for me.
First, I'd note that installing a module isn't the only way to know if it works. If it's simple enough, and you know Drupal APIs well enough, you can know if it works just by looking at the code. Some applications also point to sites where the modules are installed, so if the functionality is all user-facing, that's another way to see if it works without installing it.
But beyond that, working isn't actually listed among the goals of reviews. If you think it should be, that should probably be discussed on that goals page.
For most project
For most project applications, reviewers are more than welcome to clone the project's git repo and click test it. I do it often for modules that don't integrate with too much.
Senior Drupal Developer for Lullabot | www.davereid.net | @davereid
i have faith it is already working
@ccardea, I believe pretty fully that anyone applying has actually tested there module, and don't really see why anyone would apply with a module that was not working. Yes, there may be a bug or two, but that's what having a contributed module on Drupal.org is all about. Security issues can be seen more easily through code. Otherwise, the rest of goals of the review are not really about a working a module and can be accomplished with a code review.
--
zzolo
Faith is good, but evidence
Faith is good, but evidence is better. In the short time that I've been doing code reviews, I ran across one that produced a white screen and another that didn't do what the project author said it would do, in fact it didn't do much of anything. In the professional world, making assumptions without checking them out can get you into a lot of trouble. Granted, this environment is a little more relaxed, but it's the principle of the thing.
In my other post on this issue, I pointed out that, in the past having a working module has always been one of the main concerns. I agree that we don't need to have perfection, but I do think the reviewer needs to have some level of confidence that the project actually performs as advertised. So how do you get that confidence?
a. From installing the module and checking it out, or
b. From seeing the project author's demo or live site, or
c. From talking to the project owner, or
d. From looking at the code, or
e. From some combination of the above.
I also agree that the other parts of the code review are not about having a working module, but as I also pointed out in my other post, they come under the heading of having the project "release ready".
While I can appreciate the
While I can appreciate the desire for reviewers to 'stick with it' from start to finish for a review, this can also be a barrier to entry for new reviewers. Personally, I feel more than confident running modules through Coder, checking to ensure that someone isn't including a License file, and ensuring that functions have appropriate doc blocks and descriptions. When it comes to giving advice on the use of an API which I may never have tried to use myself; that confidence start to fall apart ... and thus, I hesitate to step up and take on a 'full' review.
However, it could help lessen the burden on the existing reviewers if someone less-experienced could 'sift through' the queue and knock off any low-hanging fruit. There are a number of common recurring 'checklist' issues for any code review ... duplicate module search, GPL issues, License.txt, // $Id$ tags (guilty!) ... it seems to me that filtering through these items in advance would allow experienced reviewers to focus on the actual code reviews, instead of validating whether someone has actually read the process.
While maybe not the optimal solution, a set of Issue tags could be established to flag when a given application has been validated against these 'checklist' items (or 'code review prerequisites'). This way, if I've got 10 minutes to kill, I could pop in, grab an app, and run it through the Coder module ... tagging the application with a "CRCoderPass" tag if things look good. Maybe that becomes all I contribute to the code review process ... as a 'Coder' review specialist, I can key off this tag in looking for other applications needing my attention. For a more experienced reviewer, an advanced search for apps containing each of the 'checklist' tags would return a list of modules ready for the 'code review' step; letting you get right to the review instead of spending a large part of your time filtering through people who didn't read the 'instructions' page.
One potential downside to this approach is the proliferation of tags, especially if also tagging for trending purposes as per Greggles post above. Another is the potential number of 'checklist' tags - typing 10 tags into an advanced search just to isolate 'code review-ready' modules would be a little painful ... though this could be addressed by initially using the 'active' status for new product applications, and only moving them over to 'needs review' once each of the pre-requisite checklist items was addressed.
As for the 'checklist' tags
As for the 'checklist' tags themselves, some suggestions might include CR_Dupes_Pass (duplicate module search), CR_License_Pass (Licensing issues and License.txt file), CR_Coder_Pass (Runs through 'Coder' cleanly), CR_Formating_Pass (for code style, doc blocks, spaces/tabs, etc), CR_Install_Pass (module installs cleanly without errors) and CR_Docs_Pass (has Readme, implements hook_help(), etc).
I like the idea, but you're
I like the idea, but you're right, all the tagging will be cumbersome. Since I haven't done any application reviews yet (making some time for that this weekend), I'm not comfortable giving the all-clear on more complex modules, But, I can definitely run down a checklist of these basic items, and (hopefully) save some time for someone who would be better equipped to do a more thorough code review.
Gates
I like the idea of more people doing smaller parts of more reviews. In my opinion, we've been focusing too much on how long reviews take, and not enough on the how long reviews site idle. I think the problem is most often than applicants feel like their reviews have been forgotten, not so much that they're in any particular hurry to get them done. If we could keep applicants engaged with smaller updates to reviews, I think that would make the process much more welcoming, even if the overall turnaround doesn't speed up at all.
That said, doing this with tags sounds like a mess to me. One approach that might work better would be if we could auto-tag new issues with a list of everything that needs to be done, and then remove those tags as the checks are complete. That way, if you just want to spend a few minutes checking for LICENSE.txt files, you could just search on the "LICENSE.txt check needed" tag and remove it from any applications where you did that check. That's a lot more like the gate approach suggested for general issue queue redesign.
I like this. We should also
I like this. We should also subscribe the node submitter to "my issues" in that queue. Too many times I've read "I didn't get an update about this".
knaddison blog | Morris Animal Foundation
I may be mistaken, but if you
I may be mistaken, but if you replace "new issues" with "new applications" in sreynen's post, I think we're suggesting autocreating tags on a 'New Project Application' request itself; as opposed to adding new 'gate' issues to the sandbox issue queue for that project. But in that context, I'm not quite clear on which queue you're referring to.
Then again, I'm assuming that applicants are already auto-subscribed to their entries in the 'Project Application' queue ... If not, then you're right - they definitely should be!
A single tag?
Would the tagging approach be more manageable if we were only concerned about a single tag to represent 'screening complete', and that technical/security review is all that remains for a given application?
'Screening complete' would represent that each of the checks in the 'screening' component of this checklist had been completed.
+1
I think this would work really well. It would allow people comfortable doing the more complex end of reviews to focus first on items tagged "screening complete" while not requiring everyone to adopt the process before it becomes useful. Unfortunately, we never arrived at much consensus on that checklist, which we'll need to do before "screening complete" can mean something useful among reviewers.
A single tag
I began adding a tag to applications that have passed screening.
PAReview: Screening Complete
Aside from using tags to collect statistics as was suggested earlier in the thread, I think adding a single, simple tag like this is all that is needed. Categorizing applications with too many tags could be confusing to new reviewers and it would be difficult to expect consistent results. Using a single tag may be more reliable and thus more efficient.
I do really like the idea of
I do really like the idea of having more people review.
I think my initial approach with people "sticking with a application" was to help with the mentoring and relationship and community building aspect of it. But, there is that clear assumption that the reviewer has the skills to do the full review.
As much as I would love to see sandbox issues queues used for reviewing, we don't have a strong enough reviewer base or Drupal.org infrastructure to support that. (even getting people to a single issue queue is hard enough).
So, I think auto-adding tags to applications is a great idea. Does anyone have any idea on how easy this would be?
--
zzolo
Infrastructure
I think "how easy" is probably something for the infrastructure team to answer, so I opened an infrastructure issue to ask about it.
Approach
I think the process would be to submit a patch to drupalorg_project_issue in the drupal.org customizations project ... we'd need to add an additional form_submit routine to the 'new issue' form (via hook_form_alter), filtering on the 'Project Applications' field, and adding tags to newly created items in the application queue.
I think using the sandbox
I think using the sandbox issue queue for reviewing is a great idea ... one advantage is educating users on the use of the issue queues (as sreynen suggested in a comment below). It just isn't practical for the quick filtering of statuses in the project application queue ... which is where the auto-tagging piece comes in.
I believe there is room for both:
1) Position the use of the issue queue for tracking issues found during a review as a 'recommended' approach (also being sure to include a pointer to each issue in the project application itself) ...
and
2) Leverage auto-tagging (and clearing tags as 'gates' are met) to simplify the process of filtering applications and tracking review progress, also encouraging less experienced reviewers to get involved without having to commit to a 'full' review.
Use the sandbox issue queue
I've done this on a few reviews and it works well in some cases.
Positives:
* Easy to track what has been done and by whom just by creating an issue for each step of the process.
* For projects with several different review points to be addressed, it makes it easy to track individual issues.
Negatives
* You have to navigate away from the project applicaiton queue to find out what's going on.
* Project owners also own their issue queues, and they tend to feel free to change the status of the issues to anything they want, like closed fixed, so that you don't see them unless you search 'Any' issue.
Use the sandbox issue queue
I'll take this one step further. If the actual review issues are posted in the sandbox, then the only thing that needs to get posted in the project application queue is notes by the reviewer as to what steps have been completed, such as:
Still there is no way to easily tell what stage of review the project is at. The ideal solution would be custom review status box, instead of just needs work and needs review. I kind of doubt that this is do-able with the current system. Since there is apparently some work being done on the issue pages, now would be the time to get this capability implemented.
It was your idea
@zzolo
I think I should point out that I got this idea from one of your posts. I don't really get your objection. If you're already on the sandbox page looking at the repository, it's actually more convenient to use the sandbox issue queue. The tools are there, but they're no good if you don't use them. I mean, c'mon, why do you need a stronger reviewer base to start using the sandbox queue for code reviews? Maybe I'm just not as aware as you of everything that goes on with code reviews. I think if you just let people know that it's OK to use the sandbox, some of them will try it, and maybe they'll like it.
good for future, but very bad for now
Hey @ccardea. Well, I am not sure which post you are referring to, but there is definitely a chance that I said that. I can change my mind, right? :) Anyway, I am all about using sandbox issue queues in theory, but I don't think the following are true:
1) We have enough momentum in the single point of entry for application (ie the current project application queue). It would be very hard to get momentum around people looking at a bunch of different projects.
2) And related, but more importantly, we don't have an interface to support reviewing in this mechanism. We would need to be able to see issues for multiple projects (in a sane way) and be able to see which issues are relevant to full project access reviewing.
I would love to see this happen in the future, but trying to make this shift right now would kill the little momentum that we have, IMO.
--
zzolo
okay for now, not for everyone
Two bits I'd like to add here:
1) I think this thread is largely based on responses taken out of context, perhaps a result of reading them via email, where there is no context. I'd suggest being overly explicit going forward, linking to what you're talking about and using the "reply" links under specific comments as much as possible.
2) I've lately been doing reviews entirely via the sandbox issue queues, and just pointing to that queue in the application issue after I've added issues. This seems to work well, and I think it gives the applicant a more realistic idea of how they'll be managing their project going forward. That said, I agree we can't expect all reviewers to do this, as would be necessary if we were to use sandbox queues for "gates." For anyone confused about why I'm now referencing "gates," I think that's where this thread actually began; see #1 above.
re: auto-adding tags
I think that has the potential to be helpful to the process, but I don't know how you would go about doing it.
Checklist needed
It's been mentioned and talked about, but I just wanted to give a 100% +1 to the idea of an easy to scan checklist of steps in a review. Sitting in the process (as I am now) can get frustrating as you just don't know if anyone is reviewing, what is left to check, what has been checked. So from a reviewee and reviewer perspective this could only serve to ease the whole shebang.
Dreamleaf Media
Sort by 'created' date, not just 'last update'
I've seen a number of applications which were opened, sat for three weeks, had their status or title updated, sat for three more weeks, then had the applicant comment "any news?", and then sat for three more weeks ...
I think it would be useful to be able to sort applications based on a 'created' date, instead of 'last updated'. Given that the project applications list is a 'view', it should be trivial for one of the d.o webmasters to add the 'node->created' timestamp as an additional view column.
Edit: Ooops ... missed bumpaw's earlier post on this very suggestion. In any case, I've opened an issue to add created dates to the queue at http://drupal.org/node/1179586.
Edit: Ooops ... missed
A suggestion is only good if you put it in the right box, and you did. :)
It's becoming more and more
It's becoming more and more obvious to me that this is absolutely necessary. Lately I have stumbled on a few applications that have fallen through the cracks because the author updates the issue by commenting, but the application is not being reviewed. After sitting for seven weeks, it looks like there is activity on the application but in reality nothing is getting done and there is no way to easily discover that seven week old applications have had no work done. For this reason I strongly support this idea and perhaps tagging for steps that need to be done.
We're close ...
Looks like the issue touched a bit of a nerve within the drupal community ... Webchick picked up the thread and extended the issue to ALL issue queues, not just the project application queue.
In any case, the patches were written and approved, and all that is left now is for them to be deployed on Drupal.org.
Issue thread (and history) can be found at http://drupal.org/node/1179586.
And Issue created dates are
And Issue created dates are now live!
Add Drupal Core Version to Project Appliction issue queue
It would be nice if the Drupal Core Version would be visible in the issue queue so that reviewers wouldn't have to view the issue to see which core version the module is for. There was some discussion of this in 2011, but I found as a first time reviewer that this slowed me down, and I found myself wanting it often. Also, I noticed that other issues queues use it. Could we get the Infrastructure team to modify this queue in that way?
I think it would just require
I think it would just require the commit of some code and creation of 6.x, 7.x, 8.x releases for that code. That could confuse people who see the releases.
I've also seen queues use "[D6, D7] Issue title" or "[D6] Issue title" as a way to indicate that an issue is related to multiple versions.
knaddison blog | Morris Animal Foundation
Good Point!
Yes, I've seen that too, and that would definitely make it easier for reviewers glancing over the list. The only thing gained by changing the issue queue as I propose would be the ability to filter by core version, which is not so important I guess. Could we update the guidelines to include a recommendation for using "[D6]", etc? That would ultimately help applicants get faster reviews methinks...
Done
I updated the guidelines to suggest adding the version in the issue title rather than the description. Someone could probably spend an hour adding versions to all the current "needs review" applications. Most of them have it in the description, and the rest should be clear from a quick peak in the Git repository.