Issue 5777 - Update crowdin interface

localeTools.py

Index: localeTools.py
diff --git a/localeTools.py b/localeTools.py
index 140f22c1f7bb8298ce12bf7c1d4547a47de346a4..d9f4c40132fce86a5df98b0a7d85bb39005b54f7 100644
--- a/localeTools.py
+++ b/localeTools.py
@@ -7,6 +7,7 @@ import os
 import sys
 import codecs
 import json
+import urlparse
 import urllib
 import urllib2
 from StringIO import StringIO
@@ -92,6 +93,46 @@ chromeLocales = [
     'zh-TW',
 ]
 
+CROWDIN_AP_URL = 'https://api.crowdin.com/api/project/{}/{}'
+
+
+def crowdin_url(project_name, action, key, get={}):
+    """Create a valid url for a crowdin endpoint."""
+    url = CROWDIN_AP_URL.format(project_name, action)
+    get['key'] = key
+
+    scheme, netloc, path, params, query, fragment = urlparse.urlparse(url)
+
+    query = dict(urlparse.parse_qsl(query))

[Sebastian Noack, 2017/09/27 04:04:53]

Is there any way that there will be a query part here? If not, it seems the
logic here could be as simple as:

  '{}?{}'.format(url, urllib.urlencode(get))

From there it can be further simplified by removing the placeholders from
CROWDIN_AP_URL, in order to have only a single format operation: 

  '{}/{}/{}?{}'.format(CROWDIN_AP_URL, project_name, action,
                       urllib.urlencode(get))

[tlucas, 2017/09/27 10:51:31]

Discussion with Vasily:

On 2017/09/26 13:11:36, Vasily Kuznetsov wrote:
Summary: it could be simplified, yes - but i somehow don't like the idea that
there shouldn't be an initial query (which e.g. helped a lot during development
while importing this function)

[Sebastian Noack, 2017/09/28 20:25:21]

