Issue 5777 - Update crowdin interface

localeTools.py

 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import re
 import os
 import sys
 import codecs
 import json
 import urlparse
 import urllib
 import urllib2
+import mimetypes
 from StringIO import StringIO
 from ConfigParser import SafeConfigParser
 from zipfile import ZipFile
 from xml.parsers.expat import ParserCreate, XML_PARAM_ENTITY_PARSING_ALWAYS
 
 langMappingGecko = {
     'bn-BD': 'bn',
     'br': 'br-FR',
     'dsb': 'dsb-DE',
     'fj-FJ': 'fj',
@@ skipping 63 matching lines @@
     'ta',
     'te',
     'th',
     'tr',
     'uk',
     'vi',
     'zh-CN',
     '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
-    ))
+CROWDIN_AP_URL = 'https://api.crowdin.com/api/project'
 
 
 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):
+                    headers={}, raw=False):
     """Perform a call to crowdin and raise an Exception on failure."""
-
     request = urllib2.Request(
-        crowdin_url(project_name, action, key, get),
+        '{}/{}/{}?{}'.format(CROWDIN_AP_URL,
+                             urllib.quote(project_name),
+                             urllib.quote(action),
+                             urllib.urlencode(dict(get, key=key, json=1))),
         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)
+    try:
+        result = urllib2.urlopen(request).read()
+    except urllib2.HTTPError as e:
+        raise Exception('Server returned HTTP Error {}:\n{}'.format(e.code,
+                                                                    e.read()))
+
+    if not raw:
+        return json.loads(result)
 
     return result
 
 
 class OrderedDict(dict):
     def __init__(self):
         self.__order = []
 
     def __setitem__(self, key, value):
         self.__order.append(key)
@@ skipping 188 matching lines @@
         for match in re.finditer(r'&lang=([\w\-]+)"', firefoxLocales):
             locales.add(mapLocale('BCP-47', match.group(1)))
         langPacks = urllib2.urlopen('https://addons.mozilla.org/en-US/firefox/language-tools/').read()
         for match in re.finditer(r'<tr>.*?</tr>', langPacks, re.S):
             if match.group(0).find('Install Language Pack') >= 0:
                 match2 = re.search(r'lang="([\w\-]+)"', match.group(0))
                 if match2:
                     locales.add(mapLocale('BCP-47', match2.group(1)))
 
     allowed = set()
-    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.

+    allowedLocales = crowdin_request(projectName, 'supported-languages', key)
 
     for locale in allowedLocales:
         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])
 
-    crowdin_request(projectName, 'edit-project', key, post_data=params,
-                    raises=True)
-
-
-def crowdin_body_headers(files):
-    """Create a post body and according headers, which Crowdin can handle."""
+    crowdin_request(projectName, 'edit-project', key, post_data=params)
+
+
+def crowdin_prepare_upload(files):
+    """Create a post body and matching headers, which Crowdin can handle."""
     boundary = '----------ThIs_Is_tHe_bouNdaRY_$'
     body = ''
-    for file, data in files:
-        body += '--%s\r\n' % boundary
-        body += 'Content-Disposition: form-data; name="files[%s]"; filename="%s"\r\n' % (file, file)
-        body += 'Content-Type: application/octet-stream\r\n'
-        body += 'Content-Transfer-Encoding: binary\r\n'
-        body += '\r\n' + data + '\r\n'
-    body += '--%s--\r\n' % boundary
+    for name, data in files:
+        mimetype = mimetypes.guess_type(name)[0]
+        body += (
+            '--{boundary}\r\n'
+            'Content-Disposition: form-data; name="files[{name}]"; '
+            'filename="{name}"\r\n'
+            'Content-Type: {mimetype}; charset=utf-8\r\n'
+            'Content-Transfer-Encoding: binary\r\n'
+            '\r\n{data}\r\n'
+            '--{boundary}--\r\n'
+        ).format(boundary=boundary, name=name, data=data, mimetype=mimetype)
 
     body = body.encode('utf-8')
     return (
         StringIO(body),
         {
-            'Content-Type': 'multipart/form-data; boundary=%s' % boundary,
+            'Content-Type': ('multipart/form-data; boundary=' + boundary),
             'Content-Length': len(body)
-        }
+        },
     )
 
 
 def updateTranslationMaster(localeConfig, metadata, dir, projectName, key):
