WIP: Add a mentioned merge requests tab to issue page
User Story
As a developer or master, I wish to see the status of related (mentioned) Merge Requests from the Issue page, so that I can better understand whether an issue is ready to merge or not. In addition, each of the issues should have a color coding regarding its build status or WIP status.
- WIP —
text-info - Merged —
text-primary - Closed —
text-link - Pass —
text-success - Running —
text-warning - Pending —
text-warning - Canceled —
text-danger - Failed —
text-danger - No CI —
text-success
See gitlab-org/gitlab-ce#3180 for more details.
Merge request reports
Activity
mentioned in issue #3180 (closed)
Added 71 commits:
-
91cbf9db..ad306efd - 71 commits from branch
gitlab-org:master
-
91cbf9db..ad306efd - 71 commits from branch
@axil I have some in gitlab-org/gitlab-ce#3180, but I can add more, if you like.
- Last reply by username-removed-123742
Last reply by username-removed-123742- Last reply by username-removed-123742
334 note =~ /.*![0-9]+.*/ 335 end 336 337 def is_satisfy_mention_for?(issue) 338 path = "#{issue.source_project.group.path}/#{issue.source_project.path}" 339 340 if note.include?(path) 341 note =~ /^(For|(Fix[e]?|Resolve|Close)[sd]?) #{path}\##{issue.iid}.*/ 342 else 343 note =~ /^(For|(Fix[e]?|Resolve|Close)[sd]?) \##{issue.iid}.*/ 344 end 345 end 346 347 def get_merge_request_mention 348 if note.start_with?("mentioned in merge request") 349 mention = note[27..-1] -
len("mentioned in merge request")it's the number of characters in that phrase plus a space char.Edited by username-removed-123742
326 330 for_issue? || (for_merge_request? && !for_diff_line?) 327 331 end 328 332 333 def merge_request_mention? 334 note =~ /.*![0-9]+.*/ -
I can, but they do already exist in the issue itself. I can try out doing PDF and other uploads if I can be given a list of what all of the accepted file types are.
Edited by username-removed-123742 -
@rspeicher, @DouweM, can you review this MR?
I will review this more thoroughly tomorrow, but be aware that !1569 (merged) does something at least technically similar.
-
@rspeicher yes, there can be. At the company I own, we have projects for libraries, clients, server, etc and extremely commonly there are many merge requests necessary to close a single issue (both client and server must change together or both a library and a project dependent upon it must change together).
!1569 (merged) already acknowledge that as @DouweM pointed out, as does the highest rated feedback request in #2772 (closed) concerning the ability to organize into many groups and projects would, in turn, require exactly this ability.
Edited by username-removed-123742 -
Beyond that there is no easy way otherwise to:
- find related, relevant merge requests in long discussions (not just mentioned ones, but ones intended to satisfy an issue's concerns).
- find attachments in long discussions a la Slack
- determine the overall status of the issue without hunting down merge requests (which are not listed), determining when they were last edited, and what state they are in, thus whether an overall code review is worth beginning yet (across all of the merge requests).
Edited by username-removed-123742 -
@DouweM I believe these are complimentary changes. The merge request meta-data (who, when, last mod, etc) and state (WIP, passing, failed, etc) are significant pieces of information necessary for high-level decision making as soon as MRs are > 1 for any 1 issue.
-
@creamzy came up with the idea of a tab in gitlab-org/gitlab-ce#3180. I like it.
Edited by username-removed-123742 -
@sytses I'd be curious to hear your feedback on this UI change. The proposed UI is shown in comments from 2 days before this one (not the ones just two comments before this comment which are just for motivating the original purpose).
Added 33 commits:
-
78171d54..55a39ba4 - 33 commits from branch
gitlab-org:master
-
78171d54..55a39ba4 - 33 commits from branch
Added 304 commits:
-
5f47b61c..a8df809d - 304 commits from branch
gitlab-org:master
-
5f47b61c..a8df809d - 304 commits from branch
Added 1 commit:
- fc36915d - revert for now
Added 1 commit:
- c90e08fa - revert for now
Added 1 commit:
- fb14d628 - add some doc
-
Beyond the idea that perhaps some workflows have exactly 1 merge request ever, there is also the need to find the merge request from the issue page. And determine its status without switching pages. This is akin to the Attachments tab shown here, which helps find the files attached to the discussion without needing to wade through the entire discussion.
The original purpose remains, however, the ability to see the build status of the issue's merge request(s) from the issue page itself, so that scanning many issues is efficient, when there are many issues in a milestone.
-
@rspeicher wouldn't issues like #3078 (closed) be an example of a use case where this feature would find some utility.
@gsmethells I'm not a fan of the colored text. We don't do that anywhere. We shouldn't start here.
@gsmethells can you attach (a) screenshot(s) of the current state?
I don't understand this issue.
As a developer or master, I wish to see the status of related (mentioned) Merge Requests from the Issue page, so that I can better understand whether an issue is ready to merge or not.
what do you mean by issue ready to merge? Why link to merge request from comment is not enough?
-
Finding mentions in the comments is not scalable. If there are many comments, then one has to search for the link in the hay stack. After that a merge request can be in one of several states. Where can someone scan them to see if they are passing without having to deep dive? Beyond that of many projects' merge requests are necessary to satisfy the issue (library, client, server projects) then they must all be ready at the same time to prevent half merged situations between client and server.
I think there is value to showing a list of all referenced Merge Request, but I have some concerns with the current implementation. Some thoughts, in no particular order:
- We should find referenced MRs using the
all_references.merge_requestsmethod that both Note and Issue have, instead of writing custom code to find references. - We don't need to do an additional check if the merge request actually mentioned the issue, that's what cross-reference notes exist for. We can just show all those MRs that show up in
all_references. - We should only show the "Merge Requests" and "Attachments" tabs if there actually were any.
- We should use the merge request list item view that's used in the merge requests list, not some comment/merge request combination with colored text. We should also show the project the MR is in, if it's different from the current project. This could look like a grey label before or after the title.
- We should have a "Go to comment" link by every MR and attachment item, that links to the comment that referenced it.
- Attachments are not necessarily images, they are everything that has a link or image tag that starts with
/uploads/, which also includes uploaded files - We should not editorialize what a specific MR build status means, no
ready to merge,work-in-progressorneeds attention. Let the build status in combination with the WIP prefix and assignee speak for itself. - We should also show a "Related Issues" tab on the issue page. No need to only show related MRs.
- We should also show this data, i.e. related issues, MRs and attachments on the MR page.
- We don't need to repeat the sidebar on these other tabs, that's just for Discussion (see the merge request "Commits" and "Changes" tabs.)
Edited by Douwe Maan- We should find referenced MRs using the
@gsmethells Thanks for helping to make GitLab better. I agree with all the points made by @DouweM I know this is a lot to ask but could you incorporate them?
-
I'll give it a look on Mon. As long as this view point is solid, I can look into it.
Recall that the opinions in the discussion previously disagreed on some of those points. Specifically on referenced MRs @stanhu and @JobV preferred relevant related MRs instead of all mentioned MRs which was what I did at first. If that is what the
all_referencesmember is then that's fine, I just did not known of its existence. If not then are you sure you are all in agreement first before I start down this path to undo all the work you asked me for last time? I happen to agree with @JobV and @stanhu and that is clearly the reason I expanded the code to find MRs. It was not an arbitrary decision. See the discussion for full details.EDIT: The current design is based upon feedback from @creamzy, though I do not believe I fully understand each of these people's roles, I assume they have some say somehow in the project, yes?
Edited by username-removed-123742 @gsmethells I think this is the comment you're referring to:
@stanhu wrote:
@gsmethells I can definitely see the utility of this change, and I like the general idea. I'm just not sure if it fits in everyone's workflow. For example, if I mention, "Take a look at !1793 (merged) as an example of a MR", suddenly the MR will be put at the top when it isn't really relevant. Obviously, this is a contrived example, but the point is that the context of the mention may not be captured completely by grouping it with MRs that need to be merged for this issue.
I think it's better to show all referenced MRs and have the user decide which are relevant, than try to capture this in code. While the
Fixes #xyzformat is an established practice to signify relevance, it is close to impossible to reliably decide this for other types of issue-mentions. The currently implementedFor #xyzformat may work for your case, but will not for people with other customs, likeSee #xyz,Related to #xyz,RE: #xyz, etc.Not implementing a check to see if the MR is actually relevant has very few false positives (really only the contrived example @stanhu mentioned), but zero false negatives.
As for the design, my main issue is that I don't think merge requests should look like they're comments, since they're not. They're a completely different kind of data, and we have an established way to display merge requests in a list.
@creamzy definitely has say in the project, especially with regards to looks of it, as she is one our great UX Designers. I don't think my view here conflicts with hers.
-
@DouweM Alright, we will flip the MRs in the list back to just whatever
all_referencesgives, then.@DouweM @creamzy My last concern, then, is that my original need is not handled after all of this steering. What I need is a way to see the status of the merge request(s) without having to literally dive in to see it on each individual MR page (once I find the related merge request(s) of interest). If we use the MR list item view that already exists, that can be fine except for the need to display the status, which that view does not have yet. So, would it be acceptable if I added something to that MR list item view to indicate the status? If CI is implemented, that would be the MR's build status; if not, then that would be the MR's open/merged/closed status. I'm picturing a label.
@creamzy what do you think?
EDIT: Looking at the current views just now, I see that the non-CI MR list item view is already handled as there is a label for "MERGED" and "CLOSED". It is just the CI case that is not handled. When the MR is open, the MR list item view has no label to distinguish open/passing from open/failed, open/pending, etc.
Edited by username-removed-123742 
@gsmethells I'll take and look and comment a little later.
@DouweM has lot of valid points in his list, so I need to check UI library we have now not to produce extra UI for existing elements. I've treaded the initial idea the same way as you - just filtering what is already here. Still after all the feedback received, there is a clear vision of MR tab development.
Extra colors and small tricks existing in your vision are not really working in the GitLab context as we're trying hard to ship basic ui patterns and library for the community to reuse, that is why it's more Spartans approach.
I understand that it's quite funny situation that a designer asks a developer to be less creative, but I hope you do understand why. Hold on till tomorrow, please.
Edited by username-removed-264303@gsmethells Are you sure the status is not shown in the MR list items? I see a bunch of checkmarks and crosses on the right here: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests.
-
Is there some config required for that? Our local gitlab does not show those, but, yes, I see them and if they are there, then great. EDIT: our CI is on the same server as gitlab (we are running 8.1 currently).
Edited by username-removed-123742 @gsmethells Are you using GitLab CI or an external CI service? The merge request index CI status currently only supports GitLab CI and external CI service that use the commit status.
@gsmethells Hmm. I just looked it up, and this feature was introduced with GitLab 8.2.0. Are you up to date?
-
8.1 as I mentioned earlier. Well, whoever added that had similar thoughts as mine, so cool.
I see no other hang ups with what you're suggesting in your bullet points. I've just been hanging back until I hear some consistent opinions. If you and @creamzy say you're on the same page tomorrow then I'll start in on the changes then (in an earlier comment here, she said she wanted to think things over and I should wait until tomorrow). I'd just rather be sure this time that it will be what will be acceptable. I've been through a few tries already.
@gsmethells Sounds good, let's wait until we hear back from @creamzy.
-
We should use the merge request list item view that's used in the merge requests list, not some comment/merge request combination with colored text. We should also show the project the MR is in, if it's different from the current project. This could look like a grey label before or after the title.
Fully agree on reusing MR list design. After chatting with @dzaporozhets I would even suggest design like on the screenshot below with grouping not to produce the same label and to reuse our UI library without any additional elements. No gray labels needed.
We should have a "Go to comment" link by every MR and attachment item, that links to the comment that referenced it.
I would say Show in Сomments instead of Go to, go to sounds a lot like we gonna redirect you somewhere else.
We should also show a "Related Issues" tab on the issue page. No need to only show related MRs.
I am for the related issues feature, but against the tab. I do believe their place is under the issue description with own header , something like Linked Issues, or Related Issues.
Plus how are we going to build this relationship? When creating the issue there is no possibility to connect it with another issue, except for mentioning which is not really a workflow. I do have the same problem with linking issues, because right now it's like list made by hands, but tabs are not gonna solve this UX. Let's avoid it for now , what do you think @DouweM ?
We should also show this data, i.e. related issues, MRs and attachments on the MR page. For related issues see above
Everything else sounds awesome.
PS I am not a big fan of MR design, so if we reuse what we have now, and after sometime MR list will have better design, our tab will profit as well!
Fully agree on reusing MR list design. After chatting with @dzaporozhets I would even suggest design like on the screenshot below with grouping not to produce the same label and to reuse our UI library without any additional elements. No gray labels needed.
I would say Show in Сomments instead of Go to, go to sounds a lot like we gonna redirect you somewhere else.
Good ideas!
I am for the related issues feature, but against the tab. I do believe their place is under the issue description with own header , something like Linked Issues, or Related Issues.
Why would this need a different method of presentation from "Related Merge Requests"?
The "Merge Requests", "Attachments" and "Related Issues" lists are all different ways of representing the list of comments by filtering one specific type of data, so I think having them as tabs alongside "Discussion" makes sense.
Plus how are we going to build this relationship? When creating the issue there is no possibility to connect it with another issue, except for mentioning which is not really a workflow. I do have the same problem with linking issues, because right now it's like list made by hands, but tabs are not gonna solve this UX. Let's avoid it for now , what do you think @DouweM ?
Mentioning issues as
#123is very much an established workflow, developers have been doing it on GitHub and similar systems for years and years, and it's more dynamic than picking issues from a list, since you can write a sentence to provide context, and easily reference from comments etc. Most developers prefer typing their references from picking them in a list :) I don't think we need to add UX to choose related issues, we just need to present the referenced issues better, which is where the "Related Issues" tab comes in!-
@DouweM nice explanation about Related Issues, let's do it like that
@gsmethells it seems like all is set, in case you have any questions or you need something extra ux related, ping me promptly. Good luck with this MR!
mentioned in merge request !1934 (merged)
-
@creamzy I agree with @DouweM that typing is the best developer workflow. It is one of the things I really like about GitLab. I'll see what I can do here.
One thing: after some thought myself, I have to say that I think hiding the tabs when there are 0 attachments, 0 merge requests, and 0 (related) issues feels a bit funny? I feel like the UI should remain constant and consistent and that means not having modes.
Edited by username-removed-123742 -
@creamzy how about "Show in Discussion" instead of "Show in Comments"? Might as well reference a UI element by name (the tab name in this instance).
-
@DouweM @gsmethells agree on • name "Show in Discussion" • keeping tabs visible all the time
sometimes thread is so long, so it's much better to get overview of main instances from the first glance, because their presence as well as their absence say something. Theory is not always for every context, but in our case theory works perfectly.
/cc @dzaporozhets
-
@DouweM I do not see that many places
all_referencesis used in the code. When I use it as@issue.all_references.merge_requestsinIssuesController.show, I get an empty array even though there are mentions from a merge request about the current issue. -
@DouweM nevermind, I think I figured it out.
-
@DouweM @creamzy FYI I am moving into a new branch for this work so !1941 (closed) now replaces the work we've been discussing here for gitlab-org/gitlab-ce#3180. I've also updated the Description in gitlab-org/gitlab-ce#3180 to have a list of checkboxes for the items I believe we discussed doing.
@gsmethells Cool, I will close this MR then, and we can continue the discussion there. Thanks for bearing with us :)
mentioned in merge request !1941 (closed)
mentioned in merge request !1991 (merged)
mentioned in commit 2bc82130