So it seems Vasily agrees with me. Anyway, if you want this code to be more
robust against potential false assumptions, you should rather escape
"project_name" and "action", which is both simpler and more accurate than
supporting query parameters injected in these arguments:

  CROWDIN_AP_URL = 'https://api.crowdin.com/api/project'

  '{}/{}/{}?{}'.format(CROWDIN_AP_URL,
                       urllib.quote(project_name),
                       urllib.quote(action),
                       urllib.urlencode(dict(get, key=key, json=1))

As for debugging, it shouldn't make a difference where in the code you add other
parameters temporarily, as long as you only need to do it in one place.

Also note, that in the suggested code above I used `dict(get, key=key, json=1)`
instead of modifying the passed in dictionary in place. This makes sense here,
because if `get` isn't a literal expression in the calling code but an object
that is reused, you wouldn't expect this function call to modify it.

+    query.update(get)
+
+    return urlparse.urlunparse((
+        scheme, netloc, path, params, urllib.urlencode(query), fragment
+    ))
+
+
+def crowdin_request(project_name, action, key, get={}, post_data=None,

[tlucas, 2017/09/26 14:49:05]

crowdin_request can perfectly handle both get and post requests. Readability
would imho not justify a

crowdin_get = _crowdin_request
crowdin_post = _crowdin_request

+                    headers={}, raises=False):
+    """Perform a call to crowdin and raise an Exception on failure."""
+
+    request = urllib2.Request(
+        crowdin_url(project_name, action, key, get),
+        post_data,
+        headers,
+    )
+
+    result = urllib2.urlopen(request).read()
+
+    if raises and result.find('<success') < 0:

[Sebastian Noack, 2017/09/27 04:04:52]

Note that `result.find('<success') < 0` is equivalent to `'<success' in result`.

But wouldn't it be sufficient/better to check the HTTP response status instead?

[Sebastian Noack, 2017/09/27 04:04:53]

Why is a flag necessary to opt-in for error checking? Can't/Shouldn't we just
always abort if we get an erroneous response?

[tlucas, 2017/09/27 10:51:31]

After further investigation, it seems like relying on urllib2 to raise a
HTTPError is indeed sufficient - Done.

+        raise Exception(
+            'Server indicated that the operation was not successful\n' + result
+        )
+
+    if 'json' in get:
+        result = json.loads(result)
+
+    return result
+
 
 class OrderedDict(dict):
     def __init__(self):
@@ -297,23 +338,24 @@ def setupTranslations(localeConfig, projectName, key):
                     locales.add(mapLocale('BCP-47', match2.group(1)))
 
     allowed = set()
-    allowedLocales = json.load(urllib2.urlopen(
-        'https://crowdin.com/languages/languages_list?callback='
-    ))
+    allowedLocales = crowdin_request(projectName, 'supported-languages', key,
+                                     {'json': 1})

[Sebastian Noack, 2017/09/27 04:04:53]

I wonder what would happen if we always set json=1, rather than explicitly (and
redundantly) specifying it? It seems the cases where we don't we don't care
about the response, and its format, anyway.

[tlucas, 2017/09/27 10:51:31]

There would be only one case where we explicitly expect a non-json response (see
last comment in this file). Requiring the response to be json seems to only
apply to this case when an error occurs, so we're safe to always require json -
done.

+
     for locale in allowedLocales:
-        allowed.add(locale['code'])
+        allowed.add(locale['crowdin_code'])
     if not allowed.issuperset(locales):
         print "Warning, following locales aren't allowed by server: " + ', '.join(locales - allowed)
 
     locales = list(locales & allowed)
     locales.sort()
     params = urllib.urlencode([('languages[]', locale) for locale in locales])
-    result = urllib2.urlopen('http://api.crowdin.net/api/project/%s/edit-project?key=%s' % (projectName, key), params).read()
-    if result.find('<success') < 0:
-        raise Exception('Server indicated that the operation was not successful\n' + result)
+
+    crowdin_request(projectName, 'edit-project', key, post_data=params,
+                    raises=True)
 
 
-def postFiles(files, url):
+def crowdin_body_headers(files):
+    """Create a post body and according headers, which Crowdin can handle."""
     boundary = '----------ThIs_Is_tHe_bouNdaRY_$'
     body = ''
     for file, data in files:
@@ -325,16 +367,17 @@ def postFiles(files, url):
     body += '--%s--\r\n' % boundary
 
     body = body.encode('utf-8')

[Sebastian Noack, 2017/09/27 04:04:52]

This seems unrelated, but I noticed that the encoding is not announced in the
headers above. If we don't mess up the encoding here that is just because
Crowdin assumes UTF-8 regardless of the headers. It would probably be better to
still set the correct Content-Type including encoding. In particular if we want
to become more robust against future minor changes on their end.

[tlucas, 2017/09/27 10:51:30]

Done.

-    request = urllib2.Request(url, StringIO(body))
-    request.add_header('Content-Type', 'multipart/form-data; boundary=%s' % boundary)
-    request.add_header('Content-Length', len(body))
-    result = urllib2.urlopen(request).read()
-    if result.find('<success') < 0:
-        raise Exception('Server indicated that the operation was not successful\n' + result)
+    return (
+        StringIO(body),
+        {
+            'Content-Type': 'multipart/form-data; boundary=%s' % boundary,
+            'Content-Length': len(body)
+        }
+    )
 
 
 def updateTranslationMaster(localeConfig, metadata, dir, projectName, key):
-    result = json.load(urllib2.urlopen('http://api.crowdin.net/api/project/%s/info?key=%s&json=1' % (projectName, key)))
+    result = crowdin_request(projectName, 'info', key, {'json': 1})
 
     existing = set(map(lambda f: f['name'], result['files']))
     add = []
@@ -362,14 +405,18 @@ def updateTranslationMaster(localeConfig, metadata, dir, projectName, key):
                     add.append((newName, data))
 
     if len(add):
-        titles = urllib.urlencode([('titles[%s]' % name, re.sub(r'\.json', '', name)) for name, data in add])
-        postFiles(add, 'http://api.crowdin.net/api/project/%s/add-file?key=%s&type=chrome&%s' % (projectName, key, titles))
+        data = {'titles[{}]'.format(name): re.sub(r'\.json', '', name)

[Sebastian Noack, 2017/09/27 04:04:53]

`re.sub(r'\.json', '', name)` is equivalent to `name.replace('.json', '')`.

Either way, I wonder whether this logic is accurate, you probably only want to
strip trailing occurrences, right? Does it even matter whether it is .json, or
can we just strip any file extension using os.path.splitext(name)[0]?

[tlucas, 2017/09/27 10:51:31]

I honestly don't know what Wladimir's intention was for this logic (he is the
original author, see git commit 059de001 or hg changeset 208:26cd8d5e8c78). I
can only assume the following:

In the lines above this, the code checks whether the given file's extension is
.json - and converts it to .json if not. I guess we wanted the title of a file
to be the filename without .json with a note of it's original format (if it was
anything other than json)

Regarding the equivalence: yes, i merely change the comprehension to a dict
comprehension, without changing the inner logic. I wouldn't mind if you want me
to use .replace().

[Sebastian Noack, 2017/09/28 20:25:21]

Yes, if removing each occurrence of the sub-string ".json" at any position is
what is intended here, we should just use .replace() instead of a regular
expression. But this certainly has been a mistake in the first place.

If any file extension should be removed we should use:

  os.path.splitext(name)[0]

If the file extension should only be removed if it is ".json", we should use:

  re.sub(r'\.json$', name)

Since we only have json files here anyway, the result is the same, so I might
lend towards the more generic approach (i.e. os.path.splitext).

+                for name, data in add}
+        data['type'] = 'chrome'
+        data, headers = crowdin_body_headers(add)
+        crowdin_request(projectName, 'add-file', key, post_data=data,
+                        headers=headers, raises=True)
     if len(update):