-    result = crowdin_request(projectName, 'info', key, {'json': 1})
+    result = crowdin_request(projectName, 'info', key)
 
     existing = set(map(lambda f: f['name'], result['files']))
     add = []
     update = []
     for file in os.listdir(dir):
         path = os.path.join(dir, file)
         if os.path.isfile(path):
             if localeConfig['file_format'] == 'chrome-json' and file.endswith('.json'):
                 data = preprocessChromeLocale(path, metadata, True)
                 newName = file
             elif localeConfig['file_format'] == 'chrome-json':
                 fileHandle = codecs.open(path, 'rb', encoding='utf-8')
                 data = json.dumps({file: {'message': fileHandle.read()}})
                 fileHandle.close()
                 newName = file + '.json'
             else:
                 data = toJSON(path)
                 newName = file + '.json'
 
             if data:
                 if newName in existing:
                     update.append((newName, data))
                     existing.remove(newName)
                 else:
                     add.append((newName, data))
 
     if len(add):
-        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)
+        query = {'titles[{}]'.format(name): os.path.splitext(name)[0]
+                 for name, _ in add}
+        query['type'] = 'chrome'
+        data, headers = crowdin_prepare_upload(add)
+        crowdin_request(projectName, 'add-file', key, query, post_data=data,
+                        headers=headers)
     if len(update):
-        data, headers = crowdin_body_headers(update)
+        data, headers = crowdin_prepare_upload(update)
         crowdin_request(projectName, 'update-file', key, post_data=data,
-                        headers=headers, raises=True)
+                        headers=headers)
     for file in existing:
         crowdin_request(projectName, 'delete-file', key, {'file': file})
 
 
 def uploadTranslations(localeConfig, metadata, dir, locale, projectName, key):
     files = []
     for file in os.listdir(dir):
         path = os.path.join(dir, file)
         if os.path.isfile(path):
             if localeConfig['file_format'] == 'chrome-json' and file.endswith('.json'):
                 data = preprocessChromeLocale(path, metadata, False)
                 newName = file
             elif localeConfig['file_format'] == 'chrome-json':
                 fileHandle = codecs.open(path, 'rb', encoding='utf-8')
                 data = json.dumps({file: {'message': fileHandle.read()}})
                 fileHandle.close()
                 newName = file + '.json'
             else:
                 data = toJSON(path)
                 newName = file + '.json'
 
             if data:
                 files.append((newName, data))
     if len(files):
         language = mapLocale(localeConfig['name_format'], locale)
-        data, headers = crowdin_body_headers(files)
+        data, headers = crowdin_prepare_upload(files)
         crowdin_request(projectName, 'upload-translation', key,
                         {'language': language}, post_data=data,
-                        headers=headers, raises=True)
+                        headers=headers)
 
 
 def getTranslations(localeConfig, projectName, key):
-    # let crowdin build the project
-    crowdin_request(projectName, 'export', key, raises=True)
-
-    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.

+    """Download all available translations from crowdin.
+
+    Trigger crowdin to build the available export, wait for crowdin to
+    finish the job and download the generated zip afterwards.
+    """
+    crowdin_request(projectName, 'export', key)
+
+    result = crowdin_request(projectName, 'download/all.zip', key, raw=True)
     zip = ZipFile(StringIO(result))
     dirs = {}
 
     normalizedDefaultLocale = localeConfig['default_locale']
     if localeConfig['name_format'] == 'ISO-15897':
         normalizedDefaultLocale = normalizedDefaultLocale.replace('_', '-')
     normalizedDefaultLocale = mapLocale(localeConfig['name_format'],
                                         normalizedDefaultLocale)
 
     for info in zip.infolist():
@@ skipping 47 matching lines @@
 
     # Remove any extra files
     for dir, files in dirs.iteritems():
         baseDir = os.path.join(localeConfig['base_path'], dir)
         if not os.path.exists(baseDir):
             continue
         for file in os.listdir(baseDir):
             path = os.path.join(baseDir, file)
             if os.path.isfile(path) and (file.endswith('.json') or file.endswith('.properties') or file.endswith('.dtd')) and not file in files:
                 os.remove(path)