Workflow for rejecting code reviews

Michael Hedgpeth
I'm New Here
I'm New Here
Those new to the Atlassian Community have posted less than three times. Give them a warm welcome!
December 10, 2012

I have been assigned a code review in Crucible. We are not using JIRA or Fisheye; the code review was automatically assigned by the checkin comment through the smart commit feature.

After doing the code review, I noticed something that needs to be updated. So I want to "reject" the change. In other words, I want the developer to take action on the code review before the review is complete.

Do I:

1) Complete the code review with defect comments to show the code reviewer that I'm done, then when he commits and adds that changeset to the code review I will re-review it?

OR

2) Don't complete it until the developer makes changes that satisfy me. Trust that the developer gets notifications of my comments.

2 answers

1 accepted

2 votes
Answer accepted
John Werner
Rising Star
Rising Star
Rising Stars are recognized for providing high-quality answers to other users. Rising Stars receive a certificate of achievement and are on the path to becoming Community Leaders.
December 18, 2012

For our development, we also run Crucible without JIRA (but with Fisheye). Our process says that a review can not be completed until all defects are resolved or postponed. (Postponing implies that we will open a tracking issue in our issue database.) The moderator is responsible for ensuring that all defects are resolved or rejected.

After trying different things, we finally found that tracking the state of the defect was best done by adding a new metric to defects, State. The state can only be changed by the writer of the defect. The state can be "open," "resolved," or "postponed." Additionally we track the Resolution of a defect: "Fixed," "Rejected," "Other," or "none".

By making the state part of the defect itself, the moderator can run queries through Crucible to look for the state of items. We also use a custom written tool that uses the REST API to generate reports that can be reviewed for before the review is summarized and/or closed.

Tini Good
Rising Star
Rising Star
Rising Stars are recognized for providing high-quality answers to other users. Rising Stars receive a certificate of achievement and are on the path to becoming Community Leaders.
December 18, 2012

Thanks for this. I didn't pay enough attention to Crucible settings, but this made me go back and figure out that you can add another Classification! Very useful!

John Werner
Rising Star
Rising Star
Rising Stars are recognized for providing high-quality answers to other users. Rising Stars receive a certificate of achievement and are on the path to becoming Community Leaders.
December 18, 2012

Don't feel bad. It took me a couple of reviews to figure out that you could add them. Before that we were trying to use keywords in comments -- not a very good way to do it.

Sten Pittet
Rising Star
Rising Star
Rising Stars are recognized for providing high-quality answers to other users. Rising Stars receive a certificate of achievement and are on the path to becoming Community Leaders.
December 18, 2012

Hi John,

Could you let me know what kind of reports you are generating via the REST API? It'd be great to know more about the way you're using Crucible.

Cheers,

Sten Pittet
FishEye / Crucible Product Manager

John Werner
Rising Star
Rising Star
Rising Stars are recognized for providing high-quality answers to other users. Rising Stars receive a certificate of achievement and are on the path to becoming Community Leaders.
December 18, 2012

The tool generates an HTML report that has hot-links back into Crucible. We use it for various stages from monitoring review progress and to generating an official report on reviews.

The basic content of a report is a section for each included review along with a summary of all reviews. For each of the section, the following information may be output:

  • Reviewed files
  • Review info (Author, Moderator, Reviewers, Create Date, etc.)
  • Comment and Defect Tables

The Defects are broken into 3 tables for the 3 states we use: Open, Resolved, Postponed. [A completed review should have no Open Defects.] Each comment and defect can include multiple configurable data columns, but typically they are:

  • Comment ID - linked to the Crucible item
  • File/Line
  • Ranking (for Defects)
  • Classification (for Defects)
  • The Comment (message)
  • Who wrote the comment/defect
  • All of the Replies
  • Status (our defect metric)
  • Resolution (our defect metric)

The basic types of reports we have found useful to generate with the tool are:

  • Report of Specific Closed Reviews (e.g. everthing for the next release)
  • Report of all reviews that are still Open with All Authors Complete
  • Report of Reviews in Summarize (waiting for our review of the review to close)
  • Report of Specific Issues (Comments/Defects) by ID
Sten Pittet
Rising Star
Rising Star
Rising Stars are recognized for providing high-quality answers to other users. Rising Stars receive a certificate of achievement and are on the path to becoming Community Leaders.
December 19, 2012

Thanks for the detailed answer!

2 votes
Tini Good
Rising Star
Rising Star
Rising Stars are recognized for providing high-quality answers to other users. Rising Stars receive a certificate of achievement and are on the path to becoming Community Leaders.
December 16, 2012

We've made Moderator mandatory for now for just this reason. We're using option 1.

Our review procedure states that the Moderator has responsibility to make sure everyone agrees with decisions and follows the process. Reviewers should verify their fixes are incorporated in the newly attached changeset, but Moderator will verify that work is complete.

We gave an exception to having all defects fixed. If the fix requires more than 10 working hours, and/or impacts a major project milestone, the Author may create JIRA Issues instead. That ensures that defects will be fixed at some point, and allows us to close the Review. Less than 10 hours, must be fixed before Review is closed.

Suggest an answer

Log in or Sign up to answer
TAGS
AUG Leaders

Atlassian Community Events