Project setting to toggle ability to overwrite merge request project settings
Resources
FE @psimyn
Description
- Update wording per the mockups / screenshots.
- New project setting that allows you to disable overwriting approvals settings per merge request. By default, the setting should allow to overwrite approvals settings.
Previous design of project settings
New design of project settings
Activity
@Marzuchowski, most of what you ask is already possible. Approvers are an EE only feature right now, with current implementation you can (on a project level):
- Define a list of approvers
- Required amount of approvers who must approve a MR.
With a combination of this two, you can achieve what you described. To prevent users from pushing directly to
masteror any other specific branch, you can protect that branch.Having the possibility to define a default value/template for that in a group level sounds like a nice to have.
Can you give an insight on what your workflow looks like?
What I'm missing is a feature to tell GitLab that a specific list of approvers is required. Currently it's possible to add additional approvers to specific merge request and bypass the predefined approvers. For example if you have one person from a vulnarability and security team, one from code quality and some one else, developers can just add other team members to reach the number approvals. There should be some thing like strictly requiring an approver x or a member from team y.
@heiko_boettger the level of granularity you propose sounds fair, but also adds some complexity. If you are able to prevent additional approvers to be specified in a merge request, do you think it will be enough?
@JobV what is your opinion on that?
Milestone changed to 8.9
Milestone changed to Backlog
Mentioned in issue #580
changed milestone to %Backlog
added SP2 label
@smcgivern : This should be a simple configuration setting in the approvals section of the project settings right? So if checked, you cannot configure approvals per merge request? I'd imagine this would be a medium t-shirt size effort with both BE+FE?
Let's consider it as a candidate for 9.3 if you think it's reasonable.
@victorwu totally reasonable, although we'd have to allow existing MRs that were already created to 'reset' themselves to the project settings, wouldn't we? Or would we update those when this setting was changed?
@smcgivern : We can make handling existing merge requests a separate issue. This can apply to only newly created merge requests (assuming that is the lower-effort implementation).
@cpallares @balameb : We'll keep this one our radar for 9.3. Please ping me again if you don't see a decision for including it in 9.3 or not by May 1. Thanks!
@victorwu my concern is if you edit an existing MR, but I guess we could just leave those as they are
changed milestone to %9.3
added Deliverable label
@tauriedavis @hazelyang : What should the design be here? Should we simply remove or disable the associated fields in the merge request form? What's better? Or something else?
assigned to @hazelyang
@tauriedavis @smcgivern : I tweaked the wording in the approvals section. Can you please review the description? (Also said that we should remove the table if it's empty.)
@tauriedavis I don't think an empty table communicates none has been added. And it just adds to the cognitive load of having the user (possibly new user) parse the web form.
For a new user, they would skim through the section and see a text field to add approvers and a clear green button to add them. There is nothing, as expected. Why have some UI to express nothingness? Why not just have nothing to express nothing?
Then they add the very first approver, and then the table pops in automatically and immediately they understand the purpose of this table.
Furthermore, without the table at the beginning, the user sees both fields (approvers and approvals required) located very close together. So especially when scrolling in small screens, the user can see both easily at the same time and understand they are subfields for this approvals section.
Should we simply remove or disable the associated fields in the merge request form?
I would remove 'Add approver' field and 'Remove' button in approvers table and then disable 'Approval required' field. So people still know the essential information such as who are the approvers and the number of approval required, but they are not able to edit them.
@victorwu looks good. No preference on whether we hide the table or not
@victorwu I disagree, I feel it is more cognitive load as the user has no information on whether there are or are not approvers and has to figure that out rather than simply being told.
Why have some UI to express nothingness?
There a few reasons: It clearly indicates to the user that they may need to add something. It also communicates that there is nothing currently there. Showing nothing could be confusing: Where are the approvers? Where did the table go? Is this a bug? How do I know who the approvers are?
I think having nothing leads to more questions and makes the UI unclear.
It doesn't have to be designed as a table, but there should be some design/copy/empty state that tells the user nothing has been added.
I do see your point about approvers and approvals not being next to each other. But in that case, shouldn't we move approvals to the top, so they can always be seen together?
Edited by Taurie DavisI agree with @tauriedavis - showing the empty UI element so people know its there. If we put the add field below the table (or inline in the last row) then it solves:
- approvers and approvals required both visible
- It communicates that there is nothing currently there
- users going between a project with and a project without approvers wouldn't be confused ("where did the table go?")
assigned to @psimyn
@victorwu @tauriedavis @hazelyang here is what I've got so far. Any thoughts on these questions:
MR Page, setting disabled
- Should suggested approvers be below Approvals Required?
- Should Approvals required helptext be shorter?
MR page, setting enabled
Settings page
-
"Approvers" is title of the search field and the table. Should table be titled "Current Approvers" and search field "Add Approvers"? Or some other way to differentiate
-
If an MR has overwritten Approvers, and the setting is disabled. It still has custom approvers. Is this the correct behaviour?
-
Still going with hiding table? I am still in favour of keeping it based on https://gitlab.com/gitlab-org/gitlab-ee/issues/582#note_28844549
MR Page, setting disabled
Should suggested approvers be below Approvals Required?
I think we can remove suggest approvers.
Should Approvals required helptext be shorter?
It's only useful when people are able to edit the amount, so I think it can be removed as well in this case.
Settings page
If an MR has overwritten Approvers, and the setting is disabled. It still has custom approvers. Is this the correct behaviour?
I think it should show the setting on project setting page.
Still going with hiding table? I am still in favour of keeping it based on https://gitlab.com/gitlab-org/gitlab-ee/issues/582#note_28844549
We are following @tauriedavis 's suggestion.
Thanks @hazelyang + @psimyn .
Yes, when the user cannot overwrite the project settings in the mr, then suggestions do not make sense, so we can remove them. For the case when you can overwrite the project settings in the merge request, we should include suggestions. They look good in your screenshot in terms of position.
The help texts should be removed. They are unnecessary like @hazelyang said. For the project settings page, we already have a link to the docs. So we don't need the extra help text. See the description mockups/screenshots. Can you make sure the text copy is the same? Thanks.
Yes, we are keeping the table. I updated the description.
I agree with the "Current Approvers". The search placeholder I think makes sense to remain as "Search for users and groups" since that's what you are doing, and we do want to communicate that you can include both types. And we do have the
Addbutton right there to indicate we are adding to the table of current approvers.If an MR has overwritten Approvers, and the setting is disabled. It still has custom approvers. Is this the correct behaviour?
What do you mean by this? Do you mean what happens to existing merge requests when you change the project settings? In that case, we should do whatever is easiest. There's no strong requirement there. I would assume the easiest thing is that when you toggle off overriding capability in the project settings, then essentially all the existing merge request approvers/numbers required just "freeze". They don't retro-actively change back to the latest project settings. In the design and your implementation, "freezing" is implied anyways with the fields being disabled. So I think that works well. Let me know if that doesn't make sense or we can make any other simplifying assumptions.
Edited by Victor Wumentioned in merge request !2016 (merged)
@psimyn @felipe_artur : If this isn't going into 9.3, can you please update to 9.4. Thanks!
changed milestone to %9.4
cc @smcgivern
added ~1918971 frontend labels
closed via merge request !2016 (merged)
mentioned in commit 37c3bf89
