When enabled, the new MR approval settings prevent users editing approval rules on the project-level when the project has an associated compliance framework label. The current UI text does not adequately communicate the intended behavior.
Steps to reproduce
Go to [GDK_URL/GITLAB_URL]/admin/push_rule#merge-request-approval-settings
Select at least one compliance framework label and optionally select checkboxes
Go to a project of your choice and go to the general settings
Select one of the compliance framework labels which matches one you ticked in the instance settings
Open the merge request approval settings and see you do not get an Add approval rule button or any way to edit existing rules
What is the current experience?
Project-level approval rules are blocked from being edited even if Prevent users from modifying merge request approvers list is unticked.
Proposal
Clarify the UI text to communicate the behavior from enabling these instance-level settings.
Further detail
There are other proposals that are WIP that will affect this non-intuitive behavior. We should consider:
Implementing #1111 (closed) to add group-level MR approval rules
Validate whether we need to bring MR approvals to the instance-level or not
Consider removing the instance-level controls depending on the validation outcome of 2
The problem occurs because the project_helper is checking can_modify_approvers for the editing ability: ee/app/helpers/ee/projects_helper.rb:99. This checks the :modify_approvers_rules policy for the current user against the project.
This policy is blocked by cannot_modify_approvers_rules (ee/app/policies/ee/project_policy.rb:358), which in turn is controlled by has_regulated_settings? (ee/app/models/ee/project.rb:221). This checks to see if the compliance framework label set on the instance level matches the projects compliance framework. If it does, then it locks down the approval rules.
To fix this we need to change the policy so this locking only occurs for MR-level approval rules by tweaking the policy or creating a new check for the frontend to use.
@tancnle you are correct that those checkboxes should be blocked. The thing that is failing is the ability to add/edit the approval rules themselves.
According to the original epic, the last checkbox Prevent users from modifying merge request approvers list should be controlling whether the MR-level approvals list is customisable.
Instead, irrespective of what you tick you cannot edit the approvals list at all, not just on the MR-level but on the project-level too.
This means that if (see the video in the description) you turn on SOX on the instance-level with nothing ticked, then create a new project with the SOX label, you cannot add project-level MR approval rules to your brand new project. You have to disable SOX, add them and then re-enable SOX. This seems really counter-intuitive .
IMHO we should be stopping MR-level approval rules ifPrevent users from modifying merge request approvers list is ticked, not stopping any and all modifications of the approvers list full stop . And we should never be blocking the project-level approver's list, as right now, there is no other way to be able to edit this feature, unlike the checkboxes which can be edited on the instance-level.
@rob.hunt My apology for a hasty response I agree with you , the approval list should take the instance-level settings into account as there is no ability to do that at the instance level.
You have raised a really good point about the Prevent users from modifying merge request approvers list setting at instance-level. From the proposal
Enable approvers list modification
Allow owners and maintainers to modify the approver list for merge requests and override approvals at the individual merge request level.
From what I understand, this instance-level setting is used to control both the the override option and the modification of the approver list at project level, and it is where lies my confusion 🤯. Perhaps we can update the copy to make it a bit clearer. I have a WIP MR which hopefully rectify this behaviour and make it clearer in the code.
@rob.hunt@aregnery Thanks so much for your due diligence that uncovered this problem
For clarity: I believe it was my guidance that @tancnle was following here and the behavior is expected though not ideal and I think was an oversight on my part as the solution evolved due to implementation challenges Tan discovered. It's definitely not a ~bug based on how I pushed this forward
There's certainly a challenge here, but the short of it is: the organizations asking for this feature did not want anyone making changes to MR approval rules either at the project or MR level. However, as we pivoted to accommodate the complex logic involved with this change, I think we lost sight of that and it became a blanket, disruptive change.
I think this also highlights the general difficulty of building instance-level (highest level) controls for project-level (lowest level) settings or behaviors. I think we will need to focus more on group-level controls and then determine the appropriate steps to bring those to the instance-level.
Before we make the change that's been proposed here, I'd like to get feedback from the customer who helped shape this direction (cc @spouliquen1 could we get feedback from this customer? - internal link)
@rob.hunt as a follow-up: If I'm an admin and I enable the instance-level settings, can I then edit the project-level approval rules or is that still locked down? Sorry if this has been answered already
If even admins cannot make this change at the project-level (the original intent, but which caused the complexity that resulted in a different approach) then my opinion is a bit different. Still not a ~bug in my opinion since the design was intentional, but requires a change nonetheless
For clarity: I believe it was my guidance that @tancnle was following here and the behavior is expected though not ideal and I think was an oversight on my part as the solution evolved due to implementation challenges Tan discovered. It's definitely not a ~bug based on how I pushed this forward
Thanks for raising this . The existing documentation wasn't all that clear about how this should be working and it feels really counterintuitive to me . Of course, if the functionality works as intended then I will do a homer and accept my confusion
the organizations asking for this feature did not want anyone making changes to MR approval rules either at the project or MR level
I'm not sure I follow here. I could possibly understand if it's an existing project but for new projects, surely you need the power to set it up? Otherwise I can just see users removing the compliance framework label, doing their thing and then putting it back on .
I think this also highlights the general difficulty of building instance-level (highest level) controls for project-level (lowest level) settings or behaviors. I think we will need to focus more on group-level controls and then determine the appropriate steps to bring those to the instance-level.
I wholeheartly agree . It would be great to see each group define their own approval rules and then let that be automatically applied for all projects which need to be compliant
If I'm an admin and I enable the instance-level settings, can I then edit the project-level approval rules or is that still locked down? Sorry if this has been answered already
No, they can't at the minute. I was testing this as an admin, group owner and project owner and even with that level of power I couldn't edit any of the approval rules
Of course, if the functionality works as intended then I will do a homer and accept my confusion
@rob.hunt I just said it was my guidance that got us here, not that it was right or clear For the record, I approve of improving this experience and just spoke with @aregnery during our design sync call today to make sure we work on this.
I appreciate you raising the confusion because I think it's valid and needs to be addressed I just didn't want @tancnle to shoulder the burden of my guidance when we implemented this
I could possibly understand if it's an existing project but for new projects, surely you need the power to set it up?
I think this is exactly (one of?) the oversight that's causing confusion. We hadn't discussed this scenario and I agree it's necessary for new projects. This solution is certainly not perfect - we knew that going in - but this is definitely an issue. I'd love to figure out how we can provide the right level of flexibility and control
It would be great to see each group define their own approval rules and then let that be automatically applied for all projects which need to be compliant
Agreed. The one caveat here is some organizations will desire an instance-level control to "rule them all". That's a future state though and it's likely most appropriate for us to focus on group-level controls for now... after we've fixed this of course
No, they can't at the minute. I was testing this as an admin, group owner and project owner and even with that level of power I couldn't edit any of the approval rules
This is the biggest problem, though I think this is also something I agreed to when @tancnle was originally working on and we just forgot about it. We should definitely at least ensure admins can make changes but if there's a way to improve the implementation overall then I'm supportive of that as a ~P1
FYI, one of our customers reported this behavior primarily because they want to be able to edit "no. of required approvers" and this, of course, blocks their ability to modify without first removing the compliance label on the project.
I've put this into Next Up and workflowplanning breakdown but please let me know if this should be in workflowdesign. Since this is improving an existing experience, I thought we could skip design; however, I know @aregnery is exploring this at a higher level and want to ensure we place this correctly
Just noticed a similar existing issue @mattgonzales: #205910 (closed). Just wanted to check there wasn't an existing plan or grand-scheme we're not aware of here which may affect which label it goes into?
To fix this we need to change the policy so this locking only occurs for MR-level approval rules by tweaking the policy or creating a new check for the frontend to use.
The checkbox in question is
Prevent users from modifying merge request approvers list
@rob.hunt I think we can safely close #205910 (closed) (which I've now done) because that was created long enough ago to be before our recent learnings around the utility of compliance framework project labels. Especially if we make them customizable
Feel free to let me know if I'm being crazy, but it seems like a less mature duplicate at this point
@aregnery I think that proposal is ok for the short-term, but we will need to be mindful that a maintainer couldn't remove or modify deliberate project-level MR approval rules, which was the original intent.
Maybe if we prevent deleting project-level MR approval rules and prevent setting the no. of required approvers to 0? Just food for thought. I'm supportive of iterating on this over a few iterations, but I'd like to ensure we have a clear idea of what our final outcome will be that still supports the original intent of providing control for admins to prevent maintainers from making undesirable changes to MR approval rules.
The customer who recently raised this concern could be a great thought partner on this to do some additional validation. (cc @oheigre)
Also @spouliquen1 FYI since one of your customers was wanting this feature for their SOX requirements and we will be making some changes to improve the experience which might mean a temporary reversion in terms of coverage for their audit requirements.
Ah gotcha @mattgonzales. The description describes a "bug" that was actually the intended behavior from the onset. I am totally fine with keeping the current functionality and refining the UI Text to make it clearer what is happening.
Just for clarity: I agree the overall UX is poor and even if we clarify what's happening in the text I think we should still figure out if there's a better, more intuitive and flexible way to solve this problem
I am equally confused, so I appreciate you asking @mattgonzales. The description of the issue is not lining up with the expectations I see you communicating in the the thread.
What I see in the thread is the behavior that is live today isn't actually a bug. So my proposal was to simply clarify the UI text to make it clear to users. This way we can keep the original intent: a maintainer can't remove or modify deliberate project-level MR approval rules.
The issue description still outlines a logic shift away from this.
What is the expected correct behavior?
Project-level approval rules are editable but MR-level approval rules are not if Prevent users from modifying merge request approvers list is ticked.
Which direction would you prefer we go? Change the logic or adjust the UI text to reflect the current logic?
Ahhh, great point @aregnery! Thanks for the sanity check
I'll update the description to reflect your proposal of updating UI text and we can tackle the general UX issues in another issue. Does that sound good?
I think the challenge here is customers want to be able to define a specific set of approval rules, which I think #1111 (closed) addresses (at the group level), and prevent maintainers from modifying that specific set of rules.
I propose the following:
Update this issue description to clarify the UI text of the current implementation
Implement #1111 (closed) to add group-level MR approval rules
Consider a proposal to bring this control to the instance-level (need to validate if this is still necessary after adding the group level experience)
Consider removing the instance-level controls depending on the validation outcome of 3
My assumption is the logic we build into 2 would allow a group owner to define a baseline set of rules and maintainers could then add additional rules on top of that, but not touch the baseline ruleset.
@mattgonzales@aregnery The current logic when the compliance label is selected and the project has the label assigned.
Please note that the below project-level settings inherit values from instance-level ones and are disabled.
Instance setting name
Instance value
Project value
Project setting name
Prevent approval of merge requests by merge request author
Prevent approval of merge requests by merge request author
Prevent approval of merge requests by merge request committers
Prevent approval of merge requests by merge request committers
Prevent users from modifying merge request approvers list
Can override approvers and approvals required per merge request
Disabled*
Approval Rules
The last setting is the problematic one. I find it is a bit confusing to align the instance-level settings with project-level ones, especially when the language does not match or flip (e.g. prevent vs can). Ideally it is a one-to-one mapping. It could be a totally misinterpretation from my end.
I think it is relatively easier to re-align the settings between these two levels and leave approval rules out from instance control.
Instance setting name
Instance value
Project value
Project setting name
Prevent approval of merge requests by merge request author
Prevent approval of merge requests by merge request author
Prevent approval of merge requests by merge request committers
Prevent approval of merge requests by merge request committers
Can override approvers and approvals required per merge request
Can override approvers and approvals required per merge request
@tancnle Thanks so much for the detail I don't think your confusion is a misunderstanding on your end. I didn't propose a good solution here and we've learned a lot since the initial MVC
I think your comment is aligned with @aregnery, which is to improve the UI text to make the expected behavior more clear to users
@mattgonzales My apology since I've missed some important points from @aregnery's notes. In addition to the UI text changes, I propose to adjust the rule engine as well.
From what I understand, if we want this instance-level control Prevent users from modifying merge request approvers list, to not allow changing MR approval rules either at the project or MR level. This translates to the follow project-settings.
When instance-level value is enabled,
Project-level: Can override approvers and approvals required per merge request is read-only and disabled. This effectively means users can't edit the approval rules at MR-level.
Project-level: approval rules are read-only. effectively means users can't edit the approval rules at Project-level.
When instance-level value is disabled,
Project-level: Can override approvers and approvals required per merge request is read-only and enabled. This effectively means users can edit the approval rules at MR-level.
Project-level: approval rules are editable.
Does the above sound right to you? The MR !40229 (merged) is effectively allow us to achieve this.
@tancnle I think that's a good solution for now, yes
Although this removes the control organizations wanted over project-level MR approval rules, I believe that will be addressed by #1111 (closed) and we can repurpose this instance-level experience based on new learnings there
@tancnle I see you have an open MR scheduled for %13.5 to fix the permissions of maintainers being unable to modify MR approvals when the instance-level rules are set.
I've opened this epic after talking with @aregnery and @rob.hunt about bringing feature parity to instance<>project MR approval rules. Among the requirements is fixing this issue.
Are you actively working on that MR and expect it to ship this milestone? It also looks like that MR is solving for the project permissions issue only and not the UI text proposal in this issue.
I'm just trying to sync up so I can add relevant issue and/or MR links in &4552 (closed) to track this work
@mattgonzales Yep, I can try to get it in for %13.5. After reading the epic, it seems like the most sensible approach for now is to divorce the instance-level settings on the MR approval rules until we bring MR rules to instance-level. The change will effectively will bring in the behaviour as mentioned in #239349 (comment 408828337).
@tancnle When you say "divorce the instance-level settings on the MR approval rules until we bring MR rules to instance-level" I understand that to mean "remove the relationship between instance and project-level MR settings until we build the more holistic experience."
Is that correct?
I ask because we shouldn't break the current experience if we don't have to and should just make the necessary iterations towards the holistic solution
@mattgonzales My apology for the confusion. What I meant is just the removal of the relationship between the instance-level settings on merge request approvers list Prevent users from modifying merge request approvers list, not all three.
So at the moment, the instance-level setting Prevent users from modifying merge request approvers list is used to control the following 2 project-level settings.
Can override approvers and approvals required per merge request
Permission to modify approval rules
My suggestion are:
Option 1:
Rename instance-level Prevent users from modifying merge request approvers list to Can override approvers and approvals required per merge request to keep it in-par with the name at project-level (or rename both with the new copy Allow users to modify approval rules when creating a merge request as suggested in this epic)
Remove the relationship between the instance-level setting Prevent users from modifying merge request approvers list and permission of modify approval rules at project-level
I agree, Option 1 sounds like the right, iterative move here
As an FYI, we've just re-re-focused on the group-level implementation rather than instance first. If this doesn't change anything, that's totally fine. I just wanted to make sure you had all the most recent context
If I've misunderstood anything, please let me know. I know we discussed (and agreed on) the path forward here, but in light of customer feedback we may need to re-think our approach. I apologize for not catching this sooner!
TL;DR: I think our wires got crossed, or I lost my mind, in #207250 (closed).
What I think was intended: My comment, "I wonder if, now that we have those labels, it makes sense to just nix that capability (allow admins to modify the local project settings after being set) altogether. At least for now?", likely meant we should remove an admins ability to edit the project-level settings after they'd been set at the instance-level
What we did: Removed the relationship to where setting instance-level MR settings would have no affect on project-level permissions, allowing anyone to edit the rules themselves.
What customers have said (who recently upgraded to 13.3.4): They need the project-level rules to be locked down and not editable by maintainers.
We may need to leave things as-is until we're able to implement the group-level and then instance-level MR approval experiences
@mattgonzales No worries. I have created the revert MR. Please note that this revert does not help with the customer's use case as they do not apply compliance framework scope.