Required approval rule “Eligible users” can result in misleading required approvals count

Summary

When adding CODEOWNERES to a repo and requiring that approval must be obtained from at least one (1) CODEOWNER, the approval workflow does not work correctly.

Steps to reproduce

  1. Running v12.2.5-ee create a repo and invite the CODEOWNERS group to it under Settings / Members / Invite Group
  2. Create a CODEOWNERS file in the repo that contains * @group-referenced-here as a default global code owner
  3. Enable settings requiring code owner approval for merge under Settings / General / Merge Request Approval
  4. Change the number of approvals required to 1
  5. tick the box 'require approval from code owners' and save changes.
  6. Push a branch with committed code and start an MR
  7. Observe as it says two (2) members are required (when you only indicated one (1)
  8. Approve the request and see the UI show approval being satisfied but the merge button is greyed out.

What is the current bug behavior?

Unable to use CODEOWNERS feature

What is the expected correct behavior?

That one (1) of the members identified in the group for default code ownership is ALL that is required to approve the MR

Relevant logs and/or screenshots

image Screen_Shot_2019-09-18_at_2.04.24_PM

Proposal

  • In the following scenario:
    • The “Eligible users” rule has a required number of approvals…
    • AND there are other required approval rules OR Code Owner rules…
  • …we should discount the number of required approvals from the “Eligible users” rule.

Here are some examples:

  1. Eligible users rule requires 5 approvals, UX rule requires 1 approval, BE rule requires 1 approval: the summary should read Requires 5 approvals from eligible users, UX, and BE.
    • Today this scenario incorrectly reads Requires 7 more approvals from UX and BE.
  2. Eligible users rule requires 1 approval, UX rule requires 1 approval, BE rule requires 1 approval: the summary should read Requires 2 approvals from UX and BE.
    • Today this scenario incorrectly reads Requires 3 more approvals from UX and BE.
  3. Eligible users rule requires 0 approvals, UX rule requires 1 approval, BE rule requires 1 approval: the summary should read Requires 2 approvals from UX and BE.
    • Today this scenario already reads Requires 2 more approvals from UX and BE.
  4. Eligible users rule requires 1 approval: Requires 1 approval from eligible users.
    • Today this scenario reads Requires approval.

Actionable insight

This was also noted as an actionable insight from the UX Department: MRs experience async critique research (insight).

Edited Sep 29, 2021 by Pedro Moreira da Silva
Assignee Loading
Time tracking Loading