-        postFiles(update, 'http://api.crowdin.net/api/project/%s/update-file?key=%s' % (projectName, key))
+        data, headers = crowdin_body_headers(update)
+        crowdin_request(projectName, 'update-file', key, post_data=data,
+                        headers=headers, raises=True)
     for file in existing:
-        result = urllib2.urlopen('http://api.crowdin.net/api/project/%s/delete-file?key=%s&file=%s' % (projectName, key, file)).read()
-        if result.find('<success') < 0:
-            raise Exception('Server indicated that the operation was not successful\n' + result)
+        crowdin_request(projectName, 'delete-file', key, {'file': file})
 
 
 def uploadTranslations(localeConfig, metadata, dir, locale, projectName, key):
@@ -392,17 +439,18 @@ def uploadTranslations(localeConfig, metadata, dir, locale, projectName, key):
             if data:
                 files.append((newName, data))
     if len(files):
-        postFiles(files, 'http://api.crowdin.net/api/project/%s/upload-translation?key=%s&language=%s' % (
-            projectName, key, mapLocale(localeConfig['name_format'], locale))
-        )
+        language = mapLocale(localeConfig['name_format'], locale)
+        data, headers = crowdin_body_headers(files)
+        crowdin_request(projectName, 'upload-translation', key,
+                        {'language': language}, post_data=data,
+                        headers=headers, raises=True)
 
 
 def getTranslations(localeConfig, projectName, key):
-    result = urllib2.urlopen('http://api.crowdin.net/api/project/%s/export?key=%s' % (projectName, key)).read()
-    if result.find('<success') < 0:
-        raise Exception('Server indicated that the operation was not successful\n' + result)
+    # let crowdin build the project
+    crowdin_request(projectName, 'export', key, raises=True)
 
-    result = urllib2.urlopen('http://api.crowdin.net/api/project/%s/download/all.zip?key=%s' % (projectName, key)).read()
+    result = crowdin_request(projectName, 'download/all.zip', key)

[tlucas, 2017/09/27 10:51:31]

This will be the only call expecting the response to be raw data.

     zip = ZipFile(StringIO(result))
     dirs = {}