Merge lp:~milo/linaro-patchmetrics/bug1013577 into lp:linaro-patchmetrics

[Milo Casagrande]

Hello,

requesting a new merge: we are still getting the error from gerrit about user not found, but this should be handled correctly. I fixed the error string we get back to be exactly as the one we receive, fixed also the query parameters to use to start a new query from the last value. I renamed one module to use a more generic name in order to store also other values than JSON keys.

[lp:~milo/linaro-patchmetrics/bug1013577]

370. By Milo Casagrande on 2012-06-27

Remove white chars from email, just in case.

371. By Milo Casagrande on 2012-06-27

Force string on the query, and remove white spaces.

372. By Milo Casagrande on 2012-06-27

Fixed param name on docstring.

[Deepti B. Kalakeri (deeptik)]

As discussed on IRC, somehow the email formation/query seems to be incorrect in the existing code even before the changes done here. I will wait for you to push the changes for the query format and will then proceed with the review.

Thanks!!!
Deepti.

[lp:~milo/linaro-patchmetrics/bug1013577]

373. By Milo Casagrande on 2012-06-27

Fixed query quoting.

[Deepti B. Kalakeri (deeptik)]

=== modified file 'apps/patchmetrics/tests/test_gerrit.py'
--- apps/patchmetrics/tests/test_gerrit.py 2012-06-22 14:33:32 +0000
+++ apps/patchmetrics/tests/test_gerrit.py 2012-06-27 10:48:42 +0000
@@ -102,10 +102,11 @@
     def test_create_request_query_url(self):
         """Tests the creation of a valid request URL."""
         params = {'format': 'JSON', 'n': 100}
- query_params = {'owner': '<email address hidden>', 'sortkey_after': 'abcd'}
+ query_params = {'owner': '<email address hidden>',
+ 'resume_sortkey': 'abcd'}
         expected_result = ROOT_REST_URL + CHANGES_QUERY_PATH + \
         QUERY_STRING_START + \
- "n=100&format=JSON&q=owner:<email address hidden>+sortkey_after:abcd"
+ "n=100&format=JSON&q=owner:<email address hidden>+resume_sortkey:abcd"

