Suggestions for code review system

[JakubH]

We have been using the Plastic inbuilt code review system for months. This system is very basic and has not many features, however we have improved it in some ways by integrating it with our other tools using attributes, triggers and other utilities.

 

Though, there are still a few things which can help us to use it more effectively. Here are our suggestions:

• The ordering of comments in code review is not very practical and it doesn't encourage discussion. Some solutions (including a really simple one – just change the sorting algorithm) are already added as a UserVoice suggestion and it has a lot of votes already.
• The properties of the code review (Status, Assignee and Title) are not editable from the code review window which is awkward. The only way how to change these from GUI is from context menu from the review list open Properties. There is a second UX issue that the Properties item is not at the end of the menu as everywhere else. There is Delete unfortunately at that position.
• Add a comment to a whole file (not just to a line of the file) – especially useful for binary files or deleted files where it is not possible to add a comment now. (see this UserVoice suggestion)
• Add a comment to a whole review target – a changeset or a branch. For general comments or suggestions like "You should write the source of these binary files into the checkin comment." (see this UserVoice suggestion)
• Filtering changesets by branch name and reviews by review title should allow wildcards (see my UserVoice suggestion):
  find changesets where branch like '%release%'
  find reviews where title like '%fail%'
• Add triggers on add/modify/delete an attribute on an object (changeset, branch). (see my UserVoice suggestion).
• A window Comments and an editing dialog of a code review comment (which has Comments title too) are modal, which is inconvenient. Sometimes, code reviewer wants to check something in Plastic GUI during editing a comment, but it is not possible; it blocks the whole Plastic GUI.
• Link from code review ("Show this changeset/branch in a Branch Explorer").
• It is not possible to copy author name, changeset number or date in the code review window which is a pity, especially in combination with a lack of the link (see former suggestion).
• Go to the commented line on first double click (see my UserVoice suggestion) – this is quite annoying now.
• An option in changeset/branch context menu to show a related code review (or a list of related code reviews if there are more of them, however in our case, we limit (with a before-mkreview trigger) the number of code reviews to one per target anyway).
• Indenting in an added file in any diff including the one in code review window is set to 8 spaces per tab and there is no option to change it. We want to choose 4 spaces per tab as we can choose for changed files already.
• A possibility to automatically fill-in the title of a code review according to a branch name or a changeset comment (shortened).
• We are using this workflow:
  • A programmer write and check-in some code.
  • A reviewer inspect the changes and may add some suggestions as code review comments.
  • The programmer may react on the suggestions with new comments and may check-in some fixes.
  • Now, the reviewer should be able to see easily his/her old comments, the programmers' replies and the new changes together, without a need for searching for these.

There is some functionality which bring a link to follow-up changes in a branch code review now, however it is limited to files which have some comments. It would be better to see the whole changeset together. What about adding a possibility to use something like an Explore changesets on a branch view. It would be great if I can select multiple (consecutive) changesets to see all their changes together, and also with their code review comments with a possibility to add new ones.

• A character & in a code review comment is changed to an underscore under the next character. We would prefer to override this functionality. UserVoice suggestion
• Minor issue: Review state Discarded is populated as Discarted in mkreview and editreview triggers

 

See my general UserVoice suggestion.

 

Edit: I've added several UserVoice links.

[manu]

We have been using the Plastic inbuilt code review system for months. This system is very basic and has not many features, however we have improved it in some ways by integrating it with our other tools using attributes, triggers and other utilities.

 

I'm very interested in your modifications, I'm sure they're pretty cool. Maybe we can prepare a blogpost explaining them referencinf as the author, of course (if you want)

 

The code review system needs a lot of improvements, we are aware of them and we want to find time to improve it greatly.

 

I agree with all your suggestions so let me share them with the team to take them into account in the future.

 

Thanks!

[JakubH]

Well, I think I can share what we have done and how we use it now. I have no problem with the blogpost, if you think it can be interesting for others too. Here is my unpolished description:

 

We use Plastic SCM as our main version system in a centralized mode and we have our custom proprietary issue tracking system (we have it much longer than Plastic and it has also much wider use than issue tracking, it is continuously evolving). Today, we do not use branch per task fully. We create feature branches for bigger tasks or modules, but a branch usually contains changesets belonging to more issues. (We may incorporate a stricter branch per task methodology in future, we improve our workflow gradually.) The integration between changesets and issues in the issue tracking system is done through checkin comments. Every changeset has a comment which must contain at least one issue ID and a brief description of changes. This is checked by a before-checkin trigger. We haven't made an integration of our issue tracking system into Plastic, even though it should be possible, maybe we can do that in future too.

 

