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
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?
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?
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?
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.
Please sign in to leave a comment.
Hi Amit,
Sounds right to rely on JIRA for management/tracking. Few roadblocks I could see in the tracking capability - I find it broken.
Kindly let me know your thoughts.
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.
Ok. It would be great if you could add it to the custom workflow feature.
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.
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.
Thoughts?
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.
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.
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.
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?
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.
I wanted to confirm upsource jira integration - Does upsource make REST API calls to jira for creating issues?
Amit,
That is correct, Upsource calls Jira API for creating issues.