Code Review Practices

We have started using upsource for doing code reviews in our organization and are in a mode of establishing a code review practice. I have a few queries related to this and would be glad to get opinions on them


 

  1. Mandatory reviews - The intellij plugin/UI does not mandate creation of a review on every commit. Would it be a good practice to have the plugin prompt for assigning a reviewer (with suggestions from old reviews) every time a developer tries to commit any code changes?

  2. End dates - I realized that since upsource follows a post commit review workflow at times developer moves ahead with other features/code fixes etc while the older reviews are just left open. Do you think having an end date when creating a review would be better or there are other practices followed that have proven to be good?

  1. What are the best practices around reviews for large code commits/fixes? I have read that code reviews are efficient when done in small focused batches ~ 300 lines/hr or less than that. While doing reviews with upsource, we at times ended up having more than 10 sources files being changed. Should a developer break them into logical chunks or could the tool help out in some way?

  1. Checklists - Having a look at collaborator as a code review, I came across this nice feature that could be pretty helpful during code reviews. Is this feature planned out in upsource?

 


Based on your experiences I would appreciate if you could share your thoughts on the above points.

12 comments

Hi Amit,

  1. While committing changes in IDEA there is a checkbox called "Review with Upsource", it remembers the last state, so once your developers select it, it will be always selected by default. However no mandatory in that case.
  2. Since Upsource is more about code review and team collaboration, rather than time management/tracking we believe that the most suitable way is to keep time tracking on issue tracker side. For instance you might integrate Upsource with YouTrack or Jira and keep all time related fields there.
  3. Well, initially Uapsource was designed to handle small batches of changes and it did work really well. However we faced out with the problem that code review in case of huge changes (10+ files and 1000+ lines) doesn't work smoothly, here I mention UI part. It's why we are going to release Upsource 2.5 with completely redesigned UI for code reviewes. Planned for mid of November this year.
  4. Yes, we do have it on our roadmap, please watch/upvote it here - https://youtrack.jetbrains.com/issue/UP-4912
0
Since Upsource is more about code review and team collaboration, rather than time management/tracking we believe that the most suitable way is to keep time tracking on issue tracker side. For instance you might integrate Upsource with YouTrack or Jira and keep all time related fields there.

Sounds right to rely on JIRA for management/tracking. Few roadblocks I could see in the tracking capability - I find it broken.

  1. It's a manual step to click on "Create Issue" link. Can upsource be configured in the settings page to always create a jira issue when a review is created?
  2. Currently the jira issue gets created with default values for few important fields like
    1. It is unassigned - Can it assigned to the reviewer? It would be best if upsource can create an epic when there are multiple reviewers and create individual tasks for each of the reviewers
    2. Timelines need to be set manually. Can the configuration accept values for jira fields like for e.g. set the fix in version field to the current release version?


Kindly let me know your thoughts.

0

Yes, correct, current approach has all of this disadvantages

1. Nope, there is no such setting. However it might be added to the product in terms of customs workflow feature we are working on right now. So far no estimates yet
2. a. In most common scenarios this button is used by the reviewer to raise some major concerns, file the issue and assign it to the author, one of the authors. So assigning such automatic issue to the reviewer might look a bit strange. Moreover review might have several reviewers, how to deal in that case?
2. b. Timelines should be set manually anyway, shouldn't it? Not sure about Jira but for instance in case of TeamCity and YouTrack such scenario might be configured: You close issue as "done", "fix in version" takes current build from TeamCity.

0

1. Nope, there is no such setting. However it might be added to the product in terms of customs workflow feature we are working on right now. So far no estimates yet


Ok. It would be great if you could add it to the custom workflow feature.

2. a. In most common scenarios this button is used by the reviewer to raise some major concerns, file the issue and assign it to the author, one of the authors. So assigning such automatic issue to the reviewer might look a bit strange. Moreover review might have several reviewers, how to deal in that case?


The issue is - If there is no one assigned it depends on manual intervention. It would be a lot painful to reply on manual jira task assignment if we were to use jira as a time tracking and management tool. For multiple reviewers, an epic can be created with one task per reviewer added to it as I suggested it in the earlier post.

2. b. Timelines should be set manually anyway, shouldn't it? Not sure about Jira but for instance in case of TeamCity and YouTrack such scenario might be configured: You close issue as "done", "fix in version" takes current build from TeamCity.


Again, manual settings turn out to be un-reliable. By setting the fix in version I meant the software version in which this issue (review task) is expected to be completed. I agree that the build number can come in only when the review task is closed. We refer that in a separate jira field.

Basically, I am trying to focus on a deeper integration of upsource with jira.
0

Amit,

Sorry for putting it off.

As for "2a" please upvote the following feature request - ttps://youtrack.jetbrains.com/issue/UP-5456

As for "2b" could you please clarify where Upsource should take dead line from? Do you mean to add some additional field on the review page for specifying time (version?) when review should be accomplished?

Thanks in advance.

0

Thanks for creating the feature request

For jira integration, the jira integration config could pull out fields from the jira project and allow the admin user to map them to a value. For e.g. Fix In Version field could be mapped to a jira function - latestReleasedVersion().
Currently our jira config has a couple of mandatory fields like FixInVersion, AffectInVersion. Due to this, we are unable to create issues from upsource.

0

Amit,

But what is Upsource role in that case? Why not to create some custom workflow on Jira side that will fill in all mandatory fields if e.g. issue title has Upsource prefix in it or something else?

Anyway I'll log a request for the further consideration- https://youtrack.jetbrains.com/issue/UP-5461 Please follow it.

Thanks.

0

So, should we have further discussion about it here or there?

A reply to your query would be - how would I intercept the issue creation done by upsource (through the REST API) and set the required fields?

0

Not sure how it might be accomplished, it looks like a question to Jira.  Note - All Jira issues created from Upsource have "[Upsource]" prefix in the title, maybe it can help some how.

Anyway we'll consider handling this case on Upsource side.

0

I wanted to confirm upsource jira integration - Does upsource make REST API calls to jira for creating issues?

0

Amit,

That is correct, Upsource calls Jira API for creating issues.

0

Please sign in to leave a comment.