Project 'gitlab-org/manage/general-discussion' was moved to 'gitlab-org/foundations/general-discussion'. Please update any links and bookmarks that may still have the old path.
As we get some internal feedback on Code Review Analytics, let's keep track of it here. Once we've got issues opened for every piece of feedback, we can close this issue.
Agree on sorting columns. I’d love to sort by review time so that I could get to the one pending for a while (long). Also related to the work undergoing in gitlab-org/gitlab-design#770 (closed). /cc @npost
Sorting by comments, commits, and line changes can also provide some insights.
Also, I would expect that I could filter by assignee on that page to see pending merge requests per project as seen on dashboard merge requests which include merge requests for all groups and projects.
Last but not least, the merge requests counter seems a bit lost over there. Maybe this could be bundled as a tab even if it's a single option? If this sounds good, let's also document this in the tab component documentation.
Are we thinking of a method to indicate when the last activity was? For example I see one with 69 days of review time, but if it's actively being worked thats less concerning
I see some with no review time that have comments from multiple people
To add to this... will the active state of the Project Analytics section in the sidebar be enough of an Indicator of where the user is if they come back to this page after some time? The breadcrumbs don't tell much of a story. Having just Code Review in title could also be a bit misleading.
Maybe we should reconsider naming convention: Project Analytics: Code Review?
CC @jeremy - for our naming conventions talk in our next 1:1
Since this automatically includes all merge requests this could fit nicely under the merge requests top-level menu item, right? Unless there are plans to expand project analytics beyond merge requests. /cc @npost@jeremy
@gweaver provided this feedback on Code Review Analytics:
Seems like a good opportunity to think about a more formal state and/or workflows. I’ve been running up against the same things with issues — when is something considered “done” even if it still open, etc. There are lots of nuances worth exploring. As we continue to scale, arbitrarily inferring state in a global sense seems problematic for many reasons. That said, this is an awesome first iteration and I was personally stoked to check it out!!!!
So happy this is released. I had a quick scan. To add to previous comments...
Deep links on filters would be great
Spacing of the columns looks a bit off
No tooltip over MR title
Padding can be reduced between MR title and metadata:
Reduce space between milestone icon and milestone number:
Embolden column titles
Some challenges with the MR title layout on mobile: ... could we just remove Merge Request column label on mobile and have the MR name and metadata left-aligned?
I think it would be useful if how the 'review time' was calculated was noted in the docs.
I'm also curious if we account for automated commenting? We have the danger bot which adds a comment when code is pushed regardless of whether a reviewer has been assigned. It can be useful to push code before it ready for review in order to run the full set of tests in the cloud.
I'm also curious if we account for automated commenting?
Yes, although sort of incidentally. The app attributes bot comments to the human author of the MR. So the criteria of "first non-author comment" ignores the bot comments.
Idea: a checkbox to "exclude WIP". This would be used by teams that use the "WIP" prefix to say "don't review yet".
Explanation: Currently WIP merge requests are displayed in Code Review Analytics. I think that's good, because the stated intention of the WIP prefix is to signal for early code review. But the stated intention isn't universal. For some (most?) teams, the "WIP" prefix is a way of saying, "don't touch this, it's not ready yet, but it's proof I'm making progress" (in a way that's more accessible than a git branch). For these teams, the signal for code review is removing the "WIP" prefix, and the ability to "exclude WIP" results from Code Review Analytics would be valuable.
I was looking at the Code Review Analytics page and loading the JSON data takes about 2secs. I couldn't get data from the performance bar so I'm not sure if it's the DB query or the calculation of the line changes takes long time.
I guess that's something that applies to all our Analytics features since they all share the same filtered search component? We might need to change the input placeholder Search or filter results... to something more specific. I don't know if our API supports full text search but I doubt?
@wortschi regarding search, I'm able to filter by label and milestone but when I search for a keyword like the example in https://gitlab.com/gitlab-org/manage/issues/15367#note_296309094 the list is still showing all items. This will usually not be the case and it's completely ok to not support full text search here but returning all items when searching for a keyword seems wrong in so many ways.
regarding search, I'm able to filter by label and milestone but when I search for a keyword like the example in https://gitlab.com/gitlab-org/manage/issues/15367#note_296309094 the list is still showing all items. This will usually not be the case and it's completely ok to not support full text search here but returning all items when searching for a keyword seems wrong in so many ways.
Right, we already have an issue to investigate if full text search is something we can easily implement on the BE side. I suppose the reason for showing all items is, that currently the search term is simply ignored.
Since those links are not code review analytics but issue analytics, I'm guessing that the search query params is being used by BE to filter the results (whereas code review analytics API ignores it).