From 9e0f2b56949957a3af8afd48992180e4859c764d Mon Sep 17 00:00:00 2001
From: "James E. Blair" <jim@acmegating.com>
Date: Thu, 1 Jun 2023 15:41:19 -0700
Subject: [PATCH] Fix setting autoholds through API with change supplied

The set autohold api endpoint incorrectly handled supplied values
such that if the user supplied a change without a ref it would always
use the default ref (.*).  This corrects the case handling and
adds tests.

Change-Id: I1ae14c327fd8fd2b866013d4d5078a9fbd85f843
---
 tests/unit/test_web.py | 72 ++++++++++++++++++++++++++++++++++++------
 zuul/web/__init__.py   |  2 +-
 2 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py
index aa70176b3c..58f08cd4fb 100644
--- a/tests/unit/test_web.py
+++ b/tests/unit/test_web.py
@@ -2220,14 +2220,7 @@ class TestTenantScopedWebApi(BaseTestWeb):
             headers={'Authorization': 'Bearer %s' % token})
         self.assertEqual(403, resp.status_code)
 
-    def test_autohold(self):
-        """Test that autohold can be set through the admin web interface"""
-        args = {"reason": "some reason",
-                "count": 1,
-                'job': 'project-test2',
-                'change': None,
-                'ref': None,
-                'node_hold_expiration': None}
+    def _test_autohold(self, args, code=200):
         authz = {'iss': 'zuul_operator',
                  'aud': 'zuul.example.com',
                  'sub': 'testuser',
@@ -2241,7 +2234,9 @@ class TestTenantScopedWebApi(BaseTestWeb):
             'api/tenant/tenant-one/project/org/project/autohold',
             headers={'Authorization': 'Bearer %s' % token},
             json=args)
-        self.assertEqual(200, req.status_code, req.text)
+        self.assertEqual(code, req.status_code, req.text)
+        if code != 200:
+            return
         data = req.json()
         self.assertEqual(True, data)
 
@@ -2253,6 +2248,18 @@ class TestTenantScopedWebApi(BaseTestWeb):
         self.assertNotEqual([], autohold_requests)
         self.assertEqual(1, len(autohold_requests))
         request = autohold_requests[0]
+        return request
+
+    def test_autohold_default_ref(self):
+        """Test that autohold can be set through the admin web interface
+        with default ref values"""
+        args = {"reason": "some reason",
+                "count": 1,
+                'job': 'project-test2',
+                'change': None,
+                'ref': None,
+                'node_hold_expiration': None}
+        request = self._test_autohold(args)
         self.assertEqual('tenant-one', request['tenant'])
         # The matcher expects a canonical project name
         self.assertEqual('review.example.com/org/project', request['project'])
@@ -2261,6 +2268,53 @@ class TestTenantScopedWebApi(BaseTestWeb):
         self.assertEqual("some reason", request['reason'])
         self.assertEqual(1, request['max_count'])
 
+    def test_autohold_change(self):
+        """Test that autohold can be set through the admin web interface
+        with a change supplied"""
+        args = {"reason": "some reason",
+                "count": 1,
+                'job': 'project-test2',
+                'change': '1',
+                'ref': None,
+                'node_hold_expiration': None}
+        request = self._test_autohold(args)
+        self.assertEqual('tenant-one', request['tenant'])
+        # The matcher expects a canonical project name
+        self.assertEqual('review.example.com/org/project', request['project'])
+        self.assertEqual('project-test2', request['job'])
+        self.assertEqual("refs/changes/01/1/.*", request['ref_filter'])
+        self.assertEqual("some reason", request['reason'])
+        self.assertEqual(1, request['max_count'])
+
+    def test_autohold_ref(self):
+        """Test that autohold can be set through the admin web interface
+        with a change supplied"""
+        args = {"reason": "some reason",
+                "count": 1,
+                'job': 'project-test2',
+                'change': None,
+                'ref': 'refs/tags/foo',
+                'node_hold_expiration': None}
+        request = self._test_autohold(args)
+        self.assertEqual('tenant-one', request['tenant'])
+        # The matcher expects a canonical project name
+        self.assertEqual('review.example.com/org/project', request['project'])
+        self.assertEqual('project-test2', request['job'])
+        self.assertEqual("refs/tags/foo", request['ref_filter'])
+        self.assertEqual("some reason", request['reason'])
+        self.assertEqual(1, request['max_count'])
+
+    def test_autohold_change_and_ref(self):
+        """Test that autohold can be set through the admin web interface
+        with a change supplied"""
+        args = {"reason": "some reason",
+                "count": 1,
+                'job': 'project-test2',
+                'change': '1',
+                'ref': 'refs/tags/foo',
+                'node_hold_expiration': None}
+        self._test_autohold(args, code=400)
+
     def _init_autohold_delete(self, authz):
         token = jwt.encode(authz, key='NoDanaOnlyZuul',
                            algorithm='HS256')
diff --git a/zuul/web/__init__.py b/zuul/web/__init__.py
index 43862aeedc..ab502a2ef1 100755
--- a/zuul/web/__init__.py
+++ b/zuul/web/__init__.py
@@ -684,7 +684,7 @@ class ZuulWebAPI(object):
 
         if jbody['change']:
             ref_filter = project.source.getRefForChange(jbody['change'])
-        if jbody['ref']:
+        elif jbody['ref']:
             ref_filter = str(jbody['ref'])
         else:
             ref_filter = ".*"