Bug in action-required-review

[tureck1y]

Impacted plugin

Jetpack

Quick summary

action-required-review will never satisfy requirements if @someuser is an author of a PR and requirements are configured like this:

  teams:
    - all-of:
      - some-team
      - '@someUser'

It's impossible to make a self-review and impossible to merge the PR. An empty team should be excluded from the requirements check. An empty virtual team for @someuser in this case/

Steps to reproduce

1. Configure requirements

- name: Misc
  paths: unmatched
  teams:
    - all-of:
      - some-team
      - '@someUser'

2. Open a PR by @someuser as an author
3. Get approval from any user of "some-team"

Result: Requirement "Misc" is not satisfied by the existing reviews.

Expected result: Requirement "Misc" is satisfied by the existing reviews.

Site owner impact

Fewer than 20% of the total website/platform users

Severity

Major

What other impact(s) does this issue have?

No response

If a workaround is available, please outline it here.

No response

Platform (Simple and/or Atomic)

No response

[github-actions]

OpenAI suggested the following labels for this issue:

• [Feature Group] Content Management: The issue pertains to how content requirements are managed within the Jetpack plugin.
• [Feature] Jetpack CLI: This feature may relate to the command line interface used for managing plugin configurations and settings.
• [Feature] Performance History: The problem ties into performance metrics, as it affects the review and approval process of pull requests.

[anomiex]

I'm hesitant to consider this a bug. Ideally your organization shouldn't have a single point of failure in @someuser being personally required to approve everything.

If we do decide to do something about this, I'd prefer adding a condition (name TBD), which for everything under it effectively considers the author as having self-reviewed. That way you would do like

    - all-of:
      - some-team
      - new-condition-name-TBD:
        - '@someUser'

to allow @someUser to auto-"self-review".

Or possibly we could do is-author to more specifically check, like

    - all-of:
      - some-team
      - any-of:
        - '@someUser'
        - is-author:
          - '@someUser'

[tureck1y]

Thanks for the quick reply.

In general, it is a common situation when one user is a codeowner of some part of the repository and he can make a pull-request to this part.

And now we have a situation that it will be impossible to merge such PR.

An author is excluded from the team at the review request stage. In my opinion, the same thing should be done here.
And if the team becomes empty, there is no point in awaiting a review from an empty team, this condition will never be fulfilled.

[anomiex]

Or possibly we could do is-author to more specifically check

Decided against this once I realized that you could wind up with a config that only allowed PRs with a particular author, and then the "request review" feature would have nothing to request.