Re-add support for filtering by submitter name, user email

With v1.0 of the Patchwork API, we did not have the ability to directly
filter (server-side) by email address or name so we made an initial
query to the '/people' resource to find the matching submitter and
extract the ID. With v1.1, we added support for searching by email
address but not name. We don't want to add server-side name filtering
because it's not unique. However, we can continue to do this client-side
and fix the regression in behavior.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Closes: #24

Author: stephenfin

git_pw/bundle.py

@@ -136,10 +136,11 @@ def list_cmd(owner, limit, page, sort, name):
 
     params = []
 
-    if api.version() >= (1, 1):
-        params.extend([('owner', own) for own in owner])
-    else:
-        for own in owner:
+    for own in owner:
+        # we support server-side filtering by username (but not email) in 1.1
+        if api.version() >= (1, 1) and '@' not in own:
+            params.append(('owner', own))
+        else:
             users = api.index('users', [('q', own)])
             if len(users) == 0:
                 LOG.error('No matching owner found: %s', own)

git_pw/patch.py

@@ -215,11 +215,11 @@ def list_cmd(state, submitter, delegate, archived, limit, page, sort, name):
     for state in state:
         params.append(('state', state))
 
-    if api.version() >= (1, 1):
-        params.extend([('submitter', subm) for subm in submitter])
-        params.extend([('delegate', delg) for delg in delegate])
-    else:
-        for subm in submitter:
+    for subm in submitter:
+        # we support server-side filtering by email (but not name) in 1.1
+        if api.version() >= (1, 1) and '@' in subm:
+            params.append(('submitter', subm))
+        else:
             people = api.index('people', [('q', subm)])
             if len(people) == 0:
                 LOG.error('No matching submitter found: %s', subm)
@@ -230,7 +230,11 @@ def list_cmd(state, submitter, delegate, archived, limit, page, sort, name):
 
             params.append(('submitter', people[0]['id']))
 
-        for delg in delegate:
+    for delg in delegate:
+        # we support server-side filtering by username (but not email) in 1.1
+        if api.version() >= (1, 1) and '@' not in delg:
+            params.append(('delegate', delg))
+        else:
             users = api.index('users', [('q', delg)])
             if len(users) == 0:
                 LOG.error('No matching delegates found: %s', delg)

git_pw/series.py

@@ -123,10 +123,11 @@ def list_cmd(submitter, limit, page, sort, name):
 
     params = []
 
-    if api.version() >= (1, 1):
-        params.extend([('submitter', subm) for subm in submitter])
-    else:
-        for subm in submitter:
+    for subm in submitter:
+        # we support server-side filtering by email (but not name) in 1.1
+        if api.version() >= (1, 1) and '@' in subm:
+            params.append(('submitter', subm))
+        else:
             people = api.index('people', [('q', subm)])
             if len(people) == 0:
                 LOG.error('No matching submitter found: %s', subm)

releasenotes/notes/issue-24-60a9fa796f666f35.yaml

@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    Resolve an issue that prevented the following filtering when using API
+    v1.1:
+
+    - Filter patches or series by submitter using the submitter's name
+    - Filter patches by delegate using the delegate's email
+    - Filter bundles by owner using the owner's email

tests/test_bundle.py

@@ -277,21 +277,27 @@ def test_list_api_v1_1(self, mock_log, mock_index, mock_version):
 
         mock_version.return_value = (1, 1)
 
+        user_rsp = [self._get_users()]
         bundle_rsp = [self._get_bundle()]
-        mock_index.side_effect = [bundle_rsp]
+        mock_index.side_effect = [user_rsp, bundle_rsp]
 
         runner = CLIRunner()
-        result = runner.invoke(bundle.list_cmd, ['--owner', 'john.doe',
-                                                 '--owner', 'user.b'])
+        result = runner.invoke(bundle.list_cmd, [
+            '--owner', 'john.doe',
+            '--owner', 'user.b',
+            '--owner', 'john@example.com'])
 
         assert result.exit_code == 0, result
 
