Organizations need visibility into the compliance status of their groups and projects. One element of compliance is the concept of "separation of duties". There isn't a single view for an admin or group owner to view the MR approval settings for their projects.
The merge request approval settings are one of the most asked-about features of GitLab. In almost every customer conversation, concerns are raised about the need to set and enforce these settings across an instance or group.
Providing an indication that some baseline setting profile is enabled would be helpful for visibility.
Proposal
Add a visual indicator for a project's MR approval settings in the Compliance Dashboard to communicate whether or not the project is adhering to separation of duties.
If MR authors are not allowed to approve MRs:
If MR committers are not allowed to approve MRs:
If required approvers is equal to or greater than 2:
If all of the above are true:
If one of the above is false:
If none of the above are true:
Design
The indicators for this MVC should copy that of CI pipeline icons.
@dmoraBerlin What are your initial thoughts? We can also discuss during our sync call. I'm not sure if there's a on using icons like the CI ones for non-CI things
I think there's a few different ways we can go here, all of which I think could work for an iteration on this
@jiaan@mattgonzales I would recommend using the circle outlined icons for now. In our UI library, status and pipeline status icons are generally outlined and circular.
As of this past Compliance weekly meeting, it looks like we're still waiting to finalize the design of this before proceeding with estimation and development. It was agreed upon that a spec would help reduce ambiguity as we weren't sure how this would look in relation to the rest of the dashboard.
@dennis@mattgonzales@jiaan I just updated the description with an visual. I used the icons from the pipeline as recommended. This mimics the visual behavior elsewhere on the site, to maintain visual consistency.
One issue is if we add both CI pipeline icons and and MR icons, that could cause confusion because they're essentially identical. We might need to add the actual pipeline component, I added a visual of that.
I agree that using the same/similar icons for different purposes would be confusing.
@jiaan I imagine we'd need a separate issue to modify the current implementation of showing the most recent pipeline status to go from a single icon to the whole pipeline component? What would be the complexity for that?
@mattgonzales@jiaan I think copying the components from pipeline/MR should be simple enough as it already exists, versus having to change it to a single icon.
@mattgonzales@jiaan@dmoraBerlin Are we sure we want to move forward with the pipeline component? I'm just thinking it takes up a lot of space if we plan to continue to add to this dashboard.
An alternative is to use table columns and have the pipeline and approval status in different columns?
Ultimate customer here: you have "Approved by:" on the right side of the Compliance Dashboard. Please consider adding "Created by:" on the left side - this would make it very clear who the different parties are involved in MR.
I agree with @dennis. I think that we should be cautious of initially displaying too much on the page, doing so could dilute the critical / important information. So I'm wondering if the pipeline component would be better suited in a details dropdown section?
Regarding the icons being next to each other as shown on the third design. I think this works fine provided there's a tooltip and some contrast between icons to provide context. For example the MR dashboard page:
@jiaan@dennis@mattgonzales This was the initial proposal I believe, simply using the existing visual we have in MRs for example, where Jiaan had left off.
My concern here has always been that this feels unintuitive or uninformative. Would adding icons be helpful, similar to comments or approval?
@dennis I did have an iteration of your comment (re: columns) but I didn't like the look of the page. It felt like an unnecessary amount of space being taken.
@dennis Great feedback! Thanks so much for raising that
@dmoraBerlin I can appreciate where you're coming from. I'm usually not a fan of a lot of white space, but knowing that we intend to fill that space in the short-term makes it a temporary pain. Personally, I like how that looks, all things considered
Also, I mentioned to @dmoraBerlin, but I solicited some customer feedback on a call today and their reaction to the current MVC was: "wow, that's amazing. I want more of that." and they proceeded to provide several pieces of feedback for data points to add
I also love the idea raised by @robf_at_conversica.com as it makes perfect sense to highlight immediate issues with segregation of duties on MRs.
I'll add these suggestions, and others collected on the customer call, to the primary epic to ensure we capture it all.
I'm supportive of the two-column approach versus the entire pipeline component to maintain our succinctness theme. Would you be ok with that @dmoraBerlin or do you have a strong preference for something alternative?
@mattgonzales@dennis I'm totally ok with adding this idea of columns. I just was not sure on if that was the intended direction. This would allow easier visibility of pipeline vs. MR. I think my initial idea of rejecting this solution was due to it not conforming to other instances of this type of feature.
I think if you both prefer the column concept I would also approve, as we would be able to add other columns in the future based around compliance needs.
@dmoraBerlin Are there special considerations here if the design doesn't conform to other instances? I think the key value of the column approach is that it clearly delineates between the MR and pipeline as you pointed out. Is there another, conforming way to achieve this?
I'd love to proceed with the column approach if there aren't any factors that prevent us from doing that and if there aren't any other existing patterns for achieving the same goal.
@mattgonzales no I think in this concept of a dashboard, columns could offer a benefit for future work. I think this would be a fine MVC and we can start to get feedback on.
My only other concern was dev work associated as it's a new "component", versus reusing the visual component from MRs as in the initial mockup.
I think my initial idea of rejecting this solution was due to it not conforming to other instances of this type of feature.
@dmoraBerlin are you specifically referring the CI pipeline status as "this type of feature"? Or the column-based dashboard?
I'm curious to know how this doesn't conform.
That said, I agree with @mattgonzales, I like this approach and it leaves us more room to add more items as this dashboard matures. However, if we need to make adjustments to make sure we're being consistent as possible, I'd love to help figure that out.
@dennis I was refering to the visual we have in the MRs page. This is the current iteration of this type of "logic" in regards to displaying pipeline/MR status.
That's why I initially rejected it. However, our use case feels different as we need the data in tables to be actionable, so filtering on this kind of content could be important down the road, eg: Show me all MRs that failed pipeline external API checks.
@robf_at_conversica.com thanks for the feedback . I can see the value of this visibility. This would also conform to the MRs and Issues tables current implementation.
@mattgonzales this is currently assigned to milestone %13.0, but is still in workflowdesign, so I think we should remove the milestone assignment for now if that's okay.
I think we have agreed on the design, so this can likely move into workflowplanning breakdown. I'm opting to skip workflowsolution validation since I've been engaging with customers, as able, about the current MVC and what they'd like to see in future updates. This issue is one of those
I'll update the main description to reflect the latest proposal and, barring any last-minute objections, I think we should be good to discuss effort if you agree
@mattgonzales sounds fine to me. It doesn't look like this is in our 13.1 planning - is it okay if we defer discussing effort until this is ~"on deck"?
@djensen Since this was originally assigned to %13.0, do you think we can still have the discussion on effort to maybe still meet that milestone?
I'm ok bumping it to %13.1+ if we find it's not feasible for the original plan of %13.0
Edit to clarify: I believe @dennis had said this could be a fairly straightforward change that just needed visual guidance from @dmoraBerlin. Now that we have that, I think we could tackle the breakdown/estimation and still meet %13.0 but WDYT?
@mattgonzales This may be impacted by the MR in the works for converting the compliance dashboard to Vue (!28361 (merged)). Of course, we could implement this and put the refactor on the backburner. I'm just worried about making more work for ourselves by trying to make this work in HAML and then porting over to Vue
@jiaan WDYT about branching off that MR to work on this or would it be better to hold off for now?
If we decide to do this in HAML we have 4 days to build, review and merge. Which seems a little tight to me
@rob.hunt What's your confidence in shipping !28361 (merged) in %13.0? I don't mind waiting if it's simply moving it +1 milestone. If we're talking a more lengthy timeline, I might advocate for the additional effort
We already have in the Project model methods to check to see if the Prevent approval of merge requests by merge request author and Prevent approval of merge requests by merge request committers settings have been set or not. We also have the approvals required within the MergeRequest model.
So we should just be able to surface these in the MergeRequestComplianceEntity that @jiaan has created in !28361 (merged) () easily.
In the interest of , we may want to split this work into two MVC chunks. The first instance would be to update the page to follow the table structure in the designs we have and the second instance would be adding the separation of duties column (with the backend tweaks).
Weight
If everyone agrees with the above, then I would give this a weight of 3. It isn't difficult but the changes will touch more than a few files and need to probably be spread of two MR's/issues.
I think either @jiaan or myself could implement both the backend and frontend changes with domain experts checking over our work .
This sounds good to me. @dmoraBerlin is on PTO until next week, so that may delay the updated mocks, but #208942 (comment 332730229) is the final design so I'll update the main description with at least that image
We already have in the Project model methods to check to see if the Prevent approval of merge requests by merge request author and Prevent approval of merge requests by merge request committers settings have been set or not.
This sounds good to me. How complex would it be to also check to see if "at least 2 approvers" were required? I included it in the proposal, but since you didn't mention it in this review, I assume it's much more difficult or not available in the way we need it
Maybe it's a separate issue/iteration if that's the case? WDYT?
Thanks @mattgonzales, I can begin work with the current design and we can see about any final changes when @dmoraBerlin returns
How complex would it be to also check to see if "at least 2 approvers" were required?
sorry, I completely missed the at least 2 approvers! We can definitely do that too. I've updated my initial proposal to include it.
If we're happy to proceed, then as with other issues I'll create the two new issues I mentioned in #208942 (comment 341152371) and update this issue with a technical implementation plan.
@dmoraBerlin on top of confirming the final design as above , I wanted to ask about the column titles:
The third column title Merge request doesn't explain to me what that column is trying to convey, it feels more like that title should be on the first column (if we wanted it at all). Not sure how to convey Meets separation of duties expectations though .
Not sure if @mattgonzales or yourself had any thoughts on the wording?
@rob.hunt@dennis@mattgonzales I've updated the image in the description. How about we make the primary column heading, Name or Title ? And the MR column Merge Status ?
@dmoraBerlin I think the challenge with "merge status" is it suggests the merge was successful or not when we're actually looking at whether or not it followed some baseline segregation of duties rules. A few ideas:
MR Approvals
Approval Rules
Approval Status
MR Audit Score
Merge Audit
MR Audit
I like the idea of "status", "score" and "audit" since all three of those words could sufficiently convey what we want. It could be the "Compliance Status of the MR" (somewhat misleading but a good mvc), "An MR Compliance Score" or "An MR Audit".
@mattgonzales@dmoraBerlin The design shows created by: AUTHOR_NAME next to each issuable reference, I just want to make sure whether this is something we want to develop as part of this issue or is it slated for another issue?