Every public release of our product – which we launch every three months or so – has its branch. We want to be sure that every change which gets into any of these release branches is at least briefly checked by a reviewer. To know, which changeset has been checked already, we are using attributes. In after-checkin trigger, an attribute "Review status" is add to the new changeset with value "none". When a reviewer check the changeset, he change the value of this attribute to "Approved". In case he has found some problems, he creates a new code review for this changeset. (Which automatically set the attribute "Review status" to "Pending" thanks to an after-mkreview trigger.) The reviewer writes some comments to the code and switch Assignee to the author of the changeset. Then the author checks reviewer's suggestions, writes answers and/or checks-in fixes. After that, the author switches Assignee back to the reviewer. Then the reviewer checks it again and sets the code review to "Approved" which automatically sets the attribute "Review status" of the target changeset to "Approved" (thanks to an after-editreview trigger). We limit the maximal number of code reviews in Plastic to one per changeset (a trigger before-mkreview doesn't allow you to make new code review for a given changeset if there is one already).

 

Now, our issue tracking system is integrated with this in several ways. Every issue in our system shows numbers of linked changesets (in a given state) and code reviews – see following screenshot:

As you can see there are two changesets in a release branch with an attribute "Review status" set to "none", they are both in a repository "install". The small button "c" copies a find query (which can be used to filter those changesets in Plastic) into a clipboard. These counts are refreshed every hour automatically or you can force the refresh by clicking on the Refresh button. It would be better if the counts were refreshed right when they are changed, however it is not fully possible, because there is no trigger for changing attribute value unfortunately.

When these counts are refreshed and all changesets in release branches are "Approved" and the issue in the issue tracking system is in a "To Review" state, it is automatically switch to "To Test" state, so testers can check the issue from a user point of view.

 

That's about it. As you can see, it's not only post-commit code review but even post-integrate code review, but it is better than nothing and we can improve it in future. Here is a workflow chart of our review system, if you are interested:

 

To do this, I wrote several scripts in C# – for all those triggers and also utilities to refresh those counts in our issue tracking system. For read-only querying I end up with direct SQL queries to the underlying database, because it has much better performance and it has the flexibility I need. For example here is an SQL query used in one of my utility for getting all changesets with their statuses (this utility is used to export these information to Excel, where we can sort or filter it as needed):

SELECT 
  attributerealizations.svalue AS Status, 
  changeset.ichangesetid AS Changeset, 
  changesetObject.dtimestamp AS Date, 
  changesetOwner.scode AS Author, 
  branch.sname AS Branch, 
  changesetObject.scomment AS Comment, 
  review.assignee AS ReviewAssignee, 
  review.author AS ReviewAuthor, 
  review.dateModified AS ReviewDateModified, 
  review.status AS ReviewStatus 
FROM 
  changeset 
  JOIN object changesetObject ON changeset.iobjid = changesetObject.iobjid 
  JOIN seid changesetOwner ON changesetObject.fidowner = changesetOwner.iseidid 
  JOIN branch ON changeset.fidbranch = branch.iobjid 
  JOIN attributerealizations ON changeset.iobjid = attributerealizations.fidsourceobject 
  JOIN attribute ON attribute.iobjid = attributerealizations.fidattribute 
  LEFT JOIN ( 
    SELECT 
      review.fidtarget AS target, 
      reviewOwner.scode AS author, 
      reviewAssignee.scode AS assignee, 
      review.dmodified AS dateModified, 
      review.istatus AS status 
    FROM 
      review 
      JOIN object reviewObject ON review.iobjid = reviewObject.iobjid 
      JOIN seid reviewOwner ON reviewObject.fidowner = reviewOwner.iseidid 
      LEFT JOIN seid reviewAssignee ON review.fidassignee = reviewAssignee.iseidid 
    WHERE 
      review.itargettype = 15 
  ) AS review ON review.target = changeset.ichangesetid 
WHERE 
   attribute.sname = 'Review status' AND 
   changesetObject.dtimestamp >= '2014-04-01' 
ORDER BY Date DESC