From 05772a0709bc91f38fe02fa92c4cd6058caece75 Mon Sep 17 00:00:00 2001
From: chrysn <chrysn@fsfe.org>
Date: Tue, 4 Jun 2024 23:08:40 +0200
Subject: [PATCH] Do not count multiple reviews from the same user

---
 check_labels.py | 45 ++++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/check_labels.py b/check_labels.py
index 0a44854..b0d817c 100755
--- a/check_labels.py
+++ b/check_labels.py
@@ -175,13 +175,13 @@ def parse_condition(condition):
 
 def check_review_approvals(pull, condition, missing_approvals_label):
     condition[1] = int(condition[1])
-    approvals = 0
+    approving_reviewers = set()
     for review in pull.get_reviews():
         if review.state == "APPROVED":
-            approvals += 1
-            if approvals > condition[1]:
+            approving_reviewers.add(review.user.login)
+            if len(approving_reviewers) > condition[1]:
                 print(
-                    f"PR#{pull} has {approvals}/{condition[1] + 1} approvals,"
+                    f"PR#{pull} has {len(approving_reviewers)}/{condition[1] + 1} approving reviewers,"
                     f" removing label '{missing_approvals_label}'"
                 )
                 if missing_approvals_label:
@@ -197,7 +197,7 @@ def check_review_approvals(pull, condition, missing_approvals_label):
 
     if missing_approvals_label and condition[1] > 0:
         print(
-            f"PR#{pull} has only {approvals}/{condition[1] + 1} approvals,"
+            f"PR#{pull} has only {len(approving_reviewers)}/{condition[1] + 1} approving reviewers,"
             f" setting label '{missing_approvals_label}'"
         )
         pull.add_to_labels(missing_approvals_label)
@@ -364,8 +364,12 @@ def name(self):
 
     # pylint: disable=too-few-public-methods
     class MockReview:
-        def __init__(self, state):
+        def __init__(self, state, reviewer):
             self._state = state
+            class MockUser:
+                def __init__(self, login):
+                    self.login = login
+            self.user = MockUser(reviewer)
 
         @property
         def state(self):
@@ -405,43 +409,50 @@ def get_reviews(self):
             (
                 ["review.approvals", 2],
                 None,
-                ["APPROVED", "APPROVED", "APPROVED"],
+                [("APPROVED", "a"), ("APPROVED", "b"), ("APPROVED", "c")],
                 "",
                 True,
             ),
-            (["review.approvals", 2], None, ["APPROVED", "APPROVED"], "", False),
-            (["review.approvals", 1], None, ["APPROVED", "APPROVED"], "", True),
-            (["review.approvals", 1], None, ["COMMENT", "APPROVED"], "", False),
-            (["review.approvals", 1], None, ["COMMENT"], "", False),
-            (["review.approvals", 1], None, ["APPROVED"], "DON'T MERGE", False),
-            (["review.approvals", 1], ["FOOBAR"], ["APPROVED"], "DON'T MERGE", False),
+            (["review.approvals", 2], None, [("APPROVED", "a"), ("APPROVED", "b")], "", False),
+            (["review.approvals", 1], None, [("APPROVED", "a"), ("APPROVED", "b")], "", True),
+            (["review.approvals", 1], None, [("COMMENT", "a"), ("APPROVED", "b")], "", False),
+            (["review.approvals", 1], None, [("COMMENT", "a")], "", False),
+            (["review.approvals", 1], None, [("APPROVED", "a")], "DON'T MERGE", False),
+            (["review.approvals", 1], ["FOOBAR"], [("APPROVED", "a")], "DON'T MERGE", False),
             (
                 ["review.approvals", 1],
                 None,
-                ["APPROVED", "APPROVED"],
+                [("APPROVED", "a"), ("APPROVED", "b")],
                 "DON'T MERGE",
                 True,
             ),
             (
                 ["review.approvals", 1],
                 ["FOOBAR"],
-                ["APPROVED", "APPROVED"],
+                [("APPROVED", "a"), ("APPROVED", "b")],
                 "DON'T MERGE",
                 True,
             ),
             (
                 ["review.approvals", 1],
                 ["DON'T MERGE", "FOOBAR"],
-                ["APPROVED", "APPROVED"],
+                [("APPROVED", "a"), ("APPROVED", "b")],
                 "DON'T MERGE",
                 True,
             ),
+            (
+                ["review.approvals", 2],
+                None,
+                [("APPROVED", "a"), ("APPROVED", "a")],
+                "DON'T MERGE",
+                False,
+            ),
         ],
     )
     def test_check_review_approvals(
         value, labels, reviews, missing_approvals_label, exp
     ):
-        pull = MockPull(labels=labels, reviews=[MockReview(state) for state in reviews])
+        pull = MockPull(labels=labels, reviews=[MockReview(state, reviewer) for (state, reviewer) in reviews])
         assert check_review_approvals(pull, value, missing_approvals_label) == exp
         if not exp and missing_approvals_label:
             assert missing_approvals_label in pull.labels