Should this be fixed to the address the <email address hidden> ?
or have one more test for it ?
         self.assertEquals(expected_result,
                           create_request_query_url(ROOT_REST_URL,
                                                    CHANGES_QUERY_PATH,

review: Needs Fixing

[lp:~milo/linaro-patchmetrics/bug1013577]

374. By Milo Casagrande on 2012-06-27

Fixed tests.

[Milo Casagrande (milo)]

Thanks Deepti,

fixed those as well, and added a new one.

[Deepti B. Kalakeri (deeptik)]

112 query_params = {'owner': '<email address hidden>', 'sortkey_after': 'abcd'}
113 expected_result = (ROOT_REST_URL + CHANGES_QUERY_PATH +
114 QUERY_STRING_START + "n=100&format=JSON&q=owner:"
115 "person%40example.org+sortkey_after:abcd")

Should not this be resume_key instead of sortkery_after ?

review: Needs Fixing

[lp:~milo/linaro-patchmetrics/bug1013577]

375. By Milo Casagrande on 2012-06-27

Fixed wrong query parameter.

[Milo Casagrande (milo)]

> Should not this be resume_key instead of sortkery_after ?

Yep, it should be "resume_sortkey", now is fixed.
Thanks again.

[Deepti B. Kalakeri (deeptik)]

looks good +1.

review: Approve

=== modified file 'apps/patchmetrics/bin/sync-gerrit-changes.py'
--- apps/patchmetrics/bin/sync-gerrit-changes.py	2012-06-25 09:12:00 +0000
+++ apps/patchmetrics/bin/sync-gerrit-changes.py	2012-06-27 12:19:18 +0000
@@ -10,7 +10,7 @@
     create_or_update_patches_from_aosp_changes,
     get_aosp_changes_from,
     )
-from patchmetrics.gerrit_json import (
+from patchmetrics.gerrit_values import (
     GERRIT_MORE_CHANGES,
     GERRIT_SORT_KEY,
 )
@@ -18,30 +18,37 @@
 
 linaro_engineers = Person.objects.filter(
     user__id__in=TeamMembership.objects.values('member').query).distinct()
+
 for person in linaro_engineers:
-    start_after = None
+    resume_key = None
+    # We are just being a little bit paranoid here.
+    user_email = person.email.strip()
     while True:
         try:
-            results = get_aosp_changes_from(person.email,
-                                            start_after=start_after)
+            results = get_aosp_changes_from(user_email, resume_key=resume_key)
         except GerritError, e:
-            if '%s not found' % person.email in str(e):
+            if ('User %s not found' % user_email) in str(e):
                 # Gerrit returns an error when you search for changes owned by
                 # an email address that's not registered, but in this case we
                 # just skip to the next email.
                 break
             raise
-        # If we get all the changes in a single batch, the GERRIT_MORE_CHANGES
-        # key will not be in the JSON output, so we set it to False.
-        more_changes = results[-1].get(GERRIT_MORE_CHANGES) or False
-        created_or_updated_patches = (
-            create_or_update_patches_from_aosp_changes(results, person))
-
-        # If there are less created/updated changes than the number of changes
-        # returned by Gerrit we know we can stop as they're sorted most
-        # recently updated first.
-        if len(results) != len(created_or_updated_patches) or \
-        (not more_changes):
+
+        if results:
+            # If we get all the changes in a single batch, the
+            # GERRIT_MORE_CHANGES key will not be in the JSON output, so we
+            # set it to False.
+            more_changes = results[-1].get(GERRIT_MORE_CHANGES) or False
+            created_or_updated_patches = (
+                create_or_update_patches_from_aosp_changes(results, person))
+
+            # If there are less created/updated changes than the number of
+            # changes returned by Gerrit we know we can stop as they're sorted
+            # most recently updated first.
+            if len(results) != len(created_or_updated_patches) or \
+            (not more_changes):
+                break
+
+            resume_key = results[-1].get(GERRIT_SORT_KEY)
+        else:
             break
-
-        start_after = results[-1].get(GERRIT_SORT_KEY)

=== modified file 'apps/patchmetrics/gerrit.py'
--- apps/patchmetrics/gerrit.py	2012-06-25 12:50:28 +0000
+++ apps/patchmetrics/gerrit.py	2012-06-27 12:19:18 +0000
@@ -1,17 +1,22 @@
 from datetime import datetime
 import email
+import urllib
 import urllib2
-from urllib import urlencode
 
 import json
 
 from patchwork.models import Patch, Project, State
 from patchmetrics.models import GerritChange
-from patchmetrics.gerrit_json import (
+from patchmetrics.gerrit_values import (
     GERRIT_CREATED_KEY,
+    GERRIT_DEFAULT_JSON,
     GERRIT_ID_KEY,
     GERRIT_NUMBER_KEY,
     GERRIT_PROJECT_KEY,
+    GERRIT_QUERY_FORMAT,
+    GERRIT_QUERY_OWNER,
+    GERRIT_QUERY_PG_SIZE,
+    GERRIT_QUERY_RESUME_SORTKEY,
     GERRIT_SANITIZABLE_VALUE,
     GERRIT_STATUS_KEY,
     GERRIT_SUBJECT_KEY,
@@ -40,12 +45,12 @@
     """The gerrit server returned an error in response to a query."""
 
 
-def get_aosp_changes_from(email, start_after=None, page_size=25,
-                          return_format='JSON_COMPACT'):
+def get_aosp_changes_from(email, resume_key=None, page_size=25,
+                          return_format=GERRIT_DEFAULT_JSON):
     """Get the list of changes authored by the given email address.
 
     :param email: The email of the user to get the changes of.
-    :param start_after: Only include changes whose 'sortkey_after' is lower
+    :param resume_key: Only include changes whose 'sortkey_after' is lower
                         than this. Use None if you want to start from the
                         beginning.
     :param page_size: Number of items to include in the list, defaults to 25.
@@ -53,8 +58,10 @@
                           'JSON_COMPACT' for JSON format without spaces, also
                           possible to specify 'JSON'.
     """
-    params = {'format': return_format, 'n': page_size}
-    query_params = {'owner': email, 'sortkey_after': start_after}
+    params = {GERRIT_QUERY_FORMAT: return_format,
+              GERRIT_QUERY_PG_SIZE: page_size}
+    query_params = {GERRIT_QUERY_OWNER: email,
+                    GERRIT_QUERY_RESUME_SORTKEY: resume_key}
     return rest_api_request(ROOT_REST_URL, CHANGES_QUERY_PATH, params,
                             query_params)
 
@@ -88,7 +95,7 @@
         path += '/'
     request_url = url + path + QUERY_STRING_START
 
-    request_url += urlencode(params)
+    request_url += urllib.urlencode(params)
     request_url += create_gerrit_rest_query(query_params)
 
     return request_url
@@ -113,7 +120,9 @@
     for key, value in query_params.iteritems():
         # We cannot have None values here, we just skip them.
         if value is not None:
-            params_list.append(":".join((key, value)))
+            # We need strings here, and we remove white spaces.
+            params_list.append(":".join((key,
+                                        urllib.quote(str(value).strip()))))
     # The resulting query string, without separators or prefixes, should be in
     # the form of:
     # key:value+key:value

=== renamed file 'apps/patchmetrics/gerrit_json.py' => 'apps/patchmetrics/gerrit_values.py'
--- apps/patchmetrics/gerrit_json.py	2012-06-22 14:33:32 +0000
+++ apps/patchmetrics/gerrit_values.py	2012-06-27 12:19:18 +0000
@@ -1,4 +1,4 @@
-# File with all the gerrit JSON valid keys.
+# File with the gerrit JSON valid keys and other gerrit values.
 # For more information about the values defined here, look:
 # https://gerrit-review.googlesource.com/Documentation/json.html
 # Also, on bug https://bugs.launchpad.net/linaro-patchmetrics/+bug/1013577,
@@ -14,7 +14,18 @@
 GERRIT_STATUS_KEY = 'status'
 GERRIT_PROJECT_KEY = 'project'
 GERRIT_SUBJECT_KEY = 'subject'
+
 # Gerrit JSON output starts with a magic prefix, making it not valid JSON.
 # For more info:
 # https://gerrit-review.googlesource.com/Documentation/rest-api.html
 GERRIT_SANITIZABLE_VALUE = ")]}'"
+
+# The possible values for the output format of the Gerrit query.
+GERRIT_DEFAULT_JSON = 'JSON_COMPACT'
+GERRIT_NORMAL_JSON = 'JSON'
+
+# Gerrit query parameters
+GERRIT_QUERY_OWNER = 'owner'
+GERRIT_QUERY_FORMAT = 'format'
+GERRIT_QUERY_PG_SIZE = 'n'
+GERRIT_QUERY_RESUME_SORTKEY = 'resume_sortkey'

=== modified file 'apps/patchmetrics/tests/test_gerrit.py'
--- apps/patchmetrics/tests/test_gerrit.py	2012-06-22 14:33:32 +0000
+++ apps/patchmetrics/tests/test_gerrit.py	2012-06-27 12:19:18 +0000
@@ -3,6 +3,13 @@
 
 from django.test import TestCase
 
+from patchmetrics.gerrit_values import (
+    GERRIT_QUERY_FORMAT,
+    GERRIT_QUERY_OWNER,
+    GERRIT_QUERY_PG_SIZE,
+    GERRIT_QUERY_RESUME_SORTKEY,
+)
+
 from patchmetrics.gerrit import (
     CHANGES_QUERY_PATH,
     QUERY_STRING_START,
@@ -103,9 +110,23 @@
         """Tests the creation of a valid request URL."""
         params = {'format': 'JSON', 'n': 100}
         query_params = {'owner': '[e-mail]', 'sortkey_after': 'abcd'}
-        expected_result = ROOT_REST_URL + CHANGES_QUERY_PATH + \
-        QUERY_STRING_START + \
-        "n=100&format=JSON&q=owner:[e-mail]+sortkey_after:abcd"
+        expected_result = (ROOT_REST_URL + CHANGES_QUERY_PATH +
+                            QUERY_STRING_START + "n=100&format=JSON&q=owner:"
+                            "person%40example.org+resume_sortkey:abcd")
+        self.assertEquals(expected_result,
+                          create_request_query_url(ROOT_REST_URL,
+                                                   CHANGES_QUERY_PATH,
+                                                   params, query_params))
+
+    def test_create_request_query_url_quoted(self):
+        """Tests the creation of a valid request URL with a complex email."""
+        params = {GERRIT_QUERY_FORMAT: 'JSON', GERRIT_QUERY_PG_SIZE: 100}
+        query_params = {GERRIT_QUERY_OWNER: '[e-mail]',
+                        GERRIT_QUERY_RESUME_SORTKEY: 'abcd'}
+        expected_result = (ROOT_REST_URL + CHANGES_QUERY_PATH +
+                            QUERY_STRING_START + "n=100&format=JSON&q=owner:"
+                            "name%2Bsurname.person%40example.org+"
+                            "resume_sortkey:abcd")
         self.assertEquals(expected_result,
                           create_request_query_url(ROOT_REST_URL,
                                                    CHANGES_QUERY_PATH,