-        # We shouldn't have to make a call to '/users' since API v1.1 supports
-        # filtering with usernames natively
+        # We should have only made a single call to '/users' (for the user
+        # specified by an email address) since API v1.1 supports filtering with
+        # usernames natively
         calls = [
+            mock.call('users', [('q', 'john@example.com')]),
             mock.call('bundles', [
-                ('owner', 'john.doe'), ('owner', 'user.b'), ('q', None),
-                ('page', None), ('per_page', None), ('order', 'name')])]
+                ('owner', 'john.doe'), ('owner', 'user.b'), ('owner', 1),
+                ('q', None), ('page', None), ('per_page', None),
+                ('order', 'name')])]
         mock_index.assert_has_calls(calls)
 
         # We shouldn't see a warning about multiple versions either

tests/test_patch.py

@@ -287,6 +287,7 @@ def _get_users(**kwargs):
         rsp = {
             'id': 1,
             'username': 'john.doe',
+            'email': 'john@example.com',
         }
         rsp.update(**kwargs)
         return rsp
@@ -385,23 +386,29 @@ def test_list_api_v1_1(self, mock_log, mock_index, mock_version):
 
         mock_version.return_value = (1, 1)
 
+        people_rsp = [self._get_person()]
+        user_rsp = [self._get_users()]
         patch_rsp = [self._get_patch()]
-        mock_index.side_effect = [patch_rsp]
+        mock_index.side_effect = [people_rsp, user_rsp, patch_rsp]
 
         runner = CLIRunner()
         result = runner.invoke(patch.list_cmd, [
-            '--submitter', 'John Doe', '--submitter', 'Jimmy Foo',
-            '--delegate', 'foo', '--delegate', 'bar'])
+            '--submitter', 'jimmy@example.com', '--submitter', 'John Doe',
+            '--delegate', 'foo', '--delegate', 'john@example.com'])
 
         assert result.exit_code == 0, result
 
-        # We shouldn't have to make calls to '/users' or '/people' since API
-        # v1.1 supports filtering with (user)names natively
+        # We should have only made a single call to each of '/users' and
+        # '/people' (for the user specified by an email address and the
+        # submitter specified by name, respectively) since API v1.1 supports
+        # filtering of users with username and people with emails natively
         calls = [
+            mock.call('people', [('q', 'John Doe')]),
+            mock.call('users', [('q', 'john@example.com')]),
             mock.call('patches', [
                 ('state', 'under-review'), ('state', 'new'),
-                ('submitter', 'John Doe'), ('submitter', 'Jimmy Foo'),
-                ('delegate', 'foo'), ('delegate', 'bar'),
+                ('submitter', 'jimmy@example.com'), ('submitter', 1),
+                ('delegate', 'foo'), ('delegate', 1),
                 ('q', None), ('archived', 'false'), ('page', None),
                 ('per_page', None), ('order', '-date')])]
 

tests/test_series.py

@@ -234,22 +234,23 @@ def test_list_api_v1_1(self, mock_log, mock_index, mock_version):
 
         mock_version.return_value = (1, 1)
 
+        people_rsp = [self._get_people()]
         series_rsp = [self._get_series()]
-        mock_index.side_effect = [series_rsp]
+        mock_index.side_effect = [people_rsp, series_rsp]
 
         runner = CLIRunner()
         result = runner.invoke(series.list_cmd, [
-            '--submitter', 'john@example.com',
-            '--submitter', 'jimmy@example.com'])
+            '--submitter', 'jimmy@example.com',
+            '--submitter', 'John Doe'])
 
         assert result.exit_code == 0, result
 
-        # We shouldn't have to make a call to '/people' since API v1.1 supports
-        # filtering with names natively
+        # We should have only made a single call to '/people' since API v1.1
+        # supports filtering with emails natively
         calls = [
+            mock.call('people', [('q', 'John Doe')]),
             mock.call('series', [
-                ('submitter', 'john@example.com'),
-                ('submitter', 'jimmy@example.com'),
+                ('submitter', 'jimmy@example.com'), ('submitter', 1),
                 ('q', None), ('page', None), ('per_page', None),
                 ('order', '-date')])]
         mock_index.assert_has_calls(calls)