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.update({'key': key})

[Vasily Kuznetsov, 2017/09/26 11:05:52]

Why not just `get['key'] = key`? Seems simpler.

[tlucas, 2017/09/26 12:13:13]

Done.

-
-    scheme, netloc, path, params, query, fragment = urlparse.urlparse(url)
-
-    query = dict(urlparse.parse_qsl(query))

[Vasily Kuznetsov, 2017/09/26 11:05:52]

Could there be anything in this query? It seems like it can only come from the
`action` argument, but in all of the calls to `crowdin_url` `action` is just a
simple path fragment without anything like "?foo=bar&baz=qux" that could end up
in the query. Or am I missing something?

[tlucas, 2017/09/26 12:13:13]

No, you are right - currently, there can't be query parameters - but i somehow
don't like the idea that there shouldn't be any (e.g. when someone imports this
function and expects it to keep the original query - as i did during
development).

In case someone absolutely opposes this, i would just drop a possibly present
query - but currently i'd like to keep it.

[Vasily Kuznetsov, 2017/09/26 13:11:36]

Acknowledged.

-    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,
+                    headers={}, raw=False):
+    """Perform a call to crowdin and raise an Exception on failure."""
+    request = urllib2.Request(
+        '{}/{}/{}?{}'.format(CROWDIN_AP_URL,
+                             urllib.quote(project_name),
+                             urllib.quote(action),
+                             urllib.urlencode(dict(get, key=key, json=1))),
+        post_data,
+        headers,
+    )
+
+    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)
         dict.__setitem__(self, key, value)
 
@@ skipping 186 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 = json.load(urllib2.urlopen(
-        crowdin_url(projectName, 'supported-languages', key, {'json': 1})))
+    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])
 
-    result = urllib2.urlopen(
-        crowdin_url(projectName, 'edit-project', key), data=params
-    ).read()
-
-    if result.find('<success') < 0:
-        raise Exception('Server indicated that the operation was not successful\n' + result)
-
-
-def postFiles(files, url):
+    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')
-    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=' + boundary),
+            'Content-Length': len(body)
+        },
+    )
 
 
 def updateTranslationMaster(localeConfig, metadata, dir, projectName, key):
-    result = json.load(urllib2.urlopen(
-        crowdin_url(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)
-                for name, data in add}
-        data.update({'type': 'chrome'})

[Vasily Kuznetsov, 2017/09/26 11:05:52]

Also could be just `data['type'] = 'chrome'`.

[tlucas, 2017/09/26 12:13:13]

Done.

-        postFiles(add, crowdin_url(projectName, 'add-file', key, data))
+        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):
-        postFiles(update, crowdin_url(projectName, 'update-file', key))
+        data, headers = crowdin_prepare_upload(update)
+        crowdin_request(projectName, 'update-file', key, post_data=data,
+                        headers=headers)
     for file in existing:
-        result = urllib2.urlopen(
-            crowdin_url(projectName, 'delete-file', key, {'file': 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):
     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)
-        postFiles(files,

[Vasily Kuznetsov, 2017/09/26 11:05:52]

It would probably be more readable if this was split into two statements: 

    url = crowdin_url(...)
    postFiles(files, url)

What do you think?

[tlucas, 2017/09/26 12:13:13]

Done.

-                  crowdin_url(projectName, 'upload-translation', key,
-                              {'language': language}))
+        data, headers = crowdin_prepare_upload(files)
+        crowdin_request(projectName, 'upload-translation', key,
+                        {'language': language}, post_data=data,
+                        headers=headers)
 
 
 def getTranslations(localeConfig, projectName, key):
-    result = urllib2.urlopen(crowdin_url(projectName, 'export', key)).read()

[Vasily Kuznetsov, 2017/09/26 11:05:52]

This pattern of `urllib2.urlopen(crowdin_url(....)).read()` seems to be a common
one. Maybe makes sense to make a function for it.

[tlucas, 2017/09/26 12:13:13]

Yes - most of the time, the result was used to detect server side errors -
outsourced this too.

-    if result.find('<success') < 0:
-        raise Exception('Server indicated that the operation was not successful\n' + result)
-
-    result = urllib2.urlopen(
-        crowdin_url(projectName, 'download/all.zip', key)).read()
+